Re: [PATCH v2 32/39] xen/riscv: add minimal stuff to asm/page.h to build full Xen

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-12-18 at 12:36 +0100, Jan Beulich wrote:
> On 18.12.2023 11:45, Oleksii wrote:
> > On Thu, 2023-12-14 at 16:57 +0100, Jan Beulich wrote:
> > > On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > 
> > > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > 
> > > I wonder though ...
> > > 
> > > > --- a/xen/arch/riscv/include/asm/page.h
> > > > +++ b/xen/arch/riscv/include/asm/page.h
> > > > @@ -6,6 +6,7 @@
> > > >  #ifndef __ASSEMBLY__
> > > >  
> > > >  #include <xen/const.h>
> > > > +#include <xen/bug.h>
> > > >  #include <xen/types.h>
> > > >  
> > > >  #include <asm/mm.h>
> > > > @@ -32,6 +33,9 @@
> > > >  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE
> > > > |
> > > > PTE_WRITABLE)
> > > >  #define PTE_TABLE                   (PTE_VALID)
> > > >  
> > > > +/* TODO */
> > > > +#define PAGE_HYPERVISOR 0
> > > 
> > > ... whether this couldn't be defined properly right away.
> > It can be introduced now but it requires some additional defines to
> > be
> > introduced in the same time:
> > 
> > #define _PAGE_W_BIT     2
> > #define _PAGE_XN_BIT    3
> > #define _PAGE_RO_BIT    1
> > #define _PAGE_XN        (1U << _PAGE_XN_BIT)
> > #define _PAGE_RO        (1U << _PAGE_RO_BIT)
> > #define _PAGE_W         (1U << _PAGE_W_BIT)
> 
> Why would you need these, when you already have
> PTE_{READABLE,WRITABLE,EXECUTABLE} just out of context from above?
I thought that the hypervisor page table is fully software-related, and
that's why a separate PAGE*BIT was introduced. These bits can be
different from PTE* bits, which are hardware-related
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/include/asm/page.h?ref_type=heads#L66

It looks like I misunderstood that, and PTE* can be used everywhere
instead of _PAGE*. Alternatively, we could consider renaming everything
to PAGE* to maintain consistency across architectures.

Does it make sense?


> (And
> that's aside from me (a) not following the naming (vs their purpose)
> and
> (b) not seeing what _PAGE_*_BIT are needed for, not even thinking
> about
> the leading underscores in these identifiers again.)
> 
> > ...
> > /*
> >  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are
> > not
> >  * meant to be used outside of this header.
> >  */
> > // #define _PAGE_DEVICE    _PAGE_XN
> > #define _PAGE_NORMAL    _PAGE_PRESENT
> > 
> > #define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL | _PAGE_RO | _PAGE_XN
> > |
> > _PAGE_W)
> > 
> > #define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> > 
> > And _PAGE_PRESENT in pgtbl-bits.h:
> > 
> > #define _PAGE_PRESENT   (1 << 0)
> > 
> > I prefer to introduce all this things when it will be really used.
> 
> I understand that, yet for easy things it may help doing them right
> away, rather than leaving them to be touched (in a straightforward
> way)
> again very soon.
> 
~ Oleksii