Re: [PATCH v2 09/17] xen/riscv: introduce page_set_xenheap_gfn()

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3876 bytes --]


On 6/30/25 5:48 PM, Jan Beulich wrote:
> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>> Introduce page_set_xenheap_gfn() helper to encode the GFN associated with
>> a Xen heap page directly into the type_info field of struct page_info.
>>
>> Introduce a GFN field in the type_info of a Xen heap page by reserving 10
>> bits (sufficient for both Sv32 and Sv39+ modes), and define PGT_gfn_mask
>> and PGT_gfn_width accordingly.
> This reads as if you wanted to encode the GFN in 10 bits.

I will reword it to:
   Reserve 10 MSB bits to store the usage counter and frame type;
   use all remaining bits to store the grant table frame GFN.
   It will be enough as Sv32 uses 22-bit GFNs and Sv{39, 47, 58} uses 44-bit GFNs.

>
> What would also help is if you said why you actually need this. x86, after
> all, gets away without anything like this. (But I understand you're more
> Arm-like here.)

I think with the rewording mentioned above it will be clear that it is needed for
grant tables. But I also can add the following:
   The grant table frame GFN will be stored directly in|struct page_info| instead
   of being maintained in separate status/shared arrays. To avoid increasing the
   size of|struct page_info|, the necessary bits are borrowed from the
|||type_info of struct page_info.|

>> @@ -229,9 +230,21 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
>>   #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)
>> + /* 9-bit count of uses of this frame as its current type. */
>> +#define PGT_count_mask    PG_mask(0x3FF, 10)
>> +
>> +/*
>> + * Sv32 has 22-bit GFN. Sv{39, 48, 57} have 44-bit GFN.
>> + * Thereby we can use for `type_info` 10 bits for all modes, having the same
>> + * amount of bits for `type_info` for all MMU modes let us avoid introducing
>> + * an extra #ifdef to that header:
>> + *   if we go with maximum possible bits for count on each configuration
>> + *   we would need to have a set of PGT_count_* and PGT_gfn_*).
>> + */
>> +#define PGT_gfn_width     PG_shift(10)
>> +#define PGT_gfn_mask      (BIT(PGT_gfn_width, UL) - 1)
>> +
>> +#define PGT_INVALID_XENHEAP_GFN   _gfn(PGT_gfn_mask)
> Commentary here would imo be preferable to be much closer to Arm's. I don't
> see the point of the extra verbosity (part of which may be fine to have in
> the description, except you already say something along these lines there).
> While in turn the comment talks of fewer bits than are actually being used
> in the RV64 case.

Sure, I will replace this comment with:
/*
  * Stored in bits [22:0] (Sv32) or [44:0] (Sv39,48,57) GFN if page is xenheap page.
  */

>> @@ -283,6 +296,19 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
>>   
>>   #define PFN_ORDER(pg) ((pg)->v.free.order)
>>   
>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
>> +{
>> +    gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN : gfn;
>> +    unsigned long x, nx, y = p->u.inuse.type_info;
>> +
>> +    ASSERT(is_xen_heap_page(p));
>> +
>> +    do {
>> +        x = y;
>> +        nx = (x & ~PGT_gfn_mask) | gfn_x(gfn_);
>> +    } while ( (y = cmpxchg(&p->u.inuse.type_info, x, nx)) != x );
>> +}
>> +
>>   extern unsigned char cpu0_boot_stack[];
>>   
>>   void setup_initial_pagetables(void);
> What about the "get" counterpart?

I haven't added it as it isn't used now and it will lead to compilation error as it will be static inline
(in a similar way as Arm introduces it).

As an option this patch could be dropped and introduced with an introduction of grant tables.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 5229 bytes --]