Re: [PATCH v2 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-22 at 09:12 +0100, Jan Beulich wrote:
> On 21.03.2023 18:08, Oleksii wrote:
> > On Mon, 2023-03-20 at 17:41 +0100, Jan Beulich wrote:
> > > On 16.03.2023 17:43, Oleksii Kurochko wrote:
> > > > +#define LEVEL_ORDER(lvl)            (lvl * PAGETABLE_ORDER)
> > > > +#define LEVEL_SHIFT(lvl)            (LEVEL_ORDER(lvl) +
> > > > PAGE_ORDER)
> > > > +#define LEVEL_SIZE(lvl)             (_AT(paddr_t, 1) <<
> > > > LEVEL_SHIFT(lvl))
> > > > +
> > > > +#define XEN_PT_LEVEL_SHIFT(lvl)     LEVEL_SHIFT(lvl)
> > > > +#define XEN_PT_LEVEL_ORDER(lvl)     LEVEL_ORDER(lvl)
> > > > +#define XEN_PT_LEVEL_SIZE(lvl)      LEVEL_SIZE(lvl)
> > > 
> > > Mind me asking what these are good for? Doesn't one set of macros
> > > suffice?
> > Do you mean XEN_PT_LEVEL_{SHIFT...}? it can be used only one pair
> > of
> > macros. I'll check how they are used in ARM ( I copied that macros
> > from
> > there ).
> 
> There's no similar duplication in Arm code: They have LEVEL_..._GS(),
> but that takes a second parameter.
> 
> > > > +#define XEN_PT_LEVEL_MAP_MASK(lvl)  (~(XEN_PT_LEVEL_SIZE(lvl)
> > > > -
> > > > 1))
> > > > +#define XEN_PT_LEVEL_MASK(lvl)      (VPN_MASK <<
> > > > XEN_PT_LEVEL_SHIFT(lvl))
> > > > +
> > > > +#define PTE_SHIFT                   10
> > > 
> > > What does this describe? According to its single use here it may
> > > simply require a better name.
> > If look at Sv39 page table entry in riscv-priviliged.pdf. It has
> > the
> > following description:
> >   63 62 61  60    54  53  28 27  19 18  10 9 8 7 6 5 4 3 2 1 0
> >   N  PBMT   Rererved  PPN[2] PPN[1] PPN[0] RSW D A G U X W R V
> > So 10 it means the 0-9 bits.
> 
> Right. As said, I think the name needs improving, so it becomes clear
> what it refers to. Possibly PTE_PPN_SHIFT?
It will be better so I'll update it in new version of the aptch series.
> 
> Another question: Do you really mean to only support Sv39?
At least for now yes, it looks like it is the most usable mode.
> 
> > > > +/* Page Table entry */
> > > > +typedef struct {
> > > > +    uint64_t pte;
> > > > +} pte_t;
> > > 
> > > Not having read the respective spec (yet) I'm wondering if this
> > > really
> > > is this way also for RV32 (despite the different
> > > PAGETABLE_ORDER).
> > RISC-V architecture support several MMU mode to translate VA to PA.
> > The following mode can be: Sv32, Sv39, Sv48, Sv57 and PAGESIZE is
> > equal
> > to 8 in all cases except Sv32 ( it is equal to 4 ).
> 
> I guess you mean PTESIZE.
Yes, I mean PTESIZE.
> 
> > but I looked at
> > different big projects who have RISC-V support and no one supports
> > Sv32.
> > 
> > So it will be correct for RV32 as it supports the same Sv32 and
> > Sv39
> > modes too.
> 
> Would you mind extending the comment then to mention that there's no
> intention to support Sv32, even on RV32? (Alternatively, as per a
> remark you had further down, some #ifdef-ary may be needed.)
I re-read documentation and it gave you incorrect information:

When SXLEN=32, the only other valid setting for MODE is Sv32, a paged
virtual-memory scheme described in Section 4.3.
When SXLEN=64, three paged virtual-memory schemes are defined: Sv39,
Sv48, and Sv57, described in Sections 4.4, 4.5, and 4.6, respectively.
One additional scheme, Sv64, will be defined in a later version of this
specification. The remaining MODE settings are reserved for future use
and may define different interpretations of the other fields in satp.

But I'll add #ifdef with the message that RV32 isn't supported now.

~ Oleksii