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

[RFC PATCH 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].

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 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.
- The ACPI case is not covered.

Also please note that we haven't figured out yet how to properly 
extend the Xen hypervisor device-tree bindings on Arm (either via new
compatible or via new property). I decided to go with new property
for now, but this can be changed. This uncertainty is the main reason
why this series is marked as RFC.

Patch series is also available at [2].

The corresponding Linux changes is not in a good shape now (require
some cleanup and refactoring), I will publish them once put them
in order (I hope, it will be in a few days).

[1] https://lore.kernel.org/xen-devel/1627489110-25633-1-git-send-email-olekstysh@gmail.com/
[2] https://github.com/otyshchenko1/xen/commits/map_opt_ml2

Oleksandr Tyshchenko (3):
  xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo
  xen/arm: Add handling of extended regions for Dom0
  toolstack/arm: Add handling of extended regions for DomU

 tools/include/libxl.h            |   7 ++
 tools/libs/light/libxl.c         |   2 +
 tools/libs/light/libxl_arm.c     |  92 +++++++++++++++-
 tools/libs/light/libxl_types.idl |   2 +
 xen/arch/arm/domain_build.c      | 233 ++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/sysctl.c            |   2 +
 xen/arch/x86/sysctl.c            |   2 +
 xen/include/public/sysctl.h      |   3 +-
 8 files changed, 338 insertions(+), 5 deletions(-)

-- 
2.7.4

[RFC PATCH 1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to pass info about maximum supported guest 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 subsequents
patch.

Use p2m_ipa_bits variable on Arm, the x86 equivalent is
hap_paddr_bits.

As we change the size of structure bump the interface version.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes since RFC:
   - update patch subject/description
   - replace arch-specific sub-struct with common gpaddr_bits
     field and update code to reflect that
---
 tools/include/libxl.h            | 7 +++++++
 tools/libs/light/libxl.c         | 2 ++
 tools/libs/light/libxl_types.idl | 2 ++
 xen/arch/arm/sysctl.c            | 2 ++
 xen/arch/x86/sysctl.c            | 2 ++
 xen/include/public/sysctl.h      | 3 ++-
 6 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d..da44944 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -855,6 +855,13 @@ typedef struct libxl__ctx libxl_ctx;
  */
 #define LIBXL_HAVE_PHYSINFO_MAX_POSSIBLE_MFN 1
 
+ /*
+  * LIBXL_HAVE_PHYSINFO_GPADDR_BITS
+  *
+  * If this is defined, libxl_physinfo has a "gpaddr_bits" field.
+  */
+ #define LIBXL_HAVE_PHYSINFO_GPADDR_BITS 1
+
 /*
  * LIBXL_HAVE_DOMINFO_OUTSTANDING_MEMKB 1
  *
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 204eb0b..c86624f 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -405,6 +405,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     physinfo->cap_vmtrace =
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vmtrace);
 
+    physinfo->gpaddr_bits = xcphysinfo.gpaddr_bits;
+
     GC_FREE;
     return 0;
 }
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff6..f7437e4 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1061,6 +1061,8 @@ libxl_physinfo = Struct("physinfo", [
     ("cap_shadow", bool),
     ("cap_iommu_hap_pt_share", bool),
     ("cap_vmtrace", bool),
+
+    ("gpaddr_bits", uint32),
     ], dir=DIR_OUT)
 
 libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index f87944e..91dca4f 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -15,6 +15,8 @@
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
 {
     pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
+
+    pi->gpaddr_bits = p2m_ipa_bits;
 }
 
 long arch_do_sysctl(struct xen_sysctl *sysctl,
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index aff52a1..7b14865 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -135,6 +135,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
     if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
+
+    pi->gpaddr_bits = hap_paddr_bits;
 }
 
 long arch_do_sysctl(
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 039ccf8..f53b42e 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@
 #include "domctl.h"
 #include "physdev.h"
 
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
 
 /*
  * Read console content from Xen buffer ring.
@@ -120,6 +120,7 @@ struct xen_sysctl_physinfo {
     uint64_aligned_t outstanding_pages;
     uint64_aligned_t max_mfn; /* Largest possible MFN on this host */
     uint32_t hw_cap[8];
+    uint32_t gpaddr_bits;
 };
 
 /*
-- 
2.7.4

[RFC PATCH 2/3] xen/arm: Add handling of extended regions for Dom0

[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, introduce new "extended-region" property to inform
domain about the presence of additional regions (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.
- The ACPI case is not covered.

***

As Dom0 is direct mapped domain on Arm (e.g. MFN == GFN)
the algorithm to choose extended regions for it is different
in comparison with the algorithm for non-direct mapped DomU.
What is more, that extended regions should be chosen differently
whether IOMMU is enabled or not.

Provide RAM not assigned to Dom0 if IOMMU is disabled or memory
holes found in host device-tree if otherwise. Make sure that
extended regions are 2MB-aligned and located within maximum possible
addressable physical memory range. The maximum number of extended
regions is 128.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/arch/arm/domain_build.c | 233 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 231 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 206038d..f9453de 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -724,6 +724,196 @@ static int __init make_memory_node(const struct domain *d,
     return res;
 }
 
+static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
+{
+    struct meminfo *ext_regions = data;
+    paddr_t start, size;
+
+    if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
+        return 0;
+
+    /* Both start and size of the extended region should be 2MB aligned */
+    start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
+    if ( start > e )
+        return 0;
+
+    size = (e - start + 1) & ~(SZ_2M - 1);
+    if ( !size )
+        return 0;
+
+    ext_regions->bank[ext_regions->nr_banks].start = start;
+    ext_regions->bank[ext_regions->nr_banks].size = size;
+    ext_regions->nr_banks ++;
+
+    return 0;
+}
+
+/*
+ * The extended regions will be prevalidated by the memory hotplug path
+ * in Linux which requires for any added address range to be within maximum
+ * possible addressable physical memory range for which the linear mapping
+ * could be created.
+ * For 48-bit VA space size the maximum addressable range are:
+ * 0x40000000 - 0x80003fffffff
+ */
+#define EXT_REGION_START   0x40000000ULL
+#define EXT_REGION_END     0x80003fffffffULL
+
+static int __init find_unallocated_memory(const struct kernel_info *kinfo,
+                                          struct meminfo *ext_regions)
+{
+    const struct meminfo *assign_mem = &kinfo->mem;
+    struct rangeset *unalloc_mem;
+    paddr_t start, end;
+    unsigned int i;
+    int res;
+
+    dt_dprintk("Find unallocated memory for extended regions\n");
+
+    unalloc_mem = rangeset_new(NULL, NULL, 0);
+    if ( !unalloc_mem )
+        return -ENOMEM;
+
+    /* Start with all available RAM */
+    for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
+    {
+        start = bootinfo.mem.bank[i].start;
+        end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size - 1;
+        res = rangeset_add_range(unalloc_mem, start, end);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
+                   start, end);
+            goto out;
+        }
+    }
+
+    /* Remove RAM assigned to Dom0 */
+    for ( i = 0; i < assign_mem->nr_banks; i++ )
+    {
+        start = assign_mem->bank[i].start;
+        end = assign_mem->bank[i].start + assign_mem->bank[i].size - 1;
+        res = rangeset_remove_range(unalloc_mem, start, end);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+                   start, end);
+            goto out;
+        }
+    }
+
+    /* Remove reserved-memory regions */
+    for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
+    {
+        start = bootinfo.reserved_mem.bank[i].start;
+        end = bootinfo.reserved_mem.bank[i].start +
+            bootinfo.reserved_mem.bank[i].size - 1;
+        res = rangeset_remove_range(unalloc_mem, start, end);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+                   start, end);
+            goto out;
+        }
+    }
+
+    /* Remove grant table region */
+    start = kinfo->gnttab_start;
+    end = kinfo->gnttab_start + kinfo->gnttab_size - 1;
+    res = rangeset_remove_range(unalloc_mem, start, end);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+               start, end);
+        goto out;
+    }
+
+    start = EXT_REGION_START;
+    end = min((1ULL << p2m_ipa_bits) - 1, EXT_REGION_END);
+    res = rangeset_report_ranges(unalloc_mem, start, end,
+                                 add_ext_regions, ext_regions);
+    if ( res )
+        ext_regions->nr_banks = 0;
+    else if ( !ext_regions->nr_banks )
+        res = -ENOENT;
+
+out:
+    rangeset_destroy(unalloc_mem);
+
+    return res;
+}
+
+static int __init find_memory_holes(const struct kernel_info *kinfo,
+                                    struct meminfo *ext_regions)
+{
+    struct dt_device_node *np;
+    struct rangeset *mem_holes;
+    paddr_t start, end;
+    unsigned int i;
+    int res;
+
+    dt_dprintk("Find memory holes for extended regions\n");
+
+    mem_holes = rangeset_new(NULL, NULL, 0);
+    if ( !mem_holes )
+        return -ENOMEM;
+
+    /* Start with maximum possible addressable physical memory range */
+    start = EXT_REGION_START;
+    end = min((1ULL << p2m_ipa_bits) - 1, EXT_REGION_END);
+    res = rangeset_add_range(mem_holes, start, end);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
+               start, end);
+        goto out;
+    }
+
+    /* Remove all regions described by "reg" property (MMIO, RAM, etc) */
+    dt_for_each_device_node( dt_host, np )
+    {
+        unsigned int naddr;
+        u64 addr, size;
+
+        naddr = dt_number_of_address(np);
+
+        for ( i = 0; i < naddr; i++ )
+        {
+            res = dt_device_get_address(np, i, &addr, &size);
+            if ( res )
+            {
+                printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+                       i, dt_node_full_name(np));
+                goto out;
+            }
+
+            start = addr & PAGE_MASK;
+            end = PAGE_ALIGN(addr + size) - 1;
+            res = rangeset_remove_range(mem_holes, start, end);
+            if ( res )
+            {
+                printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n",
+                       start, end);
+                goto out;
+            }
+        }
+    }
+
+    start = EXT_REGION_START;
+    end = min((1ULL << p2m_ipa_bits) - 1, EXT_REGION_END);
+    res = rangeset_report_ranges(mem_holes, start, end,
+                                 add_ext_regions,  ext_regions);
+    if ( res )
+        ext_regions->nr_banks = 0;
+    else if ( !ext_regions->nr_banks )
+        res = -ENOENT;
+
+out:
+    rangeset_destroy(mem_holes);
+
+    return res;
+}
+
 static int __init make_hypervisor_node(struct domain *d,
                                        const struct kernel_info *kinfo,
                                        int addrcells, int sizecells)
@@ -731,11 +921,13 @@ static int __init make_hypervisor_node(struct domain *d,
     const char compat[] =
         "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
         "xen,xen";
-    __be32 reg[4];
+    __be32 reg[(NR_MEM_BANKS + 1) * 4];
     gic_interrupt_t intr;
     __be32 *cells;
     int res;
     void *fdt = kinfo->fdt;
+    struct meminfo *ext_regions;
+    unsigned int i;
 
     dt_dprintk("Create hypervisor node\n");
 
@@ -757,15 +949,52 @@ static int __init make_hypervisor_node(struct domain *d,
     if ( res )
         return res;
 
+    ext_regions = xzalloc(struct meminfo);
+    if ( !ext_regions )
+        return -ENOMEM;
+
+    if ( is_32bit_domain(d) )
+        printk(XENLOG_WARNING "The extended region is only supported for 64-bit guest\n");
+    else
+    {
+        if ( !is_iommu_enabled(d) )
+            res = find_unallocated_memory(kinfo, ext_regions);
+        else
+            res = find_memory_holes(kinfo, ext_regions);
+
+        if ( res )
+            printk(XENLOG_WARNING "Failed to allocate extended regions\n");
+    }
+
     /* reg 0 is grant table space */
     cells = &reg[0];
     dt_child_set_range(&cells, addrcells, sizecells,
                        kinfo->gnttab_start, kinfo->gnttab_size);
+    /* reg 1...N are extended regions */
+    for ( i = 0; i < ext_regions->nr_banks; i++ )
+    {
+        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);
+
+        dt_child_set_range(&cells, addrcells, sizecells, start, size);
+    }
+    xfree(ext_regions);
+
     res = fdt_property(fdt, "reg", reg,
-                       dt_cells_to_size(addrcells + sizecells));
+                       dt_cells_to_size(addrcells + sizecells) * (i + 1));
     if ( res )
         return res;
 
+    if ( i > 0 )
+    {
+        res = fdt_property(fdt, "extended-region", NULL, 0);
+        if ( res )
+            return res;
+    }
+
     BUG_ON(d->arch.evtchn_irq == 0);
 
     /*
-- 
2.7.4

[RFC PATCH 3/3] toolstack/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, introduce new "extended-region" property to inform
domain about the presence of additional regions (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.
- 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. As we have a lot of unused space above 4GB, provide single
1GB-aligned region from the second RAM bank taking into the account
the maximum supported guest address space size and the amount of
memory assigned to the guest. The maximum size of the region is 128GB.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 tools/libs/light/libxl_arm.c | 92 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6..41cf017 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -615,9 +615,23 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
                               "xen,xen");
     if (res) return res;
 
-    /* reg 0 is grant table space */
+    /*
+     * reg 0 is a placeholder for grant table space, reg 1 is a placeholder
+     * for the extended region.
+     */
     res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+                            2, 0, 0, 0, 0);
+    if (res) return res;
+
+    /*
+     * TODO: It would be correct to advertise that we provide the extended
+     * region after allocating it in finalise_ext_region(). The reason why we do
+     * this right now (in advance) is because we cannot insert a properly in
+     * finalise_ext_region(), but we have a mechanism to remove it there.
+     * So, if we are not able to allocate the region later on, we will remove
+     * the property.
+     */
+    res = fdt_property(fdt, "extended-region", NULL, 0);
     if (res) return res;
 
     /*
@@ -1069,6 +1083,78 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
     }
 }
 
+#define ALIGN_UP_TO_GB(x)   (((x) + GB(1) - 1) & (~(GB(1) - 1)))
+
+#define EXT_REGION_SIZE   GB(128)
+
+static void finalise_ext_region(libxl__gc *gc, struct xc_dom_image *dom)
+{
+    void *fdt = dom->devicetree_blob;
+    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
+    be32 *cells = &regs[0];
+    uint64_t region_size = 0, region_base, bank1end_align, bank1end_max;
+    uint32_t gpaddr_bits;
+    libxl_physinfo info;
+    int offset, rc;
+
+    offset = fdt_path_offset(fdt, "/hypervisor");
+    assert(offset > 0);
+
+    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
+        LOG(WARN, "The extended region is only supported for 64-bit guest");
+        goto out;
+    }
+
+    rc = libxl_get_physinfo(CTX, &info);
+    assert(!rc);
+
+    gpaddr_bits = info.gpaddr_bits;
+    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
+
+    /*
+     * Try to allocate single 1GB-aligned extended region from the second RAM
+     * bank (above 4GB) taking into the account the maximum supported guest
+     * address space size and the amount of memory assigned to the guest.
+     * The maximum size of the region is 128GB.
+     */
+    bank1end_max = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
+    bank1end_align = GUEST_RAM1_BASE +
+        ALIGN_UP_TO_GB((uint64_t)dom->rambank_size[1] << XC_PAGE_SHIFT);
+
+    if (bank1end_max <= bank1end_align) {
+        LOG(WARN, "The extended region cannot be allocated, not enough space");
+        goto out;
+    }
+
+    if (bank1end_max - bank1end_align > EXT_REGION_SIZE) {
+        region_base = bank1end_max - EXT_REGION_SIZE;
+        region_size = EXT_REGION_SIZE;
+    } else {
+        region_base = bank1end_align;
+        region_size = bank1end_max - bank1end_align;
+    }
+
+out:
+    /*
+     * The first region for grant table space must be always present.
+     * If we managed to allocate the extended region then insert it as
+     * a second region. Otherwise, remove the property that advertises it.
+     */
+    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+    if (region_size > 0) {
+        LOG(DEBUG, "Extended region: %#"PRIx64"->%#"PRIx64"\n",
+            region_base, region_base + region_size);
+
+        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                  region_base, region_size);
+    } else
+        fdt_nop_property(fdt, offset, "extended-region");
+
+    rc = fdt_setprop_inplace(fdt, offset, "reg", regs, sizeof(regs));
+    assert(!rc);
+}
+
 int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
                                                uint32_t domid,
                                                libxl_domain_config *d_config,
@@ -1109,6 +1195,8 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
 
     }
 
+    finalise_ext_region(gc, dom);
+
     for (i = 0; i < GUEST_RAM_BANKS; i++) {
         const uint64_t size = (uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT;
 
-- 
2.7.4

Re: [RFC PATCH 1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

[Andrew Cooper]

On 07/09/2021 18:09, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> We need to pass info about maximum supported guest 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 subsequents
> patch.
>
> Use p2m_ipa_bits variable on Arm, the x86 equivalent is
> hap_paddr_bits.
>
> As we change the size of structure bump the interface version.
>
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

So while I think this is a suitable way forward, you're painting
yourself into a corner WRT migration.

On x86, the correct value is d->arch.cpuid->extd.maxphysaddr and this
value is under toolstack control, not Xen control.  In particular, it
needs to be min(hostA, hostB) to make migration safe, and yes - this is
currently a hole in x86's migration logic that will cause large VMs to
explode.

The same will be true on ARM as/when you gain migration support.

I think this would be better as a domctl.  On ARM, it can reference
p2m_ipa_bits for now along with a /* TODO - make per-domain for
migration support */, while on x86 it can take the appropriate value
(which will soon actually be safe in migration scenarios).

However, that does change the ordering requirements in the toolstack -
this hypercall would need to be made after the domain is created, and
has been levelled, and before its main memory layout is decided.

Alternatively, the abstraction could be hidden in libxl itself in arch
specific code, with x86 referring to the local cpu policy (as libxl has
the copy it is/has worked on), and ARM making a hypercall.  This does
make the ordering more obvious.

(As a note on the x86 specifics of this patch, hap_paddr_bits is
actually unused in practice.  It was a proposal from AMD for the
hypervisor to fill in those details, which wasn't implemented by anyone,
not even Xen, because the important field to modify is maxphysaddr if
you don't want to rewrite every kernel to behave differently when
virtualised.)

~Andrew

Re: [RFC PATCH 1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

[Oleksandr]

On 07.09.21 20:35, Andrew Cooper wrote:

Hi Andrew

> On 07/09/2021 18:09, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to pass info about maximum supported guest 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 subsequents
>> patch.
>>
>> Use p2m_ipa_bits variable on Arm, the x86 equivalent is
>> hap_paddr_bits.
>>
>> As we change the size of structure bump the interface version.
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> So while I think this is a suitable way forward, you're painting
> yourself into a corner WRT migration.
>
> On x86, the correct value is d->arch.cpuid->extd.maxphysaddr and this
> value is under toolstack control, not Xen control.  In particular, it
> needs to be min(hostA, hostB) to make migration safe, and yes - this is
> currently a hole in x86's migration logic that will cause large VMs to
> explode.
>
> The same will be true on ARM as/when you gain migration support.

Oh, I admit, I didn't keep in mind migration support while creating this 
patch.


>
> I think this would be better as a domctl.  On ARM, it can reference
> p2m_ipa_bits for now along with a /* TODO - make per-domain for
> migration support */, while on x86 it can take the appropriate value
> (which will soon actually be safe in migration scenarios).

OK, could you please clarify, did you mean to introduce new domctl 
(something like get_maxphysaddr) or reuse some existing?

And ...

>
> However, that does change the ordering requirements in the toolstack -
> this hypercall would need to be made after the domain is created, and
> has been levelled, and before its main memory layout is decided.

... I am not sure I totally understand the ordering requirements in the 
toolstack.

On Arm this information (gpaddr_bits) should be obtained by the time we 
call libxl__arch_domain_finalise_hw_description()
where we actually calculate extended region and insert it in domain's 
device-tree. At that time, the domain memory is already populated, so 
it's layout is known.
Please see the last patch of this series, which demonstrates the usage:

https://lore.kernel.org/xen-devel/1631034578-12598-4-git-send-email-olekstysh@gmail.com/


> Alternatively, the abstraction could be hidden in libxl itself in arch
> specific code, with x86 referring to the local cpu policy (as libxl has
> the copy it is/has worked on), and ARM making a hypercall.  This does
> make the ordering more obvious.

May I please ask what would be the preferred option to move forward? I 
am fine with both proposed options, with a little preference for the 
former (common domctl, abstraction is hidden in Xen itself in arch 
specific code). It is highly appreciated if we could choose the option 
which would satisfy all parties, this would save me some time.

> (As a note on the x86 specifics of this patch, hap_paddr_bits is
> actually unused in practice.  It was a proposal from AMD for the
> hypervisor to fill in those details, which wasn't implemented by anyone,
> not even Xen, because the important field to modify is maxphysaddr if
> you don't want to rewrite every kernel to behave differently when
> virtualised.)

Thank you for the clarification.


>
> ~Andrew
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Oleksandr]

Hello all

On 07.09.21 20:09, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> You can find an initial discussion at [1].
>
> 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 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.
> - The ACPI case is not covered.
>
> Also please note that we haven't figured out yet how to properly
> extend the Xen hypervisor device-tree bindings on Arm (either via new
> compatible or via new property). I decided to go with new property
> for now, but this can be changed. This uncertainty is the main reason
> why this series is marked as RFC.

Sorry, I messed up the device tree binding's purpose here.

New DT property "extended-region" (to inform guest about the presence of 
additional regions in "reg" property) is not really needed. Guest can 
simply infer that from the number of regions
in "reg" property (region 0 - reserved for grant table space, regions 
1...n - extended regions).
Instead, new compatible/property will be needed (but only after this 
patch [1] 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.

Thanks to Julien for clarifying these bits.

I am going to remove the advertisement of unneeded "extended-region" 
property in the code and send new version soon.

[1] 
https://lore.kernel.org/xen-devel/1631228688-30347-1-git-send-email-olekstysh@gmail.com/


>
> Patch series is also available at [2].
>
> The corresponding Linux changes is not in a good shape now (require
> some cleanup and refactoring), I will publish them once put them
> in order (I hope, it will be in a few days).
>
> [1] https://lore.kernel.org/xen-devel/1627489110-25633-1-git-send-email-olekstysh@gmail.com/
> [2] https://github.com/otyshchenko1/xen/commits/map_opt_ml2
>
> Oleksandr Tyshchenko (3):
>    xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo
>    xen/arm: Add handling of extended regions for Dom0
>    toolstack/arm: Add handling of extended regions for DomU
>
>   tools/include/libxl.h            |   7 ++
>   tools/libs/light/libxl.c         |   2 +
>   tools/libs/light/libxl_arm.c     |  92 +++++++++++++++-
>   tools/libs/light/libxl_types.idl |   2 +
>   xen/arch/arm/domain_build.c      | 233 ++++++++++++++++++++++++++++++++++++++-
>   xen/arch/arm/sysctl.c            |   2 +
>   xen/arch/x86/sysctl.c            |   2 +
>   xen/include/public/sysctl.h      |   3 +-
>   8 files changed, 338 insertions(+), 5 deletions(-)
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Oleksandr]

Hello all


On 10.09.21 02:20, Oleksandr wrote:
>
> Hello all
>
> On 07.09.21 20:09, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> You can find an initial discussion at [1].
>>
>> 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 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.
>> - The ACPI case is not covered.
>>
>> Also please note that we haven't figured out yet how to properly
>> extend the Xen hypervisor device-tree bindings on Arm (either via new
>> compatible or via new property). I decided to go with new property
>> for now, but this can be changed. This uncertainty is the main reason
>> why this series is marked as RFC.
>
> Sorry, I messed up the device tree binding's purpose here.
>
> New DT property "extended-region" (to inform guest about the presence 
> of additional regions in "reg" property) is not really needed. Guest 
> can simply infer that from the number of regions
> in "reg" property (region 0 - reserved for grant table space, regions 
> 1...n - extended regions).
> Instead, new compatible/property will be needed (but only after this 
> patch [1] 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.
>
> Thanks to Julien for clarifying these bits.
>
> I am going to remove the advertisement of unneeded "extended-region" 
> property in the code and send new version soon.

I have just pushed new version [1]. Please ignore this one. Sorry for 
the inconvenience.

[1] 
https://lore.kernel.org/xen-devel/1631297924-8658-1-git-send-email-olekstysh@gmail.com/


-- 
Regards,

Oleksandr Tyshchenko

Re: [RFC PATCH 1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

[Oleksandr]

Hi Andrew.


On 08.09.21 00:28, Oleksandr wrote:
>
> On 07.09.21 20:35, Andrew Cooper wrote:
>
> Hi Andrew
>
>> On 07/09/2021 18:09, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> We need to pass info about maximum supported guest 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 subsequents
>>> patch.
>>>
>>> Use p2m_ipa_bits variable on Arm, the x86 equivalent is
>>> hap_paddr_bits.
>>>
>>> As we change the size of structure bump the interface version.
>>>
>>> Suggested-by: Julien Grall <jgrall@amazon.com>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> So while I think this is a suitable way forward, you're painting
>> yourself into a corner WRT migration.
>>
>> On x86, the correct value is d->arch.cpuid->extd.maxphysaddr and this
>> value is under toolstack control, not Xen control.  In particular, it
>> needs to be min(hostA, hostB) to make migration safe, and yes - this is
>> currently a hole in x86's migration logic that will cause large VMs to
>> explode.
>>
>> The same will be true on ARM as/when you gain migration support.
>
> Oh, I admit, I didn't keep in mind migration support while creating 
> this patch.
>
>
>>
>> I think this would be better as a domctl.  On ARM, it can reference
>> p2m_ipa_bits for now along with a /* TODO - make per-domain for
>> migration support */, while on x86 it can take the appropriate value
>> (which will soon actually be safe in migration scenarios).
>
> OK, could you please clarify, did you mean to introduce new domctl 
> (something like get_maxphysaddr) or reuse some existing?


May I please ask for clarification here ^ and ...


>
>
> And ...
>
>>
>> However, that does change the ordering requirements in the toolstack -
>> this hypercall would need to be made after the domain is created, and
>> has been levelled, and before its main memory layout is decided.
>
> ... I am not sure I totally understand the ordering requirements in 
> the toolstack.
>
> On Arm this information (gpaddr_bits) should be obtained by the time 
> we call libxl__arch_domain_finalise_hw_description()
> where we actually calculate extended region and insert it in domain's 
> device-tree. At that time, the domain memory is already populated, so 
> it's layout is known.
> Please see the last patch of this series, which demonstrates the usage:
>
> https://lore.kernel.org/xen-devel/1631034578-12598-4-git-send-email-olekstysh@gmail.com/ 
>
>
>
>> Alternatively, the abstraction could be hidden in libxl itself in arch
>> specific code, with x86 referring to the local cpu policy (as libxl has
>> the copy it is/has worked on), and ARM making a hypercall.  This does
>> make the ordering more obvious.
>
> May I please ask what would be the preferred option to move forward? I 
> am fine with both proposed options, with a little preference for the 
> former (common domctl, abstraction is hidden in Xen itself in arch 
> specific code). It is highly appreciated if we could choose the option 
> which would satisfy all parties, this would save me some time.


... here ^ ?


I have just pushed third version [1] of this series without these being 
addressed. I decided to push V3 to let the reviewers to focus on the 
main aspects, but the way how the information in question is passed to 
the toolstack also needs to be clarified and addressed.


[1] 
https://lore.kernel.org/xen-devel/1632437334-12015-1-git-send-email-olekstysh@gmail.com/


[snip]


-- 
Regards,

Oleksandr Tyshchenko