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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Henry,

On Tue, Mar 12, 2024 at 4:25 AM Henry Wang <xin.wang2@xxxxxxx> wrote:
>
> 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?

Nothing at the moment.

Thanks.

> Kind regards,
> Henry
> > ~Michal
>