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

[Weiwei Li]

在 2022/3/17 下午12:39, Alistair Francis 写道:

> On Thu, Mar 17, 2022 at 12:10 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> > 在 2022/3/17 上午6:35, Alistair Francis 写道:
> > > On Thu, Mar 17, 2022 at 1:13 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> > > > > >            riscv_raise_exception(env, ret, GETPC());
> > > > > > @@ -90,7 +90,7 @@ void helper_csrw_i128(CPURISCVState *env, int csr,
> > > > > >    {
> > > > > >        RISCVException ret = riscv_csrrw_i128(env, csr, NULL,
> > > > > >                                              int128_make128(srcl, srch),
> > > > > > -                                          UINT128_MAX);
> > > > > > +                                          UINT128_MAX, true);
> > > > > >
> > > > > >        if (ret != RISCV_EXCP_NONE) {
> > > > > >            riscv_raise_exception(env, ret, GETPC());
> > > > > > @@ -104,7 +104,7 @@ target_ulong helper_csrrw_i128(CPURISCVState *env, int csr,
> > > > > >        Int128 rv = int128_zero();
> > > > > >        RISCVException ret = riscv_csrrw_i128(env, csr, &rv,
> > > > > >                                              int128_make128(srcl, srch),
> > > > > > -                                          int128_make128(maskl, maskh));
> > > > > > +                                          int128_make128(maskl, maskh), true);
> > > > > >
> > > > > >        if (ret != RISCV_EXCP_NONE) {
> > > > > >            riscv_raise_exception(env, ret, GETPC());
> > > > > >               
> > > > > I am afraid the adding of "bool write_op" argument was done on many
> > > > > functions, some of which do not look good to me, e.g.: predicate
> > > > > funcs. v1 is much better.*>
> > > > >             
> > > > Yeah, I agree adding  argument to predicate is not a good idea. However
> > > > it seems that the csr(like seed)
> > > >
> > > > itself cannot  distinguish whether it's to be written or not except
> > > > these two ways(in v1 and v2).
> > > >
> > > > Or we can take seed CSR as a special case  in riscv_csrrw_check since no
> > > > other csr will limit itself
> > > >
> > > > to be accessed only with read-write instruction currently.
> > > >
> > > > > Or maybe, is that possible we do something in trans_csrrs() instead?
> > > > >             
> > > >           
> > > That might be a better option. Unless there are other CSRs we expect
> > > to do this I think trans_csrr*() is probably the place to have this,
> > > similar to the `rd == 0` case.
> > >
> > > It could also go in helper_csrr() which would just be a simple if check.
> > >
> > > Alistair
> > >
> > >         
> > Sorry. I don't understand what can be done in trans_csrr*(). As I said
> > in last email: trans_csr*
> >
> > have made all the read operation will go to the helper_csrr. However
> > helper_csrr share the progress
> >
> > of riscv_csrrw*  with helper_csrrw/helper_csrw to implement its function.
> >       
> Yep, so the problem you are trying to solve is the seed CSR access right?
>

To solve the seed CSR access problem is my initial purpose.

The other purpose is to write back the  unmodified value back  as the coments in trans_csrrs/trans_csrrc:

"

* Note that 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.

*/

"

For this purpose,  we still should distinguish the two cases in riscv_csrrw, since whether write_mask

is zero is used to distinguish read and write operation currently.

> "The seed CSR must be accessed with a read-write instruction. A
> read-only instruction such as CSRRS/CSRRC with rs1=x0 or CSRRSI/CSRRCI
> with uimm=0 will raise an illegal instruction exception. The write
> value (in rs1 or uimm) must be ignored by implementations. The purpose
> of the write is to signal polling and flushing."
>
> So trans_csr*() probably isn't the right place, but you could do the
> check in helper_csr*()
>
> helper_csrr() is only called from do_csrr(), which should only be
> called on an invalid access for the seed CSR.
>
> > The truely question is that helper_csrr will call riscv_csrrw*with
> > write_mask = zero, new_value=zero,
> >
> > which cannot distinguished from the call from helper_csrrw of which its
> > write_mask all can be zero
> >       
> Agreed. But helper_csrrw() isn't called if rs1=x0 or uimm=0. So if
> helper_csrrw() is called no matter the write mask then you should be
> fine.
>
> So a simple:
> if (csr == CSR_SEED) {
>     riscv_raise_exception()
> }
>
> in helper_csrr() and helper_csrw() should be fine. Or am I missing something?
>

It's OK to do this to solve the seed csr problem. Maybe it's unnecessary to do this in helper_csrw since the spec

seems not specify the case that rd = x0 to raise an illegal instruction exception.

Regards,

Weiwei Li

> Alistair
>
> > (origin from trans_csrrs/trans_csrrc when rs1 is not x0 and the value of
> > rs1 reg is zero).
> >
> > Regards,
> >
> > Weiwei Li
> >
> >
> > > > The read and write operation in trans_csr*  have been truely
> > > > distinguished in original code:
> > > >
> > > > all the read operation will go to  the helper_csrr,  write to
> > > > helper_csrrw, read/write to helper_csrrw.
> > > >
> > > > However, they all go to the same progress  riscv_csrrw* in the helper
> > > > with different arguments.
> > > >
> > > > Regards,
> > > >
> > > > Weiwei Li
> > > >
> > > > > Regards,
> > > > > Bin
> > > > >             
> > > >
> > > >
> > > >
> > > >           
> > >         
> >       
>