On Thu, Mar 13, 2025 at 12:40 PM Daniel Henrique Barboza <
[email protected]> wrote:
>
>
> On 2/25/25 1:00 PM, Loïc Lefort wrote:
> > When Smepmp is supported, RLB allows bypassing locks when writing CSRs
> but
> > should not affect interpretation of actual PMP rules.
> >
> > pmp_is_locked is changed to only check LOCK bit and a new pmp_is_readonly
> > function is added that checks both LOCK bit and mseccfg.RLB.
> pmp_write_cfg and
> > pmpaddr_csr_write are changed to use pmp_is_readonly while
> pmp_hart_has_privs
> > keeps using pmp_is_locked.
>
>
> Note that this change (removing MSECCFG_RLB_ISSET(env) check from
> pmp_is_locked())
> can impact the behavior of at least one caller in pmp_hart_has_privs():
>
>
> if (!MSECCFG_MML_ISSET(env)) {
> /*
> * If mseccfg.MML Bit is not set, do pmp priv check
> * This will always apply to regular PMP.
> */
> *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> if ((mode != PRV_M) || pmp_is_locked(env, i)) {
> *allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> }
> }
>
> Setting allowed_privs requires !MSECCFG_RLB_ISSET(env), and after this
> patch allowed_privs
> will be set regardless of MSECCFG_RLB_ISSET(env), at least w.r.t how
> pmp_is_locked() works.
>
> This might not be an issue and there's not behavioral change. In this case
> it would be good
> to mention in the commit msg why this is the case.
>
> We can just add a !MSECCFG_RLB_ISSET(env) at this point to preserve
> behavior too.
>
> Not checking RLB in this code path is the main point of this commit:
allowed_privs should not depend on RLB.
I'll reword the commit message in v2 to make it more explicit.
> >
> > Signed-off-by: Loïc Lefort <[email protected]>
> > ---
> > target/riscv/pmp.c | 43 ++++++++++++++++++++++++-------------------
> > 1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index 85ab270dad..ddb7e0d23c 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -45,11 +45,6 @@ static inline uint8_t pmp_get_a_field(uint8_t cfg)
> > */
> > static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> > {
> > - /* mseccfg.RLB is set */
> > - if (MSECCFG_RLB_ISSET(env)) {
> > - return 0;
> > - }
> > -
> > if (env->pmp_state.pmp[pmp_index].cfg_reg & PMP_LOCK) {
> > return 1;
> > }
> > @@ -62,6 +57,15 @@ static inline int pmp_is_locked(CPURISCVState *env,
> uint32_t pmp_index)
> > return 0;
> > }
> >
> > +/*
> > + * Check whether a PMP is locked for writing or not.
> > + * (i.e. has LOCK flag and mseccfg.RLB is unset)
> > + */
> > +static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
> > +{
> > + return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
> > +}
> > +
> > /*
> > * Count the number of active rules.
> > */
> > @@ -90,39 +94,40 @@ static inline uint8_t pmp_read_cfg(CPURISCVState
> *env, uint32_t pmp_index)
> > static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index,
> uint8_t val)
> > {
> > if (pmp_index < MAX_RISCV_PMPS) {
> > - bool locked = true;
> > + bool readonly = true;
> >
> > if (riscv_cpu_cfg(env)->ext_smepmp) {
> > /* mseccfg.RLB is set */
> > if (MSECCFG_RLB_ISSET(env)) {
> > - locked = false;
> > + readonly = false;
> > }
> >
> > /* mseccfg.MML is not set */
> > - if (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env,
> pmp_index)) {
> > - locked = false;
> > + if (!MSECCFG_MML_ISSET(env) && !pmp_is_readonly(env,
> pmp_index)) {
> > + readonly = false;
> > }
> >
> > /* mseccfg.MML is set */
> > if (MSECCFG_MML_ISSET(env)) {
> > /* not adding execute bit */
> > if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) !=
> PMP_EXEC) {
> > - locked = false;
> > + readonly = false;
> > }
> > /* shared region and not adding X bit */
> > if ((val & PMP_LOCK) != PMP_LOCK &&
> > (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> > - locked = false;
> > + readonly = false;
> > }
> > }
> > } else {
> > - if (!pmp_is_locked(env, pmp_index)) {
> > - locked = false;
> > + if (!pmp_is_readonly(env, pmp_index)) {
> > + readonly = false;
>
> Here we can do:
>
> readonly = pmp_is_readonly(env, pmp_index);
>
> And spare an extra if.
>
> Sure, I will change this in v2.
>
> Thanks,
>
> Daniel
>
> > }
> > }
> >
> > - if (locked) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write -
> locked\n");
> > + if (readonly) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "ignoring pmpcfg write - read only\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) &&
> > @@ -524,14 +529,14 @@ void pmpaddr_csr_write(CPURISCVState *env,
> uint32_t addr_index,
> > uint8_t pmp_cfg = env->pmp_state.pmp[addr_index +
> 1].cfg_reg;
> > is_next_cfg_tor = PMP_AMATCH_TOR ==
> pmp_get_a_field(pmp_cfg);
> >
> > - if (pmp_is_locked(env, addr_index + 1) && is_next_cfg_tor) {
> > + if (pmp_is_readonly(env, addr_index + 1) &&
> is_next_cfg_tor) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > - "ignoring pmpaddr write - pmpcfg + 1
> locked\n");
> > + "ignoring pmpaddr write - pmpcfg+1 read
> only\n");
> > return;
> > }
> > }
> >
> > - if (!pmp_is_locked(env, addr_index)) {
> > + if (!pmp_is_readonly(env, addr_index)) {
> > if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> > env->pmp_state.pmp[addr_index].addr_reg = val;
> > pmp_update_rule_addr(env, addr_index);
> > @@ -542,7 +547,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t
> addr_index,
> > }
> > } else {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > - "ignoring pmpaddr write - locked\n");
> > + "ignoring pmpaddr write - read only\n");
> > }
> > } else {
> > qemu_log_mask(LOG_GUEST_ERROR,
>
>
