[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/riscv: write back unmodified value for csrrc/csrrs wi
|
From: |
Alistair Francis |
|
Subject: |
Re: [PATCH] target/riscv: write back unmodified value for csrrc/csrrs with rs1 is not x0 but holding zero |
|
Date: |
Fri, 11 Mar 2022 17:54:15 +1000 |
On Fri, Mar 11, 2022 at 2:58 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/3/11 上午10:58, Alistair Francis 写道:
> > On Wed, Mar 2, 2022 at 11:50 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >> For csrrs and csrrc, if rs1 specifies a register other than x0,
> >> holding
> >> a zero value, the instruction will still attempt to write the
> >> unmodified
> >> value back to the csr and will cause side effects
> >>
> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> >> ---
> >> target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------
> >> target/riscv/op_helper.c | 7 +++++-
> >> 2 files changed, 39 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index aea82dff4a..f4774ca07b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState
> >> *env, int csrno,
> >>
> >> static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> >> int csrno,
> >> - bool write_mask,
> >> + bool write_csr,
> >> RISCVCPU *cpu)
> >> {
> >> /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check
> >> fails */
> >> @@ -2895,7 +2895,7 @@ static inline RISCVException
> >> riscv_csrrw_check(CPURISCVState *env,
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >> #endif
> >> - if (write_mask && read_only) {
> >> + if (write_csr && read_only) {
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >>
> >> @@ -2915,7 +2915,8 @@ static inline RISCVException
> >> riscv_csrrw_check(CPURISCVState *env,
> >> static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> >> target_ulong *ret_value,
> >> target_ulong new_value,
> >> - target_ulong write_mask)
> >> + target_ulong write_mask,
> >> + bool write_csr)
> >> {
> >> RISCVException ret;
> >> target_ulong old_value;
> >> @@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState
> >> *env, int csrno,
> >> return ret;
> >> }
> >>
> >> - /* write value if writable and write mask set, otherwise drop writes
> >> */
> >> - if (write_mask) {
> >> + /* write value if needed, otherwise drop writes */
> >> + if (write_csr) {
> >> new_value = (old_value & ~write_mask) | (new_value & write_mask);
> >> if (csr_ops[csrno].write) {
> >> ret = csr_ops[csrno].write(env, csrno, new_value);
> >> @@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int
> >> csrno,
> >> {
> >> RISCVCPU *cpu = env_archcpu(env);
> >>
> >> - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
> >> + /*
> >> + * write value when write_mask is set or rs1 is not x0 but holding
> >> zero
> >> + * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
> > I don't understand this. Won't write_mask also be zero and when reading?
> >
> > Alistair
> >
> Yeah. It's true. To distinguish only-read operation with the special
> write case(write_mask = 0), I also modified the new_value of riscv_csrrw
> from 0 to 1 in helper_csrr :
>
> target_ulong helper_csrr(CPURISCVState *env, int csr)
> {
> target_ulong val = 0;
> - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
> +
> + /*
> + * new_value here should be none-zero or none-all-ones here to
> + * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
> + */
> + RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
This is confusing though and I worry a future change will break this.
I think we should be explicit instead of using special combinations of
masks. What if a write operation occurred that wanted to write 1 with
a mark of 0?
The two options seem to either add a check for the seed CSR in
helper_csrr() to fault if the address matches. That's not the best as
then we have specific code, but this requirement seems pretty specific
as well so it's probably ok.
The other option would be to modify riscv_csrrw() to explicitly pass
in a `bool write_op` that you check against.
Alistair
>
> if (ret != RISCV_EXCP_NONE) {
> riscv_raise_exception(env, ret, GETPC());
>
>
> After modification, the cases for all csr related instructions is as follows:
>
> index instruction helper write_mask
> new_value Read/Write write_csr
>
> 1 csrrw csrrw/csrw all-ones
> src1 (R)W true
>
> 2 csrrs(rs1=0) csrr zero
> 1 R false
>
> 3 csrrs(rs1!=0) csrrw src1
> all-ones RW true
>
> 4 csrrs(rs1=0) csrr zero
> 1 R false
>
> 5 csrrc(rs1!=0) csrrw src1
> zero RW true
>
> 6 csrrc(rs1=0) csrr zero
> 1 R false
>
> 7 csrrwi csrrw/csrw
> all-ones rs1 (R)W true
>
> 8 csrrsi(rs1=0) csrr zero
> 1 R false
>
> 9 csrrsi(rs1!=0) csrrw rs1
> all-ones RW true
>
> 10 csrrci(rs1=0) csrr zero
> 1 R false
>
> 11 csrrci(rs1!=0) csrrw rs1
> zero RW true
>
>
> Only row 3 and 5 can be Write-operation with write_mask = 0 when src1 =
> 0. And it's the special case will be identified by :
>
> ((write_mask == 0) && ((new_value == 0) || (new_value == (target_ulong)-1)));
>
> for other only-read instructions, the write_mask is zero, but the new_value
> is changed to 1 (none-zero and none-all-ones), so they will make write_csr to
> be false.
>
> Regards,
> Weiwei Li
>
> >> + */
> >> + bool write_csr = write_mask || ((write_mask == 0) &&
> >> + ((new_value == 0) ||
> >> + (new_value == (target_ulong)-1)));
> >> +
> >>
> >> --
> >> 2.17.1
> >>
> >>
>