On Fri, Jan 21, 2022 at 6:48 AM LIU Zhiwei <zhiwei_...@c-sky.com> wrote:
>
>
> On 2022/1/20 δΈε9:47, Guo Ren wrote:
> > Hi Alistair and Anup,
> >
> > On Tue, Jan 18, 2022 at 12:56 PM Alistair Francis <alistai...@gmail.com>
> > wrote:
> >> On Tue, Jan 18, 2022 at 1:31 PM Anup Patel <a...@brainfault.org> wrote:
> >>> On Tue, Jan 18, 2022 at 6:47 AM Weiwei Li <liwei...@iscas.ac.cn> wrote:
> >>>> From: Guo Ren <ren_...@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_...@c-sky.com>
> >>>> Tested-by: Bin Meng <bmeng...@gmail.com>
> >>>> Reviewed-by: Liu Zhiwei <zhiwei_...@c-sky.com>
> >>>> Reviewed-by: Bin Meng <bmeng...@gmail.com>
> >>>> Reviewed-by: Alistair Francis <alistair.fran...@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."
> > How about:
> >
> > + RISCVCPU *cpu = env_archcpu(env);
> > + hwaddr ppn;
> > +
> > + if (get_field(env->mstatus, MSTATUS64_SXL) == MXL_RV32) {
> Use riscv_cpu_mxl currently. Or define a new function riscv_cpu_sxl in cpu.h
I perfer riscv_cpu_sxl.
> > + ppn = pte >> PTE_PPN_SHIFT;
> > + } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
> > + ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > + } else {
> > + ppn = pte >> PTE_PPN_SHIFT;
> > + if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT)
> Just if (pte & ~PTE_PPN_MASK)
Why? low bits in pte is correct. R W X A D
> > + return TRANSLATE_FAIL;
> > + }
>
> Otherwise looks good to me.
>
> Thanks,
> Zhiwei
>
> >> Alistair
> >>
> >>> Regards,
> >>> Anup
> >>>
> >>>> if (!(pte & PTE_V)) {
> >>>> /* Invalid PTE */
> >>>> --
> >>>> 2.17.1
> >>>>
> >
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
