| Message ID | 7efb55362999a5b709ea8e7fa0ea188e740d9c67.1617393702.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Steps towards running 32-bit guests on | expand |
On 4/2/21 1:02 PM, Alistair Francis wrote: > @@ -369,6 +369,9 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) > static void mark_fs_dirty(DisasContext *ctx) > { > TCGv tmp; > + CPUState *cpu = ctx->cs; > + CPURISCVState *env = cpu->env_ptr; > + > if (ctx->mstatus_fs == MSTATUS_FS) { > return; > } > @@ -377,12 +380,24 @@ 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 | MSTATUS_SD); > + if (riscv_cpu_is_32bit(env)) { This is less than ideal, and will be incorrect long term. You should check ctx->misa instead. Eventually you'll need to change riscv_tr_init_disas_context to not just copy ctx->misa from env. At present we flush all translation blocks when misa changes, which works. But you won't want to do that when the hypervisor is 64-bit and the guest is 32-bit. Anyway, I think it would be a good idea to create a helper local to translate, akin to has_ext(). > + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS32_SD); > + } else { > +#if defined(TARGET_RISCV64) > + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS64_SD); > +#endif The ifdefs are ugly. I presume there's some sort of compiler warning here? Does it go away if you cast to target_ulong? How about target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD; tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); r~
On Mon, Apr 5, 2021 at 11:10 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/2/21 1:02 PM, Alistair Francis wrote: > > @@ -369,6 +369,9 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) > > static void mark_fs_dirty(DisasContext *ctx) > > { > > TCGv tmp; > > + CPUState *cpu = ctx->cs; > > + CPURISCVState *env = cpu->env_ptr; > > + > > if (ctx->mstatus_fs == MSTATUS_FS) { > > return; > > } > > @@ -377,12 +380,24 @@ 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 | MSTATUS_SD); > > + if (riscv_cpu_is_32bit(env)) { > > This is less than ideal, and will be incorrect long term. > You should check ctx->misa instead. > > Eventually you'll need to change riscv_tr_init_disas_context to not just copy > ctx->misa from env. At present we flush all translation blocks when misa > changes, which works. But you won't want to do that when the hypervisor is > 64-bit and the guest is 32-bit. > > Anyway, I think it would be a good idea to create a helper local to translate, > akin to has_ext(). > > > + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS32_SD); > > + } else { > > +#if defined(TARGET_RISCV64) > > + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS64_SD); > > +#endif > > The ifdefs are ugly. I presume there's some sort of compiler warning here? > Does it go away if you cast to target_ulong? > > How about > > target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD; > tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); > That works, thanks! Alistair > > r~
On Mon, Apr 5, 2021 at 11:10 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/2/21 1:02 PM, Alistair Francis wrote: > > @@ -369,6 +369,9 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) > > static void mark_fs_dirty(DisasContext *ctx) > > { > > TCGv tmp; > > + CPUState *cpu = ctx->cs; > > + CPURISCVState *env = cpu->env_ptr; > > + > > if (ctx->mstatus_fs == MSTATUS_FS) { > > return; > > } > > @@ -377,12 +380,24 @@ 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 | MSTATUS_SD); > > + if (riscv_cpu_is_32bit(env)) { > > This is less than ideal, and will be incorrect long term. > You should check ctx->misa instead. > > Eventually you'll need to change riscv_tr_init_disas_context to not just copy > ctx->misa from env. At present we flush all translation blocks when misa > changes, which works. But you won't want to do that when the hypervisor is > 64-bit and the guest is 32-bit. > > Anyway, I think it would be a good idea to create a helper local to translate, > akin to has_ext(). > > > + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS32_SD); > > + } else { > > +#if defined(TARGET_RISCV64) > > + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS64_SD); > > +#endif > > The ifdefs are ugly. I presume there's some sort of compiler warning here? > Does it go away if you cast to target_ulong? > > How about > > target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD; It turns out clang doesn't like this, so I might still be stuck with ifdefs. Alistair > tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); > > > r~
On 4/8/21 8:20 AM, Alistair Francis wrote: >> target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD; > > It turns out clang doesn't like this, so I might still be stuck with ifdefs. I think we need #ifdef TARGET_RISCV32 #define is_32bit(ctx) true #else ... #endif based on $ cat z.c int foo(int x) { return x ? 1 : 1ul << 32; } int bar(void) { return 1 ? 1 : 1ul << 32; } rth@cloudburst:~$ clang-11 -S -Wall z.c z.c:1:37: warning: implicit conversion from 'unsigned long' to 'int' changes value from 4294967296 to 0 [-Wconstant-conversion] int foo(int x) { return x ? 1 : 1ul << 32; } ~~~~~~ ~~~~^~~~~ r~
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index 8caab23b62..dd643d0f63 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -387,16 +387,6 @@ #define MXL_RV64 2 #define MXL_RV128 3 -#if defined(TARGET_RISCV32) -#define MSTATUS_SD MSTATUS32_SD -#define MISA_MXL MISA32_MXL -#define MXL_VAL MXL_RV32 -#elif defined(TARGET_RISCV64) -#define MSTATUS_SD MSTATUS64_SD -#define MISA_MXL MISA64_MXL -#define MXL_VAL MXL_RV64 -#endif - /* sstatus CSR bits */ #define SSTATUS_UIE 0x00000001 #define SSTATUS_SIE 0x00000002 diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 832c3bf7fd..6052b2d6e9 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -492,7 +492,11 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val) dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) | ((mstatus & MSTATUS_XS) == MSTATUS_XS); - mstatus = set_field(mstatus, MSTATUS_SD, dirty); + if (riscv_cpu_is_32bit(env)) { + mstatus = set_field(mstatus, MSTATUS32_SD, dirty); + } else { + mstatus = set_field(mstatus, MSTATUS64_SD, dirty); + } env->mstatus = mstatus; return 0; @@ -564,7 +568,11 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) } /* misa.MXL writes are not supported by QEMU */ - val = (env->misa & MISA_MXL) | (val & ~MISA_MXL); + if (riscv_cpu_is_32bit(env)) { + val = (env->misa & MISA32_MXL) | (val & ~MISA32_MXL); + } else { + val = (env->misa & MISA64_MXL) | (val & ~MISA64_MXL); + } /* flush translation cache */ if (val != env->misa) { diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 2f9f5ccc62..9c6d998efa 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -369,6 +369,9 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) static void mark_fs_dirty(DisasContext *ctx) { TCGv tmp; + CPUState *cpu = ctx->cs; + CPURISCVState *env = cpu->env_ptr; + if (ctx->mstatus_fs == MSTATUS_FS) { return; } @@ -377,12 +380,24 @@ 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 | MSTATUS_SD); + if (riscv_cpu_is_32bit(env)) { + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS32_SD); + } else { +#if defined(TARGET_RISCV64) + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS64_SD); +#endif + } tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus)); if (ctx->virt_enabled) { tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs)); - tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS_SD); + if (riscv_cpu_is_32bit(env)) { + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS32_SD); + } else { +#if defined(TARGET_RISCV64) + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS64_SD); +#endif + } tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs)); } tcg_temp_free(tmp);
Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- target/riscv/cpu_bits.h | 10 ---------- target/riscv/csr.c | 12 ++++++++++-- target/riscv/translate.c | 19 +++++++++++++++++-- 3 files changed, 27 insertions(+), 14 deletions(-)