| Message ID | 20221020104154.4276-2-zhiwei_liu@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some TCG RISC-V backend bugs | expand |
On 10/20/22 20:41, LIU Zhiwei wrote: > When guest base is zero, we should use addr_regl as base regiser instead of > the initial register TCG_REG_TMP0. > > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > --- > tcg/riscv/tcg-target.c.inc | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc > index 81a83e45b1..32f4bc7bfc 100644 > --- a/tcg/riscv/tcg-target.c.inc > +++ b/tcg/riscv/tcg-target.c.inc > @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64) > } > if (guest_base != 0) { > tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl); > + } else { > + base = addr_regl; > } You're right that there's a bug here, where TMP0 remains uninitialized. I think it would be better to reorg the other direction: begin with initializeing base = addr_regl, and then change it away only when we make modifications. r~
On 20/10/22 12:41, LIU Zhiwei wrote: > When guest base is zero, we should use addr_regl as base regiser instead of Typo "register" here and in patch subject. > the initial register TCG_REG_TMP0. > > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > --- > tcg/riscv/tcg-target.c.inc | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc > index 81a83e45b1..32f4bc7bfc 100644 > --- a/tcg/riscv/tcg-target.c.inc > +++ b/tcg/riscv/tcg-target.c.inc > @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64) > } > if (guest_base != 0) { > tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl); > + } else { > + base = addr_regl; > } > tcg_out_qemu_ld_direct(s, data_regl, data_regh, base, opc, is_64); > #endif > @@ -1257,6 +1259,8 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64) > } > if (guest_base != 0) { > tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl); > + } else { > + base = addr_regl; > } > tcg_out_qemu_st_direct(s, data_regl, data_regh, base, opc); > #endif Maybe worth inverting the if statement, otherwise LGTM.
On 2022/10/20 19:18, Richard Henderson wrote: > On 10/20/22 20:41, LIU Zhiwei wrote: >> When guest base is zero, we should use addr_regl as base regiser >> instead of >> the initial register TCG_REG_TMP0. >> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> >> --- >> tcg/riscv/tcg-target.c.inc | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc >> index 81a83e45b1..32f4bc7bfc 100644 >> --- a/tcg/riscv/tcg-target.c.inc >> +++ b/tcg/riscv/tcg-target.c.inc >> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, >> const TCGArg *args, bool is_64) >> } >> if (guest_base != 0) { >> tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, >> addr_regl); >> + } else { >> + base = addr_regl; >> } > > You're right that there's a bug here, where TMP0 remains > uninitialized. I think it would be better to reorg the other > direction: begin with initializeing base = addr_regl, Do you mean only in user mode? I see TCG_REG_TMP0 has been used in tcg_out_tlb_load when system mode. Best Regards, Zhiwei > and then change it away only when we make modifications. > > r~
On 2022/10/20 19:26, Philippe Mathieu-Daudé wrote: > On 20/10/22 12:41, LIU Zhiwei wrote: >> When guest base is zero, we should use addr_regl as base regiser >> instead of > > Typo "register" here and in patch subject. > >> the initial register TCG_REG_TMP0. >> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> >> --- >> tcg/riscv/tcg-target.c.inc | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc >> index 81a83e45b1..32f4bc7bfc 100644 >> --- a/tcg/riscv/tcg-target.c.inc >> +++ b/tcg/riscv/tcg-target.c.inc >> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, >> const TCGArg *args, bool is_64) >> } >> if (guest_base != 0) { >> tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, >> addr_regl); >> + } else { >> + base = addr_regl; >> } >> tcg_out_qemu_ld_direct(s, data_regl, data_regh, base, opc, is_64); >> #endif >> @@ -1257,6 +1259,8 @@ static void tcg_out_qemu_st(TCGContext *s, >> const TCGArg *args, bool is_64) >> } >> if (guest_base != 0) { >> tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, >> addr_regl); >> + } else { >> + base = addr_regl; >> } >> tcg_out_qemu_st_direct(s, data_regl, data_regh, base, opc); >> #endif > > Maybe worth inverting the if statement, otherwise LGTM. Thanks. Is there any different? Sorry, I don't get why we should invert the if statement. Best Regards, Zhiwei
On 10/20/22 21:42, LIU Zhiwei wrote: > > On 2022/10/20 19:18, Richard Henderson wrote: >> On 10/20/22 20:41, LIU Zhiwei wrote: >>> When guest base is zero, we should use addr_regl as base regiser instead of >>> the initial register TCG_REG_TMP0. >>> >>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> >>> --- >>> tcg/riscv/tcg-target.c.inc | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc >>> index 81a83e45b1..32f4bc7bfc 100644 >>> --- a/tcg/riscv/tcg-target.c.inc >>> +++ b/tcg/riscv/tcg-target.c.inc >>> @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, >>> bool is_64) >>> } >>> if (guest_base != 0) { >>> tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl); >>> + } else { >>> + base = addr_regl; >>> } >> >> You're right that there's a bug here, where TMP0 remains uninitialized. I think it >> would be better to reorg the other direction: begin with initializeing base = addr_regl, > > Do you mean only in user mode? I see TCG_REG_TMP0 has been used in tcg_out_tlb_load when > system mode. Well, yes, since that's what you're patching here... r~
diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc index 81a83e45b1..32f4bc7bfc 100644 --- a/tcg/riscv/tcg-target.c.inc +++ b/tcg/riscv/tcg-target.c.inc @@ -1185,6 +1185,8 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64) } if (guest_base != 0) { tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl); + } else { + base = addr_regl; } tcg_out_qemu_ld_direct(s, data_regl, data_regh, base, opc, is_64); #endif @@ -1257,6 +1259,8 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64) } if (guest_base != 0) { tcg_out_opc_reg(s, OPC_ADD, base, TCG_GUEST_BASE_REG, addr_regl); + } else { + base = addr_regl; } tcg_out_qemu_st_direct(s, data_regl, data_regh, base, opc); #endif
When guest base is zero, we should use addr_regl as base regiser instead of the initial register TCG_REG_TMP0. Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> --- tcg/riscv/tcg-target.c.inc | 4 ++++ 1 file changed, 4 insertions(+)