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

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

[Stewart Hildebrand]

v3->v4:
* see individual patches

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          | 110 +++++++++++++++++++++-----
 xen/arch/arm/domain_build.c           |  77 ++++++++++++++++--
 xen/common/device-tree/domain-build.c |   5 ++
 3 files changed, 168 insertions(+), 24 deletions(-)


base-commit: 86a12671c5d33063b6f958bdcca7c9d14cd5aac8
-- 
2.49.0

[PATCH v4 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.

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

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* conditionally allocate xen_reg
* use rangeset_report_ranges()
* make find_unallocated_memory() cope with NULL entry

v2->v3:
* new patch
---
 xen/arch/arm/domain_build.c           | 77 +++++++++++++++++++++++++--
 xen/common/device-tree/domain-build.c |  5 ++
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 590f38e52053..6632191cf602 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -792,15 +792,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)
@@ -882,6 +884,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),
@@ -962,11 +971,48 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
     return res;
 }
 
+static int __init count(unsigned long s, unsigned long e, void *data)
+{
+    unsigned int *cnt = data;
+
+    (*cnt)++;
+
+    return 0;
+}
+
+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 =
+        kinfo->xen_reg_assigned
+        ? ({
+            unsigned int xen_reg_cnt = 0;
+
+            rangeset_report_ranges(kinfo->xen_reg_assigned, 0,
+                                   PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
+                                   &xen_reg_cnt);
+            membanks_xzalloc(xen_reg_cnt, MEMORY);
+          })
+        : NULL;
 
     /*
      * Exclude the following regions:
@@ -974,6 +1020,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),
@@ -982,12 +1029,29 @@ 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 ( kinfo->xen_reg_assigned )
+    {
+        if ( !xen_reg )
+        {
+            res = -ENOMEM;
+            goto out;
+        }
+
+        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;
@@ -995,6 +1059,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/device-tree/domain-build.c b/xen/common/device-tree/domain-build.c
index 6b8b8d7cacb6..99ea81198a76 100644
--- a/xen/common/device-tree/domain-build.c
+++ b/xen/common/device-tree/domain-build.c
@@ -193,6 +193,10 @@ int __init find_unallocated_memory(const struct kernel_info *kinfo,
 
     /* Remove all regions listed in mem_banks */
     for ( i = 0; i < nr_mem_banks; i++ )
+    {
+        if ( !mem_banks[i] )
+            continue;
+
         for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
         {
             start = mem_banks[i]->bank[j].start;
@@ -212,6 +216,7 @@ int __init find_unallocated_memory(const struct kernel_info *kinfo,
                 goto out;
             }
         }
+    }
 
     start = 0;
     end = (1ULL << p2m_ipa_bits) - 1;
-- 
2.49.0

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

[Orzel, Michal]

On 09/06/2025 20:34, 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.
> 
> Take the opportunity to update the comment ahead of find_memory_holes().
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * conditionally allocate xen_reg
> * use rangeset_report_ranges()
> * make find_unallocated_memory() cope with NULL entry
> 
> v2->v3:
> * new patch
> ---
>  xen/arch/arm/domain_build.c           | 77 +++++++++++++++++++++++++--
>  xen/common/device-tree/domain-build.c |  5 ++
>  2 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 590f38e52053..6632191cf602 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -792,15 +792,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)
> @@ -882,6 +884,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),
> @@ -962,11 +971,48 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>      return res;
>  }
>  
> +static int __init count(unsigned long s, unsigned long e, void *data)
> +{
> +    unsigned int *cnt = data;
> +
> +    (*cnt)++;
> +
> +    return 0;
> +}
> +
> +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;
Why do you subtract 1 here if ...

> +
> +    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;
you add it again here.

> +    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 =
> +        kinfo->xen_reg_assigned
> +        ? ({
> +            unsigned int xen_reg_cnt = 0;
> +
> +            rangeset_report_ranges(kinfo->xen_reg_assigned, 0,
> +                                   PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
> +                                   &xen_reg_cnt);
This does not look really nice with ({. Why can't we create a helper function to
report the count for xen_reg_assigned and call it here? You could then also open
code your 'count' function that is not used by anything else and is quite ambiguous.

~Michal

> +            membanks_xzalloc(xen_reg_cnt, MEMORY);
> +          })
> +        : NULL;
>  
>      /*
>       * Exclude the following regions:
> @@ -974,6 +1020,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),
> @@ -982,12 +1029,29 @@ 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 ( kinfo->xen_reg_assigned )
> +    {
> +        if ( !xen_reg )
> +        {
> +            res = -ENOMEM;
> +            goto out;
> +        }
> +
> +        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;
> @@ -995,6 +1059,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/device-tree/domain-build.c b/xen/common/device-tree/domain-build.c
> index 6b8b8d7cacb6..99ea81198a76 100644
> --- a/xen/common/device-tree/domain-build.c
> +++ b/xen/common/device-tree/domain-build.c
> @@ -193,6 +193,10 @@ int __init find_unallocated_memory(const struct kernel_info *kinfo,
>  
>      /* Remove all regions listed in mem_banks */
>      for ( i = 0; i < nr_mem_banks; i++ )
> +    {
> +        if ( !mem_banks[i] )
> +            continue;
> +
>          for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
>          {
>              start = mem_banks[i]->bank[j].start;
> @@ -212,6 +216,7 @@ int __init find_unallocated_memory(const struct kernel_info *kinfo,
>                  goto out;
>              }
>          }
> +    }
>  
>      start = 0;
>      end = (1ULL << p2m_ipa_bits) - 1;

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

[Stewart Hildebrand]

On 6/10/25 03:27, Orzel, Michal wrote:
> On 09/06/2025 20:34, 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.
>>
>> Take the opportunity to update the comment ahead of find_memory_holes().
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> v3->v4:
>> * conditionally allocate xen_reg
>> * use rangeset_report_ranges()
>> * make find_unallocated_memory() cope with NULL entry
>>
>> v2->v3:
>> * new patch
>> ---
>>  xen/arch/arm/domain_build.c           | 77 +++++++++++++++++++++++++--
>>  xen/common/device-tree/domain-build.c |  5 ++
>>  2 files changed, 77 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 590f38e52053..6632191cf602 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -792,15 +792,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)
>> @@ -882,6 +884,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),
>> @@ -962,11 +971,48 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>      return res;
>>  }
>>  
>> +static int __init count(unsigned long s, unsigned long e, void *data)
>> +{
>> +    unsigned int *cnt = data;
>> +
>> +    (*cnt)++;
>> +
>> +    return 0;
>> +}
>> +
>> +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;
> Why do you subtract 1 here if ...
> 
>> +
>> +    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;
> you add it again here.

To be consistent with add_ext_regions() and add_hwdom_free_regions(),
but I suppose there's no need for that. I'll drop the extraneous -1 and
+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 =
>> +        kinfo->xen_reg_assigned
>> +        ? ({
>> +            unsigned int xen_reg_cnt = 0;
>> +
>> +            rangeset_report_ranges(kinfo->xen_reg_assigned, 0,
>> +                                   PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
>> +                                   &xen_reg_cnt);
> This does not look really nice with ({. Why can't we create a helper function to
> report the count for xen_reg_assigned and call it here? You could then also open
> code your 'count' function that is not used by anything else and is quite ambiguous.

If I'm reading this right, I think you're suggesting something like this
(in domain_build.c):

static unsigned int __init count_ranges(struct rangeset *r)
{
    unsigned int xen_reg_cnt = 0;

    rangeset_report_ranges(r,
                           0,
                           PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
                           ({
                               int count(unsigned long s, unsigned long e, void *data)
                               {
                                   unsigned int *cnt = data;

                                   (*cnt)++;

                                   return 0;
                               }
                               count;
                           }),
                           &xen_reg_cnt);

    return xen_reg_cnt;
}

...

    struct membanks *xen_reg =
        kinfo->xen_reg_assigned
        ? membanks_xzalloc(count_ranges(kinfo->xen_reg_assigned), MEMORY)
        : NULL;


However, the open-coded/anonymous count function, aside from being a
compiler extension, doesn't seem to play well with __init. As written,
this doesn't link:

Error: size of arch/arm/domain_build.o:.text is 0x00000014

Adding __init leads to:

aarch64-none-linux-gnu-ld: prelink.o: in function `count_ranges':
/home/stew/xen/xen/arch/arm/domain_build.c:978: undefined reference to `count.5'

Making it static leads to:

arch/arm/domain_build.c: In function ‘count_ranges’:
arch/arm/domain_build.c:982:43: error: invalid storage class for function ‘count’
  982 |                                static int count(unsigned long s, unsigned long e, void *data)
      |                                           ^~~~~

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

[Orzel, Michal]

On 11/06/2025 15:49, Stewart Hildebrand wrote:
> On 6/10/25 03:27, Orzel, Michal wrote:
>> On 09/06/2025 20:34, 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.
>>>
>>> Take the opportunity to update the comment ahead of find_memory_holes().
>>>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> ---
>>> v3->v4:
>>> * conditionally allocate xen_reg
>>> * use rangeset_report_ranges()
>>> * make find_unallocated_memory() cope with NULL entry
>>>
>>> v2->v3:
>>> * new patch
>>> ---
>>>  xen/arch/arm/domain_build.c           | 77 +++++++++++++++++++++++++--
>>>  xen/common/device-tree/domain-build.c |  5 ++
>>>  2 files changed, 77 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 590f38e52053..6632191cf602 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -792,15 +792,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)
>>> @@ -882,6 +884,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),
>>> @@ -962,11 +971,48 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>>>      return res;
>>>  }
>>>  
>>> +static int __init count(unsigned long s, unsigned long e, void *data)
>>> +{
>>> +    unsigned int *cnt = data;
>>> +
>>> +    (*cnt)++;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +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;
>> Why do you subtract 1 here if ...
>>
>>> +
>>> +    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;
>> you add it again here.
> 
> To be consistent with add_ext_regions() and add_hwdom_free_regions(),
> but I suppose there's no need for that. I'll drop the extraneous -1 and
> +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 =
>>> +        kinfo->xen_reg_assigned
>>> +        ? ({
>>> +            unsigned int xen_reg_cnt = 0;
>>> +
>>> +            rangeset_report_ranges(kinfo->xen_reg_assigned, 0,
>>> +                                   PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
>>> +                                   &xen_reg_cnt);
>> This does not look really nice with ({. Why can't we create a helper function to
>> report the count for xen_reg_assigned and call it here? You could then also open
>> code your 'count' function that is not used by anything else and is quite ambiguous.
> 
> If I'm reading this right, I think you're suggesting something like this
> (in domain_build.c):
> 
> static unsigned int __init count_ranges(struct rangeset *r)
> {
>     unsigned int xen_reg_cnt = 0;
> 
>     rangeset_report_ranges(r,
>                            0,
>                            PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
>                            ({
>                                int count(unsigned long s, unsigned long e, void *data)
>                                {
>                                    unsigned int *cnt = data;
> 
>                                    (*cnt)++;
> 
>                                    return 0;
>                                }
>                                count;
>                            }),
>                            &xen_reg_cnt);
> 
>     return xen_reg_cnt;
> }
> 
> ...
> 
>     struct membanks *xen_reg =
>         kinfo->xen_reg_assigned
>         ? membanks_xzalloc(count_ranges(kinfo->xen_reg_assigned), MEMORY)
>         : NULL;
> 
> 
> However, the open-coded/anonymous count function, aside from being a
> compiler extension, doesn't seem to play well with __init. As written,
> this doesn't link:
Sorry, I don't know why I wrote to open code count(). In conclusion my remark
was to place this code in a separate function to avoid ({ in
find_host_extended_regions(). So there will be count() helper and count_ranges().

~Michal

[PATCH v4 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")

v3->v4:
* use "else" instead of continue + another if statement
* remove assert
* s/iomem.end >=/unallocated.start >/ in two conditions
* new variable to distinguish unallocated size from region size
* print base + size - 1

v2->v3:
* no change

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

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 75c811053c7c..3086c52acf83 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,82 @@ 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 unallocated_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)
+            unallocated_size = unallocated.end - unallocated.start + 1;
+
+        if (unallocated_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 (unallocated.start > unallocated.end)
+                        break;
+                } else {
+                    uint64_t 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 (unallocated.start > unallocated.end)
+                        break;
+                }
+            }
+        }
+
+        if (unallocated.end >= unallocated.start
+            && nr_regions < MAX_NR_EXT_REGIONS)
+        {
+            uint64_t 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 +1641,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] - 1);
 
         set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
                   region_base[i], region_size[i]);
-        nr_regions++;
     }
 
     if (!nr_regions)
@@ -1626,7 +1698,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 v4 2/2] tools/arm: exclude iomem from domU extended regions

[Anthony PERARD]

On Mon, Jun 09, 2025 at 02:34:33PM -0400, Stewart Hildebrand wrote:
> 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>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD