Message ID | 20210805025312.15720-3-zhiwei_liu@c-sky.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Support UXL field in mstatus | expand |
On 8/4/21 4:53 PM, LIU Zhiwei wrote: > +static TCGv gpr_src_u(DisasContext *ctx, int reg_num) > +{ > + if (reg_num == 0) { > + return ctx->zero; > + } > + if (ctx->uxl32) { > + tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); > + } > + return cpu_gpr[reg_num]; > +} > + > +static TCGv gpr_src_s(DisasContext *ctx, int reg_num) > +{ > + if (reg_num == 0) { > + return ctx->zero; > + } > + if (ctx->uxl32) { > + tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); > + } > + return cpu_gpr[reg_num]; > +} This is bad: you cannot modify the source registers like this. These incorrect modifications will be visible to the kernel on transition back to S-mode. > > +static bool gen_branch_u(DisasContext *ctx, arg_b *a, TCGCond cond) > +{ > + TCGv src1 = gpr_src_u(ctx, a->rs1); > + TCGv src2 = gpr_src_u(ctx, a->rs2); > + > + return gen_branch_internal(ctx, a, cond, src1, src2); > +} This is unnecessary. Unsigned comparisons work just fine with sign-extended values. It will be simpler to keep all values sign-extended. r~
On 2021/8/6 上午3:06, Richard Henderson wrote: > On 8/4/21 4:53 PM, LIU Zhiwei wrote: >> +static TCGv gpr_src_u(DisasContext *ctx, int reg_num) >> +{ >> + if (reg_num == 0) { >> + return ctx->zero; >> + } >> + if (ctx->uxl32) { >> + tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); >> + } >> + return cpu_gpr[reg_num]; >> +} >> + >> +static TCGv gpr_src_s(DisasContext *ctx, int reg_num) >> +{ >> + if (reg_num == 0) { >> + return ctx->zero; >> + } >> + if (ctx->uxl32) { >> + tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); >> + } >> + return cpu_gpr[reg_num]; >> +} > > This is bad: you cannot modify the source registers like this. In my opinion, when uxl32, the only meaningful part is the low 32 bits, and it doesn't matter to modify the high parts. > > These incorrect modifications will be visible to the kernel on > transition back to S-mode. When transition back to S-mode, I think the kernel will save the U-mode registers to memory. Thanks, Zhiwei > >> >> +static bool gen_branch_u(DisasContext *ctx, arg_b *a, TCGCond cond) >> +{ >> + TCGv src1 = gpr_src_u(ctx, a->rs1); >> + TCGv src2 = gpr_src_u(ctx, a->rs2); >> + >> + return gen_branch_internal(ctx, a, cond, src1, src2); >> +} > > This is unnecessary. Unsigned comparisons work just fine with > sign-extended values. It will be simpler to keep all values > sign-extended. > > > r~
On 8/8/21 3:45 PM, LIU Zhiwei wrote: > > On 2021/8/6 上午3:06, Richard Henderson wrote: >> On 8/4/21 4:53 PM, LIU Zhiwei wrote: >>> +static TCGv gpr_src_u(DisasContext *ctx, int reg_num) >>> +{ >>> + if (reg_num == 0) { >>> + return ctx->zero; >>> + } >>> + if (ctx->uxl32) { >>> + tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); >>> + } >>> + return cpu_gpr[reg_num]; >>> +} >>> + >>> +static TCGv gpr_src_s(DisasContext *ctx, int reg_num) >>> +{ >>> + if (reg_num == 0) { >>> + return ctx->zero; >>> + } >>> + if (ctx->uxl32) { >>> + tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); >>> + } >>> + return cpu_gpr[reg_num]; >>> +} >> >> This is bad: you cannot modify the source registers like this. > > In my opinion, when uxl32, the only meaningful part is the low 32 bits, and it doesn't > matter to modify the high parts. Then why does the architecture manual specify that when registers are modified the value written sign-extended? This effect should be visible... > >> >> These incorrect modifications will be visible to the kernel on transition back to S-mode. > > When transition back to S-mode, I think the kernel will save the U-mode registers to memory. ... here. Once we're in S-mode, we have SXLEN, and if SXLEN > UXLEN, the high part of the register will be visible. It really must be either (1) sign-extended because U-mode wrote to the register or (2) unmodified from the last time S-mode wrote to the register. r~
On 2021/8/10 上午3:34, Richard Henderson wrote: > On 8/8/21 3:45 PM, LIU Zhiwei wrote: >> >> On 2021/8/6 上午3:06, Richard Henderson wrote: >>> On 8/4/21 4:53 PM, LIU Zhiwei wrote: >>>> +static TCGv gpr_src_u(DisasContext *ctx, int reg_num) >>>> +{ >>>> + if (reg_num == 0) { >>>> + return ctx->zero; >>>> + } >>>> + if (ctx->uxl32) { >>>> + tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); >>>> + } >>>> + return cpu_gpr[reg_num]; >>>> +} >>>> + >>>> +static TCGv gpr_src_s(DisasContext *ctx, int reg_num) >>>> +{ >>>> + if (reg_num == 0) { >>>> + return ctx->zero; >>>> + } >>>> + if (ctx->uxl32) { >>>> + tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); >>>> + } >>>> + return cpu_gpr[reg_num]; >>>> +} >>> >>> This is bad: you cannot modify the source registers like this. >> >> In my opinion, when uxl32, the only meaningful part is the low 32 >> bits, and it doesn't matter to modify the high parts. > > Then why does the architecture manual specify that when registers are > modified the value written sign-extended? This effect should be > visible... > Hi Richard, I still don't know why the value written sign-extended. If that's the the rule of final specification, I will try to obey it although our Linux will not depend on the high part. Thanks, Zhiwei >> >>> >>> These incorrect modifications will be visible to the kernel on >>> transition back to S-mode. >> >> When transition back to S-mode, I think the kernel will save the >> U-mode registers to memory. > > ... here. Once we're in S-mode, we have SXLEN, and if SXLEN > UXLEN, > the high part of the register will be visible. It really must be > either (1) sign-extended because U-mode wrote to the register or (2) > unmodified from the last time S-mode wrote to the register. > > > r~
On 8/11/21 4:57 AM, LIU Zhiwei wrote: > I still don't know why the value written sign-extended. If that's the the rule of final > specification, I will try to obey it although our Linux will not depend on the high part. The text that I'm looking at is https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMFDQC-and-Priv-v1.11/riscv-privileged-20190608.pdf 3.1.6.2 Base ISA Control in mstatus Register In the fifth paragraph, the requirement for sign-extension is detailed. r~
On 2021/8/12 上午1:56, Richard Henderson wrote: > On 8/11/21 4:57 AM, LIU Zhiwei wrote: >> I still don't know why the value written sign-extended. If that's >> the the rule of final specification, I will try to obey it although >> our Linux will not depend on the high part. > > The text that I'm looking at is > > https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMFDQC-and-Priv-v1.11/riscv-privileged-20190608.pdf > > > 3.1.6.2 Base ISA Control in mstatus Register > > In the fifth paragraph, the requirement for sign-extension is detailed. > Thanks. I have already seen this rule. /"Whenever XLEN in any mode is set to a value less than the widest supported XLEN, all operations////must ignore source operand register bits above the configured XLEN, and must sign-extend results////to fill the entire widest supported XLEN in the destination register.//" / I still don't know why the specification has this constraint. It just requires that fill hardware registers with defined sign-extension value. But it doesn't give the real benefit of this constraint. If the software doesn't use the high part, who cares the really value in high part? Do you know the benefit? Thanks again. Best Regards, Zhiwei// > > r~
On 8/11/21 12:40 PM, LIU Zhiwei wrote: > If the software doesn't use the high part, who cares the really value in high part? Do you > know the benefit? Thanks again. I do not. I simply presume that they already have the hardware, in the form of the addw instruction, etc. The mistake, I think, was changing the definition of "add" in the first place, which required the addition of a different opcode "addw", which is then undefined for RV32. They should simply have had "addw" and "addq" as different opcodes that didn't change behaviour. Etc. But what's done is done. r~
On 2021/8/12 下午12:42, Richard Henderson wrote: > On 8/11/21 12:40 PM, LIU Zhiwei wrote: >> If the software doesn't use the high part, who cares the really value >> in high part? Do you know the benefit? Thanks again. > > I do not. > > I simply presume that they already have the hardware, in the form of > the addw instruction, etc. > > The mistake, I think, was changing the definition of "add" in the > first place, which required the addition of a different opcode "addw", > which is then undefined for RV32. Sorry, I don't get "the mistake" here. Do you think the specification is not right. Or the QEMU implementation of this patch set is not right? Currently I don't know there is a 64-bit hardware which has done with UXL32. > They should simply have had "addw" and "addq" as different opcodes > that didn't change behaviour. Etc. I don't get this statement. Is it related to UXL32? Best Regards, Zhiwei > > But what's done is done. > > > r~
On 8/11/21 7:03 PM, LIU Zhiwei wrote: > > On 2021/8/12 下午12:42, Richard Henderson wrote: >> On 8/11/21 12:40 PM, LIU Zhiwei wrote: >>> If the software doesn't use the high part, who cares the really value in high part? Do >>> you know the benefit? Thanks again. >> >> I do not. >> >> I simply presume that they already have the hardware, in the form of the addw >> instruction, etc. >> >> The mistake, I think, was changing the definition of "add" in the first place, which >> required the addition of a different opcode "addw", which is then undefined for RV32. > > Sorry, I don't get "the mistake" here. Do you think the specification is not right. I was critiquing the development of the risc-v specification, in that there are complications in the current specification that could have been foreseen and avoided with different choices years ago. >> They should simply have had "addw" and "addq" as different opcodes that didn't change >> behaviour. Etc. > > I don't get this statement. Is it related to UXL32? No. I was just musing. It's not important. r~
On 2021/8/12 下午2:12, Richard Henderson wrote: > On 8/11/21 7:03 PM, LIU Zhiwei wrote: >> >> On 2021/8/12 下午12:42, Richard Henderson wrote: >>> On 8/11/21 12:40 PM, LIU Zhiwei wrote: >>>> If the software doesn't use the high part, who cares the really >>>> value in high part? Do you know the benefit? Thanks again. >>> >>> I do not. >>> >>> I simply presume that they already have the hardware, in the form of >>> the addw instruction, etc. >>> >>> The mistake, I think, was changing the definition of "add" in the >>> first place, which required the addition of a different opcode >>> "addw", which is then undefined for RV32. >> >> Sorry, I don't get "the mistake" here. Do you think the specification >> is not right. > > I was critiquing the development of the risc-v specification, in that > there are complications in the current specification that could have > been foreseen and avoided with different choices years ago. > > >>> They should simply have had "addw" and "addq" as different opcodes >>> that didn't change behaviour. Etc. >> >> I don't get this statement. Is it related to UXL32? > > No. I was just musing. It's not important. > > Although I don't know what really you mean, I think "addw" and "addq" will be better than current "addw" and "add". At least we can avoid adjust almost every instruction like "add" for UXL32. Best Regards, Zhiwei > r~ >
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc index 3705aad380..ea41d1de2d 100644 --- a/target/riscv/insn_trans/trans_rvi.c.inc +++ b/target/riscv/insn_trans/trans_rvi.c.inc @@ -84,11 +84,11 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a) return true; } -static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond) +static bool gen_branch_internal(DisasContext *ctx, arg_b *a, + TCGCond cond, + TCGv src1, TCGv src2) { TCGLabel *l = gen_new_label(); - TCGv src1 = gpr_src(ctx, a->rs1); - TCGv src2 = gpr_src(ctx, a->rs2); tcg_gen_brcond_tl(cond, src1, src2, l); gen_goto_tb(ctx, 1, ctx->pc_succ_insn); @@ -106,6 +106,30 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond) return true; } +static bool gen_branch_s(DisasContext *ctx, arg_b *a, TCGCond cond) +{ + TCGv src1 = gpr_src_s(ctx, a->rs1); + TCGv src2 = gpr_src_s(ctx, a->rs2); + + return gen_branch_internal(ctx, a, cond, src1, src2); +} + +static bool gen_branch_u(DisasContext *ctx, arg_b *a, TCGCond cond) +{ + TCGv src1 = gpr_src_u(ctx, a->rs1); + TCGv src2 = gpr_src_u(ctx, a->rs2); + + return gen_branch_internal(ctx, a, cond, src1, src2); +} + +static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond) +{ + TCGv src1 = gpr_src(ctx, a->rs1); + TCGv src2 = gpr_src(ctx, a->rs2); + + return gen_branch_internal(ctx, a, cond, src1, src2); +} + static bool trans_beq(DisasContext *ctx, arg_beq *a) { return gen_branch(ctx, a, TCG_COND_EQ); @@ -118,22 +142,22 @@ static bool trans_bne(DisasContext *ctx, arg_bne *a) static bool trans_blt(DisasContext *ctx, arg_blt *a) { - return gen_branch(ctx, a, TCG_COND_LT); + return gen_branch_s(ctx, a, TCG_COND_LT); } static bool trans_bge(DisasContext *ctx, arg_bge *a) { - return gen_branch(ctx, a, TCG_COND_GE); + return gen_branch_s(ctx, a, TCG_COND_GE); } static bool trans_bltu(DisasContext *ctx, arg_bltu *a) { - return gen_branch(ctx, a, TCG_COND_LTU); + return gen_branch_u(ctx, a, TCG_COND_LTU); } static bool trans_bgeu(DisasContext *ctx, arg_bgeu *a) { - return gen_branch(ctx, a, TCG_COND_GEU); + return gen_branch_u(ctx, a, TCG_COND_GEU); } static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop) diff --git a/target/riscv/translate.c b/target/riscv/translate.c index ac4a545da8..d334a9db86 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -176,6 +176,28 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest) } } +static TCGv gpr_src_u(DisasContext *ctx, int reg_num) +{ + if (reg_num == 0) { + return ctx->zero; + } + if (ctx->uxl32) { + tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); + } + return cpu_gpr[reg_num]; +} + +static TCGv gpr_src_s(DisasContext *ctx, int reg_num) +{ + if (reg_num == 0) { + return ctx->zero; + } + if (ctx->uxl32) { + tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]); + } + return cpu_gpr[reg_num]; +} + /* Wrapper for getting reg values - need to check of reg is zero since * cpu_gpr[0] is not actually allocated */
When UXLEN is 32 on 64-bit CPU, only use the LSB 32 bits of source registers and sign-extend or zero-extend it according to different operations. Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> --- target/riscv/insn_trans/trans_rvi.c.inc | 38 ++++++++++++++++++++----- target/riscv/translate.c | 22 ++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-)