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.kuroc...@gmail.com>
> > >
> > > Acked-by: Jan Beulich <jbeul...@suse.com>
> > >
> > > 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
