Hi Henry,
On Tue, Mar 12, 2024 at 4:25 AM Henry Wang <xin.wa...@amd.com> 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
>
