Re: [PATCH v1 2/2] target/riscv: Make PMP CSRs conform to WARL constraints

[Jay Chang <jay.chang@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 6027 bytes --]

I’ll change UL to ULL.

As for the second point, according to the spec:

“Although changing pmpcfgA[1] affects the value read from pmpaddr, it does
not affect the underlying value stored in that register.”

If we modify the value at write time instead of masking on read, it could
cause issues during software context switches.
For example, if pmpaddr is programmed in NAPOT mode and software switches
to TOR mode for context save/restore (i.e., read pmpaddr → write pmpaddr in
TOR mode → switch back to NAPOT), the NAPOT granularity bits pmpaddr[G-2:0]
would be lost because pmpaddr[G-1:0] reads as all zeros in TOR/OFF mode.
This behavior would be incorrect.

On Fri, Oct 17, 2025 at 9:02 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Tue, Oct 14, 2025 at 6:25 PM Jay Chang <jay.chang@sifive.com> wrote:
> >
> > This patch ensure pmpcfg and pmpaddr comply with WARL constraints.
> >
> > When the PMP granularity is greater than 4 bytes, NA4 mode is not valid
> > per the spec and will be silently ignored.
> >
> > According to the spec, changing pmpcfg.A only affects the "read" value
> > of pmpaddr. When G > 2 and pmpcfg.A is NAPOT, bits pmpaddr[G-2:0] read
> > as all ones. When G > 1 and pmpcfg.A is OFF or TOR, bits pmpaddr[G-1:0]
> > read as all zeros. This allows software to read back the correct
> > granularity value.
> >
> > In addition, when updating the PMP address rule in TOR mode,
> > the start and end addresses of the PMP region should be aligned
> > to the PMP granularity. (The current SPEC only state in TOR mode
> > that bits pmpaddr[G-1:0] do not affect the TOR address-matching logic.)
> >
> > Signed-off-by: Jay Chang <jay.chang@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > Reviewed-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  target/riscv/pmp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index 72f1372a49..a15265c8d2 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -108,6 +108,17 @@ static int pmp_is_invalid_smepmp_cfg(CPURISCVState
> *env, uint8_t val)
> >          g_assert_not_reached();
> >      }
> >  }
> > +/*
> > + * Calculate PMP granularity value 'g'
> > + *
> > + * The granularity value 'g' is defined as log2(granularity) - 2, where
> > + * granularity is the minimum alignment requirement for PMP regions in
> bytes.
> > + */
> > +static inline int pmp_get_granularity_g(CPURISCVState *env)
> > +{
> > +    return __builtin_ctz(riscv_cpu_cfg(env)->pmp_granularity >> 2);
> > +}
> > +
> >
> >  /*
> >   * Count the number of active rules.
> > @@ -153,6 +164,15 @@ static bool pmp_write_cfg(CPURISCVState *env,
> uint32_t pmp_index, uint8_t val)
> >              qemu_log_mask(LOG_GUEST_ERROR,
> >                            "ignoring pmpcfg write - invalid\n");
> >          } else {
> > +            uint8_t a_field = pmp_get_a_field(val);
> > +            /*
> > +             * When granularity g >= 1 (i.e., granularity > 4 bytes),
> > +             * the NA4 (Naturally Aligned 4-byte) mode is not selectable
> > +             */
> > +            if ((riscv_cpu_cfg(env)->pmp_granularity >
> > +                MIN_RISCV_PMP_GRANULARITY) && (a_field ==
> PMP_AMATCH_NA4)) {
> > +                    return false;
> > +            }
> >              env->pmp_state.pmp[pmp_index].cfg_reg = val;
> >              pmp_update_rule_addr(env, pmp_index);
> >              return true;
> > @@ -199,6 +219,7 @@ void pmp_update_rule_addr(CPURISCVState *env,
> uint32_t pmp_index)
> >      target_ulong prev_addr = 0u;
> >      hwaddr sa = 0u;
> >      hwaddr ea = 0u;
> > +    int g = pmp_get_granularity_g(env);
> >
> >      if (pmp_index >= 1u) {
> >          prev_addr = env->pmp_state.pmp[pmp_index - 1].addr_reg;
> > @@ -211,6 +232,11 @@ void pmp_update_rule_addr(CPURISCVState *env,
> uint32_t pmp_index)
> >          break;
> >
> >      case PMP_AMATCH_TOR:
> > +        /* Bits pmpaddr[G-1:0] do not affect the TOR address-matching
> logic. */
> > +        if (g >= 1) {
> > +            prev_addr &= ~((1UL << g) - 1UL);
> > +            this_addr &= ~((1UL << g) - 1UL);
> > +        }
> >          if (prev_addr >= this_addr) {
> >              sa = ea = 0u;
> >              break;
> > @@ -577,6 +603,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t
> addr_index,
> >
> >  /*
> >   * Handle a read from a pmpaddr CSR
> > + * Change A field of pmpcfg affects the read value of pmpaddr
> >   */
> >  target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index)
> >  {
> > @@ -585,6 +612,25 @@ target_ulong pmpaddr_csr_read(CPURISCVState *env,
> uint32_t addr_index)
> >
> >      if (addr_index < pmp_regions) {
> >          val = env->pmp_state.pmp[addr_index].addr_reg;
> > +        int g = pmp_get_granularity_g(env);
> > +        switch
> (pmp_get_a_field(env->pmp_state.pmp[addr_index].cfg_reg)) {
> > +        case PMP_AMATCH_OFF:
> > +            /* fallthrough */
> > +        case PMP_AMATCH_TOR:
> > +            /* Bit [g-1:0] read all zero */
> > +            if (g >= 1 && g < TARGET_LONG_BITS) {
> > +                val &= ~((1UL << g) - 1UL);
> > +            }
> > +            break;
> > +        case PMP_AMATCH_NAPOT:
> > +            /* Bit [g-2:0] read all one */
> > +            if (g >= 2 && g < TARGET_LONG_BITS) {
> > +                val |= ((1UL << (g - 1)) - 1UL);
> > +            }
>
> ULL instead of UL?
>
> Also should we just ensure a valid value is written? Instead of
> writing something invalid and then masking the read
>
> Alistair
>
> > +            break;
> > +        default:
> > +            break;
> > +        }
> >          trace_pmpaddr_csr_read(env->mhartid, addr_index, val);
> >      } else {
> >          qemu_log_mask(LOG_GUEST_ERROR,
> > --
> > 2.48.1
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 7820 bytes --]