[PATCH v3 0/2] arm: extended regions fixes

[PATCH v3 0/2] arm: extended regions fixes

[Stewart Hildebrand]

v2->v3:
* drop committed patches
* add ("xen/arm: exclude xen,reg from direct-map domU extended regions")

v1->v2:
* rebase
* address feedback

Stewart Hildebrand (2):
  xen/arm: exclude xen,reg from direct-map domU extended regions
  tools/arm: exclude iomem from domU extended regions

 tools/libs/light/libxl_arm.c | 118 +++++++++++++++++++++++++++++------
 xen/arch/arm/domain_build.c  |  57 +++++++++++++++--
 xen/common/rangeset.c        |  14 +++++
 xen/include/xen/rangeset.h   |   2 +
 4 files changed, 167 insertions(+), 24 deletions(-)


base-commit: 5873740e41acb8593f92623ddd03caebda2718f6
-- 
2.49.0

[PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Stewart Hildebrand]

Similarly to fba1b0974dd8, when a device is passed through to a
direct-map dom0less domU, the xen,reg ranges may overlap with the
extended regions. Remove xen,reg from direct-map domU extended regions.

Introduce rangeset_count_ranges().

Take the opportunity to update the comment ahead of find_memory_holes().

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v2->v3:
* new patch
---
 xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
 xen/common/rangeset.c       | 14 +++++++++
 xen/include/xen/rangeset.h  |  2 ++
 3 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae9f..3cdf5839bc98 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
 }
 
 /*
- * 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:
+ * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
+ * domU 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 aperture
  * - Static shared memory regions, which are described by special property
  *   "xen,shared-mem"
+ * - xen,reg mappings
  */
 static int __init find_memory_holes(const struct kernel_info *kinfo,
                                     struct membanks *ext_regions)
@@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
         }
     }
 
+    if ( kinfo->xen_reg_assigned )
+    {
+        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
+        if ( res )
+            goto out;
+    }
+
     start = 0;
     end = (1ULL << p2m_ipa_bits) - 1;
     res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
@@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
     return res;
 }
 
+static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
+                                      void *data)
+{
+    struct membanks *membank = data;
+    paddr_t s = pfn_to_paddr(s_gfn);
+    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
+
+    if ( membank->nr_banks >= membank->max_banks )
+        return 0;
+
+    membank->bank[membank->nr_banks].start = s;
+    membank->bank[membank->nr_banks].size = e - s + 1;
+    membank->nr_banks++;
+
+    return 0;
+}
+
 static int __init find_host_extended_regions(const struct kernel_info *kinfo,
                                              struct membanks *ext_regions)
 {
     int res;
     struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
+    struct membanks *xen_reg = membanks_xzalloc(
+        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
 
     /*
      * Exclude the following regions:
@@ -1006,6 +1034,7 @@ static int __init find_host_extended_regions(const struct kernel_info *kinfo,
      * 2) Remove reserved memory
      * 3) Grant table assigned to domain
      * 4) Remove static shared memory (when the feature is enabled)
+     * 5) Remove xen,reg
      */
     const struct membanks *mem_banks[] = {
         kernel_info_get_mem_const(kinfo),
@@ -1014,12 +1043,27 @@ static int __init find_host_extended_regions(const struct kernel_info *kinfo,
 #ifdef CONFIG_STATIC_SHM
         bootinfo_get_shmem(),
 #endif
+        xen_reg,
     };
 
     dt_dprintk("Find unallocated memory for extended regions\n");
 
     if ( !gnttab )
-        return -ENOMEM;
+    {
+        res = -ENOMEM;
+        goto out;
+    }
+
+    if ( !xen_reg )
+    {
+        res = -ENOMEM;
+        goto out;
+    }
+
+    if ( kinfo->xen_reg_assigned )
+        rangeset_report_ranges(kinfo->xen_reg_assigned, 0,
+                               PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
+                               rangeset_to_membank, xen_reg);
 
     gnttab->nr_banks = 1;
     gnttab->bank[0].start = kinfo->gnttab_start;
@@ -1027,6 +1071,9 @@ static int __init find_host_extended_regions(const struct kernel_info *kinfo,
 
     res = find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
                                   ext_regions, add_ext_regions);
+
+ out:
+    xfree(xen_reg);
     xfree(gnttab);
 
     return res;
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index b9e8912fb1c3..77b37ad9b196 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -433,6 +433,20 @@ bool rangeset_is_empty(
     return ((r == NULL) || list_empty(&r->range_list));
 }
 
+int rangeset_count_ranges(const struct rangeset *r)
+{
+    int nr = 0;
+    struct list_head *list;
+
+    if ( r == NULL )
+        return 0;
+
+    list_for_each( list, &r->range_list )
+        nr++;
+
+    return nr;
+}
+
 struct rangeset *rangeset_new(
     struct domain *d, const char *name, unsigned int flags)
 {
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 817505badf6f..96cc0b5d7930 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -56,6 +56,8 @@ void rangeset_limit(
 bool __must_check rangeset_is_empty(
     const struct rangeset *r);
 
+int __must_check rangeset_count_ranges(const struct rangeset *r);
+
 /* Add/claim/remove/query/purge a numeric range. */
 int __must_check rangeset_add_range(
     struct rangeset *r, unsigned long s, unsigned long e);
-- 
2.49.0

[PATCH v3 2/2] tools/arm: exclude iomem from domU extended regions

[Stewart Hildebrand]

When a device is passed through to a xl domU, the iomem ranges may
overlap with the extended regions. Remove iomem from extended regions.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Not sure if we need a Fixes: tag, but if we do:
Fixes: 57f87857dc2d ("libxl/arm: Add handling of extended regions for DomU")

v2->v3:
* no change

v1->v2:
* no change
---
 tools/libs/light/libxl_arm.c | 118 +++++++++++++++++++++++++++++------
 1 file changed, 99 insertions(+), 19 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 75c811053c7c..8ae16a1726fc 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -798,6 +798,8 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+#define MAX_NR_EXT_REGIONS   256
+
 static int make_hypervisor_node(libxl__gc *gc, void *fdt,
                                 const libxl_version_info *vers)
 {
@@ -821,7 +823,7 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
      */
     res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
                                        GUEST_ROOT_SIZE_CELLS,
-                                       GUEST_RAM_BANKS + 1);
+                                       MAX_NR_EXT_REGIONS + 1);
     if (res) return res;
 
     /*
@@ -1517,17 +1519,29 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
 
 #define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
 
-static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
+static int compare_iomem(const void *a, const void *b)
+{
+    const libxl_iomem_range *x = a, *y = b;
+
+    if (x->gfn < y->gfn)
+        return -1;
+    if (x->gfn > y->gfn)
+        return 1;
+    return 0;
+}
+
+static int finalize_hypervisor_node(libxl__gc *gc,
+                                    libxl_domain_build_info *b_info,
+                                    struct xc_dom_image *dom)
 {
     void *fdt = dom->devicetree_blob;
-    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
-        bankend[GUEST_RAM_BANKS];
+    uint64_t region_base[MAX_NR_EXT_REGIONS], region_size[MAX_NR_EXT_REGIONS];
     uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
-                  (GUEST_RAM_BANKS + 1)];
+                  (MAX_NR_EXT_REGIONS + 1)];
     be32 *cells = &regs[0];
     const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
     const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
-    unsigned int i, len, nr_regions = 0;
+    unsigned int i, j, len, nr_regions = 0;
     libxl_dominfo info;
     int offset, rc;
 
@@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
     if (info.gpaddr_bits > 64)
         return ERROR_INVAL;
 
+    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
+          compare_iomem);
+
     /*
      * Try to allocate separate 2MB-aligned extended regions from the first
      * and second RAM banks taking into the account the maximum supported
      * guest physical address space size and the amount of memory assigned
      * to the guest.
      */
-    for (i = 0; i < GUEST_RAM_BANKS; i++) {
-        region_base[i] = bankbase[i] +
+    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
+        struct {
+            uint64_t start;
+            uint64_t end; /* inclusive */
+        } unallocated;
+        uint64_t size = 0;
+
+        unallocated.start = bankbase[i] +
             ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
 
-        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
-        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
-        if (bankend[i] > region_base[i])
-            region_size[i] = bankend[i] - region_base[i] + 1;
+        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
+        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
+
+        if (unallocated.end > unallocated.start)
+            size = unallocated.end - unallocated.start + 1;
+
+        if (size < EXT_REGION_MIN_SIZE)
+            continue;
+
+        /* Exclude iomem */
+        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
+             j++) {
+            struct {
+                uint64_t start;
+                uint64_t end; /* inclusive */
+            } iomem;
+
+            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
+            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
+                         << XC_PAGE_SHIFT) - 1;
+
+            if (iomem.end >= unallocated.start
+                && iomem.start <= unallocated.end) {
+
+                if (iomem.start <= unallocated.start) {
+                    unallocated.start = iomem.end + 1;
+
+                    if (iomem.end >= unallocated.end)
+                        /* Complete overlap, discard unallocated region */
+                        break;
+
+                    /* Beginning overlap */
+                    continue;
+                }
+
+                if (iomem.start > unallocated.start) {
+                    assert(unallocated.end > unallocated.start);
+                    size = iomem.start - unallocated.start;
+
+                    if (size >= EXT_REGION_MIN_SIZE) {
+                        region_base[nr_regions] = unallocated.start;
+                        region_size[nr_regions] = size;
+                        nr_regions++;
+                    }
+
+                    unallocated.start = iomem.end + 1;
+
+                    if (iomem.end >= unallocated.end)
+                        /* End overlap, discard remaining unallocated region */
+                        break;
+                }
+            }
+        }
+
+        if (unallocated.end > unallocated.start
+            && nr_regions < MAX_NR_EXT_REGIONS)
+        {
+            size = unallocated.end - unallocated.start + 1;
+
+            if (size >= EXT_REGION_MIN_SIZE) {
+                region_base[nr_regions] = unallocated.start;
+                region_size[nr_regions] = size;
+                nr_regions++;
+            }
+        }
     }
 
     /*
@@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
     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] < EXT_REGION_MIN_SIZE)
-            continue;
-
+    for (i = 0; i < nr_regions; i++) {
         LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
-            nr_regions, region_base[i], region_base[i] + region_size[i]);
+            i, 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)
@@ -1626,7 +1706,7 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
 
     }
 
-    res = finalize_hypervisor_node(gc, dom);
+    res = finalize_hypervisor_node(gc, &d_config->b_info, dom);
     if (res)
         return res;
 
-- 
2.49.0

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Jan Beulich]

On 13.05.2025 21:54, Stewart Hildebrand wrote:
> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -433,6 +433,20 @@ bool rangeset_is_empty(
>      return ((r == NULL) || list_empty(&r->range_list));
>  }
>  
> +int rangeset_count_ranges(const struct rangeset *r)
> +{
> +    int nr = 0;

Ehem - this and the function's return type want to be unsigned.

> +    struct list_head *list;
> +
> +    if ( r == NULL )
> +        return 0;
> +
> +    list_for_each( list, &r->range_list )

Nit: Either you deem list_for_each a pseudo-keyword (then a blank is
missing) or you don't (then there are excess blanks).

Further I don't think this is valid to do without holding the rangeset's
lock in read mode (irrespective of the function return value potentially
being stale by the time the caller gets to look at it, which is no
different from other functions, i.e. falls in the caller's
responsibilities).

> +        nr++;

And then, if already abstraction is wanted, wouldn't this loop better be
yet another helper (macro?) in xen/list.h?

> +    return nr;
> +}

Finally: If this is to be commonly used in several places, having such a
helper is likely fine. As it stands, the sole caller is an __init
function, and hence this is unreachable code post-init (which while not
formally a Misra violation in my eyes effectively still is one). Aiui
the same can be achieved using rangeset_report_ranges(), with a new
(__init and static) callback function.

Jan

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Orzel, Michal]

On 13/05/2025 21:54, Stewart Hildebrand wrote:
> Similarly to fba1b0974dd8, when a device is passed through to a
> direct-map dom0less domU, the xen,reg ranges may overlap with the
> extended regions. Remove xen,reg from direct-map domU extended regions.
> 
> Introduce rangeset_count_ranges().
> 
> Take the opportunity to update the comment ahead of find_memory_holes().
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v2->v3:
> * new patch
> ---
>  xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
>  xen/common/rangeset.c       | 14 +++++++++
>  xen/include/xen/rangeset.h  |  2 ++
>  3 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b189a7cfae9f..3cdf5839bc98 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
>  }
>  
>  /*
> - * 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:
> + * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
> + * domU 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 aperture
>   * - Static shared memory regions, which are described by special property
>   *   "xen,shared-mem"
> + * - xen,reg mappings
>   */
>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>                                      struct membanks *ext_regions)
> @@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>          }
>      }
>  
> +    if ( kinfo->xen_reg_assigned )
> +    {
> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
> +        if ( res )
> +            goto out;
> +    }
> +
>      start = 0;
>      end = (1ULL << p2m_ipa_bits) - 1;
>      res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
> @@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>      return res;
>  }
>  
> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
> +                                      void *data)
> +{
> +    struct membanks *membank = data;
> +    paddr_t s = pfn_to_paddr(s_gfn);
> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
> +
> +    if ( membank->nr_banks >= membank->max_banks )
> +        return 0;
> +
> +    membank->bank[membank->nr_banks].start = s;
> +    membank->bank[membank->nr_banks].size = e - s + 1;
> +    membank->nr_banks++;
> +
> +    return 0;
> +}
> +
>  static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>                                               struct membanks *ext_regions)
>  {
>      int res;
>      struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
> +    struct membanks *xen_reg = membanks_xzalloc(
> +        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
You allocate at least 1 membank even though xen_reg_assigned may be empty because:
 - this function is called for hwdom - no xen,reg
 - there may be no xen,reg i.e. no passthrough

~Michal

Re: [PATCH v3 2/2] tools/arm: exclude iomem from domU extended regions

[Anthony PERARD]

On Tue, May 13, 2025 at 03:54:50PM -0400, Stewart Hildebrand wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 75c811053c7c..8ae16a1726fc 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>      if (info.gpaddr_bits > 64)
>          return ERROR_INVAL;
>  
> +    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
> +          compare_iomem);
> +
>      /*
>       * Try to allocate separate 2MB-aligned extended regions from the first
>       * and second RAM banks taking into the account the maximum supported
>       * guest physical address space size and the amount of memory assigned
>       * to the guest.
>       */
> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> -        region_base[i] = bankbase[i] +
> +    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
> +        struct {
> +            uint64_t start;
> +            uint64_t end; /* inclusive */
> +        } unallocated;
> +        uint64_t size = 0;
> +
> +        unallocated.start = bankbase[i] +
>              ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
>  
> -        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> -        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> -        if (bankend[i] > region_base[i])
> -            region_size[i] = bankend[i] - region_base[i] + 1;
> +        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
> +        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
> +
> +        if (unallocated.end > unallocated.start)
> +            size = unallocated.end - unallocated.start + 1;
> +
> +        if (size < EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        /* Exclude iomem */
> +        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
> +             j++) {
> +            struct {
> +                uint64_t start;
> +                uint64_t end; /* inclusive */
> +            } iomem;
> +
> +            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
> +            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
> +                         << XC_PAGE_SHIFT) - 1;
> +
> +            if (iomem.end >= unallocated.start
> +                && iomem.start <= unallocated.end) {
> +
> +                if (iomem.start <= unallocated.start) {
> +                    unallocated.start = iomem.end + 1;
> +
> +                    if (iomem.end >= unallocated.end)
> +                        /* Complete overlap, discard unallocated region */
> +                        break;
> +
> +                    /* Beginning overlap */
> +                    continue;

Instead of a `continue` and a comment that I don't understand what it is
supposed to mean, you could just do if-else:

if (iomem.start <= unallocated.start) {
    // code before this continue
} else { // we have: iomem.start > unallocated.start
    // the block of code bellow.
}

> +                }
> +
> +                if (iomem.start > unallocated.start) {
> +                    assert(unallocated.end > unallocated.start);

I think this assert should be removed.

Instead, you could check that this property hold true every time there's
a modification to `unallocated.start` in this function.

Maybe one way to make the algo easier to read, and to check that this
property is still true, is to rewrite:

    unallocated.start = iomem.end + 1;
    if (iomem.end >= unallocated.end)
        // discard `unallocated`
        break;

with

    unallocated.start = iomem.end + 1;
    if (unallocated.start > unallocated.end)
        // obvious: all allocated already
        break;

Because checking for:
    iomem.end >= unallocated.end
is the same as checking for:
    iomem.end + 1 > unallocated.end
    unallocated.start > unallocated.end

> +                    size = iomem.start - unallocated.start;

Isn't `size` the size of the unallocated region? Why is it recalculated
with `iomem`? I think it would be better to create a new variable.

> +
> +                    if (size >= EXT_REGION_MIN_SIZE) {
> +                        region_base[nr_regions] = unallocated.start;
> +                        region_size[nr_regions] = size;
> +                        nr_regions++;
> +                    }
> @@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>      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] < EXT_REGION_MIN_SIZE)
> -            continue;
> -
> +    for (i = 0; i < nr_regions; i++) {
>          LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> -            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +            i, region_base[i], region_base[i] + region_size[i]);

Shouldn't we print "base + size - 1" for the end address?

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Stewart Hildebrand]

On 5/14/25 03:31, Orzel, Michal wrote:
> On 13/05/2025 21:54, Stewart Hildebrand wrote:
>> Similarly to fba1b0974dd8, when a device is passed through to a
>> direct-map dom0less domU, the xen,reg ranges may overlap with the
>> extended regions. Remove xen,reg from direct-map domU extended regions.
>>
>> Introduce rangeset_count_ranges().
>>
>> Take the opportunity to update the comment ahead of find_memory_holes().
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> v2->v3:
>> * new patch
>> ---
>>  xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
>>  xen/common/rangeset.c       | 14 +++++++++
>>  xen/include/xen/rangeset.h  |  2 ++
>>  3 files changed, 68 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index b189a7cfae9f..3cdf5839bc98 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
>>  }
>>  
>>  /*
>> - * 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:
>> + * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
>> + * domU 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 aperture
>>   * - Static shared memory regions, which are described by special property
>>   *   "xen,shared-mem"
>> + * - xen,reg mappings
>>   */
>>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>>                                      struct membanks *ext_regions)
>> @@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>          }
>>      }
>>  
>> +    if ( kinfo->xen_reg_assigned )
>> +    {
>> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
>> +        if ( res )
>> +            goto out;
>> +    }
>> +
>>      start = 0;
>>      end = (1ULL << p2m_ipa_bits) - 1;
>>      res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
>> @@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>      return res;
>>  }
>>  
>> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
>> +                                      void *data)
>> +{
>> +    struct membanks *membank = data;
>> +    paddr_t s = pfn_to_paddr(s_gfn);
>> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
>> +
>> +    if ( membank->nr_banks >= membank->max_banks )
>> +        return 0;
>> +
>> +    membank->bank[membank->nr_banks].start = s;
>> +    membank->bank[membank->nr_banks].size = e - s + 1;
>> +    membank->nr_banks++;
>> +
>> +    return 0;
>> +}
>> +
>>  static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>>                                               struct membanks *ext_regions)
>>  {
>>      int res;
>>      struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
>> +    struct membanks *xen_reg = membanks_xzalloc(
>> +        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
> You allocate at least 1 membank even though xen_reg_assigned may be empty because:
>  - this function is called for hwdom - no xen,reg
>  - there may be no xen,reg i.e. no passthrough

Ah, sorry, there's no need to allocate at least 1. This can just be:

    struct membanks *xen_reg = membanks_xzalloc(
        rangeset_count_ranges(kinfo->arch.xen_reg_assigned), MEMORY);

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Orzel, Michal]

On 03/06/2025 23:15, Stewart Hildebrand wrote:
> On 5/14/25 03:31, Orzel, Michal wrote:
>> On 13/05/2025 21:54, Stewart Hildebrand wrote:
>>> Similarly to fba1b0974dd8, when a device is passed through to a
>>> direct-map dom0less domU, the xen,reg ranges may overlap with the
>>> extended regions. Remove xen,reg from direct-map domU extended regions.
>>>
>>> Introduce rangeset_count_ranges().
>>>
>>> Take the opportunity to update the comment ahead of find_memory_holes().
>>>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> ---
>>> v2->v3:
>>> * new patch
>>> ---
>>>  xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
>>>  xen/common/rangeset.c       | 14 +++++++++
>>>  xen/include/xen/rangeset.h  |  2 ++
>>>  3 files changed, 68 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index b189a7cfae9f..3cdf5839bc98 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
>>>  }
>>>  
>>>  /*
>>> - * 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:
>>> + * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
>>> + * domU 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 aperture
>>>   * - Static shared memory regions, which are described by special property
>>>   *   "xen,shared-mem"
>>> + * - xen,reg mappings
>>>   */
>>>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>                                      struct membanks *ext_regions)
>>> @@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>          }
>>>      }
>>>  
>>> +    if ( kinfo->xen_reg_assigned )
>>> +    {
>>> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
>>> +        if ( res )
>>> +            goto out;
>>> +    }
>>> +
>>>      start = 0;
>>>      end = (1ULL << p2m_ipa_bits) - 1;
>>>      res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
>>> @@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>>      return res;
>>>  }
>>>  
>>> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
>>> +                                      void *data)
>>> +{
>>> +    struct membanks *membank = data;
>>> +    paddr_t s = pfn_to_paddr(s_gfn);
>>> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
>>> +
>>> +    if ( membank->nr_banks >= membank->max_banks )
>>> +        return 0;
>>> +
>>> +    membank->bank[membank->nr_banks].start = s;
>>> +    membank->bank[membank->nr_banks].size = e - s + 1;
>>> +    membank->nr_banks++;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>>>                                               struct membanks *ext_regions)
>>>  {
>>>      int res;
>>>      struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
>>> +    struct membanks *xen_reg = membanks_xzalloc(
>>> +        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
>> You allocate at least 1 membank even though xen_reg_assigned may be empty because:
>>  - this function is called for hwdom - no xen,reg
>>  - there may be no xen,reg i.e. no passthrough
> 
> Ah, sorry, there's no need to allocate at least 1. This can just be:
> 
>     struct membanks *xen_reg = membanks_xzalloc(
>         rangeset_count_ranges(kinfo->arch.xen_reg_assigned), MEMORY);
No, it cannot. membanks_xzalloc() calls xzalloc_flex_struct(). If you pass 0
as size, the latter will calculate offset to FAM[0]. In other words, the
allocation will succeed but only for members up to FAM[0] (i.e. only for struct
membanks_hdr).

Also, even if you conditionally allocate for xen_reg_assigned or set NULL, in
latter case you will end up with mem_banks containing NULL member. AFAICT that's
not something expected by the users of mem_banks (+ it gives unneeded iteration).

~Michal

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Stewart Hildebrand]

On 6/4/25 03:00, Orzel, Michal wrote:
> On 03/06/2025 23:15, Stewart Hildebrand wrote:
>> On 5/14/25 03:31, Orzel, Michal wrote:
>>> On 13/05/2025 21:54, Stewart Hildebrand wrote:
>>>> Similarly to fba1b0974dd8, when a device is passed through to a
>>>> direct-map dom0less domU, the xen,reg ranges may overlap with the
>>>> extended regions. Remove xen,reg from direct-map domU extended regions.
>>>>
>>>> Introduce rangeset_count_ranges().
>>>>
>>>> Take the opportunity to update the comment ahead of find_memory_holes().
>>>>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>> ---
>>>> v2->v3:
>>>> * new patch
>>>> ---
>>>>  xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
>>>>  xen/common/rangeset.c       | 14 +++++++++
>>>>  xen/include/xen/rangeset.h  |  2 ++
>>>>  3 files changed, 68 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index b189a7cfae9f..3cdf5839bc98 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
>>>>  }
>>>>  
>>>>  /*
>>>> - * 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:
>>>> + * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
>>>> + * domU 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 aperture
>>>>   * - Static shared memory regions, which are described by special property
>>>>   *   "xen,shared-mem"
>>>> + * - xen,reg mappings
>>>>   */
>>>>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>>                                      struct membanks *ext_regions)
>>>> @@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>>          }
>>>>      }
>>>>  
>>>> +    if ( kinfo->xen_reg_assigned )
>>>> +    {
>>>> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
>>>> +        if ( res )
>>>> +            goto out;
>>>> +    }
>>>> +
>>>>      start = 0;
>>>>      end = (1ULL << p2m_ipa_bits) - 1;
>>>>      res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
>>>> @@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>>>      return res;
>>>>  }
>>>>  
>>>> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
>>>> +                                      void *data)
>>>> +{
>>>> +    struct membanks *membank = data;
>>>> +    paddr_t s = pfn_to_paddr(s_gfn);
>>>> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
>>>> +
>>>> +    if ( membank->nr_banks >= membank->max_banks )
>>>> +        return 0;
>>>> +
>>>> +    membank->bank[membank->nr_banks].start = s;
>>>> +    membank->bank[membank->nr_banks].size = e - s + 1;
>>>> +    membank->nr_banks++;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>>>>                                               struct membanks *ext_regions)
>>>>  {
>>>>      int res;
>>>>      struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
>>>> +    struct membanks *xen_reg = membanks_xzalloc(
>>>> +        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
>>> You allocate at least 1 membank even though xen_reg_assigned may be empty because:
>>>  - this function is called for hwdom - no xen,reg
>>>  - there may be no xen,reg i.e. no passthrough
>>
>> Ah, sorry, there's no need to allocate at least 1. This can just be:
>>
>>     struct membanks *xen_reg = membanks_xzalloc(
>>         rangeset_count_ranges(kinfo->arch.xen_reg_assigned), MEMORY);
> No, it cannot. membanks_xzalloc() calls xzalloc_flex_struct(). If you pass 0
> as size, the latter will calculate offset to FAM[0]. In other words, the
> allocation will succeed but only for members up to FAM[0] (i.e. only for struct
> membanks_hdr).

If we pass 0 as the size, these members (and their ->common.*
counterparts) will be allocated:
xen_reg->nr_banks
xen_reg->max_banks
xen_reg->type

but there will not be allocated any space for the flexible array member:
xen_reg->bank[]

Since ->max_banks will be set to 0, and ->nr_banks shouldn't exceed
->max_banks, it should work. At least for the (inner) loop in
find_unallocated_memory(), when ->nr_banks is 0, it won't dereference
->bank[]. FWIW, I also tested this with UBSAN enabled.

I admit it does give me a weird feeling not allocating any space for a
member in a struct, but it's a C standard flexible array member, and the
array's size would be 0. We deviated relevant MISRA rule 18.7 in
b87697fc1a6f ("automation/eclair: fully deviate MISRA C:2012 Rules 5.7
and 18.7").

With that said, I'd be happy either way (i.e. either allocating exactly
what's returned by rangeset_count_ranges() or max(1,
rangeset_count_ranges()), but I just want to ensure we have the same
understanding on the technicalities.

> Also, even if you conditionally allocate for xen_reg_assigned or set NULL, in
> latter case you will end up with mem_banks containing NULL member. AFAICT that's
> not something expected by the users of mem_banks (+ it gives unneeded iteration).

Agreed, it would be a bad idea to set xen_reg = NULL (leading to a NULL
member in mem_banks), because then find_unallocated_memory() would not
be happy.

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Orzel, Michal]

On 04/06/2025 21:51, Stewart Hildebrand wrote:
> On 6/4/25 03:00, Orzel, Michal wrote:
>> On 03/06/2025 23:15, Stewart Hildebrand wrote:
>>> On 5/14/25 03:31, Orzel, Michal wrote:
>>>> On 13/05/2025 21:54, Stewart Hildebrand wrote:
>>>>> Similarly to fba1b0974dd8, when a device is passed through to a
>>>>> direct-map dom0less domU, the xen,reg ranges may overlap with the
>>>>> extended regions. Remove xen,reg from direct-map domU extended regions.
>>>>>
>>>>> Introduce rangeset_count_ranges().
>>>>>
>>>>> Take the opportunity to update the comment ahead of find_memory_holes().
>>>>>
>>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>>> ---
>>>>> v2->v3:
>>>>> * new patch
>>>>> ---
>>>>>  xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
>>>>>  xen/common/rangeset.c       | 14 +++++++++
>>>>>  xen/include/xen/rangeset.h  |  2 ++
>>>>>  3 files changed, 68 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>> index b189a7cfae9f..3cdf5839bc98 100644
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
>>>>>  }
>>>>>  
>>>>>  /*
>>>>> - * 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:
>>>>> + * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
>>>>> + * domU 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 aperture
>>>>>   * - Static shared memory regions, which are described by special property
>>>>>   *   "xen,shared-mem"
>>>>> + * - xen,reg mappings
>>>>>   */
>>>>>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>>>                                      struct membanks *ext_regions)
>>>>> @@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>>>          }
>>>>>      }
>>>>>  
>>>>> +    if ( kinfo->xen_reg_assigned )
>>>>> +    {
>>>>> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
>>>>> +        if ( res )
>>>>> +            goto out;
>>>>> +    }
>>>>> +
>>>>>      start = 0;
>>>>>      end = (1ULL << p2m_ipa_bits) - 1;
>>>>>      res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
>>>>> @@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>>>>      return res;
>>>>>  }
>>>>>  
>>>>> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
>>>>> +                                      void *data)
>>>>> +{
>>>>> +    struct membanks *membank = data;
>>>>> +    paddr_t s = pfn_to_paddr(s_gfn);
>>>>> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
>>>>> +
>>>>> +    if ( membank->nr_banks >= membank->max_banks )
>>>>> +        return 0;
>>>>> +
>>>>> +    membank->bank[membank->nr_banks].start = s;
>>>>> +    membank->bank[membank->nr_banks].size = e - s + 1;
>>>>> +    membank->nr_banks++;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>  static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>>>>>                                               struct membanks *ext_regions)
>>>>>  {
>>>>>      int res;
>>>>>      struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
>>>>> +    struct membanks *xen_reg = membanks_xzalloc(
>>>>> +        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
>>>> You allocate at least 1 membank even though xen_reg_assigned may be empty because:
>>>>  - this function is called for hwdom - no xen,reg
>>>>  - there may be no xen,reg i.e. no passthrough
>>>
>>> Ah, sorry, there's no need to allocate at least 1. This can just be:
>>>
>>>     struct membanks *xen_reg = membanks_xzalloc(
>>>         rangeset_count_ranges(kinfo->arch.xen_reg_assigned), MEMORY);
>> No, it cannot. membanks_xzalloc() calls xzalloc_flex_struct(). If you pass 0
>> as size, the latter will calculate offset to FAM[0]. In other words, the
>> allocation will succeed but only for members up to FAM[0] (i.e. only for struct
>> membanks_hdr).
> 
> If we pass 0 as the size, these members (and their ->common.*
> counterparts) will be allocated:
> xen_reg->nr_banks
> xen_reg->max_banks
> xen_reg->type
> 
> but there will not be allocated any space for the flexible array member:
> xen_reg->bank[]
> 
> Since ->max_banks will be set to 0, and ->nr_banks shouldn't exceed
> ->max_banks, it should work. At least for the (inner) loop in
> find_unallocated_memory(), when ->nr_banks is 0, it won't dereference
> ->bank[]. FWIW, I also tested this with UBSAN enabled.
> 
> I admit it does give me a weird feeling not allocating any space for a
> member in a struct, but it's a C standard flexible array member, and the
> array's size would be 0. We deviated relevant MISRA rule 18.7 in
> b87697fc1a6f ("automation/eclair: fully deviate MISRA C:2012 Rules 5.7
> and 18.7").
> 
> With that said, I'd be happy either way (i.e. either allocating exactly
> what's returned by rangeset_count_ranges() or max(1,
> rangeset_count_ranges()), but I just want to ensure we have the same
> understanding on the technicalities.
Hmm, why do you want to allocate memory in the first place? If xen_reg_assigned
is NULL, we should not allocate anything. Instead you suggest to allocate either
full structure or part of it. That's where I disagree.

> 
>> Also, even if you conditionally allocate for xen_reg_assigned or set NULL, in
>> latter case you will end up with mem_banks containing NULL member. AFAICT that's
>> not something expected by the users of mem_banks (+ it gives unneeded iteration).
> 
> Agreed, it would be a bad idea to set xen_reg = NULL (leading to a NULL
> member in mem_banks), because then find_unallocated_memory() would not
> be happy.

~Michal

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Stewart Hildebrand]

On 6/5/25 02:45, Orzel, Michal wrote:
> On 04/06/2025 21:51, Stewart Hildebrand wrote:
>> On 6/4/25 03:00, Orzel, Michal wrote:
>>> On 03/06/2025 23:15, Stewart Hildebrand wrote:
>>>> On 5/14/25 03:31, Orzel, Michal wrote:
>>>>> On 13/05/2025 21:54, Stewart Hildebrand wrote:
>>>>>> Similarly to fba1b0974dd8, when a device is passed through to a
>>>>>> direct-map dom0less domU, the xen,reg ranges may overlap with the
>>>>>> extended regions. Remove xen,reg from direct-map domU extended regions.
>>>>>>
>>>>>> Introduce rangeset_count_ranges().
>>>>>>
>>>>>> Take the opportunity to update the comment ahead of find_memory_holes().
>>>>>>
>>>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>>>> ---
>>>>>> v2->v3:
>>>>>> * new patch
>>>>>> ---
>>>>>>  xen/arch/arm/domain_build.c | 57 +++++++++++++++++++++++++++++++++----
>>>>>>  xen/common/rangeset.c       | 14 +++++++++
>>>>>>  xen/include/xen/rangeset.h  |  2 ++
>>>>>>  3 files changed, 68 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>>> index b189a7cfae9f..3cdf5839bc98 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -824,15 +824,17 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
>>>>>>  }
>>>>>>  
>>>>>>  /*
>>>>>> - * 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:
>>>>>> + * Find the holes in the Host DT which can be exposed to Dom0 or a direct-map
>>>>>> + * domU 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 aperture
>>>>>>   * - Static shared memory regions, which are described by special property
>>>>>>   *   "xen,shared-mem"
>>>>>> + * - xen,reg mappings
>>>>>>   */
>>>>>>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>>>>                                      struct membanks *ext_regions)
>>>>>> @@ -914,6 +916,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
>>>>>>          }
>>>>>>      }
>>>>>>  
>>>>>> +    if ( kinfo->xen_reg_assigned )
>>>>>> +    {
>>>>>> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
>>>>>> +        if ( res )
>>>>>> +            goto out;
>>>>>> +    }
>>>>>> +
>>>>>>      start = 0;
>>>>>>      end = (1ULL << p2m_ipa_bits) - 1;
>>>>>>      res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
>>>>>> @@ -994,11 +1003,30 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>>>>>      return res;
>>>>>>  }
>>>>>>  
>>>>>> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
>>>>>> +                                      void *data)
>>>>>> +{
>>>>>> +    struct membanks *membank = data;
>>>>>> +    paddr_t s = pfn_to_paddr(s_gfn);
>>>>>> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
>>>>>> +
>>>>>> +    if ( membank->nr_banks >= membank->max_banks )
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    membank->bank[membank->nr_banks].start = s;
>>>>>> +    membank->bank[membank->nr_banks].size = e - s + 1;
>>>>>> +    membank->nr_banks++;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>  static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>>>>>>                                               struct membanks *ext_regions)
>>>>>>  {
>>>>>>      int res;
>>>>>>      struct membanks *gnttab = membanks_xzalloc(1, MEMORY);
>>>>>> +    struct membanks *xen_reg = membanks_xzalloc(
>>>>>> +        max(1, rangeset_count_ranges(kinfo->xen_reg_assigned)), MEMORY);
>>>>> You allocate at least 1 membank even though xen_reg_assigned may be empty because:
>>>>>  - this function is called for hwdom - no xen,reg
>>>>>  - there may be no xen,reg i.e. no passthrough
>>>>
>>>> Ah, sorry, there's no need to allocate at least 1. This can just be:
>>>>
>>>>     struct membanks *xen_reg = membanks_xzalloc(
>>>>         rangeset_count_ranges(kinfo->arch.xen_reg_assigned), MEMORY);
>>> No, it cannot. membanks_xzalloc() calls xzalloc_flex_struct(). If you pass 0
>>> as size, the latter will calculate offset to FAM[0]. In other words, the
>>> allocation will succeed but only for members up to FAM[0] (i.e. only for struct
>>> membanks_hdr).
>>
>> If we pass 0 as the size, these members (and their ->common.*
>> counterparts) will be allocated:
>> xen_reg->nr_banks
>> xen_reg->max_banks
>> xen_reg->type
>>
>> but there will not be allocated any space for the flexible array member:
>> xen_reg->bank[]
>>
>> Since ->max_banks will be set to 0, and ->nr_banks shouldn't exceed
>> ->max_banks, it should work. At least for the (inner) loop in
>> find_unallocated_memory(), when ->nr_banks is 0, it won't dereference
>> ->bank[]. FWIW, I also tested this with UBSAN enabled.
>>
>> I admit it does give me a weird feeling not allocating any space for a
>> member in a struct, but it's a C standard flexible array member, and the
>> array's size would be 0. We deviated relevant MISRA rule 18.7 in
>> b87697fc1a6f ("automation/eclair: fully deviate MISRA C:2012 Rules 5.7
>> and 18.7").
>>
>> With that said, I'd be happy either way (i.e. either allocating exactly
>> what's returned by rangeset_count_ranges() or max(1,
>> rangeset_count_ranges()), but I just want to ensure we have the same
>> understanding on the technicalities.
> Hmm, why do you want to allocate memory in the first place? If xen_reg_assigned
> is NULL, we should not allocate anything. Instead you suggest to allocate either
> full structure or part of it. That's where I disagree.

Ah, alright, I see now. I'll conditionally allocate xen_reg, and ...

>>
>>> Also, even if you conditionally allocate for xen_reg_assigned or set NULL, in
>>> latter case you will end up with mem_banks containing NULL member. AFAICT that's
>>> not something expected by the users of mem_banks (+ it gives unneeded iteration).
>>
>> Agreed, it would be a bad idea to set xen_reg = NULL (leading to a NULL
>> member in mem_banks), because then find_unallocated_memory() would not
>> be happy.

... I'll adjust find_unallocated_memory() to skip over NULL entries.

Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Stewart Hildebrand]

On 5/14/25 02:49, Jan Beulich wrote:
> On 13.05.2025 21:54, Stewart Hildebrand wrote:
>> --- a/xen/common/rangeset.c
>> +++ b/xen/common/rangeset.c
>> @@ -433,6 +433,20 @@ bool rangeset_is_empty(
>>      return ((r == NULL) || list_empty(&r->range_list));
>>  }
>>  
>> +int rangeset_count_ranges(const struct rangeset *r)
>> +{
>> +    int nr = 0;
> 
> Ehem - this and the function's return type want to be unsigned.
> 
>> +    struct list_head *list;
>> +
>> +    if ( r == NULL )
>> +        return 0;
>> +
>> +    list_for_each( list, &r->range_list )
> 
> Nit: Either you deem list_for_each a pseudo-keyword (then a blank is
> missing) or you don't (then there are excess blanks).
> 
> Further I don't think this is valid to do without holding the rangeset's
> lock in read mode (irrespective of the function return value potentially
> being stale by the time the caller gets to look at it, which is no
> different from other functions, i.e. falls in the caller's
> responsibilities).
> 
>> +        nr++;
> 
> And then, if already abstraction is wanted, wouldn't this loop better be
> yet another helper (macro?) in xen/list.h?
> 
>> +    return nr;
>> +}
> 
> Finally: If this is to be commonly used in several places, having such a
> helper is likely fine. As it stands, the sole caller is an __init
> function, and hence this is unreachable code post-init (which while not
> formally a Misra violation in my eyes effectively still is one). Aiui
> the same can be achieved using rangeset_report_ranges(), with a new
> (__init and static) callback function.

Moving rangeset_count_ranges() to rangeset.h might change the situation
slightly, but I'm not aware of any other potential callers so I'll just
use rangeset_report_ranges().

Re: [PATCH v3 2/2] tools/arm: exclude iomem from domU extended regions

[Stewart Hildebrand]

Thanks for taking a look!

On 5/26/25 07:40, Anthony PERARD wrote:
> On Tue, May 13, 2025 at 03:54:50PM -0400, Stewart Hildebrand wrote:
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 75c811053c7c..8ae16a1726fc 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>>      if (info.gpaddr_bits > 64)
>>          return ERROR_INVAL;
>>  
>> +    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
>> +          compare_iomem);
>> +
>>      /*
>>       * Try to allocate separate 2MB-aligned extended regions from the first
>>       * and second RAM banks taking into the account the maximum supported
>>       * guest physical address space size and the amount of memory assigned
>>       * to the guest.
>>       */
>> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> -        region_base[i] = bankbase[i] +
>> +    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
>> +        struct {
>> +            uint64_t start;
>> +            uint64_t end; /* inclusive */
>> +        } unallocated;
>> +        uint64_t size = 0;
>> +
>> +        unallocated.start = bankbase[i] +
>>              ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
>>  
>> -        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
>> -        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
>> -        if (bankend[i] > region_base[i])
>> -            region_size[i] = bankend[i] - region_base[i] + 1;
>> +        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
>> +        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
>> +
>> +        if (unallocated.end > unallocated.start)
>> +            size = unallocated.end - unallocated.start + 1;
>> +
>> +        if (size < EXT_REGION_MIN_SIZE)
>> +            continue;
>> +
>> +        /* Exclude iomem */
>> +        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
>> +             j++) {
>> +            struct {
>> +                uint64_t start;
>> +                uint64_t end; /* inclusive */
>> +            } iomem;
>> +
>> +            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
>> +            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
>> +                         << XC_PAGE_SHIFT) - 1;
>> +
>> +            if (iomem.end >= unallocated.start
>> +                && iomem.start <= unallocated.end) {
>> +
>> +                if (iomem.start <= unallocated.start) {
>> +                    unallocated.start = iomem.end + 1;
>> +
>> +                    if (iomem.end >= unallocated.end)
>> +                        /* Complete overlap, discard unallocated region */
>> +                        break;
>> +
>> +                    /* Beginning overlap */
>> +                    continue;
> 
> Instead of a `continue` and a comment that I don't understand what it is
> supposed to mean, you could just do if-else:
> 
> if (iomem.start <= unallocated.start) {
>     // code before this continue
> } else { // we have: iomem.start > unallocated.start
>     // the block of code bellow.
> }

Yep, that makes sense, will do.

>> +                }
>> +
>> +                if (iomem.start > unallocated.start) {
>> +                    assert(unallocated.end > unallocated.start);
> 
> I think this assert should be removed.

OK

> Instead, you could check that this property hold true every time there's
> a modification to `unallocated.start` in this function.
> 
> Maybe one way to make the algo easier to read, and to check that this
> property is still true, is to rewrite:
> 
>     unallocated.start = iomem.end + 1;
>     if (iomem.end >= unallocated.end)
>         // discard `unallocated`
>         break;
> 
> with
> 
>     unallocated.start = iomem.end + 1;
>     if (unallocated.start > unallocated.end)
>         // obvious: all allocated already
>         break;
> 
> Because checking for:
>     iomem.end >= unallocated.end
> is the same as checking for:
>     iomem.end + 1 > unallocated.end
>     unallocated.start > unallocated.end

Ah, yes, this makes more sense. Will do, thanks.

>> +                    size = iomem.start - unallocated.start;
> 
> Isn't `size` the size of the unallocated region? Why is it recalculated
> with `iomem`? I think it would be better to create a new variable.

You're right, "size" is being used here for something different. I'll
create a new variable to better distinguish.

>> +
>> +                    if (size >= EXT_REGION_MIN_SIZE) {
>> +                        region_base[nr_regions] = unallocated.start;
>> +                        region_size[nr_regions] = size;
>> +                        nr_regions++;
>> +                    }
>> @@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>>      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] < EXT_REGION_MIN_SIZE)
>> -            continue;
>> -
>> +    for (i = 0; i < nr_regions; i++) {
>>          LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
>> -            nr_regions, region_base[i], region_base[i] + region_size[i]);
>> +            i, region_base[i], region_base[i] + region_size[i]);
> 
> Shouldn't we print "base + size - 1" for the end address?

OK