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

[Guo Ren]

On Tue, Jan 18, 2022 at 7:28 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Jan 18, 2022 at 4:45 PM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Tue, Jan 18, 2022 at 4:51 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Tue, Jan 18, 2022 at 2:16 PM Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 18, 2022 at 11:32 AM 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.
> > > > Do you mean?
> > > >
> > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > index 9fffaccffb..071b7ea4cf 100644
> > > > --- a/target/riscv/cpu_helper.c
> > > > +++ b/target/riscv/cpu_helper.c
> > > > @@ -619,7 +619,11 @@ restart:
> > > >              return TRANSLATE_FAIL;
> > > >          }
> > > >
> > > > -        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > > > +        if (riscv_cpu_mxl(env) == MXL_RV32) {
> > > > +               hwaddr ppn = pte  >> PTE_PPN_SHIFT;
> > > > +       } else {
> > > > +               hwaddr ppn = (pte &  0x3fffffffffffffULL) >> 
> > > > PTE_PPN_SHIFT;
> > > > +       }
> > >
> > > Yes, something like this but use a define for 0x3fffffffffffffULL
> > Just as Alistair said: This will need to be dynamic based on get_xl()
> >
> >  I still prefer:
> > +#if defined(TARGET_RISCV32)
> > +#define PTE_PPN_MASK        0xffffffffUL
> > +#elif defined(TARGET_RISCV64)
> > +#define PTE_PPN_MASK        0x3fffffffffffffULL
> > +#endif
> >
> > +        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>
> Actually, using cpu_get_xl() is even better because it allows
> having lower privilege mode running at different XLEN.
Good point. You have convinced me.

>
> Regards,
> Anup
>
> >
> > >
> > > Regards,
> > > Anup
> > >
> > > >
> > > >          RISCVCPU *cpu = env_archcpu(env);
> > > >          if (!(pte & PTE_V)) {
> > > >
> > > > >
> > > > > Regards,
> > > > > Anup
> > > > >
> > > > > >
> > > > > >          if (!(pte & PTE_V)) {
> > > > > >              /* Invalid PTE */
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Best Regards
> > > >  Guo Ren
> > > >
> > > > ML: https://lore.kernel.org/linux-csky/
> > > >
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/



-- 
Best Regards
 Guo Ren

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