| Message ID | 20210511101951.165287-37-alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v3,01/42] target/riscv: Remove privilege v1.9 specific CSR related code | expand |
On Tue, 11 May 2021 at 11:22, Alistair Francis <alistair.francis@wdc.com> wrote: > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Message-id: fcc125d96da941b56c817c9dd6068dc36478fc53.1619234854.git.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(-) > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 26eccc5eb1..a596f80f20 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -78,6 +78,17 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) > return ctx->misa & ext; > } > > +#ifdef TARGET_RISCV32 > +# define is_32bit(ctx) true > +#elif defined(CONFIG_USER_ONLY) > +# define is_32bit(ctx) false > +#else > +static inline bool is_32bit(DisasContext *ctx) > +{ > + return (ctx->misa & RV32) == RV32; > +} > +#endif Hi; Coverity points out (CID 1453107) that this is_32bit() function can never return true for at least some build configs, because RV32 is defined as ((target_ulong)1 << (TARGET_LONG_BITS - 2)) but ctx->misa is a uint32_t field, which (if TARGET_LONG_BITS is 64) is not big enough for the RV32 bit. Bug, or false positive ? thanks -- PMM
On Thu, May 20, 2021 at 11:55 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 11 May 2021 at 11:22, Alistair Francis <alistair.francis@wdc.com> wrote: > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Message-id: fcc125d96da941b56c817c9dd6068dc36478fc53.1619234854.git.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(-) > > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > > index 26eccc5eb1..a596f80f20 100644 > > --- a/target/riscv/translate.c > > +++ b/target/riscv/translate.c > > @@ -78,6 +78,17 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) > > return ctx->misa & ext; > > } > > > > +#ifdef TARGET_RISCV32 > > +# define is_32bit(ctx) true > > +#elif defined(CONFIG_USER_ONLY) > > +# define is_32bit(ctx) false > > +#else > > +static inline bool is_32bit(DisasContext *ctx) > > +{ > > + return (ctx->misa & RV32) == RV32; > > +} > > +#endif > > Hi; Coverity points out (CID 1453107) that this is_32bit() function > can never return true for at least some build configs, because RV32 > is defined as ((target_ulong)1 << (TARGET_LONG_BITS - 2)) > but ctx->misa is a uint32_t field, which (if TARGET_LONG_BITS is > 64) is not big enough for the RV32 bit. This seems like a false positive as RV32 is defined as: ((target_ulong)1 << (TARGET_LONG_BITS - 2)) while ctx->misa is a target_ulong. So it should always be able to return true. Alistair > > Bug, or false positive ? > > thanks > -- PMM
On 5/21/21 6:55 AM, Alistair Francis wrote: > On Thu, May 20, 2021 at 11:55 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> On Tue, 11 May 2021 at 11:22, Alistair Francis <alistair.francis@wdc.com> wrote: >>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> Message-id: fcc125d96da941b56c817c9dd6068dc36478fc53.1619234854.git.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(-) >>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c >>> index 26eccc5eb1..a596f80f20 100644 >>> --- a/target/riscv/translate.c >>> +++ b/target/riscv/translate.c >>> @@ -78,6 +78,17 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) >>> return ctx->misa & ext; >>> } >>> >>> +#ifdef TARGET_RISCV32 >>> +# define is_32bit(ctx) true >>> +#elif defined(CONFIG_USER_ONLY) >>> +# define is_32bit(ctx) false >>> +#else >>> +static inline bool is_32bit(DisasContext *ctx) >>> +{ >>> + return (ctx->misa & RV32) == RV32; >>> +} >>> +#endif >> Hi; Coverity points out (CID 1453107) that this is_32bit() function >> can never return true for at least some build configs, because RV32 >> is defined as ((target_ulong)1 << (TARGET_LONG_BITS - 2)) >> but ctx->misa is a uint32_t field, which (if TARGET_LONG_BITS is >> 64) is not big enough for the RV32 bit. > This seems like a false positive as RV32 is defined as: > > ((target_ulong)1 << (TARGET_LONG_BITS - 2)) > > while ctx->misa is a target_ulong. Although the misa in RISCVCPUState is target_ulong, the misa in DisasContext is uint32_t. I think we should fix up the misa in DisasContext. Zhiwei > > So it should always be able to return true. > > Alistair > >> Bug, or false positive ? >> >> thanks >> -- PMM
On Fri, May 21, 2021 at 12:09 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > > On 5/21/21 6:55 AM, Alistair Francis wrote: > > On Thu, May 20, 2021 at 11:55 PM Peter Maydell <peter.maydell@linaro.org> wrote: > >> On Tue, 11 May 2021 at 11:22, Alistair Francis <alistair.francis@wdc.com> wrote: > >>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >>> Message-id: fcc125d96da941b56c817c9dd6068dc36478fc53.1619234854.git.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(-) > >>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c > >>> index 26eccc5eb1..a596f80f20 100644 > >>> --- a/target/riscv/translate.c > >>> +++ b/target/riscv/translate.c > >>> @@ -78,6 +78,17 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) > >>> return ctx->misa & ext; > >>> } > >>> > >>> +#ifdef TARGET_RISCV32 > >>> +# define is_32bit(ctx) true > >>> +#elif defined(CONFIG_USER_ONLY) > >>> +# define is_32bit(ctx) false > >>> +#else > >>> +static inline bool is_32bit(DisasContext *ctx) > >>> +{ > >>> + return (ctx->misa & RV32) == RV32; > >>> +} > >>> +#endif > >> Hi; Coverity points out (CID 1453107) that this is_32bit() function > >> can never return true for at least some build configs, because RV32 > >> is defined as ((target_ulong)1 << (TARGET_LONG_BITS - 2)) > >> but ctx->misa is a uint32_t field, which (if TARGET_LONG_BITS is > >> 64) is not big enough for the RV32 bit. > > This seems like a false positive as RV32 is defined as: > > > > ((target_ulong)1 << (TARGET_LONG_BITS - 2)) > > > > while ctx->misa is a target_ulong. > > Although the misa in RISCVCPUState is target_ulong, the misa in > DisasContext is uint32_t. > > I think we should fix up the misa in DisasContext. Good catch, I'll send a patch. Alistair > > Zhiwei > > > > > So it should always be able to return true. > > > > Alistair > > > >> Bug, or false positive ? > >> > >> thanks > >> -- PMM
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index d738e2fdbd..6e30b312f0 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -368,16 +368,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 41951a0a84..e955753441 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -538,7 +538,11 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, 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 RISCV_EXCP_NONE; @@ -614,7 +618,11 @@ static RISCVException write_misa(CPURISCVState *env, int csrno, } /* 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 26eccc5eb1..a596f80f20 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -78,6 +78,17 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext) return ctx->misa & ext; } +#ifdef TARGET_RISCV32 +# define is_32bit(ctx) true +#elif defined(CONFIG_USER_ONLY) +# define is_32bit(ctx) false +#else +static inline bool is_32bit(DisasContext *ctx) +{ + return (ctx->misa & RV32) == RV32; +} +#endif + /* * RISC-V requires NaN-boxing of narrower width floating point values. * This applies when a 32-bit value is assigned to a 64-bit FP register. @@ -369,6 +380,8 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) static void mark_fs_dirty(DisasContext *ctx) { TCGv tmp; + target_ulong sd; + if (ctx->mstatus_fs == MSTATUS_FS) { return; } @@ -376,13 +389,15 @@ static void mark_fs_dirty(DisasContext *ctx) ctx->mstatus_fs = MSTATUS_FS; tmp = tcg_temp_new(); + sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD; + tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus)); - tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | MSTATUS_SD); + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); 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); + tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd); tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs)); } tcg_temp_free(tmp);