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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2021 22:44, Oleksandr Tyshchenko wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1376,14 +1376,18 @@ unsigned long domain_get_maximum_gpfn(struct domain 
> *d)
>  void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>                                 enum XENSHARE_flags flags)
>  {
> +    unsigned long type_info;
> +
>      if ( page_get_owner(page) == d )
>          return;
>  
>      spin_lock(&d->page_alloc_lock);
>  
>      /* The incremented type count pins as writable or read-only. */
> -    page->u.inuse.type_info =
> -        (flags == SHARE_ro ? PGT_none : PGT_writable_page) | 1;
> +    type_info = page->u.inuse.type_info & ~(PGT_type_mask | PGT_count_mask);
> +    page->u.inuse.type_info = type_info |
> +        (flags == SHARE_ro ? PGT_none : PGT_writable_page) |
> +        (1UL << PGT_count_base);

Just as a note: If this was x86 code, I'd request the redundant
PGT_count_base to be dropped. Constructs like the above is what we
have MASK_INSR() for.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2204,7 +2204,10 @@ void *alloc_xenheap_pages(unsigned int order, unsigned 
> int memflags)
>          return NULL;
>  
>      for ( i = 0; i < (1u << order); i++ )
> +    {
>          pg[i].count_info |= PGC_xen_heap;
> +        arch_alloc_xenheap_page(&pg[i]);
> +    }
>  
>      return page_to_virt(pg);
>  }
> @@ -2222,7 +2225,10 @@ void free_xenheap_pages(void *v, unsigned int order)
>      pg = virt_to_page(v);
>  
>      for ( i = 0; i < (1u << order); i++ )
> +    {
>          pg[i].count_info &= ~PGC_xen_heap;
> +        arch_free_xenheap_page(&pg[i]);
> +    }
>  
>      free_heap_pages(pg, order, true);
>  }

You look to only be adjusting the !SEPARATE_XENHEAP instances of the
functions. Isn't 32-bit Arm using SEPARATE_XENHEAP?

> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -98,9 +98,18 @@ struct page_info
>  #define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
>  #define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
>  
> - /* Count of uses of this frame as its current type. */
> -#define PGT_count_width   PG_shift(2)
> -#define PGT_count_mask    ((1UL<<PGT_count_width)-1)
> + /* 3-bit count of uses of this frame as its current type. */
> +#define PGT_count_base    PG_shift(4)
> +#define PGT_count_mask    PG_mask(7, 4)
> +
> +/*
> + * Stored in bits [27:0] or [59:0] GFN if page is used for grant table frame.

I don't know enough Arm details to tell whether this is properly
one bit more than the maximum number of physical address bits.
Without the extra bit you wouldn't be able to tell apart a
guest specified GFN matching the value of PGT_INVALID_FRAME_GFN
from an entry which was set from INVALID_GFN.

> + * This only valid for the xenheap pages.
> + */
> +#define PGT_gfn_width     PG_shift(4)
> +#define PGT_gfn_mask      ((1UL<<PGT_gfn_width)-1)

Any reason you don't use PG_mask() here? Any open-coding is prone
to people later making mistakes.

Jan