On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_...@c-sky.com> wrote: > > > > 在 2019年9月25日，上午7:33，Alistair Francis <alistai...@gmail.com> 写道： > > > > On Tue, Sep 24, 2019 at 12:58 AM <guo...@kernel.org> wrote: > >> > >> From: Guo Ren <ren_...@c-sky.com> > >> > >> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we > >> need to ignore them. They can not 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 > > > > Thanks for the patch! > > > > The spec says "must be zeroed by software for forward compatibility" > > so I don't think it's correct for QEMU to zero out the bits. > QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
Yes, from reading the spec that seems to be the correct behaviour. > > > > > Does this fix a bug you are seeing? > Yes, because we try to reuse these bits as attributes. That isn't really a bug then, the spec says not to do that. > > > > >> > >> Changelog V2: > >> - Bugfix pte destroyed cause boot fail > >> - Change to AND with a mask instead of shifting both directions > > > > The changelog shouldn't be in the commit, it should be kept under the > > line line below. > I just prefer to save them in commit. Fair, but in QEMU we don't commit the change log in the commit. > > > > >> > >> Signed-off-by: Guo Ren <ren_...@c-sky.com> > >> Reviewed-by: Liu Zhiwei <zhiwei_...@c-sky.com> > >> --- > > > > The change log should go here. > OK, but git am we’ll lose them. > > > > >> target/riscv/cpu_bits.h | 3 +++ > >> target/riscv/cpu_helper.c | 3 ++- > >> 2 files changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > >> index e998348..ae8aa0f 100644 > >> --- a/target/riscv/cpu_bits.h > >> +++ b/target/riscv/cpu_bits.h > >> @@ -470,6 +470,9 @@ > >> #define PTE_D 0x080 /* Dirty */ > >> #define PTE_SOFT 0x300 /* Reserved for Software */ > >> > >> +/* Reserved highest 10 bits in PTE */ > >> +#define PTE_RESERVED ((target_ulong)0x3ff << 54) > > > > I think it's just easier to define this as 0xFFC0000000000000ULL and > > remove the cast. > OK follow your rule, but I still prefer prior. > > > > >> + > >> /* Page table PPN shift amount */ > >> #define PTE_PPN_SHIFT 10 > >> > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > >> index 87dd6a6..7a540cc 100644 > >> --- a/target/riscv/cpu_helper.c > >> +++ b/target/riscv/cpu_helper.c > >> @@ -258,10 +258,11 @@ restart: > >> } > >> #if defined(TARGET_RISCV32) > >> target_ulong pte = ldl_phys(cs->as, pte_addr); > >> + hwaddr ppn = pte >> PTE_PPN_SHIFT; > >> #elif defined(TARGET_RISCV64) > >> target_ulong pte = ldq_phys(cs->as, pte_addr); > >> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT; > >> #endif > >> - hwaddr ppn = pte >> PTE_PPN_SHIFT; > > > > You don't have to move this shift > En… Do you want this: ? > > #if defined(TARGET_RISCV32) > target_ulong pte = ldl_phys(cs->as, pte_addr); > + hwaddr ppn = pte; > #elif defined(TARGET_RISCV64) > target_ulong pte = ldq_phys(cs->as, pte_addr); > + hwaddr ppn = (pte & ~PTE_RESERVED); > #endif > + ppn = ppn >> PTE_PPN_SHIFT; > Yeah, it seems a little cleaner. Alistair > The pte couldn’t be destroyed, just ppn ignore the RESERVED bits. > >
