Fair enough on the A and L fields, I'll follow the spec there.
The reason v2 went further... v1 leaves pmp_state.addr[] and
num_rules stale. Those are QEMU's own derived state, recomputed
only on the pmpaddr/pmpcfg write paths and never at reset.
pmp_is_in_range() compares against addr[].sa and .ea without
looking at the A field, and the partially-inside check in
pmp_hart_has_privs() runs before A is tested. So a range left from
before the reset can still deny a straddling access with A=OFF.
That part isn't a spec question, it's bookkeeping the spec doesn't
know about.
I'll respin as two patches:
1/2 bound the loop by the implemented entry count instead of
pmp_get_num_rules(), clearing A and L. This is v1.
2/2 reset addr[] and num_rules alongside it.
Happy for you to take just 1/2 and leave 2/2 to stand on its own.
Sebastián
El mié, 21 oct 2026 a las 21:39, Alistair Francis (<[email protected]>)
escribió:
> On Sat, 2026-10-10 at 22:06 -0600, Sebastian Alba Vives wrote:
> > pmp_unlock_entries() bounded its loop with pmp_get_num_rules(), which
> > returns the number of entries whose A field is not OFF rather than
> > the
> > number of implemented entries. With a sparse configuration the loop
> > stops early: with entries 0 and 1 OFF and entries 2 and 3 locked,
> > num_rules is 2, the loop covers i = 0 and i = 1, and entries 2 and 3
> > keep their lock and address-match bits across reset.
> >
> > riscv_cpu_reset_hold() clears mseccfg, and with it RLB, before
> > calling
> > this, so pmp_is_readonly() then silently discards every subsequent
> > pmpcfg write to those entries. The stale rules survive for the
> > lifetime of the machine.
> >
> > Clearing only PMP_LOCK and PMP_AMATCH is not enough either. The priv
> > spec only mandates that the A and L fields be reset, but QEMU keeps
> > derived state next to the architectural registers: pmp_state.addr[]
> > and pmp_state.num_rules are recomputed from the pmpaddr and pmpcfg
> > write paths only, never at reset. pmp_is_in_range() compares against
> > addr[].sa and addr[].ea without consulting the A field, and the
> > partially-inside check in pmp_hart_has_privs() runs before the A
> > field is tested, so a range left over from before the reset can still
> > deny an access that straddles its boundary once the guest configures
> > any entry again.
> >
> > Zero the whole structure instead. Zero is the correct reset value for
> > every field: a cfg_reg of 0 is A=OFF with no permissions, and a
> > num_rules of 0 restores the no-rules short cut. Rename the function
> > since it no longer only unlocks entries.
> >
> > Fixes: 4bf501dc0118 ("target/riscv: pmp: Clear pmp/smepmp bits on
> > reset")
> > Suggested-by: Richard Henderson <[email protected]>
> > Signed-off-by: Sebastian Alba Vives <[email protected]>
> > ---
> > Changes in v2:
> > - Zero the whole env->pmp_state instead of masking PMP_LOCK and
> > PMP_AMATCH over a bounded set of entries, as suggested by Richard.
> > v1 also left pmp_state.addr[] and pmp_state.num_rules stale across
> > reset, which the memset covers as well.
>
> I think your v1 is actually better.
>
> Richard asked why you don't just zero `pmp_state`, but the answer is
> because the RISC-V spec states that on reset "Writable PMP
> registers’ A and L fields are set to 0". So we should follow that
>
> Alistair
>
> >
> > v1:
> >
> https://lore.kernel.org/qemu-devel/[email protected]/
> >
> > target/riscv/cpu.c | 4 ++--
> > target/riscv/tcg/pmp.c | 9 ++-------
> > target/riscv/tcg/pmp.h | 2 +-
> > 3 files changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index da4c1a090b..0b71196507 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1048,7 +1048,7 @@ static void riscv_cpu_reset_hold(Object *obj,
> > ResetType type)
> > }
> >
> > /*
> > - * Clear mseccfg and unlock all the PMP entries upon reset.
> > + * Clear mseccfg and the PMP state upon reset.
> > * This is required as per the priv, smepmp, and other security
> > * extension specifications that share this CSR, and is needed
> > * to clear stale entries across reboots.
> > @@ -1060,7 +1060,7 @@ static void riscv_cpu_reset_hold(Object *obj,
> > ResetType type)
> > env->mseccfg = 0;
> > }
> >
> > - pmp_unlock_entries(env);
> > + pmp_reset(env);
> > #endif /* ifdef CONFIG_TCG */
> > #else
> > env->priv = PRV_U;
> > diff --git a/target/riscv/tcg/pmp.c b/target/riscv/tcg/pmp.c
> > index 41b55519a8..437e0ff2af 100644
> > --- a/target/riscv/tcg/pmp.c
> > +++ b/target/riscv/tcg/pmp.c
> > @@ -199,14 +199,9 @@ static bool pmp_write_cfg(CPURISCVState *env,
> > uint32_t pmp_index, uint8_t val)
> > return false;
> > }
> >
> > -void pmp_unlock_entries(CPURISCVState *env)
> > +void pmp_reset(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);
> > - }
> > + memset(&env->pmp_state, 0, sizeof(env->pmp_state));
> > }
> >
> > static void pmp_decode_napot(hwaddr a, hwaddr *sa, hwaddr *ea)
> > diff --git a/target/riscv/tcg/pmp.h b/target/riscv/tcg/pmp.h
> > index 4c95c2767a..99546934fc 100644
> > --- a/target/riscv/tcg/pmp.h
> > +++ b/target/riscv/tcg/pmp.h
> > @@ -79,7 +79,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);
> > +void pmp_reset(CPURISCVState *env);
> >
> > #define MSECCFG_MML_ISSET(env) get_field(env->mseccfg, MSECCFG_MML)
> > #define MSECCFG_MMWP_ISSET(env) get_field(env->mseccfg,
> > MSECCFG_MMWP)
>
--
Sebastián Alba
