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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> --- a/xen/arch/ppc/include/asm/mm.h
> +++ b/xen/arch/ppc/include/asm/mm.h
> @@ -256,4 +256,9 @@ static inline bool arch_mfns_in_directmap(unsigned long 
> mfn, unsigned long nr)
>      return true;
>  }
>  
> +static inline bool is_magic_gpfn(xen_pfn_t gpfn)
> +{
> +    return false;
> +}
> +
>  #endif /* _ASM_PPC_MM_H */
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -3,6 +3,7 @@
>  #ifndef _ASM_RISCV_MM_H
>  #define _ASM_RISCV_MM_H
>  
> +#include <public/xen.h>
>  #include <asm/page-bits.h>
>  
>  #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
> @@ -20,4 +21,9 @@ unsigned long calc_phys_offset(void);
>  
>  void turn_on_mmu(unsigned long ra);
>  
> +static inline bool is_magic_gpfn(xen_pfn_t gpfn)
> +{
> +    return false;
> +}
> +
>  #endif /* _ASM_RISCV_MM_H */
> --- 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. 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.

> --- 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.)

> --- 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.

Jan