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

[Alistair Francis]

On Thu, Oct 19, 2023 at 4:57 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. As required by the spec, also clear
> the 'A' field of pmp entries.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
> Changes in v2:
> ====
> - Rebase on latest riscv-to-apply.next
> - Clear 'A' field.
>
>  target/riscv/cpu.c | 11 +++++++++++
>  target/riscv/pmp.c | 10 ++++++++++
>  target/riscv/pmp.h |  2 ++
>  3 files changed, 23 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index dad167833cc..491e0e46e2e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -883,6 +883,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 21d2489e27e..4dfaa28fce2 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -135,6 +135,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 | PMP_AMATCH);
> +    }
> +}
> +
>  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 cf5c99f8e68..9af8614cd4f 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -28,6 +28,7 @@ typedef enum {
>      PMP_READ  = 1 << 0,
>      PMP_WRITE = 1 << 1,
>      PMP_EXEC  = 1 << 2,
> +    PMP_AMATCH = (3 << 3),
>      PMP_LOCK  = 1 << 7
>  } pmp_priv_t;
>
> @@ -81,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
>
>