I sent a v3 a few minutes ago, before seeing this. It splits the change in
two:
1/2 this fix, with a reworded commit message. Functionally the
same, though I kept the variable as pmp_num instead of
renaming it, so let me know whether you'd rather I carry your
Reviewed-by across or just take v1 from this thread.
2/2 clears pmp_state.addr[] and num_rules. The spec doesn't cover
those, but they stay stale across reset and the difference is
observable from a guest. There's a measurement in the v3
cover letter.
v3:
https://lore.kernel.org/qemu-devel/[email protected]/
Sebastián
El mié, 21 oct 2026 a las 21:40, Alistair Francis (<[email protected]>)
escribió:
> 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~
>
--
Sebastián Alba
