Message ID | 20230914101751.772576-1-mchitale@ventanamicro.com |
---|---|
State | New |
Headers | show |
Series | target/riscv: pmp: Ignore writes when RW=01 | expand |
On Thu, Sep 14, 2023 at 10:35 PM Mayuresh Chitale <mchitale@ventanamicro.com> wrote: > > As per the Priv spec: "The R, W, and X fields form a collective WARL > field for which the combinations with R=0 and W=1 are reserved." > However currently such writes are not ignored as ought to be. The > combinations with RW=01 are allowed only when the Smepmp extension > is enabled and mseccfg.MML is set. > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > --- > target/riscv/pmp.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index f3eb6e6585..5b430be18c 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -119,6 +119,14 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val) > if (locked) { > qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n"); > } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) { > + /* If !mseccfg.MML then ignore writes with encoding RW=01 */ > + if ((val & PMP_WRITE) && !(val & PMP_READ) && > + (!riscv_cpu_cfg(env)->ext_smepmp || > + !MSECCFG_MML_ISSET(env))) { MSECCFG_MML can only be set if ext_smepmp exists, so I don't think we need both checks here > + val &= ~(PMP_WRITE | PMP_READ); > + val |= env->pmp_state.pmp[pmp_index].cfg_reg & > + (PMP_WRITE | PMP_READ); Why do we restore the previous value? It's a WARL so we should just guarantee a legal value (which would be 0x0 I guess) Alistair > + } > env->pmp_state.pmp[pmp_index].cfg_reg = val; > pmp_update_rule_addr(env, pmp_index); > return true; > -- > 2.34.1 > >
On Mon, Sep 18, 2023 at 7:02 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Thu, Sep 14, 2023 at 10:35 PM Mayuresh Chitale > <mchitale@ventanamicro.com> wrote: > > > > As per the Priv spec: "The R, W, and X fields form a collective WARL > > field for which the combinations with R=0 and W=1 are reserved." > > However currently such writes are not ignored as ought to be. The > > combinations with RW=01 are allowed only when the Smepmp extension > > is enabled and mseccfg.MML is set. > > > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > > --- > > target/riscv/pmp.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > > index f3eb6e6585..5b430be18c 100644 > > --- a/target/riscv/pmp.c > > +++ b/target/riscv/pmp.c > > @@ -119,6 +119,14 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val) > > if (locked) { > > qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n"); > > } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) { > > + /* If !mseccfg.MML then ignore writes with encoding RW=01 */ > > + if ((val & PMP_WRITE) && !(val & PMP_READ) && > > + (!riscv_cpu_cfg(env)->ext_smepmp || > > + !MSECCFG_MML_ISSET(env))) { > > MSECCFG_MML can only be set if ext_smepmp exists, so I don't think we > need both checks here Ok. > > > + val &= ~(PMP_WRITE | PMP_READ); > > + val |= env->pmp_state.pmp[pmp_index].cfg_reg & > > + (PMP_WRITE | PMP_READ); > > Why do we restore the previous value? It's a WARL so we should just > guarantee a legal value (which would be 0x0 I guess) Yes. It can be set to 0x0. > > Alistair > > > + } > > env->pmp_state.pmp[pmp_index].cfg_reg = val; > > pmp_update_rule_addr(env, pmp_index); > > return true; > > -- > > 2.34.1 > > > >
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index f3eb6e6585..5b430be18c 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -119,6 +119,14 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val) if (locked) { qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n"); } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) { + /* If !mseccfg.MML then ignore writes with encoding RW=01 */ + if ((val & PMP_WRITE) && !(val & PMP_READ) && + (!riscv_cpu_cfg(env)->ext_smepmp || + !MSECCFG_MML_ISSET(env))) { + val &= ~(PMP_WRITE | PMP_READ); + val |= env->pmp_state.pmp[pmp_index].cfg_reg & + (PMP_WRITE | PMP_READ); + } env->pmp_state.pmp[pmp_index].cfg_reg = val; pmp_update_rule_addr(env, pmp_index); return true;
As per the Priv spec: "The R, W, and X fields form a collective WARL field for which the combinations with R=0 and W=1 are reserved." However currently such writes are not ignored as ought to be. The combinations with RW=01 are allowed only when the Smepmp extension is enabled and mseccfg.MML is set. Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> --- target/riscv/pmp.c | 8 ++++++++ 1 file changed, 8 insertions(+)