| Message ID | 20190807002311.9906-3-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/6] xen/arm: introduce p2m_is_mmio | expand |
On 07.08.2019 02:23, Stefano Stabellini wrote: > @@ -950,9 +951,29 @@ 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 policy=%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_nGnRE: > + p2mt = p2m_mmio_direct_dev; > + break; > +#endif > +#ifdef CONFIG_X86 > + case MEMORY_POLICY_DEFAULT: > + p2mt = p2m_mmio_direct; > + break; > +#endif > + default: > + domctl_lock_release(); > + ret = -EINVAL; > + goto domctl_out_unlock_domonly; > + } Indentation is wrong here: The case labels ought to align with their switch(). Also please have blank lines between case blocks. For the Arm part it would be nice if the case being MEMORY_POLICY_DEFAULT would gain a respective comment, perhaps in the form of a commented out case label. > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -571,12 +571,30 @@ 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_nGnRE > + * x86: Memory type UNCACHABLE > + */ I continue to be unhappy about the imprecise "uncachable" here for x86: In shadow mode and on AMD/NPT we produce UC- mappings, while the way EPT works it further depends on host and guest MTRR settings. Andrew - do you have any thoughts how to best describe this here without growing the text too large, but also without producing an overly imprecise description? > +#define MEMORY_POLICY_DEFAULT 0 > +#if defined(__arm__) || defined (__aarch64__) > +/* Arm only. Outer Shareable, Device-nGnRE memory (Device Memory on Armv7) */ > +# define MEMORY_POLICY_ARM_DEV_nGnRE 0 > +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */ > +# define MEMORY_POLICY_ARM_MEM_WB 1 > +/* > + * On Arm, MEMORY_POLICY selects the stage-2 memory attributes, but note > + * that the resulting memory attributes will be a combination of stage-2 > + * and stage-1 memory attributes: it will be the strongest between the 2 > + * stages attributes. > + */ > +#endif > 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 */ The comment looks to start one column too far to the right. Jan
Hi, On 07/08/2019 01:23, Stefano Stabellini wrote: > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 2674caa005..063523c7f7 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -920,6 +920,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; > + unsigned int memory_policy = op->u.memory_mapping.memory_policy; > > ret = -EINVAL; > if ( mfn_end < mfn || /* wrap? */ > @@ -950,9 +951,29 @@ 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 policy=%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_nGnRE: > + p2mt = p2m_mmio_direct_dev; > + break; > +#endif > +#ifdef CONFIG_X86 > + case MEMORY_POLICY_DEFAULT: > + p2mt = p2m_mmio_direct; > + break; > +#endif I would prefer if the arch specific bits are done in arch code and not in common code. This could be done in a separate patch if you don't plan to respin it. The rest looks good to me. Cheers,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c index 05d771f2ce..075ffb9ed1 100644 --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -2070,6 +2070,7 @@ int xc_domain_memory_mapping( domctl.cmd = XEN_DOMCTL_memory_mapping; domctl.domain = domid; domctl.u.memory_mapping.add_mapping = add_mapping; + domctl.u.memory_mapping.memory_policy = MEMORY_POLICY_DEFAULT; max_batch_sz = nr_mfns; do { diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 2674caa005..063523c7f7 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -920,6 +920,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; + unsigned int memory_policy = op->u.memory_mapping.memory_policy; ret = -EINVAL; if ( mfn_end < mfn || /* wrap? */ @@ -950,9 +951,29 @@ 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 policy=%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_nGnRE: + p2mt = p2m_mmio_direct_dev; + break; +#endif +#ifdef CONFIG_X86 + case MEMORY_POLICY_DEFAULT: + p2mt = p2m_mmio_direct; + break; +#endif + default: + domctl_lock_release(); + ret = -EINVAL; + goto domctl_out_unlock_domonly; + } ret = map_mmio_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 19486d5e32..b9078400fa 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -571,12 +571,30 @@ 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_nGnRE + * x86: Memory type UNCACHABLE + */ +#define MEMORY_POLICY_DEFAULT 0 +#if defined(__arm__) || defined (__aarch64__) +/* Arm only. Outer Shareable, Device-nGnRE memory (Device Memory on Armv7) */ +# define MEMORY_POLICY_ARM_DEV_nGnRE 0 +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */ +# define MEMORY_POLICY_ARM_MEM_WB 1 +/* + * On Arm, MEMORY_POLICY selects the stage-2 memory attributes, but note + * that the resulting memory attributes will be a combination of stage-2 + * and stage-1 memory attributes: it will be the strongest between the 2 + * stages attributes. + */ +#endif 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-nGnRE (Device Memory on Armv7) at stage-2, which is the default and the only possibility today, or cacheable memory write-back. The resulting memory attributes will be a combination of stage-2 and stage-1 memory attributes: it will actually be the strongest between the 2 stages attributes. On x86, the only option is uncachable. The current behavior becomes the default (numerically '0'). Also explicitely set the memory_policy field to 0 in libxc. On Arm, map Device-nGnRE as p2m_mmio_direct_dev (as it is already done today) and WB cacheable memory as p2m_mmio_direct_c. On x86, there is just one policy which is the default. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com --- Changes in v4: - return -EINVAL on XEN_DOMCTL_memory_mapping on default label - use MEMORY_POLICY_DEFAULT instead of 0 - uint32_t memory_policy -> unsigned int memory_policy - cache= -> policy= - MEMORY_POLICY_X86_UC_MINUS -> MEMORY_POLICY_DEFAULT - ARM -> Arm Changes in v3: - error handling in default label of the switch - set memory_policy to 0 in libxc - improve commit message - improve comments - s/Device-nGRE/Device-nGnRE/g - add in-code comment - s/MEMORY_POLICY_X86_UC/MEMORY_POLICY_X86_UC_MINUS/g - #ifdef hypercall defines according to arch 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 --- tools/libxc/xc_domain.c | 1 + xen/common/domctl.c | 25 +++++++++++++++++++++++-- xen/include/public/domctl.h | 20 +++++++++++++++++++- 3 files changed, 43 insertions(+), 3 deletions(-)