diff mbox series

riscv: Raise an exception if pte reserved bits are not cleared

Message ID 20230412091716.126601-1-alexghiti@rivosinc.com
State New
Headers show
Series riscv: Raise an exception if pte reserved bits are not cleared | expand

Commit Message

Alexandre Ghiti April 12, 2023, 9:17 a.m. UTC 
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(+)

Comments

Alistair Francis April 17, 2023, 3:24 a.m. UTC | #1 
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
>
>
Andrea Parri April 17, 2023, 10:46 a.m. UTC | #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
Alistair Francis April 18, 2023, 2:22 a.m. UTC | #3 
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
Alexandre Ghiti April 18, 2023, 8:56 a.m. UTC | #4 
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 mbox series

Patch

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;