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

[Mayuresh Chitale]

On Tue, Sep 19, 2023 at 10:10 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Sep 7, 2023 at 4:25 PM Mayuresh Chitale
> <mchitale@ventanamicro.com> 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 0843461660..77ed653b8d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -896,6 +896,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) {
>
> Does this compile? ext_smepmp doesn't seem to exist
It requires the patch below as well:
https://lists.nongnu.org/archive/html/qemu-riscv/2023-06/msg00122.html

I will rebase and resend it.
>
> Alistair
>
> > +        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 3f6c8cf08d..f3eb6e6585 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -131,6 +131,16 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t 
> > pmp_index, uint8_t val)
> >      return false;
> >  }
> >
> > +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 cf5c99f8e6..2c5ec3cdf1 100644
> > --- a/target/riscv/pmp.h
> > +++ b/target/riscv/pmp.h
> > @@ -81,6 +81,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
> >
> >