[v1] target/riscv: pmp: Clear pmp/smepmp bits on reset

[PATCH] target/riscv: pmp: Clear pmp/smepmp bits on reset

Posted by Mayuresh Chitale 7 months, 1 week ago

As per the Priv and Smepmp specifications, certain bits such as the 'L'
bit of pmp entries and mseccfg.MML can only be cleared upon reset and it
is necessary to do so to allow 'M' mode firmware to correctly reinitialize
the pmp/smpemp state across reboots.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/cpu.c | 11 +++++++++++
 target/riscv/pmp.c | 10 ++++++++++
 target/riscv/pmp.h |  1 +
 3 files changed, 22 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 0fb01788e7..561567651e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -761,6 +761,17 @@ static void riscv_cpu_reset_hold(Object *obj)
     }
     /* mmte is supposed to have pm.current hardwired to 1 */
     env->mmte |= (EXT_STATUS_INITIAL | MMTE_M_PM_CURRENT);
+
+    /*
+     * Clear mseccfg and unlock all the PMP entries upon reset.
+     * This is allowed as per the priv and smepmp specifications
+     * and is needed to clear stale entries across reboots.
+     */
+    if (riscv_cpu_cfg(env)->ext_smepmp) {
+        env->mseccfg = 0;
+    }
+
+    pmp_unlock_entries(env);
 #endif
     env->xl = riscv_cpu_mxl(env);
     riscv_cpu_update_mask(env);
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index f498e414f0..5b14eb511a 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -129,6 +129,16 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
     }
 }
 
+void pmp_unlock_entries(CPURISCVState *env)
+{
+    uint32_t pmp_num = pmp_get_num_rules(env);
+    int i;
+
+    for (i = 0; i < pmp_num; i++) {
+        env->pmp_state.pmp[i].cfg_reg &= ~PMP_LOCK;
+    }
+}
+
 static void pmp_decode_napot(target_ulong a, target_ulong *sa,
                              target_ulong *ea)
 {
diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
index b296ea1fc6..0ab60fe15f 100644
--- a/target/riscv/pmp.h
+++ b/target/riscv/pmp.h
@@ -82,6 +82,7 @@ void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
 void pmp_update_rule_nums(CPURISCVState *env);
 uint32_t pmp_get_num_rules(CPURISCVState *env);
 int pmp_priv_to_page_prot(pmp_priv_t pmp_priv);
+void pmp_unlock_entries(CPURISCVState *env);
 
 #define MSECCFG_MML_ISSET(env) get_field(env->mseccfg, MSECCFG_MML)
 #define MSECCFG_MMWP_ISSET(env) get_field(env->mseccfg, MSECCFG_MMWP)
-- 
2.34.1

Re: [PATCH] target/riscv: pmp: Clear pmp/smepmp bits on reset

Posted by Vladimir Isaev 6 months, 4 weeks ago

Hi Mayuresh,

25.09.2023 14:09, Mayuresh Chitale wrote:
> As per the Priv and Smepmp specifications, certain bits such as the 'L'
> bit of pmp entries and mseccfg.MML can only be cleared upon reset and it
> is necessary to do so to allow 'M' mode firmware to correctly reinitialize
> the pmp/smpemp state across reboots.
> 
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 11 +++++++++++
>  target/riscv/pmp.c | 10 ++++++++++
>  target/riscv/pmp.h |  1 +
>  3 files changed, 22 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 0fb01788e7..561567651e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -761,6 +761,17 @@ static void riscv_cpu_reset_hold(Object *obj)
>      }
>      /* mmte is supposed to have pm.current hardwired to 1 */
>      env->mmte |= (EXT_STATUS_INITIAL | MMTE_M_PM_CURRENT);
> +
> +    /*
> +     * Clear mseccfg and unlock all the PMP entries upon reset.
> +     * This is allowed as per the priv and smepmp specifications
> +     * and is needed to clear stale entries across reboots.
> +     */
> +    if (riscv_cpu_cfg(env)->ext_smepmp) {
> +        env->mseccfg = 0;
> +    }
> +
> +    pmp_unlock_entries(env);
>  #endif
>      env->xl = riscv_cpu_mxl(env);
>      riscv_cpu_update_mask(env);
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index f498e414f0..5b14eb511a 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -129,6 +129,16 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>      }
>  }
> 
> +void pmp_unlock_entries(CPURISCVState *env)
> +{
> +    uint32_t pmp_num = pmp_get_num_rules(env);
> +    int i;
> +
> +    for (i = 0; i < pmp_num; i++) {
> +        env->pmp_state.pmp[i].cfg_reg &= ~PMP_LOCK;

According to spec:

Writable PMP registers’ A and L fields are set to 0, unless the
platform mandates a different reset value for some PMP registers’ A and L fields.

So should we also set PMP_AMATCH_OFF in cfg?

Thank you,
Vladimir Isaev

> +    }
> +}
> +
>  static void pmp_decode_napot(target_ulong a, target_ulong *sa,
>                               target_ulong *ea)
>  {
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index b296ea1fc6..0ab60fe15f 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -82,6 +82,7 @@ void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>  void pmp_update_rule_nums(CPURISCVState *env);
>  uint32_t pmp_get_num_rules(CPURISCVState *env);
>  int pmp_priv_to_page_prot(pmp_priv_t pmp_priv);
> +void pmp_unlock_entries(CPURISCVState *env);
> 
>  #define MSECCFG_MML_ISSET(env) get_field(env->mseccfg, MSECCFG_MML)
>  #define MSECCFG_MMWP_ISSET(env) get_field(env->mseccfg, MSECCFG_MMWP)
> --
> 2.34.1
> 
>

Re: [PATCH] target/riscv: pmp: Clear pmp/smepmp bits on reset

Posted by Mayuresh Chitale 6 months, 3 weeks ago

Hi Vladimir,

On Fri, Oct 6, 2023 at 5:08 PM Vladimir Isaev
<vladimir.isaev@syntacore.com> wrote:
>
> Hi Mayuresh,
>
> 25.09.2023 14:09, Mayuresh Chitale wrote:
> > As per the Priv and Smepmp specifications, certain bits such as the 'L'
> > bit of pmp entries and mseccfg.MML can only be cleared upon reset and it
> > is necessary to do so to allow 'M' mode firmware to correctly reinitialize
> > the pmp/smpemp state across reboots.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >  target/riscv/cpu.c | 11 +++++++++++
> >  target/riscv/pmp.c | 10 ++++++++++
> >  target/riscv/pmp.h |  1 +
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 0fb01788e7..561567651e 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -761,6 +761,17 @@ static void riscv_cpu_reset_hold(Object *obj)
> >      }
> >      /* mmte is supposed to have pm.current hardwired to 1 */
> >      env->mmte |= (EXT_STATUS_INITIAL | MMTE_M_PM_CURRENT);
> > +
> > +    /*
> > +     * Clear mseccfg and unlock all the PMP entries upon reset.
> > +     * This is allowed as per the priv and smepmp specifications
> > +     * and is needed to clear stale entries across reboots.
> > +     */
> > +    if (riscv_cpu_cfg(env)->ext_smepmp) {
> > +        env->mseccfg = 0;
> > +    }
> > +
> > +    pmp_unlock_entries(env);
> >  #endif
> >      env->xl = riscv_cpu_mxl(env);
> >      riscv_cpu_update_mask(env);
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index f498e414f0..5b14eb511a 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -129,6 +129,16 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> >      }
> >  }
> >
> > +void pmp_unlock_entries(CPURISCVState *env)
> > +{
> > +    uint32_t pmp_num = pmp_get_num_rules(env);
> > +    int i;
> > +
> > +    for (i = 0; i < pmp_num; i++) {
> > +        env->pmp_state.pmp[i].cfg_reg &= ~PMP_LOCK;
>
> According to spec:
>
> Writable PMP registers’ A and L fields are set to 0, unless the
> platform mandates a different reset value for some PMP registers’ A and L fields.
Yes.
>
> So should we also set PMP_AMATCH_OFF in cfg?
I think clearing the 'A' field should be sufficient.

>
> Thank you,
> Vladimir Isaev
>
> > +    }
> > +}
> > +
> >  static void pmp_decode_napot(target_ulong a, target_ulong *sa,
> >                               target_ulong *ea)
> >  {
> > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> > index b296ea1fc6..0ab60fe15f 100644
> > --- a/target/riscv/pmp.h
> > +++ b/target/riscv/pmp.h
> > @@ -82,6 +82,7 @@ void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
> >  void pmp_update_rule_nums(CPURISCVState *env);
> >  uint32_t pmp_get_num_rules(CPURISCVState *env);
> >  int pmp_priv_to_page_prot(pmp_priv_t pmp_priv);
> > +void pmp_unlock_entries(CPURISCVState *env);
> >
> >  #define MSECCFG_MML_ISSET(env) get_field(env->mseccfg, MSECCFG_MML)
> >  #define MSECCFG_MMWP_ISSET(env) get_field(env->mseccfg, MSECCFG_MMWP)
> > --
> > 2.34.1
> >
> >