On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
implementation, the second parameter of pmp_write_cfg is
"reg_index * sizeof(target_ulong)", and we get the the result
which is started from 16 if reg_index is 2, but we expect that
it should be started from 8. Separate the implementation for
RV32 and RV64 respectively.

Signed-off-by: Zong Li <zong.li@sifive.com>
---
 target/riscv/pmp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 2a2b9f5363..adcdd411e6 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -320,8 +320,13 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
 
     for (i = 0; i < sizeof(target_ulong); i++) {
         cfg_val = (val >> 8 * i)  & 0xff;
+#if defined(TARGET_RISCV32)
         pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
             cfg_val);
+#elif defined(TARGET_RISCV64)
+        pmp_write_cfg(env, ((reg_index >> 1) * sizeof(target_ulong)) + i,
+            cfg_val);
+#endif
     }
 }
 
@@ -336,7 +341,11 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
     target_ulong val = 0;
 
     for (i = 0; i < sizeof(target_ulong); i++) {
+#if defined(TARGET_RISCV32)
         val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
+#elif defined(TARGET_RISCV64)
+        val = pmp_read_cfg(env, ((reg_index >> 1) * sizeof(target_ulong)) + i);
+#endif
         cfg_val |= (val << (i * 8));
     }
     trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);
-- 
2.27.0

Hi Zong,

On Mon, Jul 20, 2020 at 5:46 PM Zong Li <zong.li@sifive.com> wrote:
>
> On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
> entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
> implementation, the second parameter of pmp_write_cfg is
> "reg_index * sizeof(target_ulong)", and we get the the result
> which is started from 16 if reg_index is 2, but we expect that
> it should be started from 8. Separate the implementation for
> RV32 and RV64 respectively.
>
> Signed-off-by: Zong Li <zong.li@sifive.com>
> ---
>  target/riscv/pmp.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 2a2b9f5363..adcdd411e6 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -320,8 +320,13 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>
>      for (i = 0; i < sizeof(target_ulong); i++) {
>          cfg_val = (val >> 8 * i)  & 0xff;
> +#if defined(TARGET_RISCV32)
>          pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
>              cfg_val);
> +#elif defined(TARGET_RISCV64)
> +        pmp_write_cfg(env, ((reg_index >> 1) * sizeof(target_ulong)) + i,
> +            cfg_val);
> +#endif

Can you please simplify this by shifting reg_index outside the for
loop for RV64?

>      }
>  }
>
> @@ -336,7 +341,11 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>      target_ulong val = 0;
>
>      for (i = 0; i < sizeof(target_ulong); i++) {
> +#if defined(TARGET_RISCV32)
>          val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> +#elif defined(TARGET_RISCV64)
> +        val = pmp_read_cfg(env, ((reg_index >> 1) * sizeof(target_ulong)) + i);
> +#endif
>          cfg_val |= (val << (i * 8));
>      }
>      trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);

Regards,
Bin

On Tue, Jul 21, 2020 at 10:41 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Zong,
>
> On Mon, Jul 20, 2020 at 5:46 PM Zong Li <zong.li@sifive.com> wrote:
> >
> > On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
> > entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
> > implementation, the second parameter of pmp_write_cfg is
> > "reg_index * sizeof(target_ulong)", and we get the the result
> > which is started from 16 if reg_index is 2, but we expect that
> > it should be started from 8. Separate the implementation for
> > RV32 and RV64 respectively.
> >
> > Signed-off-by: Zong Li <zong.li@sifive.com>
> > ---
> >  target/riscv/pmp.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index 2a2b9f5363..adcdd411e6 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -320,8 +320,13 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> >
> >      for (i = 0; i < sizeof(target_ulong); i++) {
> >          cfg_val = (val >> 8 * i)  & 0xff;
> > +#if defined(TARGET_RISCV32)
> >          pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
> >              cfg_val);
> > +#elif defined(TARGET_RISCV64)
> > +        pmp_write_cfg(env, ((reg_index >> 1) * sizeof(target_ulong)) + i,
> > +            cfg_val);
> > +#endif
>
> Can you please simplify this by shifting reg_index outside the for
> loop for RV64?
>

OK, that would be great. Change it in the next version, thanks.

> >      }
> >  }
> >
> > @@ -336,7 +341,11 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
> >      target_ulong val = 0;
> >
> >      for (i = 0; i < sizeof(target_ulong); i++) {
> > +#if defined(TARGET_RISCV32)
> >          val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> > +#elif defined(TARGET_RISCV64)
> > +        val = pmp_read_cfg(env, ((reg_index >> 1) * sizeof(target_ulong)) + i);
> > +#endif
> >          cfg_val |= (val << (i * 8));
> >      }
> >      trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);
>
> Regards,
> Bin