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

Stewart Hildebrand posted 2 patches 4 months, 3 weeks ago
There is a newer version of this series
[PATCH v5 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions
Posted by Stewart Hildebrand 4 months, 3 weeks ago
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>
---
v4->v5:
* remove extranous -1 and +1
* create local helper function to count ranges

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           | 80 +++++++++++++++++++++++++--
 xen/common/device-tree/domain-build.c |  5 ++
 2 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 590f38e52053..cef6c85e22ec 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,51 @@ 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 unsigned int __init count_ranges(struct rangeset *r)
+{
+    unsigned int cnt = 0;
+
+    rangeset_report_ranges(r, 0, PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
+                           &cnt);
+
+    return cnt;
+}
+
+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);
+
+    if ( membank->nr_banks >= membank->max_banks )
+        return 0;
+
+    membank->bank[membank->nr_banks].start = s;
+    membank->bank[membank->nr_banks].size = e - s;
+    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
+        ? membanks_xzalloc(count_ranges(kinfo->xen_reg_assigned), MEMORY)
+        : NULL;
 
     /*
      * Exclude the following regions:
@@ -974,6 +1023,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 +1032,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 +1062,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 v5 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions
Posted by Orzel, Michal 4 months, 2 weeks ago

On 11/06/2025 19:51, 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>
> ---
> v4->v5:
> * remove extranous -1 and +1
> * create local helper function to count ranges
> 
> 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           | 80 +++++++++++++++++++++++++--
>  xen/common/device-tree/domain-build.c |  5 ++
>  2 files changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 590f38e52053..cef6c85e22ec 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
If you took occasion to improve this comment, then I would replace Dom0 with hwdom.

> + * 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,51 @@ 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 unsigned int __init count_ranges(struct rangeset *r)
> +{
> +    unsigned int cnt = 0;
> +
> +    rangeset_report_ranges(r, 0, PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
I don't think it's ok with MISRA C to not check the return code, even though in
this case it's always 0. Either we should check or have (void).

> +                           &cnt);
> +
> +    return cnt;
> +}
> +
> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
Here you use s_gfn, e_gfn but for count() you used s,e. Even if unused, I would
prefer consistency.

> +                                      void *data)
> +{
> +    struct membanks *membank = data;
> +    paddr_t s = pfn_to_paddr(s_gfn);
> +    paddr_t e = pfn_to_paddr(e_gfn + 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;
> +    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
> +        ? membanks_xzalloc(count_ranges(kinfo->xen_reg_assigned), MEMORY)
> +        : NULL;
>  
>      /*
>       * Exclude the following regions:
> @@ -974,6 +1023,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 +1032,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 +1062,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;

Other than that, LGTM:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Re: [PATCH v5 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions
Posted by Stewart Hildebrand 4 months, 2 weeks ago
On 6/12/25 05:06, Orzel, Michal wrote:
> On 11/06/2025 19:51, 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>
>> ---
>> v4->v5:
>> * remove extranous -1 and +1
>> * create local helper function to count ranges
>>
>> 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           | 80 +++++++++++++++++++++++++--
>>  xen/common/device-tree/domain-build.c |  5 ++
>>  2 files changed, 80 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 590f38e52053..cef6c85e22ec 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
> If you took occasion to improve this comment, then I would replace Dom0 with hwdom.

Will do.

>> + * 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,51 @@ 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 unsigned int __init count_ranges(struct rangeset *r)
>> +{
>> +    unsigned int cnt = 0;
>> +
>> +    rangeset_report_ranges(r, 0, PFN_DOWN((1ULL << p2m_ipa_bits) - 1), count,
> I don't think it's ok with MISRA C to not check the return code, even though in
> this case it's always 0. Either we should check or have (void).

I'll add (void).

>> +                           &cnt);
>> +
>> +    return cnt;
>> +}
>> +
>> +static int __init rangeset_to_membank(unsigned long s_gfn, unsigned long e_gfn,
> Here you use s_gfn, e_gfn but for count() you used s,e. Even if unused, I would
> prefer consistency.

I'll make it consistent.

>> +                                      void *data)
>> +{
>> +    struct membanks *membank = data;
>> +    paddr_t s = pfn_to_paddr(s_gfn);
>> +    paddr_t e = pfn_to_paddr(e_gfn + 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;
>> +    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
>> +        ? membanks_xzalloc(count_ranges(kinfo->xen_reg_assigned), MEMORY)
>> +        : NULL;
>>  
>>      /*
>>       * Exclude the following regions:
>> @@ -974,6 +1023,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 +1032,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 +1062,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;
> 
> Other than that, LGTM:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks!