| Message ID | 20230412091716.126601-1-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Raise an exception if pte reserved bits are not cleared | expand |
On Wed, Apr 12, 2023 at 7:18 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > As per the specification, in 64-bit, if any of the pte reserved bits 60-54 > is set, an exception should be triggered (see 4.4.1, "Addressing and Memory > Protection"), so implement this behaviour in the address translation process. > > Reported-by: Andrea Parri <andrea@rivosinc.com> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > target/riscv/cpu_bits.h | 1 + > target/riscv/cpu_helper.c | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > index fca7ef0cef..8d9ba2ce11 100644 > --- a/target/riscv/cpu_bits.h > +++ b/target/riscv/cpu_bits.h > @@ -640,6 +640,7 @@ typedef enum { > #define PTE_SOFT 0x300 /* Reserved for Software */ > #define PTE_PBMT 0x6000000000000000ULL /* Page-based memory types */ > #define PTE_N 0x8000000000000000ULL /* NAPOT translation */ > +#define PTE_RESERVED 0x1FC0000000000000ULL /* Reserved bits */ > #define PTE_ATTR (PTE_N | PTE_PBMT) /* All attributes bits */ > > /* Page table PPN shift amount */ > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index f88c503cf4..39c323a865 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -936,6 +936,11 @@ restart: > return TRANSLATE_FAIL; > } > > + /* PTE reserved bits must be cleared otherwise an exception is raised */ > + if (riscv_cpu_mxl(env) == MXL_RV64 && (pte & PTE_RESERVED)) { > + return TRANSLATE_FAIL; > + } Isn't this caught by our existing check? if ((pte & ~(target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT) { return TRANSLATE_FAIL; } Alistair > + > bool pbmte = env->menvcfg & MENVCFG_PBMTE; > bool hade = env->menvcfg & MENVCFG_HADE; > > -- > 2.37.2 > >
Hi Alistair, > > @@ -936,6 +936,11 @@ restart: > > return TRANSLATE_FAIL; > > } > > > > + /* PTE reserved bits must be cleared otherwise an exception is raised */ > > + if (riscv_cpu_mxl(env) == MXL_RV64 && (pte & PTE_RESERVED)) { > > + return TRANSLATE_FAIL; > > + } > > Isn't this caught by our existing check? > > if ((pte & ~(target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT) { > return TRANSLATE_FAIL; > } Thanks for checking this out. AFAICS, the existing check/code doesn't work if either svnapot or svpbmt are active. Please let me know if you need other information. Andrea
On Mon, Apr 17, 2023 at 8:47 PM Andrea Parri <andrea@rivosinc.com> wrote: > > Hi Alistair, > > > > @@ -936,6 +936,11 @@ restart: > > > return TRANSLATE_FAIL; > > > } > > > > > > + /* PTE reserved bits must be cleared otherwise an exception is raised */ > > > + if (riscv_cpu_mxl(env) == MXL_RV64 && (pte & PTE_RESERVED)) { > > > + return TRANSLATE_FAIL; > > > + } > > > > Isn't this caught by our existing check? > > > > if ((pte & ~(target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT) { > > return TRANSLATE_FAIL; > > } > > Thanks for checking this out. AFAICS, the existing check/code doesn't > work if either svnapot or svpbmt are active. svpbmt uses some of the reserved fields right? I'm not sure why svnapot excludes the check. The correct fix should be to change this check as required (instead of adding a new check). Alistair > > Please let me know if you need other information. > > Andrea
Hi Alistair, Sorry for the late reply, I was on PTO. On Tue, Apr 18, 2023 at 4:22 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Apr 17, 2023 at 8:47 PM Andrea Parri <andrea@rivosinc.com> wrote: > > > > Hi Alistair, > > > > > > @@ -936,6 +936,11 @@ restart: > > > > return TRANSLATE_FAIL; > > > > } > > > > > > > > + /* PTE reserved bits must be cleared otherwise an exception is raised */ > > > > + if (riscv_cpu_mxl(env) == MXL_RV64 && (pte & PTE_RESERVED)) { > > > > + return TRANSLATE_FAIL; > > > > + } > > > > > > Isn't this caught by our existing check? > > > > > > if ((pte & ~(target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT) { > > > return TRANSLATE_FAIL; > > > } > > > > Thanks for checking this out. AFAICS, the existing check/code doesn't > > work if either svnapot or svpbmt are active. > > svpbmt uses some of the reserved fields right? No, pbmt uses bits 61-62 and napot uses bit 63, this patchset deals with bits 54-60. > > I'm not sure why svnapot excludes the check. The correct fix should be > to change this check as required (instead of adding a new check). napot and pbmt exclude the check because PTE_PPN_MASK only deals with PPN bits, and then ~PTE_PPN_MASK is too large as it tests the PBMT/NAPOT bits AND the reserved bits. But you made me notice that the pbmt/napot check isn't right either, as the napot bit could be set if !napot and then an exception would not be triggered (the same for pbmt). Let me check this completely and come back with a proper fix for that too. Thanks! Alex > > Alistair > > > > > Please let me know if you need other information. > > > > Andrea
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index fca7ef0cef..8d9ba2ce11 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -640,6 +640,7 @@ typedef enum { #define PTE_SOFT 0x300 /* Reserved for Software */ #define PTE_PBMT 0x6000000000000000ULL /* Page-based memory types */ #define PTE_N 0x8000000000000000ULL /* NAPOT translation */ +#define PTE_RESERVED 0x1FC0000000000000ULL /* Reserved bits */ #define PTE_ATTR (PTE_N | PTE_PBMT) /* All attributes bits */ /* Page table PPN shift amount */ diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index f88c503cf4..39c323a865 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -936,6 +936,11 @@ restart: return TRANSLATE_FAIL; } + /* PTE reserved bits must be cleared otherwise an exception is raised */ + if (riscv_cpu_mxl(env) == MXL_RV64 && (pte & PTE_RESERVED)) { + return TRANSLATE_FAIL; + } + bool pbmte = env->menvcfg & MENVCFG_PBMTE; bool hade = env->menvcfg & MENVCFG_HADE;
As per the specification, in 64-bit, if any of the pte reserved bits 60-54 is set, an exception should be triggered (see 4.4.1, "Addressing and Memory Protection"), so implement this behaviour in the address translation process. Reported-by: Andrea Parri <andrea@rivosinc.com> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- target/riscv/cpu_bits.h | 1 + target/riscv/cpu_helper.c | 5 +++++ 2 files changed, 6 insertions(+)