[Xen-devel] [PATCH v4 0/6] iomem memory policy

[Xen-devel] [PATCH v4 0/6] iomem memory policy

[Stefano Stabellini]

Hi all,

This series introduces a memory policy parameter for the iomem option,
so that we can map an iomem region into a guest as cacheable memory.

(Other things related to reserved-memory on Arm have been sent
separately.)

Cheers,

Stefano


The following changes since commit 45ce5b8749a220ad7c4ce5d5eba7c201a9418078:

  mm: Safe to clear PGC_allocated on xenheap pages without an extra reference (2019-08-06 12:19:55 +0100)

are available in the Git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git 

for you to fetch changes up to 36be38da2f540aefbfb3d14076b835fc4f852aaf:

  xen/arm: clarify the support status of iomem configurations (2019-08-06 17:15:00 -0700)

----------------------------------------------------------------
Stefano Stabellini (6):
      xen/arm: introduce p2m_is_mmio
      xen: add a p2mt parameter to map_mmio_regions
      xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
      libxc: introduce xc_domain_mem_map_policy
      libxl/xl: add memory policy option to iomem
      xen/arm: clarify the support status of iomem configurations

 SUPPORT.md                       |  2 +-
 docs/man/xl.cfg.5.pod.in         | 10 +++++++++-
 tools/libxc/include/xenctrl.h    |  8 ++++++++
 tools/libxc/xc_domain.c          | 24 ++++++++++++++++++++----
 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      |  8 ++++++++
 tools/libxl/libxl_x86.c          | 10 ++++++++++
 tools/xl/xl_parse.c              | 22 +++++++++++++++++++++-
 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               | 19 ++-----------------
 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              | 32 +++++++++++++++++++++++++++++---
 xen/drivers/vpci/header.c        |  2 +-
 xen/include/asm-arm/p2m.h        | 21 ++++++---------------
 xen/include/asm-x86/p2m.h        |  8 ++++++++
 xen/include/public/domctl.h      | 20 +++++++++++++++++++-
 xen/include/xen/p2m-common.h     | 11 +++++++----
 28 files changed, 206 insertions(+), 68 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 1/6] xen/arm: introduce p2m_is_mmio

[Stefano Stabellini]

Add a p2m_is_mmio macro for easy checkings.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/include/asm-arm/p2m.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 03f2ee75c1..31902317da 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -132,6 +132,11 @@ typedef enum {
 #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
                        p2m_to_mask(p2m_ram_ro))
 
+/* MMIO types */
+#define P2M_MMIO_TYPES (p2m_to_mask(p2m_mmio_direct_dev) | \
+                        p2m_to_mask(p2m_mmio_direct_nc)  | \
+                        p2m_to_mask(p2m_mmio_direct_c))
+
 /* Grant mapping types, which map to a real frame in another VM */
 #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
                          p2m_to_mask(p2m_grant_map_ro))
@@ -146,6 +151,7 @@ typedef enum {
 #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
                             (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
                              P2M_FOREIGN_TYPES))
+#define p2m_is_mmio(_t) (p2m_to_mask(_t) & P2M_MMIO_TYPES)
 
 /* All common type definitions should live ahead of this inclusion. */
 #ifdef _XEN_P2M_COMMON_H
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Stefano Stabellini]

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_region.
Use map_mmio_region 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 v4:
- rename __map_mmio_regions to map_mmio_region
- use p2m_is_mmio

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               | 19 ++-----------------
 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 ++++++-
 xen/drivers/vpci/header.c        |  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, 49 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
index 1b1cfabb00..09f91cc8bf 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 4c8404155a..544b0040ce 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1168,7 +1168,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..4b26bca92a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1310,31 +1310,16 @@ 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(p2m_is_mmio(p2mt));
     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 f062ae6f6a..7209405d80 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1879,7 +1879,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..2d3940b0fb 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_region(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 fef97c82f6..e602cd229c 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2277,10 +2277,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_region(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 b48e408583..2674caa005 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -919,6 +919,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? */
@@ -931,6 +932,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;
@@ -948,7 +953,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",
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 3c794f486d..76b33af58e 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_region(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 31902317da..f970c53764 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -258,21 +258,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 aff34e3adf..a7050ee21c 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -1001,6 +1001,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_region(d, start_gfn, nr, mfn)
+int map_mmio_region(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

[Xen-devel] [PATCH v4 3/6] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy

[Stefano Stabellini]

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(-)

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 */
 };
 
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 4/6] libxc: introduce xc_domain_mem_map_policy

[Stefano Stabellini]

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 0ff6ed9e70..2396929960 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1714,6 +1714,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 075ffb9ed1..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 = MEMORY_POLICY_DEFAULT;
+    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

[Xen-devel] [PATCH v4 5/6] libxl/xl: add memory policy option to iomem

[Stefano Stabellini]

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
- default

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
---
Changes in v4:
- ARM -> Arm
- libxl__memory_policy_to_xc -> libxl__arch_memory_policy_to_xc
- keep Arm policies together

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 |  8 ++++++++
 tools/libxl/libxl_x86.c     | 10 ++++++++++
 tools/xl/xl_parse.c         | 22 +++++++++++++++++++++-
 8 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..77df9c1bdb 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 both Arm and x86 platforms:
+  - "default" which is Uncachable Memory on x86, and arm_dev_nGnRE on Arm
 
 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..796c961568 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__arch_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..6b8e7ddb06 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__arch_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..9b375cdf69 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__arch_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..ad5d9cdae2 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -272,6 +272,12 @@ libxl_ioport_range = Struct("ioport_range", [
     ("number", uint32),
     ])
 
+libxl_memory_policy = Enumeration("memory_policy", [
+    (0, "default"),
+    (1, "ARM_Dev_nGnRE"),
+    (2, "ARM_Mem_WB"),
+    ], 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 +285,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..32c211c3ae 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -631,6 +631,16 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+int libxl__arch_memory_policy_to_xc(libxl_memory_policy c)
+{
+    switch (c) {
+    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..c019bca728 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, "arm_mem_WB"))
+                    b_info->iomem[i].memory_policy =
+                        LIBXL_MEMORY_POLICY_ARM_MEM_WB;
+                else if (!strcmp(mempolicy, "default"))
+                    b_info->iomem[i].memory_policy =
+                        LIBXL_MEMORY_POLICY_DEFAULT;
+                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

[Xen-devel] [PATCH v4 6/6] xen/arm: clarify the support status of iomem configurations

[Stefano Stabellini]

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

Re: [Xen-devel] [PATCH v4 6/6] xen/arm: clarify the support status of iomem configurations

[Julien Grall]

Hi,

On 07/08/2019 01:23, Stefano Stabellini wrote:
> 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

I don't understand the new title. What are the other use case of IOMEM 
configurations?

Cheers,

>   
>       Status: Supported, not security supported
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 6/6] xen/arm: clarify the support status of iomem configurations

[Julien Grall]

Hi,

On 07/08/2019 10:08, Julien Grall wrote:
> Hi,
> 
> On 07/08/2019 01:23, Stefano Stabellini wrote:
>> 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

I just realized after sending the e-mail that the list of CC is smaller than it 
should and Wei's e-mail is incorrect. On the other e-mails, you forgot to CC 
Volodymyr as Arm reviewer.

I am pretty sure I already pointed that out in the past... We have tools in 
place to find out the maintainers to CC and add them (see 
scripts/add_maintainers.pl). So there are no need to do this manually anymore.

>> ---
>>   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
> 
> I don't understand the new title. What are the other use case of IOMEM 
> configurations?
> 
> Cheers,
> 
>>       Status: Supported, not security supported
>>
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 1/6] xen/arm: introduce p2m_is_mmio

[Julien Grall]

Hi Stefano,

On 07/08/2019 01:23, Stefano Stabellini wrote:
> Add a p2m_is_mmio macro for easy checkings.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
>   xen/include/asm-arm/p2m.h | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 03f2ee75c1..31902317da 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -132,6 +132,11 @@ typedef enum {
>   #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
>                          p2m_to_mask(p2m_ram_ro))
>   
> +/* MMIO types */
> +#define P2M_MMIO_TYPES (p2m_to_mask(p2m_mmio_direct_dev) | \
> +                        p2m_to_mask(p2m_mmio_direct_nc)  | \
> +                        p2m_to_mask(p2m_mmio_direct_c))
> +
>   /* Grant mapping types, which map to a real frame in another VM */
>   #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
>                            p2m_to_mask(p2m_grant_map_ro))
> @@ -146,6 +151,7 @@ typedef enum {
>   #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
>                               (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
>                                P2M_FOREIGN_TYPES))
> +#define p2m_is_mmio(_t) (p2m_to_mask(_t) & P2M_MMIO_TYPES)
>   
>   /* All common type definitions should live ahead of this inclusion. */
>   #ifdef _XEN_P2M_COMMON_H
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Julien Grall]

Hi,

On 07/08/2019 01:23, Stefano Stabellini 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_region.
> Use map_mmio_region in vpci as it is x86-only today.

This feels quite wrong. You have a "plural" function calling a "singular" 
function. This is usually the other way around. This is also quite difficult for 
a user to understand why the 's' is been dropped/added (depending how you view 
it) because in both case you only deal with a single region.

The confusion is added because there are no unmap_mmio_region so the code looks 
strange to read:

 > +        rc = map->map ? map_mmio_region(map->d, _gfn(s), size, _mfn(s))
 >                         : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));

Anyway, I realized that Jan suggested it and it is x86 code. So if he is happy 
with it so it be.

[...]

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index b48e408583..2674caa005 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -919,6 +919,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? */
> @@ -931,6 +932,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;

I think it is a pretty bad idea to have arch specific code in common code. This 
is only to make more difficult to add new arch (such as RISCv). Instead we 
should provide helper in arch code.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Jan Beulich]

On 09.08.2019 12:23, Julien Grall wrote:
> On 07/08/2019 01:23, Stefano Stabellini 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_region.
>> Use map_mmio_region in vpci as it is x86-only today.
> 
> This feels quite wrong. You have a "plural" function calling a "singular" function. This is usually the other way around. This is also quite difficult for a user to understand why the 's' is been dropped/added (depending how you view it) because in both case you only deal with a single region.

"Happy" is the wrong term. I'd welcome any better suggestion. I
simply couldn't think of a sensible alternative, but I do want
to see the leading underscores gone that were there originally.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Julien Grall]

Hi,

On 09/08/2019 11:37, Jan Beulich wrote:
> On 09.08.2019 12:23, Julien Grall wrote:
>> On 07/08/2019 01:23, Stefano Stabellini 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_region.
>>> Use map_mmio_region in vpci as it is x86-only today.
>>
>> This feels quite wrong. You have a "plural" function calling a "singular" 
>> function. This is usually the other way around. This is also quite difficult 
>> for a user to understand why the 's' is been dropped/added (depending how you 
>> view it) because in both case you only deal with a single region.
> 
> "Happy" is the wrong term. I'd welcome any better suggestion. I
> simply couldn't think of a sensible alternative, but I do want
> to see the leading underscores gone that were there originally.

We are already modifying all the callers in this series, so doing the renaming 
should not make the diff worst.

A few of suggestion:
     1) map_mmio_region() calling map_mmio_regions()
     2) arch_map_mmio_region() calling map_mmio_region()
     3) map_mmio_region() calling arch_map_mmio_region()

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Jan Beulich]

On 09.08.2019 12:51, Julien Grall wrote:
> Hi,
> 
> On 09/08/2019 11:37, Jan Beulich wrote:
>> On 09.08.2019 12:23, Julien Grall wrote:
>>> On 07/08/2019 01:23, Stefano Stabellini 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_region.
>>>> Use map_mmio_region in vpci as it is x86-only today.
>>>
>>> This feels quite wrong. You have a "plural" function calling a "singular" function. This is usually the other way around. This is also quite difficult for a user to understand why the 's' is been dropped/added (depending how you view it) because in both case you only deal with a single region.
>>
>> "Happy" is the wrong term. I'd welcome any better suggestion. I
>> simply couldn't think of a sensible alternative, but I do want
>> to see the leading underscores gone that were there originally.
> 
> We are already modifying all the callers in this series, so doing the renaming should not make the diff worst.
> 
> A few of suggestion:
>      1) map_mmio_region() calling map_mmio_regions()
>      2) arch_map_mmio_region() calling map_mmio_region()
>      3) map_mmio_region() calling arch_map_mmio_region()

Ideally the top level functions (i.e. also the unmap one) would lose
their plurals. Seeing that both require a per-arch implementation, I
guess the best choice from the above would be 2 (with the unmap one
following suit).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Jan Beulich]

On 07.08.2019 02:23, Stefano Stabellini 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_region.
> Use map_mmio_region 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>

I guess apart from the naming question (see other sub-thread) I'm
fine with this. However, ...

> ---
>   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               | 19 ++-----------------
>   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 ++++++-
>   xen/drivers/vpci/header.c        |  2 +-

... these two and ...

>   xen/include/asm-arm/p2m.h        | 15 ---------------
>   xen/include/asm-x86/p2m.h        |  8 ++++++++
>   xen/include/xen/p2m-common.h     | 11 +++++++----

... this one require you to widen the Cc list. This would the also
allow George to become more aware of the asm-x86/p2m.h change,
which strictly by ./MAINTAINERS he may not need to ack, but which
is part of "X86 MEMORY MANAGEMENT" really.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 3/6] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 5/6] libxl/xl: add memory policy option to iomem

[Julien Grall]

Hi Stefano,

On 07/08/2019 01:23, 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
> - arm_mem_WB, WB cachable memory
> - default
> 
> 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>

For the Arm bits in the doc:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 3/6] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel