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

[Jan Beulich <jbeulich@xxxxxxxx>]

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