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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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 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.

Could you confirm if my understanding is correct?
Additionally, I am curious about where the PTE bits are used to store
one of the values of the enum `p2m_type_t`.

> 
> > +typedef enum {
> > +    p2m_invalid = 0,    /* Nothing mapped here */
> > +    p2m_ram_rw,         /* Normal read/write guest RAM */
> 
> s/guest/domain/ as this also applies for dom0.
Thanks. I'll update that.
> 
> > +} p2m_type_t;
> > +
> > +#include <xen/p2m-common.h>
> > +
> > +static inline int get_page_and_type(struct page_info *page,
> > +                                    struct domain *domain,
> > +                                    unsigned long type)
> > +{
> > +    BUG();
> 
> I understand your goal with the BUG() but I find it risky. This is
> not a 
> problem right now, it is more when we will decide to have RISC-V 
> supported. You will have to go through all the BUG() to figure out
> which 
> one are warrant or not.
> 
> To reduce the load, I would recommend to switch to
> ASSERT_UNREACHABLE() 
> (or maybe introduced a different macro) that would lead to a crash on
> debug build but propagate the error normally on production build.
> 
> Of course, if you can't propagate an error, then the right course of 
> action is a BUG(). But I expect this case to be limited.
Thanks.

I'm currently transitioning to using ASSERT_UNREACHABLE() or BUG_ON()
throughout the codebase, and this is one of the instances where I
overlooked the update.

> 
> [...]
> 
> > +static inline mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
> > +{
> > +    BUG();
> > +    return _mfn(0);
> 
> This wants to be INVALID_MFN.
> 
> [...]
> 

~ Oleksii