Re: [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64

[Guo Ren]

The patch didn't wrap the physical address space directly, just follow the spec.
I admit that I am trying to use the compliance specification to allow
qemu to support some non-standard software.
But compliance specification and wrapping the physical address space
are different things.
I'm preparing c910 pachset for linux riscv and you can question me there.

On Sun, Oct 13, 2019 at 1:33 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Wed, 25 Sep 2019 17:14:21 PDT (-0700), address@hidden wrote:
> > From: Guo Ren <address@hidden>
> >
> > Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> > need to ignore them. They cannot be a part of ppn.
> >
> > 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> >    4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> >    4.5 Sv48: Page-Based 48-bit Virtual-Memory System
> >
> > Signed-off-by: Guo Ren <address@hidden>
> > Tested-by: Bin Meng <address@hidden>
> > Reviewed-by: Liu Zhiwei <address@hidden>
> > Reviewed-by: Bin Meng <address@hidden>
> > Reviewed-by: Alistair Francis <address@hidden>
> > ---
> >  target/riscv/cpu_bits.h   | 7 +++++++
> >  target/riscv/cpu_helper.c | 2 +-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> >  Changelog V6:
> >   - Add Reviewer: Alistair Francis
> >
> >  Changelog V5:
> >   - Add Reviewer and Tester: Bin Meng
> >
> >  Changelog V4:
> >   - Change title to Ignore not Bugfix
> >   - Use PTE_PPN_MASK for RV32 and RV64
> >
> >  Changelog V3:
> >   - Use UUL define for PTE_RESERVED
> >   - Keep ppn >> PTE_PPN_SHIFT
> >
> >  Changelog V2:
> >   - Bugfix pte destroyed cause boot fail
> >   - Change to AND with a mask instead of shifting both directions
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index e998348..399c2c6 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -473,6 +473,13 @@
> >  /* Page table PPN shift amount */
> >  #define PTE_PPN_SHIFT       10
> >
> > +/* Page table PPN mask */
> > +#if defined(TARGET_RISCV32)
> > +#define PTE_PPN_MASK        0xffffffffUL
> > +#elif defined(TARGET_RISCV64)
> > +#define PTE_PPN_MASK        0x3fffffffffffffULL
> > +#endif
> > +
> >  /* Leaf page shift amount */
> >  #define PGSHIFT             12
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 87dd6a6..9961b37 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -261,7 +261,7 @@ restart:
> >  #elif defined(TARGET_RISCV64)
> >          target_ulong pte = ldq_phys(cs->as, pte_addr);
> >  #endif
> > -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > +        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >
> >          if (!(pte & PTE_V)) {
> >              /* Invalid PTE */
>
> I know I'm a bit late to the party here, but I don't like this.  There's ample
> evidence that wrapping the physical address space is a bad idea, and just
> because the ISA allows implementations to do this doesn't mean we should.



--
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/