| Message ID | 80450819119ba8493f18c825517f1b4f37eeb600.1595335112.git.zong.li@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some PMP implementation | expand |
Hi Zong, On Tue, Jul 21, 2020 at 8:41 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> > > Changed in v3: > - Refine the implementation. Suggested by Bin Meng. > > Changed in v2: > - Move out the shifting operation from loop. Suggested by Bin Meng. As I mentioned previously, these changelog should go after --- below. It should not appear in the commit message. > --- > target/riscv/pmp.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 2a2b9f5363..f2d50bace5 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, > return; > } > > +#if defined(TARGET_RISCV64) > + reg_index >>= 1; > +#endif > + > for (i = 0; i < sizeof(target_ulong); i++) { > cfg_val = (val >> 8 * i) & 0xff; > pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i, > @@ -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 We should also move the following: trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val); before shifting reg_index. Otherwise it traces the wrong pmpcfg CSR read. > + > for (i = 0; i < sizeof(target_ulong); i++) { > val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i); > cfg_val |= (val << (i * 8)); > -- Regards, Bin
On Wed, Jul 22, 2020 at 12:58 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Zong, > > On Tue, Jul 21, 2020 at 8:41 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> > > > > Changed in v3: > > - Refine the implementation. Suggested by Bin Meng. > > > > Changed in v2: > > - Move out the shifting operation from loop. Suggested by Bin Meng. > > As I mentioned previously, these changelog should go after --- below. > It should not appear in the commit message. > OK, remove it in the next version. > > --- > > target/riscv/pmp.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > > index 2a2b9f5363..f2d50bace5 100644 > > --- a/target/riscv/pmp.c > > +++ b/target/riscv/pmp.c > > @@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, > > return; > > } > > > > +#if defined(TARGET_RISCV64) > > + reg_index >>= 1; > > +#endif > > + > > for (i = 0; i < sizeof(target_ulong); i++) { > > cfg_val = (val >> 8 * i) & 0xff; > > pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i, > > @@ -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 > > We should also move the following: > > trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val); > > before shifting reg_index. Otherwise it traces the wrong pmpcfg CSR read. Yes, thanks for the reminding, Fix it in the next version. > > > + > > for (i = 0; i < sizeof(target_ulong); i++) { > > val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i); > > cfg_val |= (val << (i * 8)); > > -- > > Regards, > Bin
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index 2a2b9f5363..f2d50bace5 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, return; } +#if defined(TARGET_RISCV64) + reg_index >>= 1; +#endif + for (i = 0; i < sizeof(target_ulong); i++) { cfg_val = (val >> 8 * i) & 0xff; pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i, @@ -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> Changed in v3: - Refine the implementation. Suggested by Bin Meng. Changed in v2: - Move out the shifting operation from loop. Suggested by Bin Meng. --- target/riscv/pmp.c | 8 ++++++++ 1 file changed, 8 insertions(+)