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

[Jan Beulich <jbeulich@xxxxxxxx>]

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