| Message ID | 20211101100143.44356-14-zhiwei_liu@c-sky.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support UXL filed in xstatus. | expand |
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~
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
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(-)