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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 11/05/2024 10:02, Henry Wang wrote:
> > > +
> > > +    rc = guest_physmap_add_pages(d, gfn, mfn, NR_MAGIC_PAGES);
> > > +    if ( rc )
> > > +    {
> > > +        free_domheap_pages(magic_pg, 
> > > get_order_from_pages(NR_MAGIC_PAGES));
> > > +        return rc;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >   static int __init construct_domU(struct domain *d,
> > >                                    const struct dt_device_node *node)
> > >   {
> > > @@ -840,6 +868,10 @@ static int __init construct_domU(struct domain *d,
> > >           if ( rc < 0 )
> > >               return rc;
> > >           d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
> > > +
> > > +        rc = alloc_magic_pages(d);
> > > +        if ( rc < 0 )
> > > +            return rc;
> > This will only be allocated xenstore is enabled. But I don't think 
> > some of the magic pages really require xenstore to work. In the future 
> > we may need some more fine graine choice (see my comment in patch #2 
> > as well).
> Sorry, but it seems that by the time that I am writing this reply, I 
> didn't get the email for patch #2 comment. I will reply both together 
> when I see it.
That was expected, I knew what I wanted to mention about patch #2 but 
the e-mail was not fully written. You should have it in your inbox now.
> > >       }
> > >         return rc;
> > > diff --git a/xen/include/public/arch-arm.h 
> > > b/xen/include/public/arch-arm.h
> > > index 289af81bd6..186520d01f 100644
> > > --- a/xen/include/public/arch-arm.h
> > > +++ b/xen/include/public/arch-arm.h
> > > @@ -476,6 +476,12 @@ typedef uint64_t xen_callback_t;
> > >   #define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
> > >   #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
> > >   +#define NR_MAGIC_PAGES 4
> > This name and the other below are far too generic to be part of the 
> > shared header. For NR_MAGIC_PAGES, can you explain why 
> > GUEST_MAGIC_SIZE cannot be used? Is it because it is "too" big?
> Yes, I don't really want to allocate whole 16MB when we merely need 4 
> pages.
What about updating GUEST_MAGIC_SIZE to 0x4000? It doesn't make any 
sense to have two define which have pretty much the same meaning.
Then on the toolstack side, you can check that NR_MAGIC_PAGES is smaller 
or equal to GUEST_MAGIC_SIZE.
> > For the rest below...
> >
> > > +#define CONSOLE_PFN_OFFSET 0
> > > +#define XENSTORE_PFN_OFFSET 1
> > > +#define MEMACCESS_PFN_OFFSET 2
> > > +#define VUART_PFN_OFFSET 3
> > ... the order we allocate the magic pages is purely an internal 
> > decision of the toolstack. For the rest of the system, they need to 
> > access HVM_PARAM_*. So it doesn't feel like they should be part of the 
> > shared headers.
> > If there is a strong reason to include them, then I think they all 
> > should be prefixed with GUEST_MAGIC_*.
> One of the benefits as Michal pointed out in comments for v1 [1] would 
> be this would also allow init-dom0less.h not to re-define 
> XENSTORE_PFN_OFFSET.
At the moment, yes they have the same values. But with series, 
XENSTORE_PFN_OFFSET should technically be 0 in init-dom0less.
This is because Xen may only allocate one page (you don't need 4) for 
the reserved area. So...
> I will rename them as suggested if both of you are 
> ok with moving them to the arch header.

... I don't think they should be shared.

Cheers,

--
Julien Grall