| Message ID | 658e49284fc564d4114307cb559cf23ea3e751fd.1595234208.git.zong.li@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some PMP implementation | expand |
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
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);
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(+)