Re: [Qemu-devel] [PATCH] RISC-V: Fix pmpcfg register indexing

[Palmer Dabbelt]

On Fri, 08 Feb 2019 10:57:17 PST (-0800), address@hidden wrote:
> On Wed, Jan 30, 2019 at 2:20 PM Luke Nelson <address@hidden> wrote:
> > pmpcfg_csr_{read,write} do not correctly handle accesses to PMP
> > configurations 8 through 15 (CSR pmpcfg2) on RV64.
> >
> > The current code computes the pmpcfg index using:
> >
> >   (reg_index * sizeof(target_ulong))
> >
> > This is incorrect on RV64.  For example, when reg_index is 2 (i.e.,
> > pmpcfg2), the computed configuration index will be 16-23, which
> > should be 8-15.
> >
> > A correct way is to use (reg_index * 4) instead, which works for
> > both RV32 and RV64.
> >
> > Cc: Xi Wang <address@hidden>
> > Signed-off-by: Luke Nelson <address@hidden>
>
> Good catch!
>
> Reviewed-by: Alistair Francis <address@hidden>

Ya, thanks -- that's a somewhat embarrassing bug, as someone else just fixed 
one on the line below :).  I'll target this for my next PR.

> Alistair
>
> > ---
> >  target/riscv/pmp.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index 15a5366616..a1bee56c86 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -311,9 +311,8 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t 
> > reg_index,
> >      }
> >
> >      for (i = 0; i < sizeof(target_ulong); i++) {
> > -        cfg_val = (val >> 8 * i)  & 0xff;
> > -        pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
> > -            cfg_val);
> > +        cfg_val = (val >> (i * 8)) & 0xff;
> > +        pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> >      }
> >  }
> >
> > @@ -328,7 +327,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t 
> > reg_index)
> >      target_ulong val = 0;
> >
> >      for (i = 0; i < sizeof(target_ulong); i++) {
> > -        val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> > +        val = pmp_read_cfg(env, (reg_index * 4) + i);
> >          cfg_val |= (val << (i * 8));
> >      }
> >
> > --
> > 2.19.1