Re: [PATCH] xen/common: Do not allocate magic pages 1:1 for direct mapped domains

[Henry Wang <xin.wang2@xxxxxxx>]

(-RISC-V and PPC people to avoid spamming their inbox as this is quite 
Arm specific)
Hi Julien,

On 2/26/2024 5:13 PM, Julien Grall wrote:
> Hi Henry,
>
> Welcome back!
Thanks!

> On 26/02/2024 01:19, Henry Wang wrote:
> > An error message can seen from the init-dom0less application on
> > direct-mapped 1:1 domains:
> > ```
> > Allocating magic pages
> > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> > Error on alloc magic pages
> > ```
> >
> > This is because populate_physmap() automatically assumes gfn == mfn
> > for direct mapped domains. This cannot be true for the magic pages
> > that are allocated later for Dom0less DomUs from the init-dom0less
> > helper application executed in Dom0.
> >
> > Force populate_physmap to take the "normal" memory allocation route for
> > the magic pages even for 1:1 Dom0less DomUs. This should work as long
> > as the 1:1 Dom0less DomU doesn't have anything else mapped at the same
> > guest address as the magic pages:
> > - gfn 0x39000 address 0x39000000
> > - gfn 0x39001 address 0x39001000
> > - gfn 0x39002 address 0x39002000
> > - gfn 0x39003 address 0x39003000
> This is very fragile. You are making the assumption that the magic 
> pages are not clashing with any RAM region. The layout defined in 
> arch-arm.h has been designed for guest where Xen is in full control of 
> the layout. This is not the case for directmapped domain. I don't 
> think it is correct to try to re-use part of the layout.
Apologies for the (a bit) late reply, it took a bit longer for me to 
understand the story about directmap stuff, and yes, now I agree with 
you, for those directmapped domains we should not reuse the guest layout 
directly.
> If you want to use 1:1 dom0less with xenstore & co, then you should 
> find a different place in memory for the magic pages (TDB how to find 
> that area). 
Yes, and maybe we can use similar strategy in find_unallocated_memory() 
or find_domU_holes() to do that.
> You will still have the problem of the 1:1 allocation, but I think 
> this could be solved bty adding a flag to force a non-1:1 allocation.
After checking the code flow, below rough plan came to my mind, I think 
what we need to do is:
(1) Find a range of un-used memory using similar method in 
find_unallocated_memory()/find_domU_holes()
(2) Change the base address, i.e. GUEST_MAGIC_BASE in alloc_xs_page() in 
init-dom0less.c to point to the address in (1) if static mem or 11 
directmap. (I think this is a bit tricky though, do you have any method 
that in your mind?)
(3) Use a flag or combination of existing flags (CDF_staticmem + 
CDF_directmap) in populate_physmap() to force the allocation of these 
magic pages using alloc_domheap_pages() - i.e. the "else" condition in 
the bottom
Would you mind sharing some thoughts on that? Thanks!

> > Create helper is_magic_gpfn() for Arm to assist this and stub helpers
> > for non-Arm architectures to avoid #ifdef. Move the definition of the
> > magic pages on Arm to a more common place.
> >
> > Note that the init-dom0less application of the diffenent Xen version
> s/diffenent/different/
Oops, will correct this in v2, thanks for spotting it.

> > +
> >   #endif /* __ASM_X86_MM_H__ */
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index b3b05c2ec0..ab4bad79e2 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -219,7 +219,7 @@ static void populate_physmap(struct memop_args *a)
> >           }
> >           else
> >           {
> > -            if ( is_domain_direct_mapped(d) )
> > +            if ( is_domain_direct_mapped(d) && !is_magic_gpfn(gpfn) )
> This path will also be reached by dom0. Effectively, this will prevent 
> dom0 to allocate the memory 1:1 for the magic GPFN (which is guest 
> specific).
I think above proposal will solve this issue.

> Also, why are you only checking the first GFN? What if the caller pass 
> an overlapped region?
I am a bit confused. My understanding is at this point we are handling 
one page at a time.
> >               {
> >                   mfn = _mfn(gpfn);
> >   diff --git a/xen/include/public/arch-arm.h 
> > b/xen/include/public/arch-arm.h
> > index a25e87dbda..58aa6ff05b 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
> > +#define CONSOLE_PFN_OFFSET 0
> > +#define XENSTORE_PFN_OFFSET 1
> > +#define MEMACCESS_PFN_OFFSET 2
> > +#define VUART_PFN_OFFSET 3
> Regardless of what I wrote above, it is not clear to me why you need 
> to move these macros in public header. Just above, we are defining the 
> magic region (see GUEST_MAGIC_BASE and GUEST_MAGIC_SIZE). This should 
> be sufficient to detect whether a GFN belongs to the magic region.
You are correct, I will undo the code movement in v2.

Kind regards,
Henry