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 ensures a consistent bit layout across
all RISC-V MMU modes, avoiding the need for mode-specific ifdefs.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - This changes were part of "xen/riscv: implement p2m mapping functionality".
   No additional changes were done.
---
 xen/arch/riscv/include/asm/mm.h | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 912bc79e1b..41bf9002d7 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -12,6 +12,7 @@
 #include <xen/sections.h>
 #include <xen/types.h>
 
+#include <asm/cmpxchg.h>
 #include <asm/page-bits.h>
 
 extern vaddr_t directmap_virt_start;
@@ -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)
 
 /*
  * Page needs to be scrubbed. Since this bit can only be set on a page that is
@@ -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);
-- 
2.49.0

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.

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

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

> @@ -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?

Jan

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

On 02.07.2025 17:59, Oleksii Kurochko wrote:
> 
> 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:

I agree it's fine with just the re-wording.

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

Why would a static inline (in a header) cause compilation errors?

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

That's up to you - you must have had a reason to include it here.

Jan