Re: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU

[Michal Orzel <michal.orzel@xxxxxxx>]

On 11/03/2024 14:46, Michal Orzel wrote:
> 
> 
> Hi Henry,
> 
> On 08/03/2024 02:54, Henry Wang wrote:
>> For 1:1 direct-mapped dom0less DomUs, the magic pages should not clash
>> with any RAM region. To find a proper region for guest magic pages,
>> we can reuse the logic of finding domain extended regions.
>>
>> Extract the logic of finding domain extended regions to a helper
>> function named find_unused_memory() and use it to find unallocated
>> spaces for magic pages before make_hypervisor_node(). The result magic
>> page region is added to the reserved memory section of the bootinfo so
>> that it is carved out from the extended regions.
>>
>> Reported-by: Alec Kwapis <alec.kwapis@xxxxxxxxxxxxx>
>> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
>> ---
>> v2:
>> - New patch
>> ---
>>  xen/arch/arm/dom0less-build.c           | 43 +++++++++++++++++++++++++
>>  xen/arch/arm/domain_build.c             | 30 ++++++++++-------
>>  xen/arch/arm/include/asm/domain_build.h |  2 ++
>>  3 files changed, 64 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index 1e1c8d83ae..99447bfb0c 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d, 
>> struct kernel_info *kinfo)
>>
>>      if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>>      {
>> +        if ( is_domain_direct_mapped(d) )
>> +        {
> This whole block is dependent on static memory feature that is compiled out 
> by default.
> Shouldn't you move it to static-memory.c ?
> 
>> +            struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
> I can't see corresponding xfree(avail_magic_regions). It's not going to be 
> used after unused memory
> regions are retrieved.
> 
>> +            struct meminfo *rsrv_mem = &bootinfo.reserved_mem;
>> +            struct mem_map_domain *mem_map = &d->arch.mem_map;
>> +            uint64_t magic_region_start = INVALID_PADDR;
> What's the purpose of this initialization? magic_region_start is going to be 
> re-assigned before making use of this value.
> 
>> +            uint64_t magic_region_size = GUEST_MAGIC_SIZE;
> Why not paddr_t?
> 
>> +            unsigned int i;
>> +
>> +            if ( !avail_magic_regions )
>> +                return -ENOMEM;
> What about memory allocated for kinfo->fdt? You should goto err;
> 
>> +
>> +            ret = find_unused_memory(d, kinfo, avail_magic_regions);
>> +            if ( ret )
>> +            {
>> +                printk(XENLOG_WARNING
>> +                       "%pd: failed to find a region for domain magic 
>> pages\n",
>> +                      d);
>> +                goto err;
> What about memory allocated for avail_magic_regions? You should free it.
> 
>> +            }
>> +
>> +            magic_region_start = avail_magic_regions->bank[0].start;
>> +
>> +            /*
>> +             * Register the magic region as reserved mem to make sure this
>> +             * region will not be counted when allocating extended regions.
> Well, this is only true in case find_unallocated_memory() is used to retrieve 
> free regions.
> What if our direct mapped domU used partial dtb and IOMMU is in use? In this 
> case,
> find_memory_holes() will be used and the behavior will be different.
> 
> Also, I'm not sure if it is a good idea to call find_unused_memory twice 
> (with lots of steps inside)
> just to retrieve 16MB (btw. add_ext_regions will only return 64MB+ regions) 
> region for magic pages.
> I'll let other maintainers share their opinion.
> 
> Also, CCing Carlo since he was in a need of retrieving free memory regions as 
> well for cache coloring with dom0.
In the end, I forgot to CC Carlo. Adding him now.

~Michal