| Message ID | 20211016171412.3163784-15-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/riscv: Rationalize XLEN and operand length | expand |
On Sun, Oct 17, 2021 at 3:32 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > The position of this read-only field is dependent on the > current cpu width. Rather than having to compute that > difference in many places, compute it only on read. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> This means that the value reported by riscv_cpu_dump_state() and GDB will both be incorrect though? Alistair
On 10/17/21 9:52 PM, Alistair Francis wrote: > On Sun, Oct 17, 2021 at 3:32 AM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> The position of this read-only field is dependent on the >> current cpu width. Rather than having to compute that >> difference in many places, compute it only on read. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > This means that the value reported by riscv_cpu_dump_state() and GDB > will both be incorrect though? Yep. Missed those; should have added another accessor. Also, for the record, it changes the vmstate, but since a previous patch in the series bumped the version number for the split on misa, we can call all of a piece and ok. r~
On Mon, Oct 18, 2021 at 3:31 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 10/17/21 9:52 PM, Alistair Francis wrote: > > On Sun, Oct 17, 2021 at 3:32 AM Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> The position of this read-only field is dependent on the > >> current cpu width. Rather than having to compute that > >> difference in many places, compute it only on read. > >> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > > > This means that the value reported by riscv_cpu_dump_state() and GDB > > will both be incorrect though? > > Yep. Missed those; should have added another accessor. Do we get much of an advantage from this though? To me it seems confusing that the mstatus register doesn't actually contain the latest value (for example when debugging QEMU and adding my own printf's). > > Also, for the record, it changes the vmstate, but since a previous patch in the series > bumped the version number for the split on misa, we can call all of a piece and ok. Works for me :) Alistair > > > r~
On 10/17/21 10:38 PM, Alistair Francis wrote: > Do we get much of an advantage from this though? To me it seems > confusing that the mstatus register doesn't actually contain the > latest value (for example when debugging QEMU and adding my own > printf's). (1) We have at least 3 places that need to check the cpu state in order to set SD correctly; we would add another couple with the VS bit that's coming in from RVV-1.0. By setting this bit during read, we reduce that to one accessor for read. (2) We would need to move this bit when changing MXL, once that's possible with the various XLEN changing patch sets. (3) The position of this bit, between MSTATUS and SSTATUS, differs if MXL != SXL, which means that there is not really one correct setting for (2). r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 429afd1f48..0d1132f39d 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -185,10 +185,9 @@ bool riscv_cpu_fp_enabled(CPURISCVState *env) void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env) { - uint64_t sd = riscv_cpu_mxl(env) == MXL_RV32 ? MSTATUS32_SD : MSTATUS64_SD; uint64_t mstatus_mask = MSTATUS_MXR | MSTATUS_SUM | MSTATUS_FS | MSTATUS_SPP | MSTATUS_SPIE | MSTATUS_SIE | - MSTATUS64_UXL | sd; + MSTATUS64_UXL; bool current_virt = riscv_cpu_virt_enabled(env); g_assert(riscv_has_ext(env, RVH)); diff --git a/target/riscv/csr.c b/target/riscv/csr.c index c4a479ddd2..69e4d65fcd 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -477,10 +477,28 @@ static RISCVException read_mhartid(CPURISCVState *env, int csrno, } /* Machine Trap Setup */ + +/* We do not store SD explicitly, only compute it on demand. */ +static uint64_t add_status_sd(RISCVMXL xl, uint64_t status) +{ + if ((status & MSTATUS_FS) == MSTATUS_FS || + (status & MSTATUS_XS) == MSTATUS_XS) { + switch (xl) { + case MXL_RV32: + return status | MSTATUS32_SD; + case MXL_RV64: + return status | MSTATUS64_SD; + default: + g_assert_not_reached(); + } + } + return status; +} + static RISCVException read_mstatus(CPURISCVState *env, int csrno, target_ulong *val) { - *val = env->mstatus; + *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus); return RISCV_EXCP_NONE; } @@ -498,7 +516,6 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, { uint64_t mstatus = env->mstatus; uint64_t mask = 0; - int dirty; /* flush tlb on mstatus fields that affect VM */ if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | @@ -520,12 +537,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, mstatus = (mstatus & ~mask) | (val & mask); - dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) | - ((mstatus & MSTATUS_XS) == MSTATUS_XS); - if (riscv_cpu_mxl(env) == MXL_RV32) { - mstatus = set_field(mstatus, MSTATUS32_SD, dirty); - } else { - mstatus = set_field(mstatus, MSTATUS64_SD, dirty); + if (riscv_cpu_mxl(env) == MXL_RV64) { /* SXL and UXL fields are for now read only */ mstatus = set_field(mstatus, MSTATUS64_SXL, MXL_RV64); mstatus = set_field(mstatus, MSTATUS64_UXL, MXL_RV64); @@ -798,13 +810,8 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno, { target_ulong mask = (sstatus_v1_10_mask); - if (riscv_cpu_mxl(env) == MXL_RV32) { - mask |= SSTATUS32_SD; - } else { - mask |= SSTATUS64_SD; - } - - *val = env->mstatus & mask; + /* TODO: Use SXL not MXL. */ + *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask); return RISCV_EXCP_NONE; } diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 0b3da060fd..1d6bf01a48 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -305,7 +305,6 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) static void mark_fs_dirty(DisasContext *ctx) { TCGv tmp; - target_ulong sd = get_xl(ctx) == MXL_RV32 ? MSTATUS32_SD : MSTATUS64_SD; if (ctx->mstatus_fs != MSTATUS_FS) { /* Remember the state change for the rest of the TB. */ @@ -313,7 +312,7 @@ static void mark_fs_dirty(DisasContext *ctx) tmp = tcg_temp_new(); tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus)); - tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS); tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus)); tcg_temp_free(tmp); } @@ -324,7 +323,7 @@ static void mark_fs_dirty(DisasContext *ctx) tmp = tcg_temp_new(); tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs)); - tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS); tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs)); tcg_temp_free(tmp); }
The position of this read-only field is dependent on the current cpu width. Rather than having to compute that difference in many places, compute it only on read. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/riscv/cpu_helper.c | 3 +-- target/riscv/csr.c | 37 ++++++++++++++++++++++--------------- target/riscv/translate.c | 5 ++--- 3 files changed, 25 insertions(+), 20 deletions(-) -- 2.25.1