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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Tue, 2024-01-16 at 17:12 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 16/01/2024 09:44, Oleksii wrote:
> > 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...
> 
> I can only guess where you saw the field used for reference counting.
> This was the domain map page infrastruture, right?
Yes, you are right.
> 
> If so, this is for stage-1 page-table (aka hypervisor table) and not
> the 
> stage-2 (e.g. P2M). For the latter, we would use the p2m_type_t.
I confused stage-1 & stage-2. Now everything fell into place. Thanks.

~ Oleksii