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 上午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.

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

(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