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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Michal,

On 3/11/2024 9:46 PM, Michal Orzel wrote:
> Hi Henry,
>
> 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 ?
This makes sense. I will convert this block to a function then move to 
static-memory.c in v3.
> > +            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.
Hmmm I realized I've made a mess here and below. I will do the proper 
error handling in v3.
> > +            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.
Personally for variables holding an address, I would like to init the 
local variable to a poison value before use. But you are right it does 
not make a difference here I think. I can drop the initialization if you 
prefer not having it, sure.
> > +            uint64_t magic_region_size = GUEST_MAGIC_SIZE;
> Why not paddr_t?
Good catch, I mixed struct meminfo with the newly added struct. Will use 
paddr_t.
> > +
> > +            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.
I agree with your point. Let's wait a bit longer for more 
ideas/comments. If no other inputs, I think I will drop the
"adding to reserved_mem" part of logic and record the found unused 
memory in kinfo, then use rangeset_remove_range() to remove this range 
in both
find_unallocated_memory() and find_memory_holes().

> Also, CCing Carlo since he was in a need of retrieving free memory regions as 
> well for cache coloring with dom0.
(+ Carlo)
Any inputs from your side for this topic Carlo?

Kind regards,
Henry
> ~Michal