Hi Vladimir,
On Fri, Oct 6, 2023 at 5:08 PM Vladimir Isaev
<vladimir.is...@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 <mchit...@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
> >
> >
