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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2021 19:09, Oleksandr wrote:
> On 10.09.21 10:52, Jan Beulich wrote:
>> 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.
> 
> Yes, I don't see what bad could happen if we would clear this field for 
> non grant table pages. Except grant table pages I could detect only one 
> page passed this check here which is, I assume, shared_info page. All 
> pages have this field initialized with INVALID_GFN. But *only* grant 
> table pages have this field in use. So, I think, no one will suffer. I 
> will add a comment. Or you meant something more than just a comment?

The answer here goes hand in hand with the pending question of whether
you wouldn't better generally introduce recording of the GFN (and hence
effectively an M2P): The less special casing here, the better imo. The
more special casing here, the more justification / explanation is imo
needed in the comment.

>>> --- 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)?
> 
> I think, these are correct.  Yes, Little Endian is used.

Well, the low 4 bits are used by type_info here. Therefore I'm having
trouble seeing what the 0 refers to.

>>> @@ -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.
> 
> Only 1 bit in the type_info field is really used on Arm, but anyway ...
> 
> 
>>
>> 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().
> 
> ... I think, this indeed makes sense. If got your request correctly, we 
> can avoid disconnect here
> and reserve 3-bit field for count for both Arm32 and Arm64. The struct 
> page_info's type_info field remains untouched.
> 
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index ded74d2..8b73e1c 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -99,8 +99,14 @@ struct page_info
>   #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)
> +#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 */
> +#define PGT_gfn_width     PG_shift(4)
> +#define PGT_gfn_mask      ((1UL<<PGT_gfn_width)-1)
> +
> +#define PGT_INVALID_FRAME_GFN   _gfn(PGT_gfn_mask)
> 
>    /* Cleared when the owning guest 'frees' this page. */
>   #define _PGC_allocated    PG_shift(1)
> @@ -163,6 +169,22 @@ 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.type_info & PGT_gfn_mask);  \
> +    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
> +})
> +
> +#define page_set_frame_gfn(_p, _gfn) ({                         \
> +    gfn_t gfn_ = gfn_eq(_gfn, INVALID_GFN) ?                    \
> +                 PGT_INVALID_FRAME_GFN : _gfn;                  \
> +    (_p)->u.inuse.type_info &= ~PGT_gfn_mask;                   \
> +    (_p)->u.inuse.type_info |= gfn_x(gfn_);                     \
> +})
> 
> [snip]
> 
> Is it close to what you keep in mind?

Yes. Just to be sure - did you check that moving the count up
from starting at bit 0 is compatible with all present uses? At
least on x86 I think we have uses where it is assumed to occupy
the bottom so many bits (and the same also for PGC_count_mask).

Jan