Re: [RFC PATCH] xen/gnttab: Store frame GFN in struct page_info on Arm

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>       */
>      if ( p2m_is_foreign(pte.p2m.type) )
>      {
> -        mfn_t mfn = lpae_get_mfn(pte);
> -
>          ASSERT(mfn_valid(mfn));
>          put_page(mfn_to_page(mfn));
>      }
> +
> +#ifdef CONFIG_GRANT_TABLE
> +    /*
> +     * Check whether we deal with grant table page. As the grant table page
> +     * is xen_heap page and its entry has known p2m type, detect it and mark
> +     * the stored GFN as invalid.
> +     */
> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
> +#endif

I take it the write done is benign for other Xen heap pages? I suppose
this would want making very explicit, as such an assumption is easy to
go stale by entirely unrelated changes.

I also wonder whether you really mean to include p2m_ram_ro pages here
as well.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1021,6 +1021,7 @@ static struct page_info *alloc_heap_pages(
>  
>          /* Initialise fields which have other uses for free pages. */
>          pg[i].u.inuse.type_info = 0;
> +        page_arch_init(&pg[i]);

u.type_info's count portion also gets checked upon freeing a page.
Don't you want to have an arch-custom check for your new use of the
field as well? Or do you consider it not a problem (bug) if a page
was freed which still has a GFN set on it?

> @@ -82,11 +53,21 @@ int replace_grant_host_mapping(unsigned long gpaddr, 
> mfn_t mfn,
>          : gnttab_shared_gfn(NULL, gt, idx);                              \
>  })
>  
> -#define gnttab_shared_gfn(d, t, i)                                       \
> -    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
> +#define gnttab_shared_page(t, i)    \
> +    mfn_to_page(_mfn(__virt_to_mfn((t)->shared_raw[i])))
> +
> +#define gnttab_status_page(t, i)    \
> +    mfn_to_page(_mfn(__virt_to_mfn((t)->status[i])))

I wonder whether you wouldn't want to at least ASSERT() here that
the virtual address you start from is actually non-NULL.

> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -39,7 +39,15 @@ struct page_info
>          /* Page is in use: ((count_info & PGC_count_mask) != 0). */
>          struct {
>              /* Type reference count and various PGT_xxx flags and fields. */
> -            unsigned long type_info;
> +            unsigned long type_info:4;
> +
> +            /*
> +             * Stored GFN if page is used for grant table frame
> +             * (bits 59(27)-0).

Are these bit numbers correct? I thought Arm, like x86, counted bits
from low to high (unlike PPC, for example)?

> +             */
> +#define PGT_FRAME_GFN_WIDTH      (BITS_PER_LONG - 4)
> +#define PGT_INVALID_FRAME_GFN    _gfn((1UL << PGT_FRAME_GFN_WIDTH) - 1)
> +            unsigned long frame_gfn:PGT_FRAME_GFN_WIDTH;
>          } inuse;
>          /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>          union {
> @@ -94,12 +102,12 @@ struct page_info
>  #define PG_shift(idx)   (BITS_PER_LONG - (idx))
>  #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>  
> -#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
> -#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
> -#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
> +#define PGT_none          (0UL << 3)  /* no special uses of this page   */
> +#define PGT_writable_page (1UL << 3)  /* has writable mappings?         */
> +#define PGT_type_mask     (1UL << 3)
>  
>   /* Count of uses of this frame as its current type. */
> -#define PGT_count_width   PG_shift(2)
> +#define PGT_count_width   1
>  #define PGT_count_mask    ((1UL<<PGT_count_width)-1)

Leaving just a single bit seems pretty risky to me, and I can't see
why you do so. You need 52 bits on Arm64. Which means you have 12
bits left. Why not use them in full? Even on Arm32 you have 3 bits
available for the count afaict.

Also there's a disconnect here: If anyone wanted to change the number
of bits used for the various fields, each such change should require
an adjustment in as few independent places as possible. That is, there
shouldn't be multiple uses of literal 4 further up, and there also
shouldn't be a hidden connection between that 4 and the PGT_* values
here. I think you'd better express the GFN as such a PGT_* construct
as well. And you better would keep using PG_mask() and PG_shift().

> @@ -163,6 +171,15 @@ extern unsigned long xenheap_base_pdx;
>  
>  #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
>  
> +#define page_get_frame_gfn(_p) ({                               \
> +    gfn_t gfn_ = _gfn((_p)->u.inuse.frame_gfn);                 \
> +    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
> +})
> +
> +#define page_set_frame_gfn(_p, _gfn) ((_p)->u.inuse.frame_gfn = gfn_x(_gfn))
> +
> +#define page_arch_init(_p)   page_set_frame_gfn(_p, INVALID_GFN)

What's the purpose of the leading underscore in the macro parameter
names? They're not in line with the C standard's designation of sub-
namespaces for identifiers. (At least for the x86 counterpart please
read this as a request to drop the underscore there.)

Jan