On Wed, Mar 2, 2022 at 11:50 PM Weiwei Li <liwei...@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 <liwei...@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqi...@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
> + */
> + bool write_csr = write_mask || ((write_mask == 0) &&
> + ((new_value == 0) ||
> + (new_value == (target_ulong)-1)));
> +
> + RISCVException ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
> if (ret != RISCV_EXCP_NONE) {
> return ret;
> }
>
> - return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask);
> + return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask,
> + write_csr);
> }
>
> static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno,
> Int128 *ret_value,
> Int128 new_value,
> - Int128 write_mask)
> + Int128 write_mask, bool write_csr)
> {
> RISCVException ret;
> Int128 old_value;
> @@ -2982,8 +2992,8 @@ static RISCVException riscv_csrrw_do128(CPURISCVState
> *env, int csrno,
> return ret;
> }
>
> - /* write value if writable and write mask set, otherwise drop writes */
> - if (int128_nz(write_mask)) {
> + /* write value if needed, otherwise drop writes */
> + if (write_csr) {
> new_value = int128_or(int128_and(old_value, int128_not(write_mask)),
> int128_and(new_value, write_mask));
> if (csr_ops[csrno].write128) {
> @@ -3015,13 +3025,22 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env,
> int csrno,
> RISCVException ret;
> RISCVCPU *cpu = env_archcpu(env);
>
> - ret = riscv_csrrw_check(env, csrno, int128_nz(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)
> + */
> + bool write_csr = write_mask || ((write_mask == 0) &&
> + ((new_value == 0) ||
> + (new_value == UINT128_MAX)));
> +
> + ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
> if (ret != RISCV_EXCP_NONE) {
> return ret;
> }
>
> if (csr_ops[csrno].read128) {
> - return riscv_csrrw_do128(env, csrno, ret_value, new_value,
> write_mask);
> + return riscv_csrrw_do128(env, csrno, ret_value, new_value,
> write_mask,
> + write_csr);
> }
>
> /*
> @@ -3033,7 +3052,8 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int
> csrno,
> target_ulong old_value;
> ret = riscv_csrrw_do64(env, csrno, &old_value,
> int128_getlo(new_value),
> - int128_getlo(write_mask));
> + int128_getlo(write_mask),
> + write_csr);
> if (ret == RISCV_EXCP_NONE && ret_value) {
> *ret_value = int128_make64(old_value);
> }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 1a75ba11e6..b2ad465533 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -40,7 +40,12 @@ void helper_raise_exception(CPURISCVState *env, uint32_t
> exception)
> 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());
> --
> 2.17.1
>
>
