| Message ID | 56c2770fc75e1e0c833248ea5d9faa038b69e328.1595581140.git.zong.li@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some PMP implementations | expand |
Hi Zong, On Fri, Jul 24, 2020 at 5:08 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 | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 2a2b9f5363..e0161d6aab 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -310,6 +310,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, > int i; > uint8_t cfg_val; > > +#if defined(TARGET_RISCV64) > + reg_index >>= 1; > +#endif > + > trace_pmpcfg_csr_write(env->mhartid, reg_index, val); > > if ((reg_index & 1) && (sizeof(target_ulong) == 8)) { > @@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index) > target_ulong cfg_val = 0; > target_ulong val = 0; > > +#if defined(TARGET_RISCV64) > + reg_index >>= 1; > +#endif > + > for (i = 0; i < sizeof(target_ulong); i++) { > val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i); > cfg_val |= (val << (i * 8)); > -- It seems you missed to address my review comments in v3? reg_index should be shifted after we call the trace function. Regards, Bin
On Fri, Jul 24, 2020 at 5:22 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Zong, > > On Fri, Jul 24, 2020 at 5:08 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 | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > > index 2a2b9f5363..e0161d6aab 100644 > > --- a/target/riscv/pmp.c > > +++ b/target/riscv/pmp.c > > @@ -310,6 +310,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, > > int i; > > uint8_t cfg_val; > > > > +#if defined(TARGET_RISCV64) > > + reg_index >>= 1; > > +#endif > > + > > trace_pmpcfg_csr_write(env->mhartid, reg_index, val); > > > > if ((reg_index & 1) && (sizeof(target_ulong) == 8)) { > > @@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index) > > target_ulong cfg_val = 0; > > target_ulong val = 0; > > > > +#if defined(TARGET_RISCV64) > > + reg_index >>= 1; > > +#endif > > + > > for (i = 0; i < sizeof(target_ulong); i++) { > > val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i); > > cfg_val |= (val << (i * 8)); > > -- > > It seems you missed to address my review comments in v3? reg_index > should be shifted after we call the trace function. > Sorry for that, there was something wrong in my local tree, I have been posting the 5th version patches, and hope it picks the suggestion already. Thanks. > Regards, > Bin
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index 2a2b9f5363..e0161d6aab 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -310,6 +310,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, int i; uint8_t cfg_val; +#if defined(TARGET_RISCV64) + reg_index >>= 1; +#endif + trace_pmpcfg_csr_write(env->mhartid, reg_index, val); if ((reg_index & 1) && (sizeof(target_ulong) == 8)) { @@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index) target_ulong cfg_val = 0; target_ulong val = 0; +#if defined(TARGET_RISCV64) + reg_index >>= 1; +#endif + for (i = 0; i < sizeof(target_ulong); i++) { val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i); cfg_val |= (val << (i * 8));
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 | 8 ++++++++ 1 file changed, 8 insertions(+)