| Message ID | 1569311902-12173-1-git-send-email-guoren@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] target/riscv: Bugfix reserved bits in PTE for RV64 | expand |
I only tested it on qemu-3.1.0, pls have a try before merge. On Tue, Sep 24, 2019 at 4:00 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 > > Changelog V2: > - Bugfix pte destroyed cause boot fail > - Change to AND with a mask instead of shifting both directions > > 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 | 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) > + > /* 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; > > if (!(pte & PTE_V)) { > /* Invalid PTE */ > -- > 2.7.4 > >
On Tue, Sep 24, 2019 at 12:58 AM <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 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. Does this fix a bug you are seeing? > > 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. > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> > Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com> > --- The change log should go here. > 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. > + > /* 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 > > if (!(pte & PTE_V)) { > /* Invalid PTE */ > -- > 2.7.4 >
> 在 2019年9月25日，上午7:33，Alistair Francis <alistair23@gmail.com> 写道： > > On Tue, Sep 24, 2019 at 12:58 AM <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 > > 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. > > Does this fix a bug you are seeing? Yes, because we try to reuse these bits as attributes. > >> >> 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. > >> >> Signed-off-by: Guo Ren <ren_guo@c-sky.com> >> Reviewed-by: Liu Zhiwei <zhiwei_liu@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; The pte couldn’t be destroyed, just ppn ignore the RESERVED bits.
On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_guo@c-sky.com> wrote: > > > > 在 2019年9月25日，上午7:33，Alistair Francis <alistair23@gmail.com> 写道： > > > > On Tue, Sep 24, 2019 at 12:58 AM <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 > > > > 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_guo@c-sky.com> > >> Reviewed-by: Liu Zhiwei <zhiwei_liu@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. > >
在 2019/9/25 上午8:21, Alistair Francis 写道: > On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_guo@c-sky.com> wrote: >> >> >>> 在 2019年9月25日，上午7:33，Alistair Francis <alistair23@gmail.com> 写道： >>> >>> On Tue, Sep 24, 2019 at 12:58 AM <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 >>> >>> 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. Thank you very much :) > >> >>> >>> 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. Yes, I'll change the tile that "Ignore reserved bits in PTE for RV64" > >> >>> >>>> >>>> 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. Ok. > >> >>> >>>> >>>> Signed-off-by: Guo Ren <ren_guo@c-sky.com> >>>> Reviewed-by: Liu Zhiwei <zhiwei_liu@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. :) Best Regards Guo Ren
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) + /* 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; if (!(pte & PTE_V)) { /* Invalid PTE */