Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/riscv/csr.c                      | 6 ++++--
 target/riscv/insn_trans/trans_rvi.c.inc | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9f41954894..471c10acf6 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -543,14 +543,16 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
          * add them to mstatush. For now, we just don't support it.
          */
         mask |= MSTATUS_MPV | MSTATUS_GVA;
+        if ((val ^ mstatus) & MSTATUS64_UXL) {
+            mask |= MSTATUS64_UXL;
+        }
     }
 
     mstatus = (mstatus & ~mask) | (val & mask);
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
-        /* SXL and UXL fields are for now read only */
+        /* SXL fields are for now read only */
         mstatus = set_field(mstatus, MSTATUS64_SXL, MXL_RV64);
-        mstatus = set_field(mstatus, MSTATUS64_UXL, MXL_RV64);
     }
     env->mstatus = mstatus;
 
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index bd9d50bb94..880026f13d 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -475,7 +475,7 @@ static bool do_csrrw(DisasContext *ctx, int rd, int rc, TCGv src, TCGv mask)
 
 static bool trans_csrrw(DisasContext *ctx, arg_csrrw *a)
 {
-    TCGv src = get_gpr(ctx, a->rs1, EXT_NONE);
+    TCGv src = get_gpr(ctx, a->rs1, EXT_ZERO);
 
     /*
      * If rd == 0, the insn shall not read the csr, nor cause any of the
-- 
2.25.1

On 11/1/21 6:01 AM, LIU Zhiwei wrote:
>           mask |= MSTATUS_MPV | MSTATUS_GVA;
> +        if ((val ^ mstatus) & MSTATUS64_UXL) {
> +            mask |= MSTATUS64_UXL;
> +        }

Why do you need the conditional here?
Why is this not just

     mask |= MSTATUS_MPV | MSTATUS_GVA | MSTATUS64_UXL;


>  static bool trans_csrrw(DisasContext *ctx, arg_csrrw *a)
>  {
> -    TCGv src = get_gpr(ctx, a->rs1, EXT_NONE);
> +    TCGv src = get_gpr(ctx, a->rs1, EXT_ZERO);

Hmm.  Not sure about this.

It looks like we should in fact change mask, just a few lines down, at which point the 
extension (or not) for the source would not matter.  And likewise in trans_csrrwi.


r~

On 2021/11/2 上午1:01, Richard Henderson wrote:
> On 11/1/21 6:01 AM, LIU Zhiwei wrote:
>>           mask |= MSTATUS_MPV | MSTATUS_GVA;
>> +        if ((val ^ mstatus) & MSTATUS64_UXL) {
>> +            mask |= MSTATUS64_UXL;
>> +        }
>
> Why do you need the conditional here?
> Why is this not just
>
>     mask |= MSTATUS_MPV | MSTATUS_GVA | MSTATUS64_UXL;
OK
>
>
>>  static bool trans_csrrw(DisasContext *ctx, arg_csrrw *a)
>>  {
>> -    TCGv src = get_gpr(ctx, a->rs1, EXT_NONE);
>> +    TCGv src = get_gpr(ctx, a->rs1, EXT_ZERO);
>
> Hmm.  Not sure about this.
>
> It looks like we should in fact change mask, just a few lines down, at 
> which point the extension (or not) for the source would not matter.  
> And likewise in trans_csrrwi.

It's better to use the mask.

Thanks,
Zhiwei

>
>
> r~

On 2021/11/2 上午1:01, Richard Henderson wrote:
> On 11/1/21 6:01 AM, LIU Zhiwei wrote:
>>           mask |= MSTATUS_MPV | MSTATUS_GVA;
>> +        if ((val ^ mstatus) & MSTATUS64_UXL) {
>> +            mask |= MSTATUS64_UXL;
>> +        }
>
> Why do you need the conditional here?
> Why is this not just
>
>     mask |= MSTATUS_MPV | MSTATUS_GVA | MSTATUS64_UXL;
>
>
>>  static bool trans_csrrw(DisasContext *ctx, arg_csrrw *a)
>>  {
>> -    TCGv src = get_gpr(ctx, a->rs1, EXT_NONE);
>> +    TCGv src = get_gpr(ctx, a->rs1, EXT_ZERO);
>
> Hmm.  Not sure about this.
>
> It looks like we should in fact change mask, just a few lines down, at 
> which point the extension (or not) for the source would not matter.  
> And likewise in trans_csrrwi.

Is there some benefits to use mask?
I see there is still a do_csrw, and we can't give it a mask at 
translation time.

Thanks,
Zhiwei

>
>
> r~