Re: [PATCH] target/riscv: write back unmodified value for csrrc/csrrs with rs1 is not x0 but holding zero

[Weiwei Li]

在 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);

     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