[v1] target/riscv: pmp: Ignore writes when RW=01

[PATCH] target/riscv: pmp: Ignore writes when RW=01

Posted by Mayuresh Chitale 2 years, 4 months ago

As per the Priv spec: "The R, W, and X fields form a collective WARL
field for which the combinations with R=0 and W=1 are reserved."
However currently such writes are not ignored as ought to be. The
combinations with RW=01 are allowed only when the Smepmp extension
is enabled and mseccfg.MML is set.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/pmp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index f3eb6e6585..5b430be18c 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -119,6 +119,14 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
         if (locked) {
             qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
         } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
+            /* If !mseccfg.MML then ignore writes with encoding RW=01 */
+            if ((val & PMP_WRITE) && !(val & PMP_READ) &&
+                (!riscv_cpu_cfg(env)->ext_smepmp ||
+                !MSECCFG_MML_ISSET(env))) {
+                val &= ~(PMP_WRITE | PMP_READ);
+                val |= env->pmp_state.pmp[pmp_index].cfg_reg &
+                       (PMP_WRITE | PMP_READ);
+            }
             env->pmp_state.pmp[pmp_index].cfg_reg = val;
             pmp_update_rule_addr(env, pmp_index);
             return true;
-- 
2.34.1

Re: [PATCH] target/riscv: pmp: Ignore writes when RW=01

Posted by Alistair Francis 2 years, 4 months ago

On Thu, Sep 14, 2023 at 10:35 PM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> As per the Priv spec: "The R, W, and X fields form a collective WARL
> field for which the combinations with R=0 and W=1 are reserved."
> However currently such writes are not ignored as ought to be. The
> combinations with RW=01 are allowed only when the Smepmp extension
> is enabled and mseccfg.MML is set.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>  target/riscv/pmp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index f3eb6e6585..5b430be18c 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -119,6 +119,14 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>          if (locked) {
>              qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
>          } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
> +            /* If !mseccfg.MML then ignore writes with encoding RW=01 */
> +            if ((val & PMP_WRITE) && !(val & PMP_READ) &&
> +                (!riscv_cpu_cfg(env)->ext_smepmp ||
> +                !MSECCFG_MML_ISSET(env))) {

MSECCFG_MML can only be set if ext_smepmp exists, so I don't think we
need both checks here

> +                val &= ~(PMP_WRITE | PMP_READ);
> +                val |= env->pmp_state.pmp[pmp_index].cfg_reg &
> +                       (PMP_WRITE | PMP_READ);

Why do we restore the previous value? It's a WARL so we should just
guarantee a legal value (which would be 0x0 I guess)

Alistair

> +            }
>              env->pmp_state.pmp[pmp_index].cfg_reg = val;
>              pmp_update_rule_addr(env, pmp_index);
>              return true;
> --
> 2.34.1
>
>

Re: [PATCH] target/riscv: pmp: Ignore writes when RW=01

Posted by Mayuresh Chitale 2 years, 4 months ago

On Mon, Sep 18, 2023 at 7:02 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Sep 14, 2023 at 10:35 PM Mayuresh Chitale
> <mchitale@ventanamicro.com> wrote:
> >
> > As per the Priv spec: "The R, W, and X fields form a collective WARL
> > field for which the combinations with R=0 and W=1 are reserved."
> > However currently such writes are not ignored as ought to be. The
> > combinations with RW=01 are allowed only when the Smepmp extension
> > is enabled and mseccfg.MML is set.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >  target/riscv/pmp.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index f3eb6e6585..5b430be18c 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -119,6 +119,14 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> >          if (locked) {
> >              qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> >          } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
> > +            /* If !mseccfg.MML then ignore writes with encoding RW=01 */
> > +            if ((val & PMP_WRITE) && !(val & PMP_READ) &&
> > +                (!riscv_cpu_cfg(env)->ext_smepmp ||
> > +                !MSECCFG_MML_ISSET(env))) {
>
> MSECCFG_MML can only be set if ext_smepmp exists, so I don't think we
> need both checks here
Ok.
>
> > +                val &= ~(PMP_WRITE | PMP_READ);
> > +                val |= env->pmp_state.pmp[pmp_index].cfg_reg &
> > +                       (PMP_WRITE | PMP_READ);
>
> Why do we restore the previous value? It's a WARL so we should just
> guarantee a legal value (which would be 0x0 I guess)
Yes. It can be set to 0x0.
>
> Alistair
>
> > +            }
> >              env->pmp_state.pmp[pmp_index].cfg_reg = val;
> >              pmp_update_rule_addr(env, pmp_index);
> >              return true;
> > --
> > 2.34.1
> >
> >