Re: [PATCH v1 4/6] xen/riscv: define pt_t and pt_walk_t structures

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.05.2025 17:57, Oleksii Kurochko wrote:
> Refactor pte_t to be a union which hold page table entry plus
> pt_t and pt_walk_t structures to simpilfy p2m functions.

Is this really simplifying things? I really view ...

> Also, introduce some helpers which are using pt_walk_t.

... these helpers as confusing things, by using the wrong part of the
union relative to what their names are. (I'll re-phrase this some at
the bottom.)

With the union it's also unclear to me how to know which part of the
union is the one that's valid to use, when there's no auxiliary info
available.

> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -99,15 +99,67 @@
>  
>  #endif
>  
> -/* Page Table entry */
>  typedef struct {
> +    unsigned long v:1;
> +    unsigned long r:1;
> +    unsigned long w:1;
> +    unsigned long x:1;
> +    unsigned long u:1;
> +    unsigned long g:1;
> +    unsigned long a:1;
> +    unsigned long d:1;
> +    unsigned long rsw:2;
> +#if RV_STAGE1_MODE == SATP_MODE_SV39
> +    unsigned long ppn0:9;
> +    unsigned long ppn1:9;
> +    unsigned long ppn2:26;
> +    unsigned long rsw2:7;
> +    unsigned long pbmt:2;
> +    unsigned long n:1;
> +#elif RV_STAGE1_MODE == SATP_MODE_SV48
> +    unsigned long ppn0:9;
> +    unsigned long ppn1:9;
> +    unsigned long ppn2:9;
> +    unsigned long ppn3:17;
> +    unsigned long rsw2:7;
> +    unsigned long pbmt:2;
> +    unsigned long n:1;
> +#else

Imo you want to settle on whether you want to use bitfields or #define-s
to manipulate page table entries. Using a mix is going to be confusing
(or worse).

> +#error "Add proper bits for SATP_MODE"
> +#endif
> +} pt_t;
> +
> +typedef struct {
> +    unsigned long rsw:10;
> +#if RV_STAGE1_MODE == SATP_MODE_SV39 || RV_STAGE1_MODE == SATP_MODE_SV48
> +    unsigned long ppn: 44;

Nit: Why's there a blank after the colon here when there's none anywhere else?

> +static inline void pte_set_mfn(pte_t *pte, mfn_t mfn)
> +{
> +    pte->walk.ppn = mfn_x(mfn);
> +}
> +
> +static inline mfn_t pte_get_mfn(pte_t pte)
> +{
> +    return _mfn(pte.walk.ppn);
> +}

Following to my remark at the top - if you do it this way, what use are the
ppn<N> fields?

Jan