* [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions
2019-06-18 23:20 [Xen-devel] [PATCH v3 0/5] iomem memory policy Stefano Stabellini
@ 2019-06-18 23:20 ` Stefano Stabellini
2019-06-19 7:42 ` Jan Beulich
2019-07-10 17:17 ` Julien Grall
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy Stefano Stabellini
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-06-18 23:20 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, JBeulich, andrew.cooper3
Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on
ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86,
introduce a macro to strip away the last parameter and rename the
existing implementation of map_mmio_regions to __map_mmio_regions.
Use __map_mmio_regions in vpci as it is x86-only today.
On ARM, given the similarity between map_mmio_regions after the change
and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to
check that only p2m_mmio_* types are passed to it.
Also fix the style of the comment on top of map_mmio_regions since we
are at it.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
---
Changes in v3:
- code style
- introduce __map_mmio_regions on x86
- fix comment style on top of map_mmio_regions
- add an assert on allowed p2mt types in map_mmio_regions
Changes in v2:
- new patch
---
xen/arch/arm/acpi/domain_build.c | 4 ++--
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/gic-v2.c | 3 ++-
xen/arch/arm/p2m.c | 21 ++++-----------------
xen/arch/arm/platforms/exynos5.c | 6 ++++--
xen/arch/arm/platforms/omap5.c | 12 ++++++++----
xen/arch/arm/traps.c | 2 +-
xen/arch/arm/vgic-v2.c | 2 +-
xen/arch/arm/vgic/vgic-v2.c | 2 +-
xen/arch/x86/hvm/dom0_build.c | 2 +-
xen/arch/x86/mm/p2m.c | 8 ++++----
xen/common/domctl.c | 7 ++++++-
| 2 +-
xen/include/asm-arm/p2m.h | 15 ---------------
xen/include/asm-x86/p2m.h | 8 ++++++++
xen/include/xen/p2m-common.h | 11 +++++++----
16 files changed, 51 insertions(+), 56 deletions(-)
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
index 5aae32ac20..f4ac91cdac 100644
--- a/xen/arch/arm/acpi/domain_build.c
+++ b/xen/arch/arm/acpi/domain_build.c
@@ -193,7 +193,7 @@ static void __init acpi_map_other_tables(struct domain *d)
{
addr = acpi_gbl_root_table_list.tables[i].address;
size = acpi_gbl_root_table_list.tables[i].length;
- res = map_regions_p2mt(d,
+ res = map_mmio_regions(d,
gaddr_to_gfn(addr),
PFN_UP(size),
maddr_to_mfn(addr),
@@ -547,7 +547,7 @@ int __init prepare_acpi(struct domain *d, struct kernel_info *kinfo)
acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add);
/* Map the EFI and ACPI tables to Dom0 */
- rc = map_regions_p2mt(d,
+ rc = map_mmio_regions(d,
gaddr_to_gfn(d->arch.efi_acpi_gpa),
PFN_UP(d->arch.efi_acpi_len),
virt_to_mfn(d->arch.efi_acpi_table),
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d9836779d1..1f808b2ff1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1171,7 +1171,7 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
if ( need_mapping )
{
- res = map_regions_p2mt(d,
+ res = map_mmio_regions(d,
gaddr_to_gfn(addr),
PFN_UP(len),
maddr_to_mfn(addr),
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 256988c665..d2ef361fc7 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -701,7 +701,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
PFN_UP(v2m_data->size),
- maddr_to_mfn(v2m_data->addr));
+ maddr_to_mfn(v2m_data->addr),
+ p2m_mmio_direct_dev);
if ( ret )
{
printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e28ea1c85a..d88df11e09 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1310,31 +1310,18 @@ static inline int p2m_remove_mapping(struct domain *d,
return rc;
}
-int map_regions_p2mt(struct domain *d,
+int map_mmio_regions(struct domain *d,
gfn_t gfn,
unsigned long nr,
mfn_t mfn,
p2m_type_t p2mt)
{
+ ASSERT( p2mt == p2m_mmio_direct_dev ||
+ p2mt == p2m_mmio_direct_nc ||
+ p2mt == p2m_mmio_direct_c );
return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
}
-int unmap_regions_p2mt(struct domain *d,
- gfn_t gfn,
- unsigned long nr,
- mfn_t mfn)
-{
- return p2m_remove_mapping(d, gfn, nr, mfn);
-}
-
-int map_mmio_regions(struct domain *d,
- gfn_t start_gfn,
- unsigned long nr,
- mfn_t mfn)
-{
- return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
-}
-
int unmap_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 6560507092..97cd080759 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -83,11 +83,13 @@ static int exynos5250_specific_mapping(struct domain *d)
{
/* Map the chip ID */
map_mmio_regions(d, gaddr_to_gfn(EXYNOS5_PA_CHIPID), 1,
- maddr_to_mfn(EXYNOS5_PA_CHIPID));
+ maddr_to_mfn(EXYNOS5_PA_CHIPID),
+ p2m_mmio_direct_dev);
/* Map the PWM region */
map_mmio_regions(d, gaddr_to_gfn(EXYNOS5_PA_TIMER), 2,
- maddr_to_mfn(EXYNOS5_PA_TIMER));
+ maddr_to_mfn(EXYNOS5_PA_TIMER),
+ p2m_mmio_direct_dev);
return 0;
}
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index aee24e4d28..c5701dfd6c 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -99,19 +99,23 @@ static int omap5_specific_mapping(struct domain *d)
{
/* Map the PRM module */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_PRM_BASE), 2,
- maddr_to_mfn(OMAP5_PRM_BASE));
+ maddr_to_mfn(OMAP5_PRM_BASE),
+ p2m_mmio_direct_dev);
/* Map the PRM_MPU */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_PRCM_MPU_BASE), 1,
- maddr_to_mfn(OMAP5_PRCM_MPU_BASE));
+ maddr_to_mfn(OMAP5_PRCM_MPU_BASE),
+ p2m_mmio_direct_dev);
/* Map the Wakeup Gen */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_WKUPGEN_BASE), 1,
- maddr_to_mfn(OMAP5_WKUPGEN_BASE));
+ maddr_to_mfn(OMAP5_WKUPGEN_BASE),
+ p2m_mmio_direct_dev);
/* Map the on-chip SRAM */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_SRAM_PA), 32,
- maddr_to_mfn(OMAP5_SRAM_PA));
+ maddr_to_mfn(OMAP5_SRAM_PA),
+ p2m_mmio_direct_dev);
return 0;
}
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3103620323..ec125cfd4f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1884,7 +1884,7 @@ static bool try_map_mmio(gfn_t gfn)
if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + 1) )
return false;
- return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
+ return !map_mmio_regions(d, gfn, 1, mfn, p2m_mmio_direct_c);
}
static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 64b141fea5..1543625ea4 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -691,7 +691,7 @@ static int vgic_v2_domain_init(struct domain *d)
* region of the guest.
*/
ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
- maddr_to_mfn(vbase));
+ maddr_to_mfn(vbase), p2m_mmio_direct_dev);
if ( ret )
return ret;
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index b5ba4ace87..04f34ddab5 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -309,7 +309,7 @@ int vgic_v2_map_resources(struct domain *d)
* region of the guest.
*/
ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
- maddr_to_mfn(vbase));
+ maddr_to_mfn(vbase), p2m_mmio_direct_dev);
if ( ret )
{
gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 8845399ae9..3a43234f69 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -79,7 +79,7 @@ static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
for ( ; ; )
{
- rc = map ? map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
+ rc = map ? __map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
: unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
if ( rc == 0 )
break;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 4c9954867c..e007eee42f 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2276,10 +2276,10 @@ static unsigned int mmio_order(const struct domain *d,
#define MAP_MMIO_MAX_ITER 64 /* pretty arbitrary */
-int map_mmio_regions(struct domain *d,
- gfn_t start_gfn,
- unsigned long nr,
- mfn_t mfn)
+int __map_mmio_regions(struct domain *d,
+ gfn_t start_gfn,
+ unsigned long nr,
+ mfn_t mfn)
{
int ret = 0;
unsigned long i;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 72a44953d0..c6fd88d285 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -927,6 +927,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
unsigned long mfn_end = mfn + nr_mfns - 1;
int add = op->u.memory_mapping.add_mapping;
+ p2m_type_t p2mt;
ret = -EINVAL;
if ( mfn_end < mfn || /* wrap? */
@@ -939,6 +940,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
/* Must break hypercall up as this could take a while. */
if ( nr_mfns > 64 )
break;
+
+ p2mt = p2m_mmio_direct_dev;
+#else
+ p2mt = p2m_mmio_direct;
#endif
ret = -EPERM;
@@ -956,7 +961,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
"memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
d->domain_id, gfn, mfn, nr_mfns);
- ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
+ ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn), p2mt);
if ( ret < 0 )
printk(XENLOG_G_WARNING
"memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
--git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 258b91deed..ade6d19b45 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -52,7 +52,7 @@ static int map_range(unsigned long s, unsigned long e, void *data,
* - {un}map_mmio_regions doesn't support preemption.
*/
- rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
+ rc = map->map ? __map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
: unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
if ( rc == 0 )
{
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 2f89bb00c3..76aadc215d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -252,21 +252,6 @@ void p2m_toggle_cache(struct vcpu *v, bool was_enabled);
void p2m_flush_vm(struct vcpu *v);
-/*
- * Map a region in the guest p2m with a specific p2m type.
- * The memory attributes will be derived from the p2m type.
- */
-int map_regions_p2mt(struct domain *d,
- gfn_t gfn,
- unsigned long nr,
- mfn_t mfn,
- p2m_type_t p2mt);
-
-int unmap_regions_p2mt(struct domain *d,
- gfn_t gfn,
- unsigned long nr,
- mfn_t mfn);
-
int map_dev_mmio_region(struct domain *d,
gfn_t gfn,
unsigned long nr,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 09ef7e02fd..8321dc1f2a 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
return 0;
}
+/* x86 doesn't use the p2mt parameter, just strip it away */
+#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
+ __map_mmio_regions(d, start_gfn, nr, mfn)
+int __map_mmio_regions(struct domain *d,
+ gfn_t start_gfn,
+ unsigned long nr,
+ mfn_t mfn);
+
#endif /* _XEN_ASM_X86_P2M_H */
/*
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 58031a6ea8..e20b4974b0 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -8,13 +8,16 @@ int __must_check
guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
unsigned int page_order);
-/* Map MMIO regions in the p2m: start_gfn and nr describe the range in
- * * the guest physical address space to map, starting from the machine
- * * frame number mfn. */
+/*
+ * Map MMIO regions in the p2m: start_gfn and nr describe the range in
+ * the guest physical address space to map, starting from the machine
+ * frame number mfn.
+ */
int map_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
- mfn_t mfn);
+ mfn_t mfn,
+ p2m_type_t p2mt);
int unmap_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions Stefano Stabellini
@ 2019-06-19 7:42 ` Jan Beulich
2019-08-06 23:05 ` Stefano Stabellini
2019-07-10 17:17 ` Julien Grall
1 sibling, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2019-06-19 7:42 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote:
> Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on
> ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86,
> introduce a macro to strip away the last parameter and rename the
> existing implementation of map_mmio_regions to __map_mmio_regions.
> Use __map_mmio_regions in vpci as it is x86-only today.
>
> On ARM, given the similarity between map_mmio_regions after the change
> and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to
> check that only p2m_mmio_* types are passed to it.
>
> Also fix the style of the comment on top of map_mmio_regions since we
> are at it.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
> ---
> Changes in v3:
> - code style
> - introduce __map_mmio_regions on x86
No. At the very least the name is badly chosen: There shouldn't be
new name space violations. But ...
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> return 0;
> }
>
> +/* x86 doesn't use the p2mt parameter, just strip it away */
> +#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
> + __map_mmio_regions(d, start_gfn, nr, mfn)
> +int __map_mmio_regions(struct domain *d,
> + gfn_t start_gfn,
> + unsigned long nr,
> + mfn_t mfn);
> +
... except for this perhaps not being everyone's taste, is there
anything wrong with just
/* x86 doesn't use the p2mt parameter, just strip it away */
#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
map_mmio_regions(d, start_gfn, nr, mfn)
(placed ahead of the p2m-common.h inclusion point, such that
the override would also affect the declaration)?
The next best (imo) solution would be to utilize the fact that the
function is mis-named right now anyway: There's no point for the
plural in its name afaics. Hence the aliasing above could also
go between map_mmio_regions() and map_mmio_region(),
depending on whether you'd want to adjust the "common" name
at the same time (but if you did so, then perhaps the unmap
function should get renamed too).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions
2019-06-19 7:42 ` Jan Beulich
@ 2019-08-06 23:05 ` Stefano Stabellini
0 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-08-06 23:05 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini,
Stefano Stabellini, xen-devel
On Wed, 19 Jun 2019, Jan Beulich wrote:
> >>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote:
> > Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on
> > ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86,
> > introduce a macro to strip away the last parameter and rename the
> > existing implementation of map_mmio_regions to __map_mmio_regions.
> > Use __map_mmio_regions in vpci as it is x86-only today.
> >
> > On ARM, given the similarity between map_mmio_regions after the change
> > and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to
> > check that only p2m_mmio_* types are passed to it.
> >
> > Also fix the style of the comment on top of map_mmio_regions since we
> > are at it.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > Changes in v3:
> > - code style
> > - introduce __map_mmio_regions on x86
>
> No. At the very least the name is badly chosen: There shouldn't be
> new name space violations. But ...
>
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> > return 0;
> > }
> >
> > +/* x86 doesn't use the p2mt parameter, just strip it away */
> > +#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
> > + __map_mmio_regions(d, start_gfn, nr, mfn)
> > +int __map_mmio_regions(struct domain *d,
> > + gfn_t start_gfn,
> > + unsigned long nr,
> > + mfn_t mfn);
> > +
>
> ... except for this perhaps not being everyone's taste, is there
> anything wrong with just
>
> /* x86 doesn't use the p2mt parameter, just strip it away */
> #define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
> map_mmio_regions(d, start_gfn, nr, mfn)
>
> (placed ahead of the p2m-common.h inclusion point, such that
> the override would also affect the declaration)?
I couldn't go with this suggestion because then usages of
map_mmio_regions under arch/x86 would fail to compile unless adjusted
by adding one useless argument, as the macro wouldn't apply correctly.
> The next best (imo) solution would be to utilize the fact that the
> function is mis-named right now anyway: There's no point for the
> plural in its name afaics. Hence the aliasing above could also
> go between map_mmio_regions() and map_mmio_region(),
I went with this suggestion, basically I renamed __map_mmio_regions to
map_mmio_region.
> depending on whether you'd want to adjust the "common" name
> at the same time (but if you did so, then perhaps the unmap
> function should get renamed too).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions Stefano Stabellini
2019-06-19 7:42 ` Jan Beulich
@ 2019-07-10 17:17 ` Julien Grall
2019-08-06 23:38 ` Stefano Stabellini
1 sibling, 1 reply; 16+ messages in thread
From: Julien Grall @ 2019-07-10 17:17 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Stefano Stabellini, JBeulich, andrew.cooper3
Hi Stefano,
The Arm code looks good to me. One comment below.
On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e28ea1c85a..d88df11e09 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1310,31 +1310,18 @@ static inline int p2m_remove_mapping(struct domain *d,
> return rc;
> }
>
> -int map_regions_p2mt(struct domain *d,
> +int map_mmio_regions(struct domain *d,
> gfn_t gfn,
> unsigned long nr,
> mfn_t mfn,
> p2m_type_t p2mt)
> {
> + ASSERT( p2mt == p2m_mmio_direct_dev ||
> + p2mt == p2m_mmio_direct_nc ||
> + p2mt == p2m_mmio_direct_c );
Could you introduce p2m_is_mmio(...) in p2m.h?
> return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
> }
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions
2019-07-10 17:17 ` Julien Grall
@ 2019-08-06 23:38 ` Stefano Stabellini
2019-08-07 10:35 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Stefano Stabellini @ 2019-08-06 23:38 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, andrew.cooper3, JBeulich,
Stefano Stabellini
On Wed, 10 Jul 2019, Julien Grall wrote:
> Hi Stefano,
>
> The Arm code looks good to me. One comment below.
Should I take it as a acked-by?
> On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index e28ea1c85a..d88df11e09 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1310,31 +1310,18 @@ static inline int p2m_remove_mapping(struct domain
> > *d,
> > return rc;
> > }
> > -int map_regions_p2mt(struct domain *d,
> > +int map_mmio_regions(struct domain *d,
> > gfn_t gfn,
> > unsigned long nr,
> > mfn_t mfn,
> > p2m_type_t p2mt)
> > {
> > + ASSERT( p2mt == p2m_mmio_direct_dev ||
> > + p2mt == p2m_mmio_direct_nc ||
> > + p2mt == p2m_mmio_direct_c );
>
> Could you introduce p2m_is_mmio(...) in p2m.h?
Good idea, I'll do that in a prior patch and use it here.
> > return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
> > }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
2019-06-18 23:20 [Xen-devel] [PATCH v3 0/5] iomem memory policy Stefano Stabellini
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions Stefano Stabellini
@ 2019-06-18 23:20 ` Stefano Stabellini
2019-06-19 7:48 ` Jan Beulich
2019-07-10 17:39 ` Julien Grall
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 3/5] libxc: introduce xc_domain_mem_map_policy Stefano Stabellini
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-06-18 23:20 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, JBeulich, andrew.cooper3
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, return error if the memory policy requested is not
MEMORY_POLICY_X86_UC_MINUS.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
---
Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS completely.
If that's the consensus I am happy to respin the series removing code.
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 | 24 ++++++++++++++++++++++--
xen/include/public/domctl.h | 23 ++++++++++++++++++++++-
3 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 05d771f2ce..8531298563 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 = 0;
max_batch_sz = nr_mfns;
do
{
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index c6fd88d285..f21f6957b0 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,28 @@ 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_nGnRE:
+ p2mt = p2m_mmio_direct_dev;
+ break;
+#endif
+#ifdef CONFIG_X86
+ case MEMORY_POLICY_X86_UC_MINUS:
+ p2mt = p2m_mmio_direct;
+ break;
+#endif
+ default:
+ domctl_lock_release();
+ 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..e51caada35 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -571,12 +571,33 @@ 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_POLICY_X86_UC_MINUS
+ */
+#define MEMORY_POLICY_DEFAULT 0
+#if defined(__i386__) || defined(__x86_64__)
+/* x86 only. Memory type UNCACHABLE */
+# define MEMORY_POLICY_X86_UC_MINUS 0
+#elif 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 */
};
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy Stefano Stabellini
@ 2019-06-19 7:48 ` Jan Beulich
2019-08-06 23:57 ` Stefano Stabellini
2019-07-10 17:39 ` Julien Grall
1 sibling, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2019-06-19 7:48 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote:
> --- 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 = 0;
Why not MEMORY_POLICY_DEFAULT?
> --- 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;
I can't see the need for a fixed-width type here.
> @@ -958,9 +959,28 @@ 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);
Why "cache=" when it's a "policy" value?
> + 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_X86_UC_MINUS:
FTR - I could certainly live with this becoming MEMORY_POLICY_DEFAULT
for now, if that's really what Andrew prefers for x86.
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -571,12 +571,33 @@ 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_POLICY_X86_UC_MINUS
> + */
> +#define MEMORY_POLICY_DEFAULT 0
> +#if defined(__i386__) || defined(__x86_64__)
> +/* x86 only. Memory type UNCACHABLE */
> +# define MEMORY_POLICY_X86_UC_MINUS 0
> +#elif 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
Further up it's Arm - why all upper case here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
2019-06-19 7:48 ` Jan Beulich
@ 2019-08-06 23:57 ` Stefano Stabellini
0 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-08-06 23:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini,
Stefano Stabellini, xen-devel
On Wed, 19 Jun 2019, Jan Beulich wrote:
> >>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote:
> > --- 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 = 0;
>
> Why not MEMORY_POLICY_DEFAULT?
OK
> > --- 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;
>
> I can't see the need for a fixed-width type here.
OK
> > @@ -958,9 +959,28 @@ 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);
>
> Why "cache=" when it's a "policy" value?
OK
> > + 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_X86_UC_MINUS:
>
> FTR - I could certainly live with this becoming MEMORY_POLICY_DEFAULT
> for now, if that's really what Andrew prefers for x86.
All right, I'll remove MEMORY_POLICY_X86_UC_MINUS then.
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -571,12 +571,33 @@ 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_POLICY_X86_UC_MINUS
> > + */
> > +#define MEMORY_POLICY_DEFAULT 0
> > +#if defined(__i386__) || defined(__x86_64__)
> > +/* x86 only. Memory type UNCACHABLE */
> > +# define MEMORY_POLICY_X86_UC_MINUS 0
> > +#elif 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
>
> Further up it's Arm - why all upper case here?
I fixed it
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy Stefano Stabellini
2019-06-19 7:48 ` Jan Beulich
@ 2019-07-10 17:39 ` Julien Grall
2019-08-06 23:42 ` Stefano Stabellini
1 sibling, 1 reply; 16+ messages in thread
From: Julien Grall @ 2019-07-10 17:39 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Stefano Stabellini, JBeulich, andrew.cooper3
Hi,
On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> 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
s/ARM/Arm/
> 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_MINUS.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
>
> ---
>
> Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS completely.
> If that's the consensus I am happy to respin the series removing code.
>
>
> 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 | 24 ++++++++++++++++++++++--
> xen/include/public/domctl.h | 23 ++++++++++++++++++++++-
> 3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 05d771f2ce..8531298563 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 = 0;
> max_batch_sz = nr_mfns;
> do
> {
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index c6fd88d285..f21f6957b0 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,28 @@ 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_nGnRE:
> + p2mt = p2m_mmio_direct_dev;
> + break;
> +#endif
> +#ifdef CONFIG_X86
> + case MEMORY_POLICY_X86_UC_MINUS:
> + p2mt = p2m_mmio_direct;
> + break;
> +#endif
> + default:
AFAICT, ret will be zero in this path and therefore the caller may think
the mapping succeeded. I think we want to set 'ret' to -EINVAL.
> + domctl_lock_release();
> + 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..e51caada35 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -571,12 +571,33 @@ 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_POLICY_X86_UC_MINUS
> + */
> +#define MEMORY_POLICY_DEFAULT 0
> +#if defined(__i386__) || defined(__x86_64__)
> +/* x86 only. Memory type UNCACHABLE */
> +# define MEMORY_POLICY_X86_UC_MINUS 0
> +#elif 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
s/ARM/Arm/
> + * 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 */
> };
>
>
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
2019-07-10 17:39 ` Julien Grall
@ 2019-08-06 23:42 ` Stefano Stabellini
0 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-08-06 23:42 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, andrew.cooper3, JBeulich,
Stefano Stabellini
On Wed, 10 Jul 2019, Julien Grall wrote:
> Hi,
>
> On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> > 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
>
> s/ARM/Arm/
OK
> > 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_MINUS.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> >
> > ---
> >
> > Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS completely.
> > If that's the consensus I am happy to respin the series removing code.
> >
> >
> > 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 | 24 ++++++++++++++++++++++--
> > xen/include/public/domctl.h | 23 ++++++++++++++++++++++-
> > 3 files changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > index 05d771f2ce..8531298563 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 = 0;
> > max_batch_sz = nr_mfns;
> > do
> > {
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index c6fd88d285..f21f6957b0 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,28 @@ 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_nGnRE:
> > + p2mt = p2m_mmio_direct_dev;
> > + break;
> > +#endif
> > +#ifdef CONFIG_X86
> > + case MEMORY_POLICY_X86_UC_MINUS:
> > + p2mt = p2m_mmio_direct;
> > + break;
> > +#endif
> > + default:
>
> AFAICT, ret will be zero in this path and therefore the caller may think the
> mapping succeeded. I think we want to set 'ret' to -EINVAL.
Good idea, I'll do that
> > + goto domctl_out_unlock_domonly;
> > + }
> > ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn),
> > p2mt);
> > if ( ret < 0 )
> > printk(XENLOG_G_WARNING
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Xen-devel] [PATCH v3 3/5] libxc: introduce xc_domain_mem_map_policy
2019-06-18 23:20 [Xen-devel] [PATCH v3 0/5] iomem memory policy Stefano Stabellini
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions Stefano Stabellini
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy Stefano Stabellini
@ 2019-06-18 23:20 ` Stefano Stabellini
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 4/5] libxl/xl: add memory policy option to iomem Stefano Stabellini
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 5/5] xen/arm: clarify the support status of iomem configurations Stefano Stabellini
4 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-06-18 23:20 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, ian.jackson, wei.liu2
Introduce a new libxc function that makes use of the new memory_policy
parameter added to the XEN_DOMCTL_memory_mapping hypercall.
The parameter values are the same for the XEN_DOMCTL_memory_mapping
hypercall (0 is MEMORY_POLICY_DEFAULT). Pass MEMORY_POLICY_DEFAULT by
default -- no changes in behavior.
We could extend xc_domain_memory_mapping, but QEMU makes use of it, so
it is easier and less disruptive to introduce a new libxc function and
change the implementation of xc_domain_memory_mapping to call into it.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: ian.jackson@eu.citrix.com
CC: wei.liu2@citrix.com
---
Changes in v2:
- 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
- introduce xc_domain_mem_map_policy
---
tools/libxc/include/xenctrl.h | 8 ++++++++
tools/libxc/xc_domain.c | 25 ++++++++++++++++++++-----
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 538007a6dc..64bd98d5fe 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1713,6 +1713,14 @@ int xc_deassign_dt_device(xc_interface *xch,
uint32_t domid,
char *path);
+int xc_domain_mem_map_policy(xc_interface *xch,
+ uint32_t domid,
+ unsigned long first_gfn,
+ unsigned long first_mfn,
+ unsigned long nr_mfns,
+ uint32_t add_mapping,
+ uint32_t memory_policy);
+
int xc_domain_memory_mapping(xc_interface *xch,
uint32_t domid,
unsigned long first_gfn,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 8531298563..02f5778212 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2042,13 +2042,14 @@ failed:
return -1;
}
-int xc_domain_memory_mapping(
+int xc_domain_mem_map_policy(
xc_interface *xch,
uint32_t domid,
unsigned long first_gfn,
unsigned long first_mfn,
unsigned long nr_mfns,
- uint32_t add_mapping)
+ uint32_t add_mapping,
+ uint32_t memory_policy)
{
DECLARE_DOMCTL;
xc_dominfo_t info;
@@ -2070,7 +2071,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 = 0;
+ domctl.u.memory_mapping.memory_policy = memory_policy;
max_batch_sz = nr_mfns;
do
{
@@ -2106,8 +2107,9 @@ int xc_domain_memory_mapping(
* Errors here are ignored.
*/
if ( ret && add_mapping != DPCI_REMOVE_MAPPING )
- xc_domain_memory_mapping(xch, domid, first_gfn, first_mfn, nr_mfns,
- DPCI_REMOVE_MAPPING);
+ xc_domain_mem_map_policy(xch, domid, first_gfn, first_mfn, nr_mfns,
+ DPCI_REMOVE_MAPPING,
+ MEMORY_POLICY_DEFAULT);
/* We might get E2BIG so many times that we never advance. */
if ( !done && !ret )
@@ -2116,6 +2118,19 @@ int xc_domain_memory_mapping(
return ret;
}
+int xc_domain_memory_mapping(
+ xc_interface *xch,
+ uint32_t domid,
+ unsigned long first_gfn,
+ unsigned long first_mfn,
+ unsigned long nr_mfns,
+ uint32_t add_mapping)
+{
+ return xc_domain_mem_map_policy(xch, domid, first_gfn, first_mfn,
+ nr_mfns, add_mapping,
+ MEMORY_POLICY_DEFAULT);
+}
+
int xc_domain_ioport_mapping(
xc_interface *xch,
uint32_t domid,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Xen-devel] [PATCH v3 4/5] libxl/xl: add memory policy option to iomem
2019-06-18 23:20 [Xen-devel] [PATCH v3 0/5] iomem memory policy Stefano Stabellini
` (2 preceding siblings ...)
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 3/5] libxc: introduce xc_domain_mem_map_policy Stefano Stabellini
@ 2019-06-18 23:20 ` Stefano Stabellini
2019-07-10 19:02 ` Julien Grall
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 5/5] xen/arm: clarify the support status of iomem configurations Stefano Stabellini
4 siblings, 1 reply; 16+ messages in thread
From: Stefano Stabellini @ 2019-06-18 23:20 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, ian.jackson, wei.liu2
Add a new memory policy option for the iomem parameter.
Possible values are:
- arm_dev_nGnRE, Device-nGnRE, the default on ARM
- arm_mem_WB, WB cachable memory
- x86_UC_minus: uncachable memory, the default on x86
Store the parameter in a new field in libxl_iomem_range.
Pass the memory policy option to xc_domain_mem_map_policy.
Do the libxl to libxc value conversion in per-arch functions so that we
can return error for x86 parameters on Arm architectures and vice versa.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: ian.jackson@eu.citrix.com
CC: wei.liu2@citrix.com
---
Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS and x86_UC_minus
completely. If that's the consensus I am happy to respin the series
removing code.
Changes in v3:
- s/nGRE/nGnRE/g
- s/LIBXL_MEMORY_POLICY_ARM_DEV_NGRE/LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE/g
- s/arm_devmem/arm_dev_nGnRE/g
- s/arm_memory/arm_mem_WB/g
- improve commit message
- improve man page
- s/MEMORY_POLICY_X86_UC/MEMORY_POLICY_X86_UC_MINUS/g
- s/x86_uc/x86_UC_minus/g
- move security support clarification to a separate patch
Changes in v2:
- add #define LIBXL_HAVE_MEMORY_POLICY
- ability to part the memory policy parameter even if gfn is not passed
- 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
- rename memory to arm_memory and devmem to arm_devmem
- expand the non-security support status to non device passthrough iomem
configurations
- rename iomem options
- add x86 specific iomem option
---
docs/man/xl.cfg.5.pod.in | 10 +++++++++-
tools/libxl/libxl.h | 5 +++++
tools/libxl/libxl_arch.h | 3 +++
tools/libxl/libxl_arm.c | 14 ++++++++++++++
tools/libxl/libxl_create.c | 12 ++++++++++--
tools/libxl/libxl_types.idl | 9 +++++++++
tools/libxl/libxl_x86.c | 12 ++++++++++++
tools/xl/xl_parse.c | 22 +++++++++++++++++++++-
8 files changed, 83 insertions(+), 4 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..fbb9e43e9e 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff>
It is recommended to only use this option for trusted VMs under
administrator's control.
-=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]>
+=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]>
Allow auto-translated domains to access specific hardware I/O memory pages.
@@ -1233,6 +1233,14 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START>
as a start in the guest's address space, therefore performing a 1:1 mapping
by default.
All of these values must be given in hexadecimal format.
+B<MEMORY_POLICY> for ARM platforms:
+ - "arm_dev_nGnRE" for Device-nGnRE (Device Memory on Armv7), the default on ARM
+ - "arm_mem_WB" for Outer Shareable Write-Back Cacheable Memory
+They select 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.
+B<MEMORY_POLICY> can be for x86 platforms:
+ - "x86_UC_minus" for Uncachable Memory, the default on x86
Note that the IOMMU won't be updated with the mappings specified with this
option. This option therefore should not be used to pass through any
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9bacfb97f0..cf12f1d3bd 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -378,6 +378,11 @@
#define LIBXL_HAVE_BUILDINFO_BOOTLOADER 1
#define LIBXL_HAVE_BUILDINFO_BOOTLOADER_ARGS 1
+/*
+ * Support specifying memory policy information for memory mappings.
+ */
+#define LIBXL_HAVE_MEMORY_POLICY 1
+
/*
* LIBXL_HAVE_EXTENDED_VKB indicates that libxl_device_vkb has extended fields:
* - unique_id;
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index d624159e53..9c858b06ce 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -77,6 +77,9 @@ int libxl__arch_extra_memory(libxl__gc *gc,
const libxl_domain_build_info *info,
uint64_t *out);
+_hidden
+int libxl__memory_policy_to_xc(libxl_memory_policy c);
+
#if defined(__i386__) || defined(__x86_64__)
#define LAPIC_BASE_ADDRESS 0xfee00000
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 141e159043..6354b852dc 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1149,6 +1149,20 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
}
+int libxl__memory_policy_to_xc(libxl_memory_policy c)
+{
+ switch (c) {
+ case LIBXL_MEMORY_POLICY_ARM_MEM_WB:
+ return MEMORY_POLICY_ARM_MEM_WB;
+ case LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE:
+ return MEMORY_POLICY_ARM_DEV_nGnRE;
+ case LIBXL_MEMORY_POLICY_DEFAULT:
+ return MEMORY_POLICY_DEFAULT;
+ default:
+ return ERROR_INVAL;
+ }
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 03ce166f4f..84da6406d0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1357,6 +1357,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
for (i = 0; i < d_config->b_info.num_iomem; i++) {
libxl_iomem_range *io = &d_config->b_info.iomem[i];
+ int memory_policy;
LOGD(DEBUG, domid, "iomem %"PRIx64"-%"PRIx64,
io->start, io->start + io->number - 1);
@@ -1370,9 +1371,16 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
ret = ERROR_FAIL;
goto error_out;
}
- ret = xc_domain_memory_mapping(CTX->xch, domid,
+ memory_policy = libxl__memory_policy_to_xc(io->memory_policy);
+ if (memory_policy < 0) {
+ LOGED(ERROR, domid,
+ "invalid memory policy %u", io->memory_policy);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+ ret = xc_domain_mem_map_policy(CTX->xch, domid,
io->gfn, io->start,
- io->number, 1);
+ io->number, 1, memory_policy);
if (ret < 0) {
LOGED(ERROR, domid,
"failed to map to domain iomem range %"PRIx64"-%"PRIx64
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b61399ce36..bd791d338d 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -272,6 +272,13 @@ libxl_ioport_range = Struct("ioport_range", [
("number", uint32),
])
+libxl_memory_policy = Enumeration("memory_policy", [
+ (0, "default"),
+ (1, "ARM_Dev_nGnRE"),
+ (2, "ARM_Mem_WB"),
+ (3, "x86_UC_MINUS"),
+ ], init_val = "LIBXL_MEMORY_POLICY_DEFAULT")
+
libxl_iomem_range = Struct("iomem_range", [
# start host frame number to be mapped to the guest
("start", uint64),
@@ -279,6 +286,8 @@ libxl_iomem_range = Struct("iomem_range", [
("number", uint64),
# guest frame number used as a start for the mapping
("gfn", uint64, {'init_val': "LIBXL_INVALID_GFN"}),
+ # memory_policy of the memory region
+ ("memory_policy", libxl_memory_policy),
])
libxl_vga_interface_info = Struct("vga_interface_info", [
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index c0f88a7eaa..c7c0ad9218 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -631,6 +631,18 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->acpi, true);
}
+int libxl__memory_policy_to_xc(libxl_memory_policy c)
+{
+ switch (c) {
+ case LIBXL_MEMORY_POLICY_X86_UC_MINUS:
+ return MEMORY_POLICY_X86_UC_MINUS;
+ case LIBXL_MEMORY_POLICY_DEFAULT:
+ return MEMORY_POLICY_DEFAULT;
+ default:
+ return ERROR_INVAL;
+ }
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e105bda2bb..46525f40f5 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1883,6 +1883,7 @@ void parse_config_data(const char *config_source,
}
for (i = 0; i < num_iomem; i++) {
int used;
+ const char *mempolicy;
buf = xlu_cfg_get_listitem (iomem, i);
if (!buf) {
@@ -1895,11 +1896,30 @@ void parse_config_data(const char *config_source,
&b_info->iomem[i].start,
&b_info->iomem[i].number, &used,
&b_info->iomem[i].gfn, &used);
- if (ret < 2 || buf[used] != '\0') {
+ if (ret < 2) {
fprintf(stderr,
"xl: Invalid argument parsing iomem: %s\n", buf);
exit(1);
}
+ mempolicy = &buf[used];
+ if (strlen(mempolicy) > 1) {
+ mempolicy++;
+ if (!strcmp(mempolicy, "arm_dev_nGnRE"))
+ b_info->iomem[i].memory_policy =
+ LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE;
+ else if (!strcmp(mempolicy, "x86_UC_minus"))
+ b_info->iomem[i].memory_policy =
+ LIBXL_MEMORY_POLICY_X86_UC_MINUS;
+ else if (!strcmp(mempolicy, "arm_mem_WB"))
+ b_info->iomem[i].memory_policy =
+ LIBXL_MEMORY_POLICY_ARM_MEM_WB;
+ else {
+ fprintf(stderr,
+ "xl: Invalid iomem memory policy parameter: %s\n",
+ mempolicy);
+ exit(1);
+ }
+ }
}
}
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH v3 4/5] libxl/xl: add memory policy option to iomem
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 4/5] libxl/xl: add memory policy option to iomem Stefano Stabellini
@ 2019-07-10 19:02 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2019-07-10 19:02 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini, ian.jackson, wei.liu2
Hi Stefano,
On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> Add a new memory policy option for the iomem parameter.
> Possible values are:
> - arm_dev_nGnRE, Device-nGnRE, the default on ARM
s/ARM/Arm/
> - arm_mem_WB, WB cachable memory
> - x86_UC_minus: uncachable memory, the default on x86
>
> Store the parameter in a new field in libxl_iomem_range.
>
> Pass the memory policy option to xc_domain_mem_map_policy.
>
> Do the libxl to libxc value conversion in per-arch functions so that we
> can return error for x86 parameters on Arm architectures and vice versa.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: ian.jackson@eu.citrix.com
> CC: wei.liu2@citrix.com
> ---
>
> Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS and x86_UC_minus
> completely. If that's the consensus I am happy to respin the series
> removing code.
>
>
> Changes in v3:
> - s/nGRE/nGnRE/g
> - s/LIBXL_MEMORY_POLICY_ARM_DEV_NGRE/LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE/g
> - s/arm_devmem/arm_dev_nGnRE/g
> - s/arm_memory/arm_mem_WB/g
> - improve commit message
> - improve man page
> - s/MEMORY_POLICY_X86_UC/MEMORY_POLICY_X86_UC_MINUS/g
> - s/x86_uc/x86_UC_minus/g
> - move security support clarification to a separate patch
>
> Changes in v2:
> - add #define LIBXL_HAVE_MEMORY_POLICY
> - ability to part the memory policy parameter even if gfn is not passed
> - 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
> - rename memory to arm_memory and devmem to arm_devmem
> - expand the non-security support status to non device passthrough iomem
> configurations
> - rename iomem options
> - add x86 specific iomem option
> ---
> docs/man/xl.cfg.5.pod.in | 10 +++++++++-
> tools/libxl/libxl.h | 5 +++++
> tools/libxl/libxl_arch.h | 3 +++
> tools/libxl/libxl_arm.c | 14 ++++++++++++++
> tools/libxl/libxl_create.c | 12 ++++++++++--
> tools/libxl/libxl_types.idl | 9 +++++++++
> tools/libxl/libxl_x86.c | 12 ++++++++++++
> tools/xl/xl_parse.c | 22 +++++++++++++++++++++-
> 8 files changed, 83 insertions(+), 4 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index c99d40307e..fbb9e43e9e 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff>
> It is recommended to only use this option for trusted VMs under
> administrator's control.
>
> -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]>
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]>
>
> Allow auto-translated domains to access specific hardware I/O memory pages.
>
> @@ -1233,6 +1233,14 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START>
> as a start in the guest's address space, therefore performing a 1:1 mapping
> by default.
> All of these values must be given in hexadecimal format.
> +B<MEMORY_POLICY> for ARM platforms:
Ditto.
> + - "arm_dev_nGnRE" for Device-nGnRE (Device Memory on Armv7), the default on ARM
Ditto.
> + - "arm_mem_WB" for Outer Shareable Write-Back Cacheable Memory
> +They select 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.
> +B<MEMORY_POLICY> can be for x86 platforms:
> + - "x86_UC_minus" for Uncachable Memory, the default on x86
>
> Note that the IOMMU won't be updated with the mappings specified with this
> option. This option therefore should not be used to pass through any
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 9bacfb97f0..cf12f1d3bd 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -378,6 +378,11 @@
> #define LIBXL_HAVE_BUILDINFO_BOOTLOADER 1
> #define LIBXL_HAVE_BUILDINFO_BOOTLOADER_ARGS 1
>
> +/*
> + * Support specifying memory policy information for memory mappings.
> + */
> +#define LIBXL_HAVE_MEMORY_POLICY 1
> +
> /*
> * LIBXL_HAVE_EXTENDED_VKB indicates that libxl_device_vkb has extended fields:
> * - unique_id;
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index d624159e53..9c858b06ce 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -77,6 +77,9 @@ int libxl__arch_extra_memory(libxl__gc *gc,
> const libxl_domain_build_info *info,
> uint64_t *out);
>
> +_hidden
> +int libxl__memory_policy_to_xc(libxl_memory_policy c);
Functions name are prefixed with libxl__arch_ in this header.
[...]
> @@ -1895,11 +1896,30 @@ void parse_config_data(const char *config_source,
> &b_info->iomem[i].start,
> &b_info->iomem[i].number, &used,
> &b_info->iomem[i].gfn, &used);
> - if (ret < 2 || buf[used] != '\0') {
> + if (ret < 2) {
> fprintf(stderr,
> "xl: Invalid argument parsing iomem: %s\n", buf);
> exit(1);
> }
> + mempolicy = &buf[used];
> + if (strlen(mempolicy) > 1) {
> + mempolicy++;
> + if (!strcmp(mempolicy, "arm_dev_nGnRE"))
> + b_info->iomem[i].memory_policy =
> + LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE;
> + else if (!strcmp(mempolicy, "x86_UC_minus"))
> + b_info->iomem[i].memory_policy =
> + LIBXL_MEMORY_POLICY_X86_UC_MINUS;
> + else if (!strcmp(mempolicy, "arm_mem_WB"))
> + b_info->iomem[i].memory_policy =
> + LIBXL_MEMORY_POLICY_ARM_MEM_WB;
Any reason to not keep all arm policies together?
> + else {
> + fprintf(stderr,
> + "xl: Invalid iomem memory policy parameter: %s\n",
> + mempolicy);
> + exit(1);
> + }
> + }
> }
> }
>
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Xen-devel] [PATCH v3 5/5] xen/arm: clarify the support status of iomem configurations
2019-06-18 23:20 [Xen-devel] [PATCH v3 0/5] iomem memory policy Stefano Stabellini
` (3 preceding siblings ...)
2019-06-18 23:20 ` [Xen-devel] [PATCH v3 4/5] libxl/xl: add memory policy option to iomem Stefano Stabellini
@ 2019-06-18 23:20 ` Stefano Stabellini
4 siblings, 0 replies; 16+ messages in thread
From: Stefano Stabellini @ 2019-06-18 23:20 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, sstabellini, wei.liu2, konrad.wilk,
andrew.cooper3, tim, julien.grall, JBeulich, ian.jackson
iomem settings fall under the broader category of "Non-PCI device
passthrough": they are not security supported. Make it clearer.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: tim@xen.org
CC: konrad.wilk@oracle.com
CC: Julien Grall <julien.grall@arm.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
CC: ian.jackson@eu.citrix.com
CC: wei.liu2@citrix.com
---
SUPPORT.md | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SUPPORT.md b/SUPPORT.md
index 375473a456..bc6fb58e04 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -639,7 +639,7 @@ to be used in addition to QEMU.
Status: Experimental
-### ARM/Non-PCI device passthrough
+### ARM/Non-PCI device passthrough and other iomem configurations
Status: Supported, not security supported
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread