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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jan,

On 2/26/2024 4:25 PM, Jan Beulich wrote:
> On 26.02.2024 02:19, Henry Wang wrote:
> > --- a/xen/arch/arm/include/asm/mm.h
> > +++ b/xen/arch/arm/include/asm/mm.h
> > @@ -428,6 +428,19 @@ static inline void page_set_xenheap_gfn(struct page_info 
> > *p, gfn_t gfn)
> >       } while ( (y = cmpxchg(&p->u.inuse.type_info, x, nx)) != x );
> >   }
> >   
> > +#define MAGIC_PAGE_N_GPFN(n)     ((GUEST_MAGIC_BASE >> PAGE_SHIFT) + n)
> > +static inline bool is_magic_gpfn(xen_pfn_t gpfn)
> > +{
> > +    unsigned int i;
> > +    for ( i = 0; i < NR_MAGIC_PAGES; i++ )
> Nit: Blank line please between declaration(s) and statement(s).
Thanks, will correct in next version.

> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -628,4 +628,9 @@ static inline bool arch_mfns_in_directmap(unsigned long 
> > mfn, unsigned long nr)
> >       return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1);
> >   }
> >   
> > +static inline bool is_magic_gpfn(xen_pfn_t gpfn)
> > +{
> > +    return false;
> > +}
> I don't think every arch should need to gain such a dummy function.
Thanks for raising the concern here and about the 
is_domain_direct_mapped(), I will try to do some clean-ups if necessary 
in next version.
> Plus
> the function name doesn't clarify at all what kind of "magic" this is
> about. Plus I think the (being phased out) term "gpfn" would better not
> be used in new functions anymore. Instead type-safe gfn_t would likely
> better be used as parameter type.
Sure, I will use gfn_t in the next version.

> > --- 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) )
> >               {
> >                   mfn = _mfn(gpfn);
> >   
> I wonder whether is_domain_direct_mapped() shouldn't either be cloned
> into e.g. is_gfn_direct_mapped(d, gfn), or be adjusted in-place to gain
> such a (then optional) 2nd parameter. (Of course there again shouldn't be
> a need for every domain to define a stub is_domain_direct_mapped() - if
> not defined by an arch header, the stub can be supplied in a single
> central place.)
Same here, it looks like you prefer the centralized 
is_domain_direct_mapped() now, as we are having more archs. I can do the 
clean-up when sending v2. Just out of curiosity, do you think it is a 
good practice to place the is_domain_direct_mapped() implementation in 
xen/domain.h with proper arch #ifdefs? If not do you have any better 
ideas? Thanks!
> > --- 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
> > +
> >   #define GUEST_RAM_BANKS   2
> Of these only NR_MAGIC_PAGES is really used in Xen, afaics.
> Also while this is added to a tools-only section, I'm also concerned of
> the ongoing additions here without suitable XEN_ prefixes. Any number
> of kinds of magic pages may exist for other reasons in a platform; which
> ones are meant would therefore better be sufficiently clear from the
> identifier used.
Yes you are correct, like I replied in another thread, I will undo the 
changes in next version.
Kind regards,
Henry
> Jan