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

["Orzel, Michal" <michal.orzel@amd.com>]

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