| Message ID | 1569386895-8726-1-git-send-email-guoren@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3] target/riscv: Bugfix reserved bits in PTE for RV64 | expand |
On Tue, Sep 24, 2019 at 9:48 PM <guoren@kernel.org> wrote: > > From: Guo Ren <ren_guo@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 Hey, As I mentioned on patch 2 I don't think this is right. It isn't up to HW to clear these bits, software should keep them clear. Alistair > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> > Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com> > --- > target/riscv/cpu_bits.h | 3 +++ > target/riscv/cpu_helper.c | 4 +++- > 2 files changed, 6 insertions(+), 1 deletion(-) > --- > 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..cdc62a8 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 0xFFC0000000000000ULL > + > /* 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..7e04ff5 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -258,10 +258,12 @@ restart: > } > #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 > - hwaddr ppn = pte >> PTE_PPN_SHIFT; > + ppn = ppn >> PTE_PPN_SHIFT; > > if (!(pte & PTE_V)) { > /* Invalid PTE */ > -- > 2.7.4 >
On Wed, Sep 25, 2019 at 12:49 PM <guoren@kernel.org> wrote: > > From: Guo Ren <ren_guo@c-sky.com> > nits: the title is probably better to be rephrased to: Ignore reserved bits when calculating PPN for RV64 > 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. nits: cannot > > 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 <ren_guo@c-sky.com> > Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com> > --- > target/riscv/cpu_bits.h | 3 +++ > target/riscv/cpu_helper.c | 4 +++- > 2 files changed, 6 insertions(+), 1 deletion(-) > --- > Changelog V3: nits: normally we put changelog before the changed file summary above, and there is no need to put another --- > - 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..cdc62a8 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 0xFFC0000000000000ULL Can we define the macro for RV32 too, so that (see below) > + > /* 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..7e04ff5 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -258,10 +258,12 @@ restart: > } > #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 > - hwaddr ppn = pte >> PTE_PPN_SHIFT; > + ppn = ppn >> PTE_PPN_SHIFT; we can just do this in this single line? > > if (!(pte & PTE_V)) { > /* Invalid PTE */ > -- Regards, Bin
On Wed, Sep 25, 2019 at 1:35 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Wed, Sep 25, 2019 at 12:49 PM <guoren@kernel.org> wrote: > > > > From: Guo Ren <ren_guo@c-sky.com> > > > > nits: the title is probably better to be rephrased to: Ignore reserved > bits when calculating PPN for RV64 Yes, I forgot change the title. > > > 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. > > nits: cannot Thx > > > > > 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 <ren_guo@c-sky.com> > > Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com> > > --- > > target/riscv/cpu_bits.h | 3 +++ > > target/riscv/cpu_helper.c | 4 +++- > > 2 files changed, 6 insertions(+), 1 deletion(-) > > --- > > Changelog V3: > > nits: normally we put changelog before the changed file summary above, > and there is no need to put another --- OK, just remove --- > > > - 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..cdc62a8 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 0xFFC0000000000000ULL > > Can we define the macro for RV32 too, so that (see below) OK > > > + > > /* 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..7e04ff5 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -258,10 +258,12 @@ restart: > > } > > #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 > > - hwaddr ppn = pte >> PTE_PPN_SHIFT; > > + ppn = ppn >> PTE_PPN_SHIFT; > > we can just do this in this single line? Yes
On Wed, Sep 25, 2019 at 1:19 PM Alistair Francis <alistair23@gmail.com> wrote: > > On Tue, Sep 24, 2019 at 9:48 PM <guoren@kernel.org> wrote: > > > > From: Guo Ren <ren_guo@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 > > Hey, > > As I mentioned on patch 2 I don't think this is right. It isn't up to > HW to clear these bits, software should keep them clear. These bits are not part of ppn in spec, so we still need to ignore them for ppn. The patch is reasonable.
Any code whose behavior is changed by this patch is wrong, so it doesn't seem like it matters much whether this is merged or not. Personally I'd lean towards making sure that attempts to use PTEs with the reserved bit set would always fault, rather than wrapping around to low memory and perhaps silently succeeding. Jonathan On Wed, Sep 25, 2019 at 8:29 AM Guo Ren <guoren@kernel.org> wrote: > On Wed, Sep 25, 2019 at 1:19 PM Alistair Francis <alistair23@gmail.com> > wrote: > > > > On Tue, Sep 24, 2019 at 9:48 PM <guoren@kernel.org> wrote: > > > > > > From: Guo Ren <ren_guo@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 > > > > Hey, > > > > As I mentioned on patch 2 I don't think this is right. It isn't up to > > HW to clear these bits, software should keep them clear. > > These bits are not part of ppn in spec, so we still need to ignore them > for ppn. > > The patch is reasonable. > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ > >
I think your thoughts are wrong. The specification is very clear: these bits are not part of ppn, not part of the translation target address. The current code is against the riscv-privilege specification. On Wed, Sep 25, 2019 at 11:20 PM Jonathan Behrens <fintelia@gmail.com> wrote: > > Any code whose behavior is changed by this patch is wrong, so it doesn't seem like it matters much whether this is merged or not. Personally I'd lean towards making sure that attempts to use PTEs with the reserved bit set would always fault, rather than wrapping around to low memory and perhaps silently succeeding. > > Jonathan > > On Wed, Sep 25, 2019 at 8:29 AM Guo Ren <guoren@kernel.org> wrote: >> >> On Wed, Sep 25, 2019 at 1:19 PM Alistair Francis <alistair23@gmail.com> wrote: >> > >> > On Tue, Sep 24, 2019 at 9:48 PM <guoren@kernel.org> wrote: >> > > >> > > From: Guo Ren <ren_guo@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 >> > >> > Hey, >> > >> > As I mentioned on patch 2 I don't think this is right. It isn't up to >> > HW to clear these bits, software should keep them clear. >> >> These bits are not part of ppn in spec, so we still need to ignore them for ppn. >> >> The patch is reasonable. >> >> -- >> Best Regards >> Guo Ren >> >> ML: https://lore.kernel.org/linux-csky/ >>
> The specification is very clear: these bits are not part of ppn, not > part of the translation target address. The current code is against > the riscv-privilege specification. If all of the reserved bits are zero then the patch changes nothing. Further the only normative mention of the reserved bits in the spec says they must be: "Bits 63–54 are reserved for future use and must be zeroed by software for forward compatibility." Provided that software follows the spec current QEMU will behave properly. For software that ignores that directive an sets some of those bits, the spec says nothing about what hardware should do, so both the old an the new behavior are fine. Jonathan
"Bits 63–54 are reserved for future use and must be zeroed by software for forward compatibility." That doesn't mean 63-54 are belong to ppn, it's reserved for future and nobody know 63-54 will be part of ppn. Current riscv qemu ppn implementation is obviously wrong. It shouldn't care the software's behavior, please follow the spec. On Wed, Sep 25, 2019 at 11:58 PM Jonathan Behrens <fintelia@gmail.com> wrote: > > > The specification is very clear: these bits are not part of ppn, not > > part of the translation target address. The current code is against > > the riscv-privilege specification. > > If all of the reserved bits are zero then the patch changes nothing. > Further the only normative mention of the reserved bits in the spec > says they must be: "Bits 63–54 are reserved for future use and must be > zeroed by software for forward compatibility." Provided that software > follows the spec current QEMU will behave properly. For software that > ignores that directive an sets some of those bits, the spec says > nothing about what hardware should do, so both the old an the new > behavior are fine. > > Jonathan
On Wed, Sep 25, 2019 at 9:16 AM Guo Ren <guoren@kernel.org> wrote: > > "Bits 63–54 are reserved for future use and must be > zeroed by software for forward compatibility." > > That doesn't mean 63-54 are belong to ppn, it's reserved for future > and nobody know 63-54 will be part of ppn. > Current riscv qemu ppn implementation is obviously wrong. It shouldn't > care the software's behavior, please follow the spec. You have convinced me, I think this is an acceptable change. Alistair > > On Wed, Sep 25, 2019 at 11:58 PM Jonathan Behrens <fintelia@gmail.com> wrote: > > > > > The specification is very clear: these bits are not part of ppn, not > > > part of the translation target address. The current code is against > > > the riscv-privilege specification. > > > > If all of the reserved bits are zero then the patch changes nothing. > > Further the only normative mention of the reserved bits in the spec > > says they must be: "Bits 63–54 are reserved for future use and must be > > zeroed by software for forward compatibility." Provided that software > > follows the spec current QEMU will behave properly. For software that > > ignores that directive an sets some of those bits, the spec says > > nothing about what hardware should do, so both the old an the new > > behavior are fine. > > > > Jonathan > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index e998348..cdc62a8 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 0xFFC0000000000000ULL + /* 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..7e04ff5 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -258,10 +258,12 @@ restart: } #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 - hwaddr ppn = pte >> PTE_PPN_SHIFT; + ppn = ppn >> PTE_PPN_SHIFT; if (!(pte & PTE_V)) { /* Invalid PTE */