Re: [PATCH v3 21/34] xen/riscv: introduce p2m.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-01-15 at 12:01 +0100, Jan Beulich wrote:
> On 15.01.2024 11:35, Oleksii wrote:
> > Hi Julien,
> > 
> > On Fri, 2024-01-12 at 10:39 +0000, Julien Grall wrote:
> > > Hi Oleksii,
> > > 
> > > On 22/12/2023 15:13, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > > ---
> > > > Changes in V3:
> > > >   - add SPDX
> > > >   - drop unneeded for now p2m types.
> > > >   - return false in all functions implemented with BUG()
> > > > inside.
> > > >   - update the commit message
> > > > ---
> > > > Changes in V2:
> > > >   - Nothing changed. Only rebase.
> > > > ---
> > > >   xen/arch/ppc/include/asm/p2m.h   |   3 +-
> > > >   xen/arch/riscv/include/asm/p2m.h | 102
> > > > +++++++++++++++++++++++++++++++
> > > >   2 files changed, 103 insertions(+), 2 deletions(-)
> > > >   create mode 100644 xen/arch/riscv/include/asm/p2m.h
> > > > 
> > > > diff --git a/xen/arch/ppc/include/asm/p2m.h
> > > > b/xen/arch/ppc/include/asm/p2m.h
> > > > index 25ba054668..3bc05b7c05 100644
> > > > --- a/xen/arch/ppc/include/asm/p2m.h
> > > > +++ b/xen/arch/ppc/include/asm/p2m.h
> > > > @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct
> > > > domain *d)
> > > >   static inline int
> > > > guest_physmap_mark_populate_on_demand(struct
> > > > domain *d, unsigned long gfn,
> > > >                                                          
> > > > unsigned
> > > > int order)
> > > >   {
> > > > -    BUG_ON("unimplemented");
> > > > -    return 1;
> > > > +    return -EOPNOTSUPP;
> > > >   }
> > > >   
> > > >   static inline int guest_physmap_add_entry(struct domain *d,
> > > > diff --git a/xen/arch/riscv/include/asm/p2m.h
> > > > b/xen/arch/riscv/include/asm/p2m.h
> > > > new file mode 100644
> > > > index 0000000000..d270ef6635
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/include/asm/p2m.h
> > > > @@ -0,0 +1,102 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +#ifndef __ASM_RISCV_P2M_H__
> > > > +#define __ASM_RISCV_P2M_H__
> > > > +
> > > > +#include <asm/page-bits.h>
> > > > +
> > > > +#define paddr_bits PADDR_BITS
> > > > +
> > > > +/*
> > > > + * List of possible type for each page in the p2m entry.
> > > > + * The number of available bit per page in the pte for this
> > > > purpose is 4 bits.
> > > > + * So it's possible to only have 16 fields. If we run out of
> > > > value
> > > > in the
> > > > + * future, it's possible to use higher value for pseudo-type
> > > > and
> > > > don't store
> > > > + * them in the p2m entry.
> > > > + */
> > > 
> > > This looks like a verbatim copy from Arm. Did you actually check
> > > RISC-V 
> > > has 4 bits available in the PTE to store this value?
> > Thanks for noticing that, in RISC-V it is available only 2 bits (
> > bits
> > 8 and 9), so I'll update the comment:
> > 53                   10 9    8 7 6 5 4 3 2 1 0
> >  Physical Page Number     RSV  D A G U X W R V
> 
> It's RSW (Reserved for Supervisor softWare use), not RSV, which is
> pretty
> important in this context.
Yes, you are right it is RSW. Thanks for the correction.

> 
> > It seems that I missed something in the Arm code/architecture.As
> > far as I recall, in Arm, bits 5-8 are ignored by the MMU, and they
> > are expected
> > to be used by the hypervisor for its purpose.
> > However, in the code, I notice that these bits are utilized for
> > storing
> > a reference counter.
> 
> Why "however"? Hardware still is going to ignore these bits.
Sure, these bits are ignored by hardware. What I meant is that,
according to the code, these bits are used for storing a reference
counter, not p2m_type_t. I guess I am missing something...

~ Oleksii