[PATCH V5 0/3] Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space")

[PATCH V5 0/3] Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space")

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

You can find an initial discussion at [1]-[5].

The extended region (safe range) is a region of guest physical address space
which is unused and could be safely used to create grant/foreign mappings instead
of wasting real RAM pages from the domain memory for establishing these mappings.

The extended regions are chosen at the domain creation time and advertised
to it via "reg" property under hypervisor node in the guest device-tree
(the indexes for extended regions are 1...N).
No device tree bindings update is needed, guest infers the presense of extended
regions from the number of regions in "reg" property.
New compatible/property will be needed (but only after this patch [6] or alternative
goes in) to indicate that "region 0 is safe to use". Until this patch is merged it is
not safe to use extended regions for the grant table space.

The extended regions are calculated differently for direct mapped Dom0 (with and without
IOMMU) and non-direct mapped DomUs.

Please note the following limitations:
- The extended region feature is only supported for 64-bit domain currently.
- The ACPI case is not covered.

Please note that support for Dom0 [7] was already committed and the last patch in this series
is a requested follow-up.

Xen patch series is also available at [8]. The corresponding Linux patch series is at [9]
for now (last 4 patches).

Tested on Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with updated virtio-disk backend [10]
running in Dom0 (256MB RAM) and DomD (2GB RAM). In both cases the backend pre-maps DomU memory
which is 3GB. All foreign memory gets mapped into extended regions (so the amount of RAM in
the backend domain is not reduced). No issues were observed.

[1] https://lore.kernel.org/xen-devel/1627489110-25633-1-git-send-email-olekstysh@gmail.com/
[2] https://lore.kernel.org/xen-devel/1631034578-12598-1-git-send-email-olekstysh@gmail.com/
[3] https://lore.kernel.org/xen-devel/1631297924-8658-1-git-send-email-olekstysh@gmail.com/
[4] https://lore.kernel.org/xen-devel/1632437334-12015-1-git-send-email-olekstysh@gmail.com/
[5] https://lore.kernel.org/xen-devel/1632955927-27911-1-git-send-email-olekstysh@gmail.com/
[6] https://lore.kernel.org/xen-devel/1632425551-18910-1-git-send-email-olekstysh@gmail.com/
[7] https://lore.kernel.org/xen-devel/1632955927-27911-3-git-send-email-olekstysh@gmail.com/
[8] https://github.com/otyshchenko1/xen/commits/map_opt_ml6
[9] https://github.com/otyshchenko1/linux/commits/map_opt_ml4
[10] https://github.com/otyshchenko1/virtio-disk/commits/map_opt_next

Oleksandr Tyshchenko (3):
  xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig
  libxl/arm: Add handling of extended regions for DomU
  xen/arm: Updates for extended regions support

 docs/misc/arm/device-tree/guest.txt |  12 ++--
 tools/include/libxl.h               |   5 ++
 tools/libs/light/libxl_arm.c        |  82 +++++++++++++++++++++++++--
 tools/libs/light/libxl_types.idl    |   1 +
 xen/arch/arm/domain.c               |   6 ++
 xen/arch/arm/domain_build.c         | 108 ++++++++++++++++++++++--------------
 xen/include/public/arch-arm.h       |   7 +++
 xen/include/public/domctl.h         |   2 +-
 8 files changed, 170 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to pass info about maximum supported guest physical
address space size to the toolstack on Arm in order to properly
calculate the base and size of the extended region (safe range)
for the guest. The extended region is unused address space which
could be safely used by domain for foreign/grant mappings on Arm.
The extended region itself will be handled by the subsequent
patch.

Currently the same guest physical address space size is used
for all guests.

As we add new field to the structure bump the interface version.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes RFC -> V2:
   - update patch subject/description
   - replace arch-specific sub-struct with common gpaddr_bits
     field and update code to reflect that

Changes V2 -> V3:
   - make the field uint8_t and add uint8_t pad[7] after
   - remove leading blanks in libxl.h

Changes V3 -> V4:
   - also print gpaddr_bits from output_physinfo()
   - add Michal's R-b

Changes V4 -> V5:
   - update patch subject and description
   - drop Michal's R-b
   - pass gpaddr_bits via createdomain domctl
     (struct xen_arch_domainconfig)
---
 tools/include/libxl.h            | 5 +++++
 tools/libs/light/libxl_arm.c     | 2 ++
 tools/libs/light/libxl_types.idl | 1 +
 xen/arch/arm/domain.c            | 6 ++++++
 xen/include/public/arch-arm.h    | 5 +++++
 xen/include/public/domctl.h      | 2 +-
 6 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d..33b4bfb 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -279,6 +279,11 @@
 #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
 
 /*
+ * libxl_domain_build_info has the gpaddr_bits field.
+ */
+#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1
+
+/*
  * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
  * 'soft reset' for domains and there is 'soft_reset' shutdown reason
  * in enum libxl_shutdown_reason.
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6..45e0386 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
 
     state->clock_frequency = config->arch.clock_frequency;
 
+    d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits;
+
     return 0;
 }
 
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff6..39482db 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
                                ("vuart", libxl_vuart_type),
+                               ("gpaddr_bits", uint8),
                               ])),
     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
                               ])),
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 19c756a..dfecc45 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d,
     if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
         goto fail;
 
+    /*
+     * Pass maximum IPA bits to the toolstack, currently the same guest
+     * physical address space size is used for all guests.
+     */
+    config->arch.gpaddr_bits = p2m_ipa_bits;
+
     return 0;
 
 fail:
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6b5a5f8..4a01f8b 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
      *
      */
     uint32_t clock_frequency;
+    /*
+     * OUT
+     * Guest physical address space size
+     */
+    uint8_t gpaddr_bits;
 };
 #endif /* __XEN__ || __XEN_TOOLS__ */
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 96696e3..f37586e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
-- 
2.7.4

[PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The extended region (safe range) is a region of guest physical
address space which is unused and could be safely used to create
grant/foreign mappings instead of wasting real RAM pages from
the domain memory for establishing these mappings.

The extended regions are chosen at the domain creation time and
advertised to it via "reg" property under hypervisor node in
the guest device-tree. As region 0 is reserved for grant table
space (always present), the indexes for extended regions are 1...N.
If extended regions could not be allocated for some reason,
Xen doesn't fail and behaves as usual, so only inserts region 0.

Please note the following limitations:
- The extended region feature is only supported for 64-bit domain
  currently.
- The ACPI case is not covered.

***

The algorithm to choose extended regions for non-direct mapped
DomU is simpler in comparison with the algorithm for direct mapped
Dom0. We usually have a lot of unused space above 4GB, and might
have some unused space below 4GB (depends on guest memory size).
Try to allocate separate 2MB-aligned extended regions from the first
(below 4GB) and second (above 4GB) RAM banks taking into the account
the maximum supported guest physical address space size and the amount
of memory assigned to the guest. The minimum size of extended region
the same as for Dom0 (64MB).

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
! Stefano, Ian, Michal I dropped your A-b/R-b. I decided to change patch
to make it more functional !

Changes RFC -> V2:
   - update patch description
   - drop uneeded "extended-region" DT property
   - clear reg array in finalise_ext_region() and add a TODO

Changes V2 -> V3:
   - update patch description, comments in code
   - only pick up regions with size >= 64MB
   - move the region calculation to make_hypervisor_node() and drop
     finalise_ext_region()
   - extend the list of arguments for make_hypervisor_node()
   - do not show warning for 32-bit domain
   - change the region alignment from 1GB to 2MB
   - move EXT_REGION_SIZE to public/arch-arm.h

Changes V3 -> V4:
   - add R-b, A-b and T-b

Changes V4 -> V5:
   - update patch description and comments in code
   - reflect changes done in previous patch to pass gpaddr_bits
     via createdomain domctl (struct xen_arch_domainconfig)
   - drop R-b, A-b and T-b
   - drop limit for maximum extended region size (128GB)
   - try to also allocate region below 4GB, optimize code
     for calculating extended regions
---
 tools/libs/light/libxl_arm.c  | 80 ++++++++++++++++++++++++++++++++++++++++---
 xen/include/public/arch-arm.h |  2 ++
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 45e0386..cd743f7 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -600,9 +600,21 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
+
 static int make_hypervisor_node(libxl__gc *gc, void *fdt,
-                                const libxl_version_info *vers)
+                                const libxl_version_info *vers,
+                                const libxl_domain_build_info *b_info,
+                                const struct xc_dom_image *dom)
 {
+    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
+        banksize[GUEST_RAM_BANKS], bankend[GUEST_RAM_BANKS], ramsize;
+    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+                  (GUEST_RAM_BANKS + 1)];
+    be32 *cells = &regs[0];
+    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+    unsigned int i, len, nr_regions = 0;
+    uint8_t gpaddr_bits;
     int res;
     gic_interrupt intr;
 
@@ -617,9 +629,67 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
                               "xen,xen");
     if (res) return res;
 
-    /* reg 0 is grant table space */
-    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
+        LOG(WARN, "The extended regions are only supported for 64-bit guest currently");
+        goto out;
+    }
+
+    gpaddr_bits = b_info->arch_arm.gpaddr_bits;
+    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
+
+    /*
+     * Try to allocate separate 2MB-aligned extended regions from the first
+     * (below 4GB) and second (above 4GB) RAM banks taking into the account
+     * the maximum supported guest physical address space size and the amount
+     * of memory assigned to the guest.
+     * As the guest memory layout is not populated yet we cannot rely on
+     * dom->rambank_size[], so calculate the actual size of both banks using
+     * "max_memkb" value.
+     */
+    ramsize = b_info->max_memkb * 1024;
+    if (ramsize <= GUEST_RAM0_SIZE) {
+        banksize[0] = ramsize;
+        banksize[1] = 0;
+    } else {
+        banksize[0] = GUEST_RAM0_SIZE;
+        banksize[1] = ramsize - GUEST_RAM0_SIZE;
+    }
+
+    bankend[0] = GUEST_RAM0_BASE + GUEST_RAM0_SIZE;
+    bankend[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
+
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        region_base[i] = bankbase[i] + ALIGN_UP_TO_2MB(banksize[i]);
+        if (bankend[i] > region_base[i])
+            region_size[i] = bankend[i] - region_base[i];
+    }
+
+out:
+    /*
+     * The region 0 for grant table space must be always present. If we managed
+     * to allocate the extended regions then insert them as regions 1...N.
+     */
+    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
+            continue;
+
+        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
+            nr_regions, region_base[i], region_base[i] + region_size[i]);
+
+        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                  region_base[i], region_size[i]);
+        nr_regions ++;
+    }
+
+    if (!nr_regions)
+        LOG(WARN, "The extended regions cannot be allocated, not enough space");
+
+    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+        (nr_regions + 1);
+    res = fdt_property(fdt, "reg", regs, len);
     if (res) return res;
 
     /*
@@ -965,7 +1035,7 @@ next_resize:
         }
 
         FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
-        FDT( make_hypervisor_node(gc, fdt, vers) );
+        FDT( make_hypervisor_node(gc, fdt, vers, info, dom) );
 
         if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
             FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 4a01f8b..f74cc0b 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -454,6 +454,8 @@ typedef uint64_t xen_callback_t;
 #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
 #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
 
+#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
+
 /* Current supported guest VCPUs */
 #define GUEST_MAX_VCPUS 128
 
-- 
2.7.4

[PATCH V5 3/3] xen/arm: Updates for extended regions support

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This is a follow-up of
"b6fe410 xen/arm: Add handling of extended regions for Dom0"

Add various in-code comments, update Xen hypervisor device tree
bindings text, change the log level for some prints and clarify
format specifier, reuse dt_for_each_range() to avoid open-coding
in find_memory_holes().

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
   New patch
---
 docs/misc/arm/device-tree/guest.txt |  12 ++--
 xen/arch/arm/domain_build.c         | 108 ++++++++++++++++++++++--------------
 2 files changed, 73 insertions(+), 47 deletions(-)

diff --git a/docs/misc/arm/device-tree/guest.txt b/docs/misc/arm/device-tree/guest.txt
index 418f1e9..c115751 100644
--- a/docs/misc/arm/device-tree/guest.txt
+++ b/docs/misc/arm/device-tree/guest.txt
@@ -7,10 +7,14 @@ the following properties:
 	compatible = "xen,xen-<version>", "xen,xen";
   where <version> is the version of the Xen ABI of the platform.
 
-- reg: specifies the base physical address and size of a region in
-  memory where the grant table should be mapped to, using an
-  HYPERVISOR_memory_op hypercall. The memory region is large enough to map
-  the whole grant table (it is larger or equal to gnttab_max_grant_frames()).
+- reg: specifies the base physical address and size of the regions in memory
+  where the special resources should be mapped to, using an HYPERVISOR_memory_op
+  hypercall.
+  Region 0 is reserved for mapping grant table, it must be always present.
+  The memory region is large enough to map the whole grant table (it is larger
+  or equal to gnttab_max_grant_frames()).
+  Regions 1...N are extended regions (unused address space) for mapping foreign
+  GFNs and grants, they might be absent if there is nothing to expose.
   This property is unnecessary when booting Dom0 using ACPI.
 
 - interrupts: the interrupt used by Xen to inject event notifications.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c5afbe2..d9f40d4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -898,7 +898,10 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
     if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
         return 0;
 
-    /* Both start and size of the extended region should be 2MB aligned */
+    /*
+     * Both start and size of the extended region should be 2MB aligned to
+     * potentially allow superpage mapping.
+     */
     start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
     if ( start > e )
         return 0;
@@ -909,6 +912,12 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
      */
     e += 1;
     size = (e - start) & ~(SZ_2M - 1);
+
+    /*
+     * Reasonable size. Not too little to pick up small ranges which are
+     * not quite useful itself but increase bookkeeping and not too much
+     * to skip a large proportion of unused address space.
+     */
     if ( size < MB(64) )
         return 0;
 
@@ -919,6 +928,14 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
     return 0;
 }
 
+/*
+ * Find unused regions of Host address space which can be exposed to Dom0
+ * as extended regions for the special memory mappings. In order to calculate
+ * regions we exclude every assigned to Dom0 region from the Host RAM:
+ * - domain RAM
+ * - reserved-memory
+ * - grant table space
+ */
 static int __init find_unallocated_memory(const struct kernel_info *kinfo,
                                           struct meminfo *ext_regions)
 {
@@ -942,7 +959,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
         res = rangeset_add_range(unalloc_mem, start, end - 1);
         if ( res )
         {
-            printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
+            printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
                    start, end);
             goto out;
         }
@@ -956,7 +973,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
         res = rangeset_remove_range(unalloc_mem, start, end - 1);
         if ( res )
         {
-            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+            printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
                    start, end);
             goto out;
         }
@@ -971,7 +988,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
         res = rangeset_remove_range(unalloc_mem, start, end - 1);
         if ( res )
         {
-            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+            printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
                    start, end);
             goto out;
         }
@@ -983,7 +1000,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
     res = rangeset_remove_range(unalloc_mem, start, end - 1);
     if ( res )
     {
-        printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+        printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
                start, end);
         goto out;
     }
@@ -1003,6 +1020,35 @@ out:
     return res;
 }
 
+static int __init handle_pci_range(const struct dt_device_node *dev,
+                                   u64 addr, u64 len, void *data)
+{
+    struct rangeset *mem_holes = data;
+    paddr_t start, end;
+    int res;
+
+    start = addr & PAGE_MASK;
+    end = PAGE_ALIGN(addr + len);
+    res = rangeset_remove_range(mem_holes, start, end - 1);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
+               start, end);
+        return res;
+    }
+
+    return 0;
+}
+
+/*
+ * Find the holes in the Host DT which can be exposed to Dom0 as extended
+ * regions for the special memory mappings. In order to calculate regions
+ * we exclude every addressable memory region described by "reg" and "ranges"
+ * properties from the maximum possible addressable physical memory range:
+ * - MMIO
+ * - Host RAM
+ * - PCI bar
+ */
 static int __init find_memory_holes(const struct kernel_info *kinfo,
                                     struct meminfo *ext_regions)
 {
@@ -1024,7 +1070,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
     res = rangeset_add_range(mem_holes, start, end);
     if ( res )
     {
-        printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
+        printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
                start, end);
         goto out;
     }
@@ -1055,49 +1101,25 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
             res = rangeset_remove_range(mem_holes, start, end - 1);
             if ( res )
             {
-                printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+                printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
                        start, end);
                 goto out;
             }
         }
 
-        if ( dt_device_type_is_equal(np, "pci" ) )
+        if ( dt_device_type_is_equal(np, "pci") )
         {
-            unsigned int range_size, nr_ranges;
-            int na, ns, pna;
-            const __be32 *ranges;
-            u32 len;
-
             /*
-             * Looking for non-empty ranges property which in this context
-             * describes the PCI host bridge aperture.
+             * The ranges property in this context describes the PCI host
+             * bridge aperture. It shall be absent if no addresses are mapped
+             * through the bridge.
              */
-            ranges = dt_get_property(np, "ranges", &len);
-            if ( !ranges || !len )
+            if ( !dt_get_property(np, "ranges", NULL) )
                 continue;
 
-            pna = dt_n_addr_cells(np);
-            na = dt_child_n_addr_cells(np);
-            ns = dt_child_n_size_cells(np);
-            range_size = pna + na + ns;
-            nr_ranges = len / sizeof(__be32) / range_size;
-
-            for ( i = 0; i < nr_ranges; i++, ranges += range_size )
-            {
-                /* Skip the child address and get the parent (CPU) address */
-                addr = dt_read_number(ranges + na, pna);
-                size = dt_read_number(ranges + na + pna, ns);
-
-                start = addr & PAGE_MASK;
-                end = PAGE_ALIGN(addr + size);
-                res = rangeset_remove_range(mem_holes, start, end - 1);
-                if ( res )
-                {
-                    printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
-                           start, end);
-                    goto out;
-                }
-            }
+            res = dt_for_each_range(np, &handle_pci_range, mem_holes);
+            if ( res )
+                goto out;
         }
     }
 
@@ -1152,12 +1174,12 @@ static int __init make_hypervisor_node(struct domain *d,
 
     if ( !opt_ext_regions )
     {
-        printk(XENLOG_DEBUG "The extended regions support is disabled\n");
+        printk(XENLOG_INFO "The extended regions support is disabled\n");
         nr_ext_regions = 0;
     }
     else if ( is_32bit_domain(d) )
     {
-        printk(XENLOG_DEBUG "The extended regions are only supported for 64-bit guest currently\n");
+        printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n");
         nr_ext_regions = 0;
     }
     else
@@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain *d,
         u64 start = ext_regions->bank[i].start;
         u64 size = ext_regions->bank[i].size;
 
-        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
-                   i, start, start + size);
+        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
+               i, start, start + size);
 
         dt_child_set_range(&cells, addrcells, sizecells, start, size);
     }
-- 
2.7.4

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Ian Jackson]

Oleksandr Tyshchenko writes ("[PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU"):
> The extended region (safe range) is a region of guest physical
> address space which is unused and could be safely used to create
> grant/foreign mappings instead of wasting real RAM pages from
> the domain memory for establishing these mappings.

Please forgive me for asking this question now, but: why is this
ARM-specific ?

Ian.

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Oleksandr]

On 06.10.21 14:34, Ian Jackson wrote:

Hi Ian

> Oleksandr Tyshchenko writes ("[PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU"):
>> The extended region (safe range) is a region of guest physical
>> address space which is unused and could be safely used to create
>> grant/foreign mappings instead of wasting real RAM pages from
>> the domain memory for establishing these mappings.
> Please forgive me for asking this question now, but: why is this
> ARM-specific ?


Sorry, I can't say for sure which x86 mode also suffers from that. I 
might be wrong, but as I understand that x86 in PVH (and HVM?) mode uses 
unpopulated memory ranges (which are unused from Linux PoV, actually 
everything not yet allocated or reserved from "iomem_resource") to 
create foreign/grant mappings. So the real RAM pages are not ballooned 
out to get an physical address space to create these mappings. The 
problem is that we cannot follow Linux advise which memory ranges are 
unused on Arm for several reasons, this is why this patch series makes 
the hypervisor to start allocating and exposing these ranges.

>
> Ian.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Stefano Stabellini]

On Wed, 6 Oct 2021, Oleksandr wrote:
> On 06.10.21 14:34, Ian Jackson wrote:
> 
> Hi Ian
> 
> > Oleksandr Tyshchenko writes ("[PATCH V5 2/3] libxl/arm: Add handling of
> > extended regions for DomU"):
> > > The extended region (safe range) is a region of guest physical
> > > address space which is unused and could be safely used to create
> > > grant/foreign mappings instead of wasting real RAM pages from
> > > the domain memory for establishing these mappings.
> > Please forgive me for asking this question now, but: why is this
> > ARM-specific ?
> 
> 
> Sorry, I can't say for sure which x86 mode also suffers from that. I might be
> wrong, but as I understand that x86 in PVH (and HVM?) mode uses unpopulated
> memory ranges (which are unused from Linux PoV, actually everything not yet
> allocated or reserved from "iomem_resource") to create foreign/grant mappings.
> So the real RAM pages are not ballooned out to get an physical address space
> to create these mappings. The problem is that we cannot follow Linux advise
> which memory ranges are unused on Arm for several reasons, this is why this
> patch series makes the hypervisor to start allocating and exposing these
> ranges.

Two more things about this being ARM-specific.

Even if x86 was affected exactly by the same problem, the code to expose
the safe memory ranges to DomU is arch-specific (currently device tree.)

Also the code to calculate the safe memory ranges is arch-specific as it
depends on the DomU memory layout which is arch-specific.

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Stefano Stabellini]

On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We need to pass info about maximum supported guest physical
> address space size to the toolstack on Arm in order to properly
> calculate the base and size of the extended region (safe range)
> for the guest. The extended region is unused address space which
> could be safely used by domain for foreign/grant mappings on Arm.
> The extended region itself will be handled by the subsequent
> patch.
> 
> Currently the same guest physical address space size is used
> for all guests.
> 
> As we add new field to the structure bump the interface version.
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> Changes RFC -> V2:
>    - update patch subject/description
>    - replace arch-specific sub-struct with common gpaddr_bits
>      field and update code to reflect that
> 
> Changes V2 -> V3:
>    - make the field uint8_t and add uint8_t pad[7] after
>    - remove leading blanks in libxl.h
> 
> Changes V3 -> V4:
>    - also print gpaddr_bits from output_physinfo()
>    - add Michal's R-b
> 
> Changes V4 -> V5:
>    - update patch subject and description
>    - drop Michal's R-b
>    - pass gpaddr_bits via createdomain domctl
>      (struct xen_arch_domainconfig)
> ---
>  tools/include/libxl.h            | 5 +++++
>  tools/libs/light/libxl_arm.c     | 2 ++
>  tools/libs/light/libxl_types.idl | 1 +
>  xen/arch/arm/domain.c            | 6 ++++++
>  xen/include/public/arch-arm.h    | 5 +++++
>  xen/include/public/domctl.h      | 2 +-
>  6 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index b9ba16d..33b4bfb 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -279,6 +279,11 @@
>  #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
>  
>  /*
> + * libxl_domain_build_info has the gpaddr_bits field.
> + */
> +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1
> +
> +/*
>   * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
>   * 'soft reset' for domains and there is 'soft_reset' shutdown reason
>   * in enum libxl_shutdown_reason.
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6..45e0386 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
>  
>      state->clock_frequency = config->arch.clock_frequency;
>  
> +    d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits;
> +
>      return 0;
>  }
>  
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 3f9fff6..39482db 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>  
>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>                                 ("vuart", libxl_vuart_type),
> +                               ("gpaddr_bits", uint8),
>                                ])),
>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                                ])),
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 19c756a..dfecc45 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d,
>      if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
>          goto fail;
>  
> +    /*
> +     * Pass maximum IPA bits to the toolstack, currently the same guest
> +     * physical address space size is used for all guests.
> +     */
> +    config->arch.gpaddr_bits = p2m_ipa_bits;

This could also be set in arch_sanitise_domain_config together with
config->arch.gic_version. I prefer if it was done in
arch_sanitise_domain_config but also here is OK I think.

Given that everything else looks fine:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>      return 0;
>  
>  fail:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 6b5a5f8..4a01f8b 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>       *
>       */
>      uint32_t clock_frequency;
> +    /*
> +     * OUT
> +     * Guest physical address space size
> +     */
> +    uint8_t gpaddr_bits;
>  };
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>  
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 96696e3..f37586e 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> -- 
> 2.7.4
> 

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Stefano Stabellini]

On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> The extended region (safe range) is a region of guest physical
> address space which is unused and could be safely used to create
> grant/foreign mappings instead of wasting real RAM pages from
> the domain memory for establishing these mappings.
> 
> The extended regions are chosen at the domain creation time and
> advertised to it via "reg" property under hypervisor node in
> the guest device-tree. As region 0 is reserved for grant table
> space (always present), the indexes for extended regions are 1...N.
> If extended regions could not be allocated for some reason,
> Xen doesn't fail and behaves as usual, so only inserts region 0.
> 
> Please note the following limitations:
> - The extended region feature is only supported for 64-bit domain
>   currently.
> - The ACPI case is not covered.
> 
> ***
> 
> The algorithm to choose extended regions for non-direct mapped
> DomU is simpler in comparison with the algorithm for direct mapped
> Dom0. We usually have a lot of unused space above 4GB, and might
> have some unused space below 4GB (depends on guest memory size).
> Try to allocate separate 2MB-aligned extended regions from the first
> (below 4GB) and second (above 4GB) RAM banks taking into the account
> the maximum supported guest physical address space size and the amount
> of memory assigned to the guest. The minimum size of extended region
> the same as for Dom0 (64MB).
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> ! Stefano, Ian, Michal I dropped your A-b/R-b. I decided to change patch
> to make it more functional !
> 
> Changes RFC -> V2:
>    - update patch description
>    - drop uneeded "extended-region" DT property
>    - clear reg array in finalise_ext_region() and add a TODO
> 
> Changes V2 -> V3:
>    - update patch description, comments in code
>    - only pick up regions with size >= 64MB
>    - move the region calculation to make_hypervisor_node() and drop
>      finalise_ext_region()
>    - extend the list of arguments for make_hypervisor_node()
>    - do not show warning for 32-bit domain
>    - change the region alignment from 1GB to 2MB
>    - move EXT_REGION_SIZE to public/arch-arm.h
> 
> Changes V3 -> V4:
>    - add R-b, A-b and T-b
> 
> Changes V4 -> V5:
>    - update patch description and comments in code
>    - reflect changes done in previous patch to pass gpaddr_bits
>      via createdomain domctl (struct xen_arch_domainconfig)
>    - drop R-b, A-b and T-b
>    - drop limit for maximum extended region size (128GB)
>    - try to also allocate region below 4GB, optimize code
>      for calculating extended regions
> ---
>  tools/libs/light/libxl_arm.c  | 80 ++++++++++++++++++++++++++++++++++++++++---
>  xen/include/public/arch-arm.h |  2 ++
>  2 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 45e0386..cd743f7 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -600,9 +600,21 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> +
>  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
> -                                const libxl_version_info *vers)
> +                                const libxl_version_info *vers,
> +                                const libxl_domain_build_info *b_info,
> +                                const struct xc_dom_image *dom)
>  {
> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
> +        banksize[GUEST_RAM_BANKS], bankend[GUEST_RAM_BANKS], ramsize;
> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +                  (GUEST_RAM_BANKS + 1)];
> +    be32 *cells = &regs[0];
> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    unsigned int i, len, nr_regions = 0;
> +    uint8_t gpaddr_bits;
>      int res;
>      gic_interrupt intr;
>  
> @@ -617,9 +629,67 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>                                "xen,xen");
>      if (res) return res;
>  
> -    /* reg 0 is grant table space */
> -    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> +        LOG(WARN, "The extended regions are only supported for 64-bit guest currently");
> +        goto out;
> +    }
> +
> +    gpaddr_bits = b_info->arch_arm.gpaddr_bits;
> +    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
> +
> +    /*
> +     * Try to allocate separate 2MB-aligned extended regions from the first
> +     * (below 4GB) and second (above 4GB) RAM banks taking into the account
> +     * the maximum supported guest physical address space size and the amount
> +     * of memory assigned to the guest.
> +     * As the guest memory layout is not populated yet we cannot rely on
> +     * dom->rambank_size[], so calculate the actual size of both banks using
> +     * "max_memkb" value.
> +     */
> +    ramsize = b_info->max_memkb * 1024;
> +    if (ramsize <= GUEST_RAM0_SIZE) {
> +        banksize[0] = ramsize;
> +        banksize[1] = 0;
> +    } else {
> +        banksize[0] = GUEST_RAM0_SIZE;
> +        banksize[1] = ramsize - GUEST_RAM0_SIZE;
> +    }
> +
> +    bankend[0] = GUEST_RAM0_BASE + GUEST_RAM0_SIZE;
> +    bankend[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
> +
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        region_base[i] = bankbase[i] + ALIGN_UP_TO_2MB(banksize[i]);
> +        if (bankend[i] > region_base[i])
> +            region_size[i] = bankend[i] - region_base[i];
> +    }

This seems correct but it looks a bit overkill. I would have written
like this:

    if (ramsize <= GUEST_RAM0_SIZE) {
        region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
        region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
        region_base[1] = GUEST_RAM1_BASE;
        region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) -
                         region_base[0];
    } else {
        region_size[0] = 0;
        region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
        region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) -
                         region_base[1];
    }

Which removes the needs for banksize, bankend, bankbase. What do you
think? Your version works too, so I am OK anyway.



> +out:
> +    /*
> +     * The region 0 for grant table space must be always present. If we managed
> +     * to allocate the extended regions then insert them as regions 1...N.
> +     */
> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
> +            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +
> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                  region_base[i], region_size[i]);
> +        nr_regions ++;
                     ^ code style


> +    }
> +
> +    if (!nr_regions)
> +        LOG(WARN, "The extended regions cannot be allocated, not enough space");
> +
> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +        (nr_regions + 1);
> +    res = fdt_property(fdt, "reg", regs, len);
>      if (res) return res;
>  
>      /*
> @@ -965,7 +1035,7 @@ next_resize:
>          }
>  
>          FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
> -        FDT( make_hypervisor_node(gc, fdt, vers) );
> +        FDT( make_hypervisor_node(gc, fdt, vers, info, dom) );
>  
>          if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
>              FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 4a01f8b..f74cc0b 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -454,6 +454,8 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>  #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>  
> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
> +
>  /* Current supported guest VCPUs */
>  #define GUEST_MAX_VCPUS 128
>  
> -- 
> 2.7.4
> 

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Stefano Stabellini]

On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This is a follow-up of
> "b6fe410 xen/arm: Add handling of extended regions for Dom0"
> 
> Add various in-code comments, update Xen hypervisor device tree
> bindings text, change the log level for some prints and clarify
> format specifier, reuse dt_for_each_range() to avoid open-coding
> in find_memory_holes().
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Thanks for the patch, it looks like you addressed all Julien's comments
well. A couple of minor issues below.


> ---
>    New patch
> ---
>  docs/misc/arm/device-tree/guest.txt |  12 ++--
>  xen/arch/arm/domain_build.c         | 108 ++++++++++++++++++++++--------------
>  2 files changed, 73 insertions(+), 47 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/guest.txt b/docs/misc/arm/device-tree/guest.txt
> index 418f1e9..c115751 100644
> --- a/docs/misc/arm/device-tree/guest.txt
> +++ b/docs/misc/arm/device-tree/guest.txt
> @@ -7,10 +7,14 @@ the following properties:
>  	compatible = "xen,xen-<version>", "xen,xen";
>    where <version> is the version of the Xen ABI of the platform.
>  
> -- reg: specifies the base physical address and size of a region in
> -  memory where the grant table should be mapped to, using an
> -  HYPERVISOR_memory_op hypercall. The memory region is large enough to map
> -  the whole grant table (it is larger or equal to gnttab_max_grant_frames()).
> +- reg: specifies the base physical address and size of the regions in memory
> +  where the special resources should be mapped to, using an HYPERVISOR_memory_op
> +  hypercall.
> +  Region 0 is reserved for mapping grant table, it must be always present.
> +  The memory region is large enough to map the whole grant table (it is larger
> +  or equal to gnttab_max_grant_frames()).
> +  Regions 1...N are extended regions (unused address space) for mapping foreign
> +  GFNs and grants, they might be absent if there is nothing to expose.
>    This property is unnecessary when booting Dom0 using ACPI.
>  
>  - interrupts: the interrupt used by Xen to inject event notifications.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c5afbe2..d9f40d4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -898,7 +898,10 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
>      if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
>          return 0;
>  
> -    /* Both start and size of the extended region should be 2MB aligned */
> +    /*
> +     * Both start and size of the extended region should be 2MB aligned to
> +     * potentially allow superpage mapping.
> +     */
>      start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
>      if ( start > e )
>          return 0;
> @@ -909,6 +912,12 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
>       */
>      e += 1;
>      size = (e - start) & ~(SZ_2M - 1);
> +
> +    /*
> +     * Reasonable size. Not too little to pick up small ranges which are
> +     * not quite useful itself but increase bookkeeping and not too much
                           ^ remove itself                             ^ large

> +     * to skip a large proportion of unused address space.
> +     */
>      if ( size < MB(64) )
>          return 0;
>  
> @@ -919,6 +928,14 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
>      return 0;
>  }
>  
> +/*
> + * Find unused regions of Host address space which can be exposed to Dom0
> + * as extended regions for the special memory mappings. In order to calculate
> + * regions we exclude every assigned to Dom0 region from the Host RAM:
                              ^ region assigned  ^ remove


> + * - domain RAM
> + * - reserved-memory
> + * - grant table space
> + */
>  static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>                                            struct meminfo *ext_regions)
>  {
> @@ -942,7 +959,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>          res = rangeset_add_range(unalloc_mem, start, end - 1);
>          if ( res )
>          {
> -            printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
> +            printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
>                     start, end);
>              goto out;
>          }
> @@ -956,7 +973,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>          res = rangeset_remove_range(unalloc_mem, start, end - 1);
>          if ( res )
>          {
> -            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
> +            printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>                     start, end);
>              goto out;
>          }
> @@ -971,7 +988,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>          res = rangeset_remove_range(unalloc_mem, start, end - 1);
>          if ( res )
>          {
> -            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
> +            printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>                     start, end);
>              goto out;
>          }
> @@ -983,7 +1000,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>      res = rangeset_remove_range(unalloc_mem, start, end - 1);
>      if ( res )
>      {
> -        printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
> +        printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>                 start, end);
>          goto out;
>      }
> @@ -1003,6 +1020,35 @@ out:
>      return res;
>  }
>  
> +static int __init handle_pci_range(const struct dt_device_node *dev,
> +                                   u64 addr, u64 len, void *data)
> +{
> +    struct rangeset *mem_holes = data;
> +    paddr_t start, end;
> +    int res;
> +
> +    start = addr & PAGE_MASK;
> +    end = PAGE_ALIGN(addr + len);
> +    res = rangeset_remove_range(mem_holes, start, end - 1);
> +    if ( res )
> +    {
> +        printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
> +               start, end);
> +        return res;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Find the holes in the Host DT which can be exposed to Dom0 as extended
> + * regions for the special memory mappings. In order to calculate regions
> + * we exclude every addressable memory region described by "reg" and "ranges"
> + * properties from the maximum possible addressable physical memory range:
> + * - MMIO
> + * - Host RAM
> + * - PCI bar
        ^ PCI aperture


> + */
>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>                                      struct meminfo *ext_regions)
>  {
> @@ -1024,7 +1070,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>      res = rangeset_add_range(mem_holes, start, end);
>      if ( res )
>      {
> -        printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
> +        printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
>                 start, end);
>          goto out;
>      }
> @@ -1055,49 +1101,25 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>              res = rangeset_remove_range(mem_holes, start, end - 1);
>              if ( res )
>              {
> -                printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
> +                printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>                         start, end);
>                  goto out;
>              }
>          }
>  
> -        if ( dt_device_type_is_equal(np, "pci" ) )
> +        if ( dt_device_type_is_equal(np, "pci") )
>          {
> -            unsigned int range_size, nr_ranges;
> -            int na, ns, pna;
> -            const __be32 *ranges;
> -            u32 len;
> -
>              /*
> -             * Looking for non-empty ranges property which in this context
> -             * describes the PCI host bridge aperture.
> +             * The ranges property in this context describes the PCI host
> +             * bridge aperture. It shall be absent if no addresses are mapped
> +             * through the bridge.
>               */
> -            ranges = dt_get_property(np, "ranges", &len);
> -            if ( !ranges || !len )
> +            if ( !dt_get_property(np, "ranges", NULL) )
>                  continue;
>  
> -            pna = dt_n_addr_cells(np);
> -            na = dt_child_n_addr_cells(np);
> -            ns = dt_child_n_size_cells(np);
> -            range_size = pna + na + ns;
> -            nr_ranges = len / sizeof(__be32) / range_size;
> -
> -            for ( i = 0; i < nr_ranges; i++, ranges += range_size )
> -            {
> -                /* Skip the child address and get the parent (CPU) address */
> -                addr = dt_read_number(ranges + na, pna);
> -                size = dt_read_number(ranges + na + pna, ns);
> -
> -                start = addr & PAGE_MASK;
> -                end = PAGE_ALIGN(addr + size);
> -                res = rangeset_remove_range(mem_holes, start, end - 1);
> -                if ( res )
> -                {
> -                    printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
> -                           start, end);
> -                    goto out;
> -                }
> -            }
> +            res = dt_for_each_range(np, &handle_pci_range, mem_holes);
> +            if ( res )
> +                goto out;
>          }
>      }
>  
> @@ -1152,12 +1174,12 @@ static int __init make_hypervisor_node(struct domain *d,
>  
>      if ( !opt_ext_regions )
>      {
> -        printk(XENLOG_DEBUG "The extended regions support is disabled\n");
> +        printk(XENLOG_INFO "The extended regions support is disabled\n");
>          nr_ext_regions = 0;
>      }
>      else if ( is_32bit_domain(d) )
>      {
> -        printk(XENLOG_DEBUG "The extended regions are only supported for 64-bit guest currently\n");
> +        printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n");
>          nr_ext_regions = 0;
>      }
>      else
> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain *d,
>          u64 start = ext_regions->bank[i].start;
>          u64 size = ext_regions->bank[i].size;
>  
> -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> -                   i, start, start + size);
> +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> +               i, start, start + size);

Also should be PRIpaddr

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Jan Beulich]

On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
> Changes V4 -> V5:
>    - update patch subject and description
>    - drop Michal's R-b
>    - pass gpaddr_bits via createdomain domctl
>      (struct xen_arch_domainconfig)

I'm afraid I can't bring this in line with ...

> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>       *
>       */
>      uint32_t clock_frequency;
> +    /*
> +     * OUT
> +     * Guest physical address space size
> +     */
> +    uint8_t gpaddr_bits;

... this being an OUT field. Is this really what Andrew had asked for?
I would have expected the entire struct to be IN (and the comment at
the top of the containing struct in public/domctl.h also suggests so,
i.e. your new field renders that comment stale). gic_version being
IN/OUT is already somewhat in conflict ... One of the problems with
_any_ of the fields being OUT is that then it is unclear how the output
is intended to be propagated to consumers other than the entity
creating the domain.

Jan

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU"):
> On Wed, 6 Oct 2021, Oleksandr wrote:
> > On 06.10.21 14:34, Ian Jackson wrote:
> > > Oleksandr Tyshchenko writes ("[PATCH V5 2/3] libxl/arm: Add handling of
> > > extended regions for DomU"):
> > > > The extended region (safe range) is a region of guest physical
> > > > address space which is unused and could be safely used to create
> > > > grant/foreign mappings instead of wasting real RAM pages from
> > > > the domain memory for establishing these mappings.
> > > Please forgive me for asking this question now, but: why is this
> > > ARM-specific ?
> > 
> > 
> > Sorry, I can't say for sure which x86 mode also suffers from
> > that. I might be wrong, but as I understand that x86 in PVH (and
> > HVM?) mode uses unpopulated memory ranges (which are unused from
> > Linux PoV, actually everything not yet allocated or reserved from
> > "iomem_resource") to create foreign/grant mappings.  So the real
> > RAM pages are not ballooned out to get an physical address space
> > to create these mappings. The problem is that we cannot follow
> > Linux advise which memory ranges are unused on Arm for several
> > reasons, this is why this patch series makes the hypervisor to
> > start allocating and exposing these ranges.

So it sounds like you are saying this is an ARM-specific problem ?
The key being the "several reasons" which you mention.  Are they
ARM-specifc problems.

> Two more things about this being ARM-specific.
> 
> Even if x86 was affected exactly by the same problem, the code to expose
> the safe memory ranges to DomU is arch-specific (currently device tree.)
> 
> Also the code to calculate the safe memory ranges is arch-specific as it
> depends on the DomU memory layout which is arch-specific.

This demonstrates that the implementation is arch-specific.  But one
of libxl's functions is to abstract away implementation details and
provide an interface that can be used to "do the right thing".

Ian.

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr]

On 07.10.21 10:42, Jan Beulich wrote:

Hi Jan.

> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
>> Changes V4 -> V5:
>>     - update patch subject and description
>>     - drop Michal's R-b
>>     - pass gpaddr_bits via createdomain domctl
>>       (struct xen_arch_domainconfig)
> I'm afraid I can't bring this in line with ...
>
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>>        *
>>        */
>>       uint32_t clock_frequency;
>> +    /*
>> +     * OUT
>> +     * Guest physical address space size
>> +     */
>> +    uint8_t gpaddr_bits;
> ... this being an OUT field. Is this really what Andrew had asked for?
> I would have expected the entire struct to be IN (and the comment at
> the top of the containing struct in public/domctl.h also suggests so,
> i.e. your new field renders that comment stale). gic_version being
> IN/OUT is already somewhat in conflict ...
I am sorry but I'm totally confused now, we want the Xen to provide 
gpaddr_bits to the toolstack, but not the other way around.
As I understand the main ask was to switch to domctl for which I wanted 
to get some clarification on how it would look like... Well, this patch 
switches to use
domctl, and I think exactly as it was suggested at [1] in case if a 
common one is a difficult to achieve. I have to admit, I felt it was 
indeed difficult to achieve.


I thought that a comment for struct xen_domctl_createdomain in 
public/domctl.h was rather related to the struct fields described in the 
public header than to the arch sub-struct internals described elsewhere. 
But if my assumption is incorrect, then yes the comment got stale and 
probably not by changes in current patch, but after adding 
clock_frequency field (OUT). If so we can add a comment on top of arch 
field clarifying that internal fields *might* be OUT.


> One of the problems with
> _any_ of the fields being OUT is that then it is unclear how the output
> is intended to be propagated to consumers other than the entity
> creating the domain.
If I *properly* understood your concern, we could hide that field in 
struct libxl__domain_build_state and not expose it to struct 
libxl_domain_build_info. Shall I?
[1] 
https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@gmail.com/


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Jan Beulich]

On 07.10.2021 14:30, Oleksandr wrote:
> On 07.10.21 10:42, Jan Beulich wrote:
>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
>>> Changes V4 -> V5:
>>>     - update patch subject and description
>>>     - drop Michal's R-b
>>>     - pass gpaddr_bits via createdomain domctl
>>>       (struct xen_arch_domainconfig)
>> I'm afraid I can't bring this in line with ...
>>
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>>>        *
>>>        */
>>>       uint32_t clock_frequency;
>>> +    /*
>>> +     * OUT
>>> +     * Guest physical address space size
>>> +     */
>>> +    uint8_t gpaddr_bits;
>> ... this being an OUT field. Is this really what Andrew had asked for?
>> I would have expected the entire struct to be IN (and the comment at
>> the top of the containing struct in public/domctl.h also suggests so,
>> i.e. your new field renders that comment stale). gic_version being
>> IN/OUT is already somewhat in conflict ...
> I am sorry but I'm totally confused now, we want the Xen to provide 
> gpaddr_bits to the toolstack, but not the other way around.
> As I understand the main ask was to switch to domctl for which I wanted 
> to get some clarification on how it would look like... Well, this patch 
> switches to use
> domctl, and I think exactly as it was suggested at [1] in case if a 
> common one is a difficult to achieve. I have to admit, I felt it was 
> indeed difficult to achieve.

Sadly the mail you reference isn't the one I was referring to. It's not
even from Andrew. Unfortunately I also can't seem to be able to locate
this, i.e. I'm now wondering whether this was under a different subject.
Julien, in any event, confirmed in a recent reply on this thread that
there was such a mail (otherwise I would have started wondering whether
the request was made on irc). In any case it is _that_ mail that would
need going through again.

> I thought that a comment for struct xen_domctl_createdomain in 
> public/domctl.h was rather related to the struct fields described in the 
> public header than to the arch sub-struct internals described elsewhere. 
> But if my assumption is incorrect, then yes the comment got stale and 
> probably not by changes in current patch, but after adding 
> clock_frequency field (OUT). If so we can add a comment on top of arch 
> field clarifying that internal fields *might* be OUT.
> 
> 
>> One of the problems with
>> _any_ of the fields being OUT is that then it is unclear how the output
>> is intended to be propagated to consumers other than the entity
>> creating the domain.
> If I *properly* understood your concern, we could hide that field in 
> struct libxl__domain_build_state and not expose it to struct 
> libxl_domain_build_info. Shall I?

I'm afraid I'm lost: I didn't talk about the tool stack at all. While
"consumer" generally means the tool stack, the remark was of more
abstract nature.

Jan

> [1] 
> https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@gmail.com/
> 
> 

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr]

On 07.10.21 15:43, Jan Beulich wrote:

Hi Jan.

> On 07.10.2021 14:30, Oleksandr wrote:
>> On 07.10.21 10:42, Jan Beulich wrote:
>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
>>>> Changes V4 -> V5:
>>>>      - update patch subject and description
>>>>      - drop Michal's R-b
>>>>      - pass gpaddr_bits via createdomain domctl
>>>>        (struct xen_arch_domainconfig)
>>> I'm afraid I can't bring this in line with ...
>>>
>>>> --- a/xen/include/public/arch-arm.h
>>>> +++ b/xen/include/public/arch-arm.h
>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>>>>         *
>>>>         */
>>>>        uint32_t clock_frequency;
>>>> +    /*
>>>> +     * OUT
>>>> +     * Guest physical address space size
>>>> +     */
>>>> +    uint8_t gpaddr_bits;
>>> ... this being an OUT field. Is this really what Andrew had asked for?
>>> I would have expected the entire struct to be IN (and the comment at
>>> the top of the containing struct in public/domctl.h also suggests so,
>>> i.e. your new field renders that comment stale). gic_version being
>>> IN/OUT is already somewhat in conflict ...
>> I am sorry but I'm totally confused now, we want the Xen to provide
>> gpaddr_bits to the toolstack, but not the other way around.
>> As I understand the main ask was to switch to domctl for which I wanted
>> to get some clarification on how it would look like... Well, this patch
>> switches to use
>> domctl, and I think exactly as it was suggested at [1] in case if a
>> common one is a difficult to achieve. I have to admit, I felt it was
>> indeed difficult to achieve.
> Sadly the mail you reference isn't the one I was referring to. It's not
> even from Andrew. Unfortunately I also can't seem to be able to locate
> this, i.e. I'm now wondering whether this was under a different subject.
> Julien, in any event, confirmed in a recent reply on this thread that
> there was such a mail (otherwise I would have started wondering whether
> the request was made on irc). In any case it is _that_ mail that would
> need going through again.

I think, this is the email [1] you are referring to. The subject was changed
to reflect changes in the particular version. This is the third 
proposition of this patch
(the first two were with arch and common field in sysctl).


>> I thought that a comment for struct xen_domctl_createdomain in
>> public/domctl.h was rather related to the struct fields described in the
>> public header than to the arch sub-struct internals described elsewhere.
>> But if my assumption is incorrect, then yes the comment got stale and
>> probably not by changes in current patch, but after adding
>> clock_frequency field (OUT). If so we can add a comment on top of arch
>> field clarifying that internal fields *might* be OUT.
>>
>>
>>> One of the problems with
>>> _any_ of the fields being OUT is that then it is unclear how the output
>>> is intended to be propagated to consumers other than the entity
>>> creating the domain.
>> If I *properly* understood your concern, we could hide that field in
>> struct libxl__domain_build_state and not expose it to struct
>> libxl_domain_build_info. Shall I?
> I'm afraid I'm lost: I didn't talk about the tool stack at all. While
> "consumer" generally means the tool stack, the remark was of more
> abstract nature.
>
> Jan
>
>> [1]
>> https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@gmail.com/
>>
>>
[1] 
https://lore.kernel.org/xen-devel/6a2a183d-c9d8-df2a-41aa-b25283fab197@gmail.com/


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Jan Beulich]

On 07.10.2021 15:12, Oleksandr wrote:
> 
> On 07.10.21 15:43, Jan Beulich wrote:
> 
> Hi Jan.
> 
>> On 07.10.2021 14:30, Oleksandr wrote:
>>> On 07.10.21 10:42, Jan Beulich wrote:
>>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
>>>>> Changes V4 -> V5:
>>>>>      - update patch subject and description
>>>>>      - drop Michal's R-b
>>>>>      - pass gpaddr_bits via createdomain domctl
>>>>>        (struct xen_arch_domainconfig)
>>>> I'm afraid I can't bring this in line with ...
>>>>
>>>>> --- a/xen/include/public/arch-arm.h
>>>>> +++ b/xen/include/public/arch-arm.h
>>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>>>>>         *
>>>>>         */
>>>>>        uint32_t clock_frequency;
>>>>> +    /*
>>>>> +     * OUT
>>>>> +     * Guest physical address space size
>>>>> +     */
>>>>> +    uint8_t gpaddr_bits;
>>>> ... this being an OUT field. Is this really what Andrew had asked for?
>>>> I would have expected the entire struct to be IN (and the comment at
>>>> the top of the containing struct in public/domctl.h also suggests so,
>>>> i.e. your new field renders that comment stale). gic_version being
>>>> IN/OUT is already somewhat in conflict ...
>>> I am sorry but I'm totally confused now, we want the Xen to provide
>>> gpaddr_bits to the toolstack, but not the other way around.
>>> As I understand the main ask was to switch to domctl for which I wanted
>>> to get some clarification on how it would look like... Well, this patch
>>> switches to use
>>> domctl, and I think exactly as it was suggested at [1] in case if a
>>> common one is a difficult to achieve. I have to admit, I felt it was
>>> indeed difficult to achieve.
>> Sadly the mail you reference isn't the one I was referring to. It's not
>> even from Andrew. Unfortunately I also can't seem to be able to locate
>> this, i.e. I'm now wondering whether this was under a different subject.
>> Julien, in any event, confirmed in a recent reply on this thread that
>> there was such a mail (otherwise I would have started wondering whether
>> the request was made on irc). In any case it is _that_ mail that would
>> need going through again.
> 
> I think, this is the email [1] you are referring to.

Well, that's still a mail you sent, not Andrew's. And while I have yours
in my mailbox, I don't have Andrew's for whatever reason.

Nevertheless there's enough context to be halfway certain that this
wasn't meant as an extension to the create domctl, but rather a separate
new one (merely replacing what you had originally as a sysctl to become
per-domain, to allow returning varying [between domains] values down the
road). I continue to think that if such a field was added to "create",
it would be an input (only).

Jan

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Oleksandr]

On 07.10.21 13:57, Ian Jackson wrote:

Hi Ian.

> Stefano Stabellini writes ("Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU"):
>> On Wed, 6 Oct 2021, Oleksandr wrote:
>>> On 06.10.21 14:34, Ian Jackson wrote:
>>>> Oleksandr Tyshchenko writes ("[PATCH V5 2/3] libxl/arm: Add handling of
>>>> extended regions for DomU"):
>>>>> The extended region (safe range) is a region of guest physical
>>>>> address space which is unused and could be safely used to create
>>>>> grant/foreign mappings instead of wasting real RAM pages from
>>>>> the domain memory for establishing these mappings.
>>>> Please forgive me for asking this question now, but: why is this
>>>> ARM-specific ?
>>>
>>> Sorry, I can't say for sure which x86 mode also suffers from
>>> that. I might be wrong, but as I understand that x86 in PVH (and
>>> HVM?) mode uses unpopulated memory ranges (which are unused from
>>> Linux PoV, actually everything not yet allocated or reserved from
>>> "iomem_resource") to create foreign/grant mappings.  So the real
>>> RAM pages are not ballooned out to get an physical address space
>>> to create these mappings. The problem is that we cannot follow
>>> Linux advise which memory ranges are unused on Arm for several
>>> reasons, this is why this patch series makes the hypervisor to
>>> start allocating and exposing these ranges.
> So it sounds like you are saying this is an ARM-specific problem ?
> The key being the "several reasons" which you mention.  Are they
> ARM-specifc problems.

Yes, you could say that. Below are reasons why we need the hypervisor to 
provide these ranges on Arm from my understanding.

[leaving aside hotplug case]

1. [Related to Dom0]  Dom0 is mapped 1:1 on Arm, but there might be some 
guest mapping, mapped identically in P2M (GFN == MFN) for PV drivers to 
work. So Xen has everything in hand to be able to choose extended 
regions (which won't clash with other resources).
2. [Related to every domain]  Not all device I/O regions might be known 
(registered) by the time the guest starts creating grant/foreign 
mappings, so guest cannot guess by itself what is really unused, but the 
entity which creates the domain (Xen, toolstack) knows these details in 
advance to be able to choose extended regions (which won't clash with 
other resources).


>
>> Two more things about this being ARM-specific.
>>
>> Even if x86 was affected exactly by the same problem, the code to expose
>> the safe memory ranges to DomU is arch-specific (currently device tree.)
>>
>> Also the code to calculate the safe memory ranges is arch-specific as it
>> depends on the DomU memory layout which is arch-specific.
> This demonstrates that the implementation is arch-specific.  But one
> of libxl's functions is to abstract away implementation details and
> provide an interface that can be used to "do the right thing".
>
> Ian.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Oleksandr]

On 07.10.21 04:29, Stefano Stabellini wrote:

Hi Stefano

> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> The extended region (safe range) is a region of guest physical
>> address space which is unused and could be safely used to create
>> grant/foreign mappings instead of wasting real RAM pages from
>> the domain memory for establishing these mappings.
>>
>> The extended regions are chosen at the domain creation time and
>> advertised to it via "reg" property under hypervisor node in
>> the guest device-tree. As region 0 is reserved for grant table
>> space (always present), the indexes for extended regions are 1...N.
>> If extended regions could not be allocated for some reason,
>> Xen doesn't fail and behaves as usual, so only inserts region 0.
>>
>> Please note the following limitations:
>> - The extended region feature is only supported for 64-bit domain
>>    currently.
>> - The ACPI case is not covered.
>>
>> ***
>>
>> The algorithm to choose extended regions for non-direct mapped
>> DomU is simpler in comparison with the algorithm for direct mapped
>> Dom0. We usually have a lot of unused space above 4GB, and might
>> have some unused space below 4GB (depends on guest memory size).
>> Try to allocate separate 2MB-aligned extended regions from the first
>> (below 4GB) and second (above 4GB) RAM banks taking into the account
>> the maximum supported guest physical address space size and the amount
>> of memory assigned to the guest. The minimum size of extended region
>> the same as for Dom0 (64MB).
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> ! Stefano, Ian, Michal I dropped your A-b/R-b. I decided to change patch
>> to make it more functional !
>>
>> Changes RFC -> V2:
>>     - update patch description
>>     - drop uneeded "extended-region" DT property
>>     - clear reg array in finalise_ext_region() and add a TODO
>>
>> Changes V2 -> V3:
>>     - update patch description, comments in code
>>     - only pick up regions with size >= 64MB
>>     - move the region calculation to make_hypervisor_node() and drop
>>       finalise_ext_region()
>>     - extend the list of arguments for make_hypervisor_node()
>>     - do not show warning for 32-bit domain
>>     - change the region alignment from 1GB to 2MB
>>     - move EXT_REGION_SIZE to public/arch-arm.h
>>
>> Changes V3 -> V4:
>>     - add R-b, A-b and T-b
>>
>> Changes V4 -> V5:
>>     - update patch description and comments in code
>>     - reflect changes done in previous patch to pass gpaddr_bits
>>       via createdomain domctl (struct xen_arch_domainconfig)
>>     - drop R-b, A-b and T-b
>>     - drop limit for maximum extended region size (128GB)
>>     - try to also allocate region below 4GB, optimize code
>>       for calculating extended regions
>> ---
>>   tools/libs/light/libxl_arm.c  | 80 ++++++++++++++++++++++++++++++++++++++++---
>>   xen/include/public/arch-arm.h |  2 ++
>>   2 files changed, 77 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 45e0386..cd743f7 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -600,9 +600,21 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
>>       return 0;
>>   }
>>   
>> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
>> +
>>   static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>> -                                const libxl_version_info *vers)
>> +                                const libxl_version_info *vers,
>> +                                const libxl_domain_build_info *b_info,
>> +                                const struct xc_dom_image *dom)
>>   {
>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
>> +        banksize[GUEST_RAM_BANKS], bankend[GUEST_RAM_BANKS], ramsize;
>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>> +                  (GUEST_RAM_BANKS + 1)];
>> +    be32 *cells = &regs[0];
>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>> +    unsigned int i, len, nr_regions = 0;
>> +    uint8_t gpaddr_bits;
>>       int res;
>>       gic_interrupt intr;
>>   
>> @@ -617,9 +629,67 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>>                                 "xen,xen");
>>       if (res) return res;
>>   
>> -    /* reg 0 is grant table space */
>> -    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
>> +        LOG(WARN, "The extended regions are only supported for 64-bit guest currently");
>> +        goto out;
>> +    }
>> +
>> +    gpaddr_bits = b_info->arch_arm.gpaddr_bits;
>> +    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
>> +
>> +    /*
>> +     * Try to allocate separate 2MB-aligned extended regions from the first
>> +     * (below 4GB) and second (above 4GB) RAM banks taking into the account
>> +     * the maximum supported guest physical address space size and the amount
>> +     * of memory assigned to the guest.
>> +     * As the guest memory layout is not populated yet we cannot rely on
>> +     * dom->rambank_size[], so calculate the actual size of both banks using
>> +     * "max_memkb" value.
>> +     */
>> +    ramsize = b_info->max_memkb * 1024;
>> +    if (ramsize <= GUEST_RAM0_SIZE) {
>> +        banksize[0] = ramsize;
>> +        banksize[1] = 0;
>> +    } else {
>> +        banksize[0] = GUEST_RAM0_SIZE;
>> +        banksize[1] = ramsize - GUEST_RAM0_SIZE;
>> +    }
>> +
>> +    bankend[0] = GUEST_RAM0_BASE + GUEST_RAM0_SIZE;
>> +    bankend[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
>> +
>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> +        region_base[i] = bankbase[i] + ALIGN_UP_TO_2MB(banksize[i]);
>> +        if (bankend[i] > region_base[i])
>> +            region_size[i] = bankend[i] - region_base[i];
>> +    }
> This seems correct but it looks a bit overkill. I would have written
> like this:
>
>      if (ramsize <= GUEST_RAM0_SIZE) {
>          region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
>          region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
>          region_base[1] = GUEST_RAM1_BASE;
>          region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) -
>                           region_base[0];
Why  "- region_base[0]" in last expression? I think it should be "- 
region_base[1]", the same as for "else" case (so it can be moved out of 
if-else construct). Also we need to check

that min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) is 
greater than region_base[1] before the subtraction. If gpaddr_bits = 32 
(on Arm64) we will get incorrect result.


>      } else {
>          region_size[0] = 0;
>          region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
>          region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) -
>                           region_base[1];
>      }
>
> Which removes the needs for banksize, bankend, bankbase. What do you
> think? Your version works too, so I am OK anyway.
Thank you for looking into this.

I think, you version will also work with adjustments. I am OK either 
way. Your version reduces the number of locals, so probably better.  
Although "min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE)" 
construction probably wants latching in local bank1end.


Below the updated version:


     if (ramsize <= GUEST_RAM0_SIZE) {
         region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
         region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
         region_base[1] = GUEST_RAM1_BASE;
     } else
         region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize - 
GUEST_RAM0_SIZE);

     bank1end = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
     if (bank1end > region_base[1])
         region_size[1] = bank1end - region_base[1];




>
>
>
>> +out:
>> +    /*
>> +     * The region 0 for grant table space must be always present. If we managed
>> +     * to allocate the extended regions then insert them as regions 1...N.
>> +     */
>> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>> +
>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> +        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
>> +            continue;
>> +
>> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
>> +            nr_regions, region_base[i], region_base[i] + region_size[i]);
>> +
>> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>> +                  region_base[i], region_size[i]);
>> +        nr_regions ++;
>                       ^ code style

ok


>
>
>> +    }
>> +
>> +    if (!nr_regions)
>> +        LOG(WARN, "The extended regions cannot be allocated, not enough space");
>> +
>> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>> +        (nr_regions + 1);
>> +    res = fdt_property(fdt, "reg", regs, len);
>>       if (res) return res;
>>   
>>       /*
>> @@ -965,7 +1035,7 @@ next_resize:
>>           }
>>   
>>           FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
>> -        FDT( make_hypervisor_node(gc, fdt, vers) );
>> +        FDT( make_hypervisor_node(gc, fdt, vers, info, dom) );
>>   
>>           if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
>>               FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 4a01f8b..f74cc0b 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -454,6 +454,8 @@ typedef uint64_t xen_callback_t;
>>   #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>>   #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>>   
>> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
>> +
>>   /* Current supported guest VCPUs */
>>   #define GUEST_MAX_VCPUS 128
>>   
>> -- 
>> 2.7.4
>>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Oleksandr]

On 07.10.21 04:50, Stefano Stabellini wrote:

Hi Stefano

> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This is a follow-up of
>> "b6fe410 xen/arm: Add handling of extended regions for Dom0"
>>
>> Add various in-code comments, update Xen hypervisor device tree
>> bindings text, change the log level for some prints and clarify
>> format specifier, reuse dt_for_each_range() to avoid open-coding
>> in find_memory_holes().
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Thanks for the patch, it looks like you addressed all Julien's comments
> well.

I believe so)


>   A couple of minor issues below.
>
>
>> ---
>>     New patch
>> ---
>>   docs/misc/arm/device-tree/guest.txt |  12 ++--
>>   xen/arch/arm/domain_build.c         | 108 ++++++++++++++++++++++--------------
>>   2 files changed, 73 insertions(+), 47 deletions(-)
>>
>> diff --git a/docs/misc/arm/device-tree/guest.txt b/docs/misc/arm/device-tree/guest.txt
>> index 418f1e9..c115751 100644
>> --- a/docs/misc/arm/device-tree/guest.txt
>> +++ b/docs/misc/arm/device-tree/guest.txt
>> @@ -7,10 +7,14 @@ the following properties:
>>   	compatible = "xen,xen-<version>", "xen,xen";
>>     where <version> is the version of the Xen ABI of the platform.
>>   
>> -- reg: specifies the base physical address and size of a region in
>> -  memory where the grant table should be mapped to, using an
>> -  HYPERVISOR_memory_op hypercall. The memory region is large enough to map
>> -  the whole grant table (it is larger or equal to gnttab_max_grant_frames()).
>> +- reg: specifies the base physical address and size of the regions in memory
>> +  where the special resources should be mapped to, using an HYPERVISOR_memory_op
>> +  hypercall.
>> +  Region 0 is reserved for mapping grant table, it must be always present.
>> +  The memory region is large enough to map the whole grant table (it is larger
>> +  or equal to gnttab_max_grant_frames()).
>> +  Regions 1...N are extended regions (unused address space) for mapping foreign
>> +  GFNs and grants, they might be absent if there is nothing to expose.
>>     This property is unnecessary when booting Dom0 using ACPI.
>>   
>>   - interrupts: the interrupt used by Xen to inject event notifications.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c5afbe2..d9f40d4 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -898,7 +898,10 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
>>       if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
>>           return 0;
>>   
>> -    /* Both start and size of the extended region should be 2MB aligned */
>> +    /*
>> +     * Both start and size of the extended region should be 2MB aligned to
>> +     * potentially allow superpage mapping.
>> +     */
>>       start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
>>       if ( start > e )
>>           return 0;
>> @@ -909,6 +912,12 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
>>        */
>>       e += 1;
>>       size = (e - start) & ~(SZ_2M - 1);
>> +
>> +    /*
>> +     * Reasonable size. Not too little to pick up small ranges which are
>> +     * not quite useful itself but increase bookkeeping and not too much
>                             ^ remove itself                             ^ large

ok


>
>> +     * to skip a large proportion of unused address space.
>> +     */
>>       if ( size < MB(64) )
>>           return 0;
>>   
>> @@ -919,6 +928,14 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
>>       return 0;
>>   }
>>   
>> +/*
>> + * Find unused regions of Host address space which can be exposed to Dom0
>> + * as extended regions for the special memory mappings. In order to calculate
>> + * regions we exclude every assigned to Dom0 region from the Host RAM:
>                                ^ region assigned  ^ remove

ok


>
>
>> + * - domain RAM
>> + * - reserved-memory
>> + * - grant table space
>> + */
>>   static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>>                                             struct meminfo *ext_regions)
>>   {
>> @@ -942,7 +959,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>>           res = rangeset_add_range(unalloc_mem, start, end - 1);
>>           if ( res )
>>           {
>> -            printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
>> +            printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
>>                      start, end);
>>               goto out;
>>           }
>> @@ -956,7 +973,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>>           res = rangeset_remove_range(unalloc_mem, start, end - 1);
>>           if ( res )
>>           {
>> -            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
>> +            printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>>                      start, end);
>>               goto out;
>>           }
>> @@ -971,7 +988,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>>           res = rangeset_remove_range(unalloc_mem, start, end - 1);
>>           if ( res )
>>           {
>> -            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
>> +            printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>>                      start, end);
>>               goto out;
>>           }
>> @@ -983,7 +1000,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>>       res = rangeset_remove_range(unalloc_mem, start, end - 1);
>>       if ( res )
>>       {
>> -        printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
>> +        printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>>                  start, end);
>>           goto out;
>>       }
>> @@ -1003,6 +1020,35 @@ out:
>>       return res;
>>   }
>>   
>> +static int __init handle_pci_range(const struct dt_device_node *dev,
>> +                                   u64 addr, u64 len, void *data)
>> +{
>> +    struct rangeset *mem_holes = data;
>> +    paddr_t start, end;
>> +    int res;
>> +
>> +    start = addr & PAGE_MASK;
>> +    end = PAGE_ALIGN(addr + len);
>> +    res = rangeset_remove_range(mem_holes, start, end - 1);
>> +    if ( res )
>> +    {
>> +        printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>> +               start, end);
>> +        return res;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Find the holes in the Host DT which can be exposed to Dom0 as extended
>> + * regions for the special memory mappings. In order to calculate regions
>> + * we exclude every addressable memory region described by "reg" and "ranges"
>> + * properties from the maximum possible addressable physical memory range:
>> + * - MMIO
>> + * - Host RAM
>> + * - PCI bar
>          ^ PCI aperture

ok


>
>
>> + */
>>   static int __init find_memory_holes(const struct kernel_info *kinfo,
>>                                       struct meminfo *ext_regions)
>>   {
>> @@ -1024,7 +1070,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>       res = rangeset_add_range(mem_holes, start, end);
>>       if ( res )
>>       {
>> -        printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
>> +        printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
>>                  start, end);
>>           goto out;
>>       }
>> @@ -1055,49 +1101,25 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>               res = rangeset_remove_range(mem_holes, start, end - 1);
>>               if ( res )
>>               {
>> -                printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
>> +                printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
>>                          start, end);
>>                   goto out;
>>               }
>>           }
>>   
>> -        if ( dt_device_type_is_equal(np, "pci" ) )
>> +        if ( dt_device_type_is_equal(np, "pci") )
>>           {
>> -            unsigned int range_size, nr_ranges;
>> -            int na, ns, pna;
>> -            const __be32 *ranges;
>> -            u32 len;
>> -
>>               /*
>> -             * Looking for non-empty ranges property which in this context
>> -             * describes the PCI host bridge aperture.
>> +             * The ranges property in this context describes the PCI host
>> +             * bridge aperture. It shall be absent if no addresses are mapped
>> +             * through the bridge.
>>                */
>> -            ranges = dt_get_property(np, "ranges", &len);
>> -            if ( !ranges || !len )
>> +            if ( !dt_get_property(np, "ranges", NULL) )
>>                   continue;
>>   
>> -            pna = dt_n_addr_cells(np);
>> -            na = dt_child_n_addr_cells(np);
>> -            ns = dt_child_n_size_cells(np);
>> -            range_size = pna + na + ns;
>> -            nr_ranges = len / sizeof(__be32) / range_size;
>> -
>> -            for ( i = 0; i < nr_ranges; i++, ranges += range_size )
>> -            {
>> -                /* Skip the child address and get the parent (CPU) address */
>> -                addr = dt_read_number(ranges + na, pna);
>> -                size = dt_read_number(ranges + na + pna, ns);
>> -
>> -                start = addr & PAGE_MASK;
>> -                end = PAGE_ALIGN(addr + size);
>> -                res = rangeset_remove_range(mem_holes, start, end - 1);
>> -                if ( res )
>> -                {
>> -                    printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
>> -                           start, end);
>> -                    goto out;
>> -                }
>> -            }
>> +            res = dt_for_each_range(np, &handle_pci_range, mem_holes);
>> +            if ( res )
>> +                goto out;
>>           }
>>       }
>>   
>> @@ -1152,12 +1174,12 @@ static int __init make_hypervisor_node(struct domain *d,
>>   
>>       if ( !opt_ext_regions )
>>       {
>> -        printk(XENLOG_DEBUG "The extended regions support is disabled\n");
>> +        printk(XENLOG_INFO "The extended regions support is disabled\n");
>>           nr_ext_regions = 0;
>>       }
>>       else if ( is_32bit_domain(d) )
>>       {
>> -        printk(XENLOG_DEBUG "The extended regions are only supported for 64-bit guest currently\n");
>> +        printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n");
>>           nr_ext_regions = 0;
>>       }
>>       else
>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain *d,
>>           u64 start = ext_regions->bank[i].start;
>>           u64 size = ext_regions->bank[i].size;
>>   
>> -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>> -                   i, start, start + size);
>> +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>> +               i, start, start + size);
> Also should be PRIpaddr

I thought I needed to change specifier only for variables of type 
"paddr_t", but here "u64".


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Stefano Stabellini]

On Thu, 7 Oct 2021, Oleksandr wrote:
> On 07.10.21 04:50, Stefano Stabellini wrote:
> 
> Hi Stefano
> 
> > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > 
> > > This is a follow-up of
> > > "b6fe410 xen/arm: Add handling of extended regions for Dom0"
> > > 
> > > Add various in-code comments, update Xen hypervisor device tree
> > > bindings text, change the log level for some prints and clarify
> > > format specifier, reuse dt_for_each_range() to avoid open-coding
> > > in find_memory_holes().
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > Thanks for the patch, it looks like you addressed all Julien's comments
> > well.
> 
> I believe so)


[...]

> > > @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain
> > > *d,
> > >           u64 start = ext_regions->bank[i].start;
> > >           u64 size = ext_regions->bank[i].size;
> > >   -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> > > -                   i, start, start + size);
> > > +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> > > +               i, start, start + size);
> > Also should be PRIpaddr
> 
> I thought I needed to change specifier only for variables of type "paddr_t",
> but here "u64".

Sorry, you are right.

I added my reviewed-by and made the small typo changes on commit.

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr]

On 07.10.21 03:49, Stefano Stabellini wrote:


Hi Stefano

> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to pass info about maximum supported guest physical
>> address space size to the toolstack on Arm in order to properly
>> calculate the base and size of the extended region (safe range)
>> for the guest. The extended region is unused address space which
>> could be safely used by domain for foreign/grant mappings on Arm.
>> The extended region itself will be handled by the subsequent
>> patch.
>>
>> Currently the same guest physical address space size is used
>> for all guests.
>>
>> As we add new field to the structure bump the interface version.
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> Changes RFC -> V2:
>>     - update patch subject/description
>>     - replace arch-specific sub-struct with common gpaddr_bits
>>       field and update code to reflect that
>>
>> Changes V2 -> V3:
>>     - make the field uint8_t and add uint8_t pad[7] after
>>     - remove leading blanks in libxl.h
>>
>> Changes V3 -> V4:
>>     - also print gpaddr_bits from output_physinfo()
>>     - add Michal's R-b
>>
>> Changes V4 -> V5:
>>     - update patch subject and description
>>     - drop Michal's R-b
>>     - pass gpaddr_bits via createdomain domctl
>>       (struct xen_arch_domainconfig)
>> ---
>>   tools/include/libxl.h            | 5 +++++
>>   tools/libs/light/libxl_arm.c     | 2 ++
>>   tools/libs/light/libxl_types.idl | 1 +
>>   xen/arch/arm/domain.c            | 6 ++++++
>>   xen/include/public/arch-arm.h    | 5 +++++
>>   xen/include/public/domctl.h      | 2 +-
>>   6 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index b9ba16d..33b4bfb 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -279,6 +279,11 @@
>>   #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
>>   
>>   /*
>> + * libxl_domain_build_info has the gpaddr_bits field.
>> + */
>> +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1
>> +
>> +/*
>>    * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
>>    * 'soft reset' for domains and there is 'soft_reset' shutdown reason
>>    * in enum libxl_shutdown_reason.
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6..45e0386 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
>>   
>>       state->clock_frequency = config->arch.clock_frequency;
>>   
>> +    d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits;
>> +
>>       return 0;
>>   }
>>   
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 3f9fff6..39482db 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>   
>>       ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>                                  ("vuart", libxl_vuart_type),
>> +                               ("gpaddr_bits", uint8),
>>                                 ])),
>>       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>                                 ])),
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 19c756a..dfecc45 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d,
>>       if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
>>           goto fail;
>>   
>> +    /*
>> +     * Pass maximum IPA bits to the toolstack, currently the same guest
>> +     * physical address space size is used for all guests.
>> +     */
>> +    config->arch.gpaddr_bits = p2m_ipa_bits;
> This could also be set in arch_sanitise_domain_config together with
> config->arch.gic_version. I prefer if it was done in
> arch_sanitise_domain_config but also here is OK I think.

I don't mind, being honest I had an idea to place this in 
arch_sanitise_domain_config(), but couldn't convince myself.


>
> Given that everything else looks fine:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks!

Sadly, according to the recent discussion most likely this version is 
also no-go.


[snip]

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Stefano Stabellini]

On Thu, 7 Oct 2021, Jan Beulich wrote:
> On 07.10.2021 15:12, Oleksandr wrote:
> > 
> > On 07.10.21 15:43, Jan Beulich wrote:
> > 
> > Hi Jan.
> > 
> >> On 07.10.2021 14:30, Oleksandr wrote:
> >>> On 07.10.21 10:42, Jan Beulich wrote:
> >>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
> >>>>> Changes V4 -> V5:
> >>>>>      - update patch subject and description
> >>>>>      - drop Michal's R-b
> >>>>>      - pass gpaddr_bits via createdomain domctl
> >>>>>        (struct xen_arch_domainconfig)
> >>>> I'm afraid I can't bring this in line with ...
> >>>>
> >>>>> --- a/xen/include/public/arch-arm.h
> >>>>> +++ b/xen/include/public/arch-arm.h
> >>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
> >>>>>         *
> >>>>>         */
> >>>>>        uint32_t clock_frequency;
> >>>>> +    /*
> >>>>> +     * OUT
> >>>>> +     * Guest physical address space size
> >>>>> +     */
> >>>>> +    uint8_t gpaddr_bits;
> >>>> ... this being an OUT field. Is this really what Andrew had asked for?
> >>>> I would have expected the entire struct to be IN (and the comment at
> >>>> the top of the containing struct in public/domctl.h also suggests so,
> >>>> i.e. your new field renders that comment stale). gic_version being
> >>>> IN/OUT is already somewhat in conflict ...
> >>> I am sorry but I'm totally confused now, we want the Xen to provide
> >>> gpaddr_bits to the toolstack, but not the other way around.
> >>> As I understand the main ask was to switch to domctl for which I wanted
> >>> to get some clarification on how it would look like... Well, this patch
> >>> switches to use
> >>> domctl, and I think exactly as it was suggested at [1] in case if a
> >>> common one is a difficult to achieve. I have to admit, I felt it was
> >>> indeed difficult to achieve.
> >> Sadly the mail you reference isn't the one I was referring to. It's not
> >> even from Andrew. Unfortunately I also can't seem to be able to locate
> >> this, i.e. I'm now wondering whether this was under a different subject.
> >> Julien, in any event, confirmed in a recent reply on this thread that
> >> there was such a mail (otherwise I would have started wondering whether
> >> the request was made on irc). In any case it is _that_ mail that would
> >> need going through again.
> > 
> > I think, this is the email [1] you are referring to.
> 
> Well, that's still a mail you sent, not Andrew's. And while I have yours
> in my mailbox, I don't have Andrew's for whatever reason.
> 
> Nevertheless there's enough context to be halfway certain that this
> wasn't meant as an extension to the create domctl, but rather a separate
> new one (merely replacing what you had originally as a sysctl to become
> per-domain, to allow returning varying [between domains] values down the
> road). I continue to think that if such a field was added to "create",
> it would be an input (only).

During the Xen Community Call on Tuesday, we briefly spoke about this.
Andrew confirmed that what he meant with his original email is to use a
domctl. We didn't discuss which domctl specifically.

This patch now follows the same pattern of clock_frequency and
gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig).
Note that gic_version is an IN/OUT parameter, showing that if in the
future we want the ability to set gpaddr_bits (in addition to get
gpaddr_bits), it will be possible.

Also it is good to keep in mind that although nobody likes to change
hypercall interfaces, especially for minor reasons, domctl are not
stable so we can be a little bit more relaxed compared to something like
grant_table_op.

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Oleksandr]

On 07.10.21 23:06, Stefano Stabellini wrote:

Hi Stefano

> On Thu, 7 Oct 2021, Oleksandr wrote:
>> On 07.10.21 04:50, Stefano Stabellini wrote:
>>
>> Hi Stefano
>>
>>> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> This is a follow-up of
>>>> "b6fe410 xen/arm: Add handling of extended regions for Dom0"
>>>>
>>>> Add various in-code comments, update Xen hypervisor device tree
>>>> bindings text, change the log level for some prints and clarify
>>>> format specifier, reuse dt_for_each_range() to avoid open-coding
>>>> in find_memory_holes().
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> Thanks for the patch, it looks like you addressed all Julien's comments
>>> well.
>> I believe so)
>
> [...]
>
>>>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain
>>>> *d,
>>>>            u64 start = ext_regions->bank[i].start;
>>>>            u64 size = ext_regions->bank[i].size;
>>>>    -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>>>> -                   i, start, start + size);
>>>> +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>>>> +               i, start, start + size);
>>> Also should be PRIpaddr
>> I thought I needed to change specifier only for variables of type "paddr_t",
>> but here "u64".
> Sorry, you are right.
>
> I added my reviewed-by and made the small typo changes on commit.

Thanks! In case if you haven't committed the patch yet, let's please 
wait for Julien (who asked for this follow-up) to review it.

In any case, I will be able to do another follow-up if needed.


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8890 bytes --]

On Thu, 7 Oct 2021, Oleksandr wrote:
> On 07.10.21 04:29, Stefano Stabellini wrote:
> 
> Hi Stefano
> 
> > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > 
> > > The extended region (safe range) is a region of guest physical
> > > address space which is unused and could be safely used to create
> > > grant/foreign mappings instead of wasting real RAM pages from
> > > the domain memory for establishing these mappings.
> > > 
> > > The extended regions are chosen at the domain creation time and
> > > advertised to it via "reg" property under hypervisor node in
> > > the guest device-tree. As region 0 is reserved for grant table
> > > space (always present), the indexes for extended regions are 1...N.
> > > If extended regions could not be allocated for some reason,
> > > Xen doesn't fail and behaves as usual, so only inserts region 0.
> > > 
> > > Please note the following limitations:
> > > - The extended region feature is only supported for 64-bit domain
> > >    currently.
> > > - The ACPI case is not covered.
> > > 
> > > ***
> > > 
> > > The algorithm to choose extended regions for non-direct mapped
> > > DomU is simpler in comparison with the algorithm for direct mapped
> > > Dom0. We usually have a lot of unused space above 4GB, and might
> > > have some unused space below 4GB (depends on guest memory size).
> > > Try to allocate separate 2MB-aligned extended regions from the first
> > > (below 4GB) and second (above 4GB) RAM banks taking into the account
> > > the maximum supported guest physical address space size and the amount
> > > of memory assigned to the guest. The minimum size of extended region
> > > the same as for Dom0 (64MB).
> > > 
> > > Suggested-by: Julien Grall <jgrall@amazon.com>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > ---
> > > ! Stefano, Ian, Michal I dropped your A-b/R-b. I decided to change patch
> > > to make it more functional !
> > > 
> > > Changes RFC -> V2:
> > >     - update patch description
> > >     - drop uneeded "extended-region" DT property
> > >     - clear reg array in finalise_ext_region() and add a TODO
> > > 
> > > Changes V2 -> V3:
> > >     - update patch description, comments in code
> > >     - only pick up regions with size >= 64MB
> > >     - move the region calculation to make_hypervisor_node() and drop
> > >       finalise_ext_region()
> > >     - extend the list of arguments for make_hypervisor_node()
> > >     - do not show warning for 32-bit domain
> > >     - change the region alignment from 1GB to 2MB
> > >     - move EXT_REGION_SIZE to public/arch-arm.h
> > > 
> > > Changes V3 -> V4:
> > >     - add R-b, A-b and T-b
> > > 
> > > Changes V4 -> V5:
> > >     - update patch description and comments in code
> > >     - reflect changes done in previous patch to pass gpaddr_bits
> > >       via createdomain domctl (struct xen_arch_domainconfig)
> > >     - drop R-b, A-b and T-b
> > >     - drop limit for maximum extended region size (128GB)
> > >     - try to also allocate region below 4GB, optimize code
> > >       for calculating extended regions
> > > ---
> > >   tools/libs/light/libxl_arm.c  | 80
> > > ++++++++++++++++++++++++++++++++++++++++---
> > >   xen/include/public/arch-arm.h |  2 ++
> > >   2 files changed, 77 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > > index 45e0386..cd743f7 100644
> > > --- a/tools/libs/light/libxl_arm.c
> > > +++ b/tools/libs/light/libxl_arm.c
> > > @@ -600,9 +600,21 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
> > >       return 0;
> > >   }
> > >   +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> > > +
> > >   static int make_hypervisor_node(libxl__gc *gc, void *fdt,
> > > -                                const libxl_version_info *vers)
> > > +                                const libxl_version_info *vers,
> > > +                                const libxl_domain_build_info *b_info,
> > > +                                const struct xc_dom_image *dom)
> > >   {
> > > +    uint64_t region_size[GUEST_RAM_BANKS] = {0},
> > > region_base[GUEST_RAM_BANKS],
> > > +        banksize[GUEST_RAM_BANKS], bankend[GUEST_RAM_BANKS], ramsize;
> > > +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> > > +                  (GUEST_RAM_BANKS + 1)];
> > > +    be32 *cells = &regs[0];
> > > +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > > +    unsigned int i, len, nr_regions = 0;
> > > +    uint8_t gpaddr_bits;
> > >       int res;
> > >       gic_interrupt intr;
> > >   @@ -617,9 +629,67 @@ static int make_hypervisor_node(libxl__gc *gc, void
> > > *fdt,
> > >                                 "xen,xen");
> > >       if (res) return res;
> > >   -    /* reg 0 is grant table space */
> > > -    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> > > GUEST_ROOT_SIZE_CELLS,
> > > -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> > > +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> > > +        LOG(WARN, "The extended regions are only supported for 64-bit
> > > guest currently");
> > > +        goto out;
> > > +    }
> > > +
> > > +    gpaddr_bits = b_info->arch_arm.gpaddr_bits;
> > > +    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
> > > +
> > > +    /*
> > > +     * Try to allocate separate 2MB-aligned extended regions from the
> > > first
> > > +     * (below 4GB) and second (above 4GB) RAM banks taking into the
> > > account
> > > +     * the maximum supported guest physical address space size and the
> > > amount
> > > +     * of memory assigned to the guest.
> > > +     * As the guest memory layout is not populated yet we cannot rely on
> > > +     * dom->rambank_size[], so calculate the actual size of both banks
> > > using
> > > +     * "max_memkb" value.
> > > +     */
> > > +    ramsize = b_info->max_memkb * 1024;
> > > +    if (ramsize <= GUEST_RAM0_SIZE) {
> > > +        banksize[0] = ramsize;
> > > +        banksize[1] = 0;
> > > +    } else {
> > > +        banksize[0] = GUEST_RAM0_SIZE;
> > > +        banksize[1] = ramsize - GUEST_RAM0_SIZE;
> > > +    }
> > > +
> > > +    bankend[0] = GUEST_RAM0_BASE + GUEST_RAM0_SIZE;
> > > +    bankend[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE +
> > > GUEST_RAM1_SIZE);
> > > +
> > > +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> > > +        region_base[i] = bankbase[i] + ALIGN_UP_TO_2MB(banksize[i]);
> > > +        if (bankend[i] > region_base[i])
> > > +            region_size[i] = bankend[i] - region_base[i];
> > > +    }
> > This seems correct but it looks a bit overkill. I would have written
> > like this:
> > 
> >      if (ramsize <= GUEST_RAM0_SIZE) {
> >          region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
> >          region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
> >          region_base[1] = GUEST_RAM1_BASE;
> >          region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE +
> > GUEST_RAM1_SIZE) -
> >                           region_base[0];
> Why  "- region_base[0]" in last expression? I think it should be "-
> region_base[1]", the same as for "else" case (so it can be moved out of
> if-else construct). Also we need to check
> 
> that min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) is greater
> than region_base[1] before the subtraction. If gpaddr_bits = 32 (on Arm64) we
> will get incorrect result.
> 
> 
> >      } else {
> >          region_size[0] = 0;
> >          region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize -
> > GUEST_RAM0_SIZE);
> >          region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE +
> > GUEST_RAM1_SIZE) -
> >                           region_base[1];
> >      }
> > 
> > Which removes the needs for banksize, bankend, bankbase. What do you
> > think? Your version works too, so I am OK anyway.
> Thank you for looking into this.
> 
> I think, you version will also work with adjustments. I am OK either way. Your
> version reduces the number of locals, so probably better.  Although "min(1ULL
> << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE)" construction probably
> wants latching in local bank1end.
> 
> 
> Below the updated version:
> 
>     if (ramsize <= GUEST_RAM0_SIZE) {
>         region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
>         region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
>         region_base[1] = GUEST_RAM1_BASE;
>     } else
>         region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
> 
>     bank1end = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
>     if (bank1end > region_base[1])
>         region_size[1] = bank1end - region_base[1];


Yeah I like this. I'd be happy to go with it.

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Stefano Stabellini]

On Thu, 7 Oct 2021, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU"):
> > On Wed, 6 Oct 2021, Oleksandr wrote:
> > > On 06.10.21 14:34, Ian Jackson wrote:
> > > > Oleksandr Tyshchenko writes ("[PATCH V5 2/3] libxl/arm: Add handling of
> > > > extended regions for DomU"):
> > > > > The extended region (safe range) is a region of guest physical
> > > > > address space which is unused and could be safely used to create
> > > > > grant/foreign mappings instead of wasting real RAM pages from
> > > > > the domain memory for establishing these mappings.
> > > > Please forgive me for asking this question now, but: why is this
> > > > ARM-specific ?
> > > 
> > > 
> > > Sorry, I can't say for sure which x86 mode also suffers from
> > > that. I might be wrong, but as I understand that x86 in PVH (and
> > > HVM?) mode uses unpopulated memory ranges (which are unused from
> > > Linux PoV, actually everything not yet allocated or reserved from
> > > "iomem_resource") to create foreign/grant mappings.  So the real
> > > RAM pages are not ballooned out to get an physical address space
> > > to create these mappings. The problem is that we cannot follow
> > > Linux advise which memory ranges are unused on Arm for several
> > > reasons, this is why this patch series makes the hypervisor to
> > > start allocating and exposing these ranges.
> 
> So it sounds like you are saying this is an ARM-specific problem ?
> The key being the "several reasons" which you mention.  Are they
> ARM-specifc problems.
> 
> > Two more things about this being ARM-specific.
> > 
> > Even if x86 was affected exactly by the same problem, the code to expose
> > the safe memory ranges to DomU is arch-specific (currently device tree.)
> > 
> > Also the code to calculate the safe memory ranges is arch-specific as it
> > depends on the DomU memory layout which is arch-specific.
> 
> This demonstrates that the implementation is arch-specific.  But one
> of libxl's functions is to abstract away implementation details and
> provide an interface that can be used to "do the right thing".

That's fair enough, I understand your question a bit better now.

I am not certain whether x86 has the same issue. But if it does, I am
pretty sure both the strategy to address the problem and the code would
be very different. At the moment, I cannot imagine how we could share
anything in this patch with x86, especially given the difference in
firmware interfaces (ACPI vs DT).

But I could see a theoretical third architecture (RISC-V?) with device
tree support potentially making use of make_hypervisor_node. But then at
the libxl API I don't imagine the API would look different or would need
to change.

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Stefano Stabellini]

On Thu, 7 Oct 2021, Oleksandr wrote:
> On 07.10.21 23:06, Stefano Stabellini wrote:
> > On Thu, 7 Oct 2021, Oleksandr wrote:
> > > On 07.10.21 04:50, Stefano Stabellini wrote:
> > > 
> > > Hi Stefano
> > > 
> > > > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > > > 
> > > > > This is a follow-up of
> > > > > "b6fe410 xen/arm: Add handling of extended regions for Dom0"
> > > > > 
> > > > > Add various in-code comments, update Xen hypervisor device tree
> > > > > bindings text, change the log level for some prints and clarify
> > > > > format specifier, reuse dt_for_each_range() to avoid open-coding
> > > > > in find_memory_holes().
> > > > > 
> > > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > > Thanks for the patch, it looks like you addressed all Julien's comments
> > > > well.
> > > I believe so)
> > 
> > [...]
> > 
> > > > > @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct
> > > > > domain
> > > > > *d,
> > > > >            u64 start = ext_regions->bank[i].start;
> > > > >            u64 size = ext_regions->bank[i].size;
> > > > >    -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> > > > > -                   i, start, start + size);
> > > > > +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> > > > > +               i, start, start + size);
> > > > Also should be PRIpaddr
> > > I thought I needed to change specifier only for variables of type
> > > "paddr_t",
> > > but here "u64".
> > Sorry, you are right.
> > 
> > I added my reviewed-by and made the small typo changes on commit.
> 
> Thanks! In case if you haven't committed the patch yet, let's please wait for
> Julien (who asked for this follow-up) to review it.
> 
> In any case, I will be able to do another follow-up if needed.
 
I committed it as I would like to squeeze as many runs out of OSSTest
and Gitlab-CI as possible as we are getting closer and closer to the
release. I am trying to avoid the last minute rush to commit 150 patches
one day before code freeze :-)

The more intermediate runs we get, the easier is to pinpoint (and fix)
regressions.

But also, this patch doesn't affect external interfances, it is just
internal and mostly comments, so it is super-easy to do follow-ups.

Re: [PATCH V5 2/3] libxl/arm: Add handling of extended regions for DomU

[Oleksandr]

On 07.10.21 23:29, Stefano Stabellini wrote:

Hi Stefano

> On Thu, 7 Oct 2021, Oleksandr wrote:
>> On 07.10.21 04:29, Stefano Stabellini wrote:
>>
>> Hi Stefano
>>
>>> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> The extended region (safe range) is a region of guest physical
>>>> address space which is unused and could be safely used to create
>>>> grant/foreign mappings instead of wasting real RAM pages from
>>>> the domain memory for establishing these mappings.
>>>>
>>>> The extended regions are chosen at the domain creation time and
>>>> advertised to it via "reg" property under hypervisor node in
>>>> the guest device-tree. As region 0 is reserved for grant table
>>>> space (always present), the indexes for extended regions are 1...N.
>>>> If extended regions could not be allocated for some reason,
>>>> Xen doesn't fail and behaves as usual, so only inserts region 0.
>>>>
>>>> Please note the following limitations:
>>>> - The extended region feature is only supported for 64-bit domain
>>>>     currently.
>>>> - The ACPI case is not covered.
>>>>
>>>> ***
>>>>
>>>> The algorithm to choose extended regions for non-direct mapped
>>>> DomU is simpler in comparison with the algorithm for direct mapped
>>>> Dom0. We usually have a lot of unused space above 4GB, and might
>>>> have some unused space below 4GB (depends on guest memory size).
>>>> Try to allocate separate 2MB-aligned extended regions from the first
>>>> (below 4GB) and second (above 4GB) RAM banks taking into the account
>>>> the maximum supported guest physical address space size and the amount
>>>> of memory assigned to the guest. The minimum size of extended region
>>>> the same as for Dom0 (64MB).
>>>>
>>>> Suggested-by: Julien Grall <jgrall@amazon.com>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>> ! Stefano, Ian, Michal I dropped your A-b/R-b. I decided to change patch
>>>> to make it more functional !
>>>>
>>>> Changes RFC -> V2:
>>>>      - update patch description
>>>>      - drop uneeded "extended-region" DT property
>>>>      - clear reg array in finalise_ext_region() and add a TODO
>>>>
>>>> Changes V2 -> V3:
>>>>      - update patch description, comments in code
>>>>      - only pick up regions with size >= 64MB
>>>>      - move the region calculation to make_hypervisor_node() and drop
>>>>        finalise_ext_region()
>>>>      - extend the list of arguments for make_hypervisor_node()
>>>>      - do not show warning for 32-bit domain
>>>>      - change the region alignment from 1GB to 2MB
>>>>      - move EXT_REGION_SIZE to public/arch-arm.h
>>>>
>>>> Changes V3 -> V4:
>>>>      - add R-b, A-b and T-b
>>>>
>>>> Changes V4 -> V5:
>>>>      - update patch description and comments in code
>>>>      - reflect changes done in previous patch to pass gpaddr_bits
>>>>        via createdomain domctl (struct xen_arch_domainconfig)
>>>>      - drop R-b, A-b and T-b
>>>>      - drop limit for maximum extended region size (128GB)
>>>>      - try to also allocate region below 4GB, optimize code
>>>>        for calculating extended regions
>>>> ---
>>>>    tools/libs/light/libxl_arm.c  | 80
>>>> ++++++++++++++++++++++++++++++++++++++++---
>>>>    xen/include/public/arch-arm.h |  2 ++
>>>>    2 files changed, 77 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>>> index 45e0386..cd743f7 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -600,9 +600,21 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
>>>>        return 0;
>>>>    }
>>>>    +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
>>>> +
>>>>    static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>>>> -                                const libxl_version_info *vers)
>>>> +                                const libxl_version_info *vers,
>>>> +                                const libxl_domain_build_info *b_info,
>>>> +                                const struct xc_dom_image *dom)
>>>>    {
>>>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0},
>>>> region_base[GUEST_RAM_BANKS],
>>>> +        banksize[GUEST_RAM_BANKS], bankend[GUEST_RAM_BANKS], ramsize;
>>>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>>>> +                  (GUEST_RAM_BANKS + 1)];
>>>> +    be32 *cells = &regs[0];
>>>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>>>> +    unsigned int i, len, nr_regions = 0;
>>>> +    uint8_t gpaddr_bits;
>>>>        int res;
>>>>        gic_interrupt intr;
>>>>    @@ -617,9 +629,67 @@ static int make_hypervisor_node(libxl__gc *gc, void
>>>> *fdt,
>>>>                                  "xen,xen");
>>>>        if (res) return res;
>>>>    -    /* reg 0 is grant table space */
>>>> -    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>>>> GUEST_ROOT_SIZE_CELLS,
>>>> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>>>> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
>>>> +        LOG(WARN, "The extended regions are only supported for 64-bit
>>>> guest currently");
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    gpaddr_bits = b_info->arch_arm.gpaddr_bits;
>>>> +    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
>>>> +
>>>> +    /*
>>>> +     * Try to allocate separate 2MB-aligned extended regions from the
>>>> first
>>>> +     * (below 4GB) and second (above 4GB) RAM banks taking into the
>>>> account
>>>> +     * the maximum supported guest physical address space size and the
>>>> amount
>>>> +     * of memory assigned to the guest.
>>>> +     * As the guest memory layout is not populated yet we cannot rely on
>>>> +     * dom->rambank_size[], so calculate the actual size of both banks
>>>> using
>>>> +     * "max_memkb" value.
>>>> +     */
>>>> +    ramsize = b_info->max_memkb * 1024;
>>>> +    if (ramsize <= GUEST_RAM0_SIZE) {
>>>> +        banksize[0] = ramsize;
>>>> +        banksize[1] = 0;
>>>> +    } else {
>>>> +        banksize[0] = GUEST_RAM0_SIZE;
>>>> +        banksize[1] = ramsize - GUEST_RAM0_SIZE;
>>>> +    }
>>>> +
>>>> +    bankend[0] = GUEST_RAM0_BASE + GUEST_RAM0_SIZE;
>>>> +    bankend[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE +
>>>> GUEST_RAM1_SIZE);
>>>> +
>>>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>>>> +        region_base[i] = bankbase[i] + ALIGN_UP_TO_2MB(banksize[i]);
>>>> +        if (bankend[i] > region_base[i])
>>>> +            region_size[i] = bankend[i] - region_base[i];
>>>> +    }
>>> This seems correct but it looks a bit overkill. I would have written
>>> like this:
>>>
>>>       if (ramsize <= GUEST_RAM0_SIZE) {
>>>           region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
>>>           region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
>>>           region_base[1] = GUEST_RAM1_BASE;
>>>           region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE +
>>> GUEST_RAM1_SIZE) -
>>>                            region_base[0];
>> Why  "- region_base[0]" in last expression? I think it should be "-
>> region_base[1]", the same as for "else" case (so it can be moved out of
>> if-else construct). Also we need to check
>>
>> that min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE) is greater
>> than region_base[1] before the subtraction. If gpaddr_bits = 32 (on Arm64) we
>> will get incorrect result.
>>
>>
>>>       } else {
>>>           region_size[0] = 0;
>>>           region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize -
>>> GUEST_RAM0_SIZE);
>>>           region_size[1] = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE +
>>> GUEST_RAM1_SIZE) -
>>>                            region_base[1];
>>>       }
>>>
>>> Which removes the needs for banksize, bankend, bankbase. What do you
>>> think? Your version works too, so I am OK anyway.
>> Thank you for looking into this.
>>
>> I think, you version will also work with adjustments. I am OK either way. Your
>> version reduces the number of locals, so probably better.  Although "min(1ULL
>> << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE)" construction probably
>> wants latching in local bank1end.
>>
>>
>> Below the updated version:
>>
>>      if (ramsize <= GUEST_RAM0_SIZE) {
>>          region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
>>          region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
>>          region_base[1] = GUEST_RAM1_BASE;
>>      } else
>>          region_base[1] = GUEST_RAM1_BASE + ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
>>
>>      bank1end = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
>>      if (bank1end > region_base[1])
>>          region_size[1] = bank1end - region_base[1];
>
> Yeah I like this. I'd be happy to go with it.

Great, thank you, will update.


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Oleksandr]

On 07.10.21 23:42, Stefano Stabellini wrote:

Hi Stefano

> On Thu, 7 Oct 2021, Oleksandr wrote:
>> On 07.10.21 23:06, Stefano Stabellini wrote:
>>> On Thu, 7 Oct 2021, Oleksandr wrote:
>>>> On 07.10.21 04:50, Stefano Stabellini wrote:
>>>>
>>>> Hi Stefano
>>>>
>>>>> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>
>>>>>> This is a follow-up of
>>>>>> "b6fe410 xen/arm: Add handling of extended regions for Dom0"
>>>>>>
>>>>>> Add various in-code comments, update Xen hypervisor device tree
>>>>>> bindings text, change the log level for some prints and clarify
>>>>>> format specifier, reuse dt_for_each_range() to avoid open-coding
>>>>>> in find_memory_holes().
>>>>>>
>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>> Thanks for the patch, it looks like you addressed all Julien's comments
>>>>> well.
>>>> I believe so)
>>> [...]
>>>
>>>>>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct
>>>>>> domain
>>>>>> *d,
>>>>>>             u64 start = ext_regions->bank[i].start;
>>>>>>             u64 size = ext_regions->bank[i].size;
>>>>>>     -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>>> -                   i, start, start + size);
>>>>>> +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>>> +               i, start, start + size);
>>>>> Also should be PRIpaddr
>>>> I thought I needed to change specifier only for variables of type
>>>> "paddr_t",
>>>> but here "u64".
>>> Sorry, you are right.
>>>
>>> I added my reviewed-by and made the small typo changes on commit.
>> Thanks! In case if you haven't committed the patch yet, let's please wait for
>> Julien (who asked for this follow-up) to review it.
>>
>> In any case, I will be able to do another follow-up if needed.
>   
> I committed it as I would like to squeeze as many runs out of OSSTest
> and Gitlab-CI as possible as we are getting closer and closer to the
> release. I am trying to avoid the last minute rush to commit 150 patches
> one day before code freeze :-)
>
> The more intermediate runs we get, the easier is to pinpoint (and fix)
> regressions.

I got it, thank you for the explanation.


>
> But also, this patch doesn't affect external interfances, it is just
> internal and mostly comments, so it is super-easy to do follow-ups.

Yes, agree.


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Jan Beulich]

On 07.10.2021 22:23, Stefano Stabellini wrote:
> On Thu, 7 Oct 2021, Jan Beulich wrote:
>> On 07.10.2021 15:12, Oleksandr wrote:
>>>
>>> On 07.10.21 15:43, Jan Beulich wrote:
>>>
>>> Hi Jan.
>>>
>>>> On 07.10.2021 14:30, Oleksandr wrote:
>>>>> On 07.10.21 10:42, Jan Beulich wrote:
>>>>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
>>>>>>> Changes V4 -> V5:
>>>>>>>      - update patch subject and description
>>>>>>>      - drop Michal's R-b
>>>>>>>      - pass gpaddr_bits via createdomain domctl
>>>>>>>        (struct xen_arch_domainconfig)
>>>>>> I'm afraid I can't bring this in line with ...
>>>>>>
>>>>>>> --- a/xen/include/public/arch-arm.h
>>>>>>> +++ b/xen/include/public/arch-arm.h
>>>>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>>>>>>>         *
>>>>>>>         */
>>>>>>>        uint32_t clock_frequency;
>>>>>>> +    /*
>>>>>>> +     * OUT
>>>>>>> +     * Guest physical address space size
>>>>>>> +     */
>>>>>>> +    uint8_t gpaddr_bits;
>>>>>> ... this being an OUT field. Is this really what Andrew had asked for?
>>>>>> I would have expected the entire struct to be IN (and the comment at
>>>>>> the top of the containing struct in public/domctl.h also suggests so,
>>>>>> i.e. your new field renders that comment stale). gic_version being
>>>>>> IN/OUT is already somewhat in conflict ...
>>>>> I am sorry but I'm totally confused now, we want the Xen to provide
>>>>> gpaddr_bits to the toolstack, but not the other way around.
>>>>> As I understand the main ask was to switch to domctl for which I wanted
>>>>> to get some clarification on how it would look like... Well, this patch
>>>>> switches to use
>>>>> domctl, and I think exactly as it was suggested at [1] in case if a
>>>>> common one is a difficult to achieve. I have to admit, I felt it was
>>>>> indeed difficult to achieve.
>>>> Sadly the mail you reference isn't the one I was referring to. It's not
>>>> even from Andrew. Unfortunately I also can't seem to be able to locate
>>>> this, i.e. I'm now wondering whether this was under a different subject.
>>>> Julien, in any event, confirmed in a recent reply on this thread that
>>>> there was such a mail (otherwise I would have started wondering whether
>>>> the request was made on irc). In any case it is _that_ mail that would
>>>> need going through again.
>>>
>>> I think, this is the email [1] you are referring to.
>>
>> Well, that's still a mail you sent, not Andrew's. And while I have yours
>> in my mailbox, I don't have Andrew's for whatever reason.
>>
>> Nevertheless there's enough context to be halfway certain that this
>> wasn't meant as an extension to the create domctl, but rather a separate
>> new one (merely replacing what you had originally as a sysctl to become
>> per-domain, to allow returning varying [between domains] values down the
>> road). I continue to think that if such a field was added to "create",
>> it would be an input (only).
> 
> During the Xen Community Call on Tuesday, we briefly spoke about this.
> Andrew confirmed that what he meant with his original email is to use a
> domctl. We didn't discuss which domctl specifically.
> 
> This patch now follows the same pattern of clock_frequency and
> gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig).
> Note that gic_version is an IN/OUT parameter, showing that if in the
> future we want the ability to set gpaddr_bits (in addition to get
> gpaddr_bits), it will be possible.

Well, as said before - I'm not convinced gic_version being IN/OUT is
appropriate. At the very least a 2nd way to merely retrieve the value
would seem to be necessary, so that it's not only the party creating
the guest which would be able to know.

Since here's we're solely after retrieving the value, I don't see
the point in altering "create". As you say, domctl can be changed,
and hence at the point this needs to become an input to "create", it
could easily be added there.

Jan

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr]

On 08.10.21 11:13, Jan Beulich wrote:

Hi Jan

> On 07.10.2021 22:23, Stefano Stabellini wrote:
>> On Thu, 7 Oct 2021, Jan Beulich wrote:
>>> On 07.10.2021 15:12, Oleksandr wrote:
>>>> On 07.10.21 15:43, Jan Beulich wrote:
>>>>
>>>> Hi Jan.
>>>>
>>>>> On 07.10.2021 14:30, Oleksandr wrote:
>>>>>> On 07.10.21 10:42, Jan Beulich wrote:
>>>>>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
>>>>>>>> Changes V4 -> V5:
>>>>>>>>       - update patch subject and description
>>>>>>>>       - drop Michal's R-b
>>>>>>>>       - pass gpaddr_bits via createdomain domctl
>>>>>>>>         (struct xen_arch_domainconfig)
>>>>>>> I'm afraid I can't bring this in line with ...
>>>>>>>
>>>>>>>> --- a/xen/include/public/arch-arm.h
>>>>>>>> +++ b/xen/include/public/arch-arm.h
>>>>>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>>>>>>>>          *
>>>>>>>>          */
>>>>>>>>         uint32_t clock_frequency;
>>>>>>>> +    /*
>>>>>>>> +     * OUT
>>>>>>>> +     * Guest physical address space size
>>>>>>>> +     */
>>>>>>>> +    uint8_t gpaddr_bits;
>>>>>>> ... this being an OUT field. Is this really what Andrew had asked for?
>>>>>>> I would have expected the entire struct to be IN (and the comment at
>>>>>>> the top of the containing struct in public/domctl.h also suggests so,
>>>>>>> i.e. your new field renders that comment stale). gic_version being
>>>>>>> IN/OUT is already somewhat in conflict ...
>>>>>> I am sorry but I'm totally confused now, we want the Xen to provide
>>>>>> gpaddr_bits to the toolstack, but not the other way around.
>>>>>> As I understand the main ask was to switch to domctl for which I wanted
>>>>>> to get some clarification on how it would look like... Well, this patch
>>>>>> switches to use
>>>>>> domctl, and I think exactly as it was suggested at [1] in case if a
>>>>>> common one is a difficult to achieve. I have to admit, I felt it was
>>>>>> indeed difficult to achieve.
>>>>> Sadly the mail you reference isn't the one I was referring to. It's not
>>>>> even from Andrew. Unfortunately I also can't seem to be able to locate
>>>>> this, i.e. I'm now wondering whether this was under a different subject.
>>>>> Julien, in any event, confirmed in a recent reply on this thread that
>>>>> there was such a mail (otherwise I would have started wondering whether
>>>>> the request was made on irc). In any case it is _that_ mail that would
>>>>> need going through again.
>>>> I think, this is the email [1] you are referring to.
>>> Well, that's still a mail you sent, not Andrew's. And while I have yours
>>> in my mailbox, I don't have Andrew's for whatever reason.
>>>
>>> Nevertheless there's enough context to be halfway certain that this
>>> wasn't meant as an extension to the create domctl, but rather a separate
>>> new one (merely replacing what you had originally as a sysctl to become
>>> per-domain, to allow returning varying [between domains] values down the
>>> road). I continue to think that if such a field was added to "create",
>>> it would be an input (only).
>> During the Xen Community Call on Tuesday, we briefly spoke about this.
>> Andrew confirmed that what he meant with his original email is to use a
>> domctl. We didn't discuss which domctl specifically.
>>
>> This patch now follows the same pattern of clock_frequency and
>> gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig).
>> Note that gic_version is an IN/OUT parameter, showing that if in the
>> future we want the ability to set gpaddr_bits (in addition to get
>> gpaddr_bits), it will be possible.
> Well, as said before - I'm not convinced gic_version being IN/OUT is
> appropriate. At the very least a 2nd way to merely retrieve the value
> would seem to be necessary, so that it's not only the party creating
> the guest which would be able to know.
>
> Since here's we're solely after retrieving the value, I don't see
> the point in altering "create". As you say, domctl can be changed,
> and hence at the point this needs to become an input to "create", it
> could easily be added there.
>
> Jan

Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be 
reused to retrieve gpaddr_bits? I don't see why not at the moment, but 
maybe there are some implications/concerns which are invisible to me.

I see that arch_get_domain_info() is present, so the field will be 
common, and each arch will write a value it considers
appropriate. This could be a good compromise to not add an extra domctl 
and to not alter domain_create.


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Jan Beulich]

On 08.10.2021 12:25, Oleksandr wrote:
> Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be 
> reused to retrieve gpaddr_bits? I don't see why not at the moment, but 
> maybe there are some implications/concerns which are invisible to me.
> 
> I see that arch_get_domain_info() is present, so the field will be 
> common, and each arch will write a value it considers
> appropriate. This could be a good compromise to not add an extra domctl 
> and to not alter domain_create.

Technically I think it could be reused. What I'm less certain of is
whether the specific piece of information is a good fit there.

Jan

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr]

On 08.10.21 15:36, Jan Beulich wrote:

Hi Jan

> On 08.10.2021 12:25, Oleksandr wrote:
>> Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be
>> reused to retrieve gpaddr_bits? I don't see why not at the moment, but
>> maybe there are some implications/concerns which are invisible to me.
>>
>> I see that arch_get_domain_info() is present, so the field will be
>> common, and each arch will write a value it considers
>> appropriate. This could be a good compromise to not add an extra domctl
>> and to not alter domain_create.
> Technically I think it could be reused. What I'm less certain of is
> whether the specific piece of information is a good fit there.

ok, thank you for your answer.

I am also not 100% sure whether it is a *good* fit there, but I cannot 
say it is not fit at all for being there. I might mistake, but it is 
almost the same piece of information describing the whole domain as 
other existing fields in that structure.



>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Stefano Stabellini]

On Fri, 8 Oct 2021, Oleksandr wrote:
> On 08.10.21 15:36, Jan Beulich wrote:
> > On 08.10.2021 12:25, Oleksandr wrote:
> > > Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be
> > > reused to retrieve gpaddr_bits? I don't see why not at the moment, but
> > > maybe there are some implications/concerns which are invisible to me.
> > > 
> > > I see that arch_get_domain_info() is present, so the field will be
> > > common, and each arch will write a value it considers
> > > appropriate. This could be a good compromise to not add an extra domctl
> > > and to not alter domain_create.
> > Technically I think it could be reused. What I'm less certain of is
> > whether the specific piece of information is a good fit there.
> 
> ok, thank you for your answer.
> 
> I am also not 100% sure whether it is a *good* fit there, but I cannot say it
> is not fit at all for being there. I might mistake, but it is almost the same
> piece of information describing the whole domain as other existing fields in
> that structure.

From a domctl point of view, it looks like XEN_DOMCTL_getdomaininfo
could be a decent fit. Looking at the data structure, the arch specific
member of struct xen_domctl_getdomaininfo is:

  struct xen_arch_domainconfig arch_config;

which is actually the very same struct used in struct
xen_domctl_createdomain for XEN_DOMCTL_createdomain, but somehow it
doesn't get populated by neither the x86 nor the ARM version of
arch_get_domain_info?


In any case, I think we could make use of XEN_DOMCTL_getdomaininfo for
this. In that case, I would add a new common field to struct
xen_domctl_getdomaininfo after cpupool and above arch_config.

Then we can set the field from arch_get_domain_info.

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support

[Julien Grall]

Hi Oleksandr,

On 07/10/2021 21:29, Oleksandr wrote:
>>>>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct 
>>>>> domain
>>>>> *d,
>>>>>            u64 start = ext_regions->bank[i].start;
>>>>>            u64 size = ext_regions->bank[i].size;
>>>>>    -        dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>> -                   i, start, start + size);
>>>>> +        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>> +               i, start, start + size);
>>>> Also should be PRIpaddr
>>> I thought I needed to change specifier only for variables of type 
>>> "paddr_t",
>>> but here "u64".
>> Sorry, you are right.
>>
>> I added my reviewed-by and made the small typo changes on commit.
> 
> Thanks! In case if you haven't committed the patch yet, let's please 
> wait for Julien (who asked for this follow-up) to review it.

I went through it. The change looks good to me. No need for...

> 
> In any case, I will be able to do another follow-up if needed.

... another follow-up. Thanks for sending a patch to handle my requests! :)

Cheers,

-- 
Julien Grall

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr]

On 09.10.21 01:14, Stefano Stabellini wrote:

Hi Stefano

> On Fri, 8 Oct 2021, Oleksandr wrote:
>> On 08.10.21 15:36, Jan Beulich wrote:
>>> On 08.10.2021 12:25, Oleksandr wrote:
>>>> Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be
>>>> reused to retrieve gpaddr_bits? I don't see why not at the moment, but
>>>> maybe there are some implications/concerns which are invisible to me.
>>>>
>>>> I see that arch_get_domain_info() is present, so the field will be
>>>> common, and each arch will write a value it considers
>>>> appropriate. This could be a good compromise to not add an extra domctl
>>>> and to not alter domain_create.
>>> Technically I think it could be reused. What I'm less certain of is
>>> whether the specific piece of information is a good fit there.
>> ok, thank you for your answer.
>>
>> I am also not 100% sure whether it is a *good* fit there, but I cannot say it
>> is not fit at all for being there. I might mistake, but it is almost the same
>> piece of information describing the whole domain as other existing fields in
>> that structure.
>  From a domctl point of view, it looks like XEN_DOMCTL_getdomaininfo
> could be a decent fit. Looking at the data structure, the arch specific
> member of struct xen_domctl_getdomaininfo is:
>
>    struct xen_arch_domainconfig arch_config;
>
> which is actually the very same struct used in struct
> xen_domctl_createdomain for XEN_DOMCTL_createdomain, but somehow it
> doesn't get populated by neither the x86 nor the ARM version of
> arch_get_domain_info?
>
>
> In any case, I think we could make use of XEN_DOMCTL_getdomaininfo for
> this. In that case, I would add a new common field to struct
> xen_domctl_getdomaininfo after cpupool and above arch_config.
>
> Then we can set the field from arch_get_domain_info.

Yes, this is what I had in mind, thank you.


-- 
Regards,

Oleksandr Tyshchenko