* [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64
@ 2019-09-26 0:14 guoren
2019-10-12 17:33 ` Palmer Dabbelt
0 siblings, 1 reply; 5+ messages in thread
From: guoren @ 2019-09-26 0:14 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair23, palmer, alistair.francis, Guo Ren, bmeng.cn
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 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
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(-)
Changelog V6:
- Add Reviewer: Alistair Francis
Changelog V5:
- Add Reviewer and Tester: Bin Meng
Changelog V4:
- Change title to Ignore not Bugfix
- Use PTE_PPN_MASK for RV32 and RV64
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..399c2c6 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -473,6 +473,13 @@
/* 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
+
/* Leaf page shift amount */
#define PGSHIFT 12
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 87dd6a6..9961b37 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -261,7 +261,7 @@ restart:
#elif defined(TARGET_RISCV64)
target_ulong pte = ldq_phys(cs->as, pte_addr);
#endif
- hwaddr ppn = pte >> PTE_PPN_SHIFT;
+ hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
if (!(pte & PTE_V)) {
/* Invalid PTE */
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64
2019-09-26 0:14 [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64 guoren
@ 2019-10-12 17:33 ` Palmer Dabbelt
2019-10-13 0:16 ` Guo Ren
2019-10-14 22:50 ` Alistair Francis
0 siblings, 2 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2019-10-12 17:33 UTC (permalink / raw)
To: guoren
Cc: qemu-riscv, qemu-devel, Alistair Francis, ren_guo, alistair23, bmeng.cn
On Wed, 25 Sep 2019 17:14:21 PDT (-0700), 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 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
>
> 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(-)
>
> Changelog V6:
> - Add Reviewer: Alistair Francis
>
> Changelog V5:
> - Add Reviewer and Tester: Bin Meng
>
> Changelog V4:
> - Change title to Ignore not Bugfix
> - Use PTE_PPN_MASK for RV32 and RV64
>
> 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..399c2c6 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -473,6 +473,13 @@
> /* 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
> +
> /* Leaf page shift amount */
> #define PGSHIFT 12
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..9961b37 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -261,7 +261,7 @@ restart:
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> #endif
> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
> + hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
I know I'm a bit late to the party here, but I don't like this. There's ample
evidence that wrapping the physical address space is a bad idea, and just
because the ISA allows implementations to do this doesn't mean we should.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64
2019-10-12 17:33 ` Palmer Dabbelt
@ 2019-10-13 0:16 ` Guo Ren
2019-10-13 0:33 ` Jonathan Behrens
2019-10-14 22:50 ` Alistair Francis
1 sibling, 1 reply; 5+ messages in thread
From: Guo Ren @ 2019-10-13 0:16 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Ren Guo, Alistair Francis, Bin Meng
The patch didn't wrap the physical address space directly, just follow the spec.
I admit that I am trying to use the compliance specification to allow
qemu to support some non-standard software.
But compliance specification and wrapping the physical address space
are different things.
I'm preparing c910 pachset for linux riscv and you can question me there.
On Sun, Oct 13, 2019 at 1:33 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 25 Sep 2019 17:14:21 PDT (-0700), 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 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
> >
> > 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(-)
> >
> > Changelog V6:
> > - Add Reviewer: Alistair Francis
> >
> > Changelog V5:
> > - Add Reviewer and Tester: Bin Meng
> >
> > Changelog V4:
> > - Change title to Ignore not Bugfix
> > - Use PTE_PPN_MASK for RV32 and RV64
> >
> > 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..399c2c6 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -473,6 +473,13 @@
> > /* 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
> > +
> > /* Leaf page shift amount */
> > #define PGSHIFT 12
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 87dd6a6..9961b37 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -261,7 +261,7 @@ restart:
> > #elif defined(TARGET_RISCV64)
> > target_ulong pte = ldq_phys(cs->as, pte_addr);
> > #endif
> > - hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > + hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >
> > if (!(pte & PTE_V)) {
> > /* Invalid PTE */
>
> I know I'm a bit late to the party here, but I don't like this. There's ample
> evidence that wrapping the physical address space is a bad idea, and just
> because the ISA allows implementations to do this doesn't mean we should.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64
2019-10-13 0:16 ` Guo Ren
@ 2019-10-13 0:33 ` Jonathan Behrens
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Behrens @ 2019-10-13 0:33 UTC (permalink / raw)
To: Guo Ren
Cc: open list:RISC-V, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Alistair Francis, Ren Guo,
Alistair Francis, Bin Meng
There is nowhere in the spec that ever says what hardware has to do if
any of those reserved bits are non-zero. Hardware is certainly not
required to ignore them and treat the PTE as being valid (which is
what this patch does). I'd argue that since only buggy code would ever
set these bits, QEMU should treat any PTE with them set as being
invalid so that programmers can realize they've made a mistake.
Jonathan
On Sat, Oct 12, 2019 at 8:16 PM Guo Ren <guoren@kernel.org> wrote:
>
> The patch didn't wrap the physical address space directly, just follow the spec.
> I admit that I am trying to use the compliance specification to allow
> qemu to support some non-standard software.
> But compliance specification and wrapping the physical address space
> are different things.
> I'm preparing c910 pachset for linux riscv and you can question me there.
>
> On Sun, Oct 13, 2019 at 1:33 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > On Wed, 25 Sep 2019 17:14:21 PDT (-0700), 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 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
> > >
> > > 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(-)
> > >
> > > Changelog V6:
> > > - Add Reviewer: Alistair Francis
> > >
> > > Changelog V5:
> > > - Add Reviewer and Tester: Bin Meng
> > >
> > > Changelog V4:
> > > - Change title to Ignore not Bugfix
> > > - Use PTE_PPN_MASK for RV32 and RV64
> > >
> > > 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..399c2c6 100644
> > > --- a/target/riscv/cpu_bits.h
> > > +++ b/target/riscv/cpu_bits.h
> > > @@ -473,6 +473,13 @@
> > > /* 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
> > > +
> > > /* Leaf page shift amount */
> > > #define PGSHIFT 12
> > >
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 87dd6a6..9961b37 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -261,7 +261,7 @@ restart:
> > > #elif defined(TARGET_RISCV64)
> > > target_ulong pte = ldq_phys(cs->as, pte_addr);
> > > #endif
> > > - hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > > + hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > >
> > > if (!(pte & PTE_V)) {
> > > /* Invalid PTE */
> >
> > I know I'm a bit late to the party here, but I don't like this. There's ample
> > evidence that wrapping the physical address space is a bad idea, and just
> > because the ISA allows implementations to do this doesn't mean we should.
>
>
>
> --
> Best Regards
> Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64
2019-10-12 17:33 ` Palmer Dabbelt
2019-10-13 0:16 ` Guo Ren
@ 2019-10-14 22:50 ` Alistair Francis
1 sibling, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2019-10-14 22:50 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Guo Ren, Guo Ren, Bin Meng
On Sat, Oct 12, 2019 at 10:33 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 25 Sep 2019 17:14:21 PDT (-0700), 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 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
> >
> > 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(-)
> >
> > Changelog V6:
> > - Add Reviewer: Alistair Francis
> >
> > Changelog V5:
> > - Add Reviewer and Tester: Bin Meng
> >
> > Changelog V4:
> > - Change title to Ignore not Bugfix
> > - Use PTE_PPN_MASK for RV32 and RV64
> >
> > 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..399c2c6 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -473,6 +473,13 @@
> > /* 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
> > +
> > /* Leaf page shift amount */
> > #define PGSHIFT 12
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 87dd6a6..9961b37 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -261,7 +261,7 @@ restart:
> > #elif defined(TARGET_RISCV64)
> > target_ulong pte = ldq_phys(cs->as, pte_addr);
> > #endif
> > - hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > + hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >
> > if (!(pte & PTE_V)) {
> > /* Invalid PTE */
>
> I know I'm a bit late to the party here, but I don't like this. There's ample
> evidence that wrapping the physical address space is a bad idea, and just
> because the ISA allows implementations to do this doesn't mean we should.
I think this is ok as the spec specifically states that "These
reserved bits may also be used to facilitate research
experimentation." and as QEMU is generally used for developing new
features it makes sense to allow guests to set these bit and we just
ignore them.
Software should always set these to zero, so the worst outcome of
doing this is that QEMU will hid software bugs if people set the bits,
but I don't see that as huge downside.
Alistair
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-14 22:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 0:14 [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64 guoren
2019-10-12 17:33 ` Palmer Dabbelt
2019-10-13 0:16 ` Guo Ren
2019-10-13 0:33 ` Jonathan Behrens
2019-10-14 22:50 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).