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)
