| Message ID | 1556658172-8824-3-git-send-email-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/10] xen: add a p2mt parameter to map_mmio_regions | expand |
>>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote: > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { > */ > #define DPCI_ADD_MAPPING 1 > #define DPCI_REMOVE_MAPPING 0 > +/* > + * Default memory policy. Corresponds to: > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE > + * x86: MEMORY_POLICY_X86_UC > + */ > +#define MEMORY_POLICY_DEFAULT 0 > +/* x86 only. Memory type UNCACHABLE */ > +#define MEMORY_POLICY_X86_UC 0 I'm afraid this may end up misleading, as on NPT and in shadow mode we use UC- instead of UC afaics. Andrew, do you have an opinion either way what exactly should be stated here? Jan
Hi Stefano, On 4/30/19 10:02 PM, Stefano Stabellini wrote: > Reuse the existing padding field to pass memory policy information. On NIT: I know that some developper like using two spaces after the final point. I don't mind if you use it but please be at least consistent within the commit message. > Arm, the caller can specify whether the memory should be mapped as > device nGRE, which is the default and the only possibility today, or I am afraid this is not correct. The default on is Device-nGnRE (it is called Device Memory on Armv7). > cacheable memory write-back. On x86, the only option is uncachable. The > current behavior becomes the default (numerically '0'). > > On ARM, map device nGRE as p2m_mmio_direct_dev (as it is already done > today) and WB cacheable memory as p2m_mmio_direct_c. As I pointed out in v1, the wording is confusing. The resulting memory attribute will be a combination of stage-2 and stage-2 memory attributes. It will actually be whatever is the strongest between the 2 stages attributes. You can see the stage-2 attributes as a way to give more or less freedom to the guest for configure the attributes. The commit message and all documentation should actually reflect that to avoid misuse of the new option. > > On x86, return error if the memory policy requested is not > MEMORY_POLICY_X86_UC. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > CC: JBeulich@suse.com > CC: andrew.cooper3@citrix.com > > --- > Changes in v2: > - rebase > - use p2m_mmio_direct_c > - use EOPNOTSUPP > - rename cache_policy to memory policy > - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE > - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB > - add MEMORY_POLICY_X86_UC > - add MEMORY_POLICY_DEFAULT and use it > --- > xen/common/domctl.c | 23 +++++++++++++++++++++-- > xen/include/public/domctl.h | 14 +++++++++++++- > 2 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 140f979..9f62ead 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -928,6 +928,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > unsigned long mfn_end = mfn + nr_mfns - 1; > int add = op->u.memory_mapping.add_mapping; > p2m_type_t p2mt; > + uint32_t memory_policy = op->u.memory_mapping.memory_policy; > > ret = -EINVAL; > if ( mfn_end < mfn || /* wrap? */ > @@ -958,9 +959,27 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > if ( add ) > { > printk(XENLOG_G_DEBUG > - "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n", > - d->domain_id, gfn, mfn, nr_mfns); > + "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx cache=%u\n", > + d->domain_id, gfn, mfn, nr_mfns, memory_policy); > > + switch ( memory_policy ) > + { > +#ifdef CONFIG_ARM > + case MEMORY_POLICY_ARM_MEM_WB: > + p2mt = p2m_mmio_direct_c; > + break; > + case MEMORY_POLICY_ARM_DEV_nGRE: > + p2mt = p2m_mmio_direct_dev; > + break; > +#endif > +#ifdef CONFIG_X86 > + case MEMORY_POLICY_X86_UC: > + p2mt = p2m_mmio_direct; > + break; > +#endif > + default: > + return -EOPNOTSUPP; > + } > ret = map_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn), p2mt); > if ( ret < 0 ) > printk(XENLOG_G_WARNING > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 19486d5..9330387 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { > */ > #define DPCI_ADD_MAPPING 1 > #define DPCI_REMOVE_MAPPING 0 > +/* > + * Default memory policy. Corresponds to: > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE > + * x86: MEMORY_POLICY_X86_UC > + */ > +#define MEMORY_POLICY_DEFAULT 0 > +/* x86 only. Memory type UNCACHABLE */ > +#define MEMORY_POLICY_X86_UC 0 > +/* Arm only. Outer Shareable, Device-nGRE memory */ Device-nGRE is an Armv8 term. You might want to also specify the Armv7 name in parenthesis to help the user. > +#define MEMORY_POLICY_ARM_DEV_nGRE 0 > +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */ > +#define MEMORY_POLICY_ARM_MEM_WB 1 I am wondering whether we should put Arm (resp. x86) defines under an ifdef arm (resp. x86). Do you see any use in the common toolstack code of those #ifdef? > struct xen_domctl_memory_mapping { > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */ > uint64_aligned_t first_mfn; /* first page (machine page) in range */ > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */ > uint32_t add_mapping; /* add or remove mapping */ > - uint32_t padding; /* padding for 64-bit aligned structure */ > + uint32_t memory_policy; /* cacheability of the memory mapping */ From a quick look at libxc, it seems the padding field will not be initialized to 0 (aka MEMORY_DEFAULT_POLICY). As the libxc support is added in a follow-up patch, I think you want to ensure memory_policy is equal to MEMORY_DEFAULT_POLICY in libxc. So there are no unexpected behavior during bisection or this patch gets applied before the rest. Cheers,
On 01.05.19 00:02, Stefano Stabellini wrote: Hi, Stefano > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 140f979..9f62ead 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -928,6 +928,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > unsigned long mfn_end = mfn + nr_mfns - 1; > int add = op->u.memory_mapping.add_mapping; > p2m_type_t p2mt; > + uint32_t memory_policy = op->u.memory_mapping.memory_policy; > > ret = -EINVAL; > if ( mfn_end < mfn || /* wrap? */ > @@ -958,9 +959,27 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > if ( add ) > { > printk(XENLOG_G_DEBUG > - "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n", > - d->domain_id, gfn, mfn, nr_mfns); > + "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx cache=%u\n", > + d->domain_id, gfn, mfn, nr_mfns, memory_policy); > > + switch ( memory_policy ) > + { > +#ifdef CONFIG_ARM > + case MEMORY_POLICY_ARM_MEM_WB: > + p2mt = p2m_mmio_direct_c; > + break; > + case MEMORY_POLICY_ARM_DEV_nGRE: > + p2mt = p2m_mmio_direct_dev; > + break; > +#endif > +#ifdef CONFIG_X86 > + case MEMORY_POLICY_X86_UC: > + p2mt = p2m_mmio_direct; > + break; > +#endif > + default: > + return -EOPNOTSUPP; If I correctly understand the code, we can't just return an error here (domctl_lock is taken, etc). Looks like we should store an error and modify code to execute exit part.
On Thu, 2 May 2019, Jan Beulich wrote: > >>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote: > > --- a/xen/include/public/domctl.h > > +++ b/xen/include/public/domctl.h > > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { > > */ > > #define DPCI_ADD_MAPPING 1 > > #define DPCI_REMOVE_MAPPING 0 > > +/* > > + * Default memory policy. Corresponds to: > > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE > > + * x86: MEMORY_POLICY_X86_UC > > + */ > > +#define MEMORY_POLICY_DEFAULT 0 > > +/* x86 only. Memory type UNCACHABLE */ > > +#define MEMORY_POLICY_X86_UC 0 > > I'm afraid this may end up misleading, as on NPT and in > shadow mode we use UC- instead of UC afaics. Andrew, > do you have an opinion either way what exactly should > be stated here? Ping? I am happy to use any naming scheme you prefer, please provide input.
On Tue, 7 May 2019, Julien Grall wrote: > > +#define MEMORY_POLICY_ARM_DEV_nGRE 0 > > +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */ > > +#define MEMORY_POLICY_ARM_MEM_WB 1 > > I am wondering whether we should put Arm (resp. x86) defines under an ifdef > arm (resp. x86). Do you see any use in the common toolstack code of those > #ifdef? Yes, they are used in libxl_create.c. I would prefer to avoid introducing #ifdef here because it will allow us to get away with no #ifdef in libxl/xl. OK too all other comments.
>>> On 17.06.19 at 23:28, <sstabellini@kernel.org> wrote: > On Thu, 2 May 2019, Jan Beulich wrote: >> >>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote: >> > --- a/xen/include/public/domctl.h >> > +++ b/xen/include/public/domctl.h >> > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { >> > */ >> > #define DPCI_ADD_MAPPING 1 >> > #define DPCI_REMOVE_MAPPING 0 >> > +/* >> > + * Default memory policy. Corresponds to: >> > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE >> > + * x86: MEMORY_POLICY_X86_UC >> > + */ >> > +#define MEMORY_POLICY_DEFAULT 0 >> > +/* x86 only. Memory type UNCACHABLE */ >> > +#define MEMORY_POLICY_X86_UC 0 >> >> I'm afraid this may end up misleading, as on NPT and in >> shadow mode we use UC- instead of UC afaics. Andrew, >> do you have an opinion either way what exactly should >> be stated here? > > Ping? To me? I've stated my opinion. Jan > I am happy to use any naming scheme you prefer, please provide input.
On 17/06/2019 23:43, Stefano Stabellini wrote: > On Tue, 7 May 2019, Julien Grall wrote: >>> +#define MEMORY_POLICY_ARM_DEV_nGRE 0 >>> +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */ >>> +#define MEMORY_POLICY_ARM_MEM_WB 1 >> >> I am wondering whether we should put Arm (resp. x86) defines under an ifdef >> arm (resp. x86). Do you see any use in the common toolstack code of those >> #ifdef? > > Yes, they are used in libxl_create.c. I would prefer to avoid > introducing #ifdef here because it will allow us to get away with no > #ifdef in libxl/xl. Well, you can introduce an arch specific function that convert the memory policy. But I will leave this decision to the tools maintainers. Cheers,
On Tue, 18 Jun 2019, Jan Beulich wrote: > >>> On 17.06.19 at 23:28, <sstabellini@kernel.org> wrote: > > On Thu, 2 May 2019, Jan Beulich wrote: > >> >>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote: > >> > --- a/xen/include/public/domctl.h > >> > +++ b/xen/include/public/domctl.h > >> > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { > >> > */ > >> > #define DPCI_ADD_MAPPING 1 > >> > #define DPCI_REMOVE_MAPPING 0 > >> > +/* > >> > + * Default memory policy. Corresponds to: > >> > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE > >> > + * x86: MEMORY_POLICY_X86_UC > >> > + */ > >> > +#define MEMORY_POLICY_DEFAULT 0 > >> > +/* x86 only. Memory type UNCACHABLE */ > >> > +#define MEMORY_POLICY_X86_UC 0 > >> > >> I'm afraid this may end up misleading, as on NPT and in > >> shadow mode we use UC- instead of UC afaics. Andrew, > >> do you have an opinion either way what exactly should > >> be stated here? > > > > Ping? > > To me? I've stated my opinion. I cannot name the macro "MEMORY_POLICY_X86_UC-" because it cannot end with a "-". Instead, I can name it MEMORY_POLICY_X86_UC_MINUS that seems to be what Linux does. I'll rename the optional xl parameter too from "x86_uc" to "x86_uc_minus".
On Tue, 18 Jun 2019, Stefano Stabellini wrote: > On Tue, 18 Jun 2019, Jan Beulich wrote: > > >>> On 17.06.19 at 23:28, <sstabellini@kernel.org> wrote: > > > On Thu, 2 May 2019, Jan Beulich wrote: > > >> >>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote: > > >> > --- a/xen/include/public/domctl.h > > >> > +++ b/xen/include/public/domctl.h > > >> > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { > > >> > */ > > >> > #define DPCI_ADD_MAPPING 1 > > >> > #define DPCI_REMOVE_MAPPING 0 > > >> > +/* > > >> > + * Default memory policy. Corresponds to: > > >> > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE > > >> > + * x86: MEMORY_POLICY_X86_UC > > >> > + */ > > >> > +#define MEMORY_POLICY_DEFAULT 0 > > >> > +/* x86 only. Memory type UNCACHABLE */ > > >> > +#define MEMORY_POLICY_X86_UC 0 > > >> > > >> I'm afraid this may end up misleading, as on NPT and in > > >> shadow mode we use UC- instead of UC afaics. Andrew, > > >> do you have an opinion either way what exactly should > > >> be stated here? > > > > > > Ping? > > > > To me? I've stated my opinion. > > I cannot name the macro "MEMORY_POLICY_X86_UC-" because it cannot end > with a "-". Instead, I can name it MEMORY_POLICY_X86_UC_MINUS that seems > to be what Linux does. I'll rename the optional xl parameter too from > "x86_uc" to "x86_uc_minus". I chatted with Andrew on IRC and he suggested to get rid of the option entirely -- there is just one on x86 and doesn't necessarily need to be explicitly visible. We could only have MEMORY_POLICY_DEFAULT, and also remove the x86_uc setting from libxl/xl. I am OK with this. However, given that I have already made all the changes to have MEMORY_POLICY_X86_UC_MINUS and x86_uc_minus everywhere, I'll send an update of the series with them. Then you can decide whether you want to keep things like that or get rid of it. Of course removing code is easy -- I am always happy to do it if that's what you want.
>>> On 19.06.19 at 01:15, <sstabellini@kernel.org> wrote: > On Tue, 18 Jun 2019, Stefano Stabellini wrote: >> On Tue, 18 Jun 2019, Jan Beulich wrote: >> > >>> On 17.06.19 at 23:28, <sstabellini@kernel.org> wrote: >> > > On Thu, 2 May 2019, Jan Beulich wrote: >> > >> >>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote: >> > >> > --- a/xen/include/public/domctl.h >> > >> > +++ b/xen/include/public/domctl.h >> > >> > @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { >> > >> > */ >> > >> > #define DPCI_ADD_MAPPING 1 >> > >> > #define DPCI_REMOVE_MAPPING 0 >> > >> > +/* >> > >> > + * Default memory policy. Corresponds to: >> > >> > + * Arm: MEMORY_POLICY_ARM_DEV_nGRE >> > >> > + * x86: MEMORY_POLICY_X86_UC >> > >> > + */ >> > >> > +#define MEMORY_POLICY_DEFAULT 0 >> > >> > +/* x86 only. Memory type UNCACHABLE */ >> > >> > +#define MEMORY_POLICY_X86_UC 0 >> > >> >> > >> I'm afraid this may end up misleading, as on NPT and in >> > >> shadow mode we use UC- instead of UC afaics. Andrew, >> > >> do you have an opinion either way what exactly should >> > >> be stated here? >> > > >> > > Ping? >> > >> > To me? I've stated my opinion. >> >> I cannot name the macro "MEMORY_POLICY_X86_UC-" because it cannot end >> with a "-". Instead, I can name it MEMORY_POLICY_X86_UC_MINUS that seems >> to be what Linux does. I'll rename the optional xl parameter too from >> "x86_uc" to "x86_uc_minus". > > I chatted with Andrew on IRC and he suggested to get rid of the option > entirely -- there is just one on x86 and doesn't necessarily need to be > explicitly visible. We could only have MEMORY_POLICY_DEFAULT, and also > remove the x86_uc setting from libxl/xl. > > I am OK with this. However, given that I have already made all the > changes to have MEMORY_POLICY_X86_UC_MINUS and x86_uc_minus everywhere, > I'll send an update of the series with them. Aren't we back to the question them whether to make this an Arm- only interface? I'm having trouble seeing the value of an interface which allows one to only "switch" from default to default. Jan
diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 140f979..9f62ead 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -928,6 +928,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) unsigned long mfn_end = mfn + nr_mfns - 1; int add = op->u.memory_mapping.add_mapping; p2m_type_t p2mt; + uint32_t memory_policy = op->u.memory_mapping.memory_policy; ret = -EINVAL; if ( mfn_end < mfn || /* wrap? */ @@ -958,9 +959,27 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) if ( add ) { printk(XENLOG_G_DEBUG - "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n", - d->domain_id, gfn, mfn, nr_mfns); + "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx cache=%u\n", + d->domain_id, gfn, mfn, nr_mfns, memory_policy); + switch ( memory_policy ) + { +#ifdef CONFIG_ARM + case MEMORY_POLICY_ARM_MEM_WB: + p2mt = p2m_mmio_direct_c; + break; + case MEMORY_POLICY_ARM_DEV_nGRE: + p2mt = p2m_mmio_direct_dev; + break; +#endif +#ifdef CONFIG_X86 + case MEMORY_POLICY_X86_UC: + p2mt = p2m_mmio_direct; + break; +#endif + default: + return -EOPNOTSUPP; + } ret = map_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn), p2mt); if ( ret < 0 ) printk(XENLOG_G_WARNING diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 19486d5..9330387 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq { */ #define DPCI_ADD_MAPPING 1 #define DPCI_REMOVE_MAPPING 0 +/* + * Default memory policy. Corresponds to: + * Arm: MEMORY_POLICY_ARM_DEV_nGRE + * x86: MEMORY_POLICY_X86_UC + */ +#define MEMORY_POLICY_DEFAULT 0 +/* x86 only. Memory type UNCACHABLE */ +#define MEMORY_POLICY_X86_UC 0 +/* Arm only. Outer Shareable, Device-nGRE memory */ +#define MEMORY_POLICY_ARM_DEV_nGRE 0 +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */ +#define MEMORY_POLICY_ARM_MEM_WB 1 struct xen_domctl_memory_mapping { uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */ uint64_aligned_t first_mfn; /* first page (machine page) in range */ uint64_aligned_t nr_mfns; /* number of pages in range (>0) */ uint32_t add_mapping; /* add or remove mapping */ - uint32_t padding; /* padding for 64-bit aligned structure */ + uint32_t memory_policy; /* cacheability of the memory mapping */ };
Reuse the existing padding field to pass memory policy information. On Arm, the caller can specify whether the memory should be mapped as device nGRE, which is the default and the only possibility today, or cacheable memory write-back. On x86, the only option is uncachable. The current behavior becomes the default (numerically '0'). On ARM, map device nGRE as p2m_mmio_direct_dev (as it is already done today) and WB cacheable memory as p2m_mmio_direct_c. On x86, return error if the memory policy requested is not MEMORY_POLICY_X86_UC. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com --- Changes in v2: - rebase - use p2m_mmio_direct_c - use EOPNOTSUPP - rename cache_policy to memory policy - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB - add MEMORY_POLICY_X86_UC - add MEMORY_POLICY_DEFAULT and use it --- xen/common/domctl.c | 23 +++++++++++++++++++++-- xen/include/public/domctl.h | 14 +++++++++++++- 2 files changed, 34 insertions(+), 3 deletions(-)