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.hildebr...@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
