Re: [PATCH v2 1/4] xen/arm: Alloc hypervisor reserved pages as magic pages for Dom0less DomUs

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien,

On 5/11/2024 7:03 PM, Julien Grall wrote:
> Hi Henry,
>
> On 11/05/2024 01:56, Henry Wang wrote:
> >   +static int __init alloc_magic_pages(struct domain *d)
> > +{
> > +    struct page_info *magic_pg;
> > +    mfn_t mfn;
> > +    gfn_t gfn;
> > +    int rc;
> > +
> > +    d->max_pages += NR_MAGIC_PAGES;
> > +    magic_pg = alloc_domheap_pages(d, 
> > get_order_from_pages(NR_MAGIC_PAGES), 0);
> > +    if ( magic_pg == NULL )
> > +        return -ENOMEM;
> > +
> > +    mfn = page_to_mfn(magic_pg);
> > +    if ( !is_domain_direct_mapped(d) )
> > +        gfn = gaddr_to_gfn(GUEST_MAGIC_BASE);
> > +    else
> > +        gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
> Summarizing the discussion we had on Matrix. Regions like the extend 
> area and shared memory may not be direct mapped. So unfortunately, I 
> think it is possible that the GFN could clash with one of those.
> At least in the shared memory case, the user can provide the address. 
> But as you use the domheap allocator, the address returned could 
> easily change if you tweak your setup.
> I am not entirely sure what's the best solution. We could ask the user 
> to provide the information for reserved region. But it feels like we 
> are exposing a bit too much to the user.
> So possibly we would want to use the same approach as extended 
> regions. Once we processed all the mappings, find some space for the 
> hypervisor regions.
One thing that I noticed when I re-visit the extended region finding 
code from the hypervisor side is:
When the domain is direct-mapped, when we find extended region for the 
domain, we either use find_unallocated_memory() or find_memory_holes(). 
It looks like the removal of shared memory regions in both functions 
uses the paddr parsed from the device tree to remove the regions, which 
indicates there is an assumption that when a domain is direct-mapped, 
the shared memory should also be direct-mapped. I might be wrong, but 
otherwise I don't think the extended region finding logic will carve out 
the correct shared memory region gpaddr range for guests.
So I think we are missing the documentation (and the corresponding 
checking when we parse the device tree) for above assumption for the 
static shared memory, i.e., when the domain is direct-mapped, the static 
shared memory should also be direct-mapped, and user should make sure 
this is satisfied in the device tree otherwise Xen should complain.
If we add this assumption and related checking code, I think your 
concern of clashing with static shared memory can be addressed. Do you 
agree?
Kind regards,
Henry

> Any other suggestions?
>
> Cheers,