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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/20/25 5:04 PM, Jan Beulich wrote:

> 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.
>     
Everything is valid to use and depends on the context if it is convenient
or not. It is hard to come up with a rule when and what should be used.

> > --- 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).
>     
Generally, I am okay to use something one.
But technically it is the same things from result point of view, just
different is access of a field by using a union or do a bit manipulation operations.

> > +#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?
>     
ppn<N> isn't actually used; it was added only to follow the PTE format specified
in the architecture spec. Technically, ppn<N> could be merged into ppn,
but IMO, keeping ppn<N> improves self-documentation of the page table format.

~ Oleksii