Re: [PATCH v5 1/5] target/riscv: Ignore reserved bits in PTE for RV64

From: Alistair Francis <alistair23@gmail.com>
To: Anup Patel <anup@brainfault.org>
Cc: "Weiwei Li" <liweiwei@iscas.ac.cn>,
	"open list:RISC-V" <qemu-riscv@nongnu.org>,
	wangjunqiang <wangjunqiang@iscas.ac.cn>,
	"Bin Meng" <bin.meng@windriver.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Alistair Francis" <alistair.francis@wdc.com>,
	"Guo Ren" <ren_guo@c-sky.com>,
	"Wei Wu (吴伟)" <lazyparser@gmail.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>
Subject: Re: [PATCH v5 1/5] target/riscv: Ignore reserved bits in PTE for RV64
Date: Tue, 18 Jan 2022 14:51:48 +1000	[thread overview]
Message-ID: <CAKmqyKNajL8kJL4HExN8CDCYwrVcUx4dcnn77wY5zCZu9M6SGg@mail.gmail.com> (raw)
In-Reply-To: <CAAhSdy3zjeW-WkbiicTJfurQkhts4m9XwvmoS+Zr1XVMzhy+3w@mail.gmail.com>

On Tue, Jan 18, 2022 at 1:31 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Jan 18, 2022 at 6:47 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >
> > From: Guo Ren <ren_guo@c-sky.com>
> >
> > Highest bits of PTE has been used for svpbmt, ref: [1], [2], 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
> >
> > 2: https://github.com/riscv/virtual-memory/blob/main/specs/663-Svpbmt-diff.pdf
> >
> > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> > Tested-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/cpu_bits.h   | 7 +++++++
> >  target/riscv/cpu_helper.c | 2 +-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 5a6d49aa64..282cd8eecd 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -490,6 +490,13 @@ typedef enum {
> >  /* 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
> > +
>
> Going forward we should avoid using target specific "#if"
> so that we can use the same qemu-system-riscv64 for both
> RV32 and RV64.
>
> >  /* Leaf page shift amount */
> >  #define PGSHIFT             12
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 434a83e66a..26608ddf1c 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -619,7 +619,7 @@ restart:
> >              return TRANSLATE_FAIL;
> >          }
> >
> > -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > +        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>
> Rather than using "#if", please use "xlen" comparison to extract
> PPN correctly from PTE.

This will need to be dynamic based on get_xl()

It does look like we should check the existence of the extensions though:

"Bit 63 is reserved for use by the Svnapot extension in Chapter 5. If
Svnapot is not implemented, bit 63 remains reserved and must be zeroed
by software for forward compatibility, or else a page-fault exception
is raised. Bits 62–61 are reserved for use by the Svpbmt extension in
Chapter 6. If Svpbmt is not implemented, bits 62–61 remain reserved
and must be zeroed by software for forward compatibility, or else a
page-fault exception is raised."

Alistair

>
> Regards,
> Anup
>
> >
> >          if (!(pte & PTE_V)) {
> >              /* Invalid PTE */
> > --
> > 2.17.1
> >
>