On Wed, 2026-09-16 at 00:10 -1000, Richard Henderson wrote:
> On 9/15/26 17:43, Sebastian Alba Vives wrote:
> > pmp_unlock_entries() bounds its loop with pmp_get_num_rules(),
> > which is
> > the count of entries whose A field is not OFF, not the number of
> > implemented PMP entries. When the active rules are not contiguous
> > from
> > index 0, every entry at an index greater than or equal to num_rules
> > keeps its lock bit across a CPU reset.
> >
> > Those entries stay locked after the reset has cleared mseccfg, so
> > mseccfg.RLB is no longer set and pmp_is_readonly() silently
> > discards
> > subsequent pmpcfg writes. Firmware started after the reset finds
> > PMP
> > entries it did not configure and cannot clear.
> >
> > Reproduced on -machine virt -cpu rv64,smepmp=true with a bare-metal
> > payload that sets pmpcfg0 = 0x99990000 (entries 2 and 3 as
> > L | NAPOT | R, entries 0 and 1 left OFF) and then resets through
> > the
> > sifive_test device. On the second boot pmpcfg0 reads back
> > 0x99990000
> > and writing 0 to it leaves the value unchanged. With this patch the
> > same payload reads 0x01010000 on the second boot and clearing
> > pmpcfg0
> > succeeds.
> >
> > Iterate over the implemented entries instead.
> >
> > Fixes: 4bf501dc0118 ("target/riscv: pmp: Clear pmp/smepmp bits on
> > reset")
> > Signed-off-by: Sebastian Alba Vives <[email protected]>
> > ---
> > target/riscv/tcg/pmp.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/tcg/pmp.c b/target/riscv/tcg/pmp.c
> > index 41b55519a8..5286c6aa90 100644
> > --- a/target/riscv/tcg/pmp.c
> > +++ b/target/riscv/tcg/pmp.c
> > @@ -201,10 +201,10 @@ static bool 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);
> > + uint8_t pmp_regions = riscv_cpu_cfg(env)->pmp_regions;
> > int i;
> >
> > - for (i = 0; i < pmp_num; i++) {
> > + for (i = 0; i < pmp_regions; i++) {
> > env->pmp_state.pmp[i].cfg_reg &= ~(PMP_LOCK |
> > PMP_AMATCH);
> > }
> > }
>
> Why is the entire env->pmp_state not zeroed instead?
The RISC-V spec specifically states that on reset:
"Writable PMP registers’ A and L fields are set to 0...", so this does
seem to be the correct approach
Reviewed-by: Alistair Francis <[email protected]>
Alistair
>
>
> r~
