1. Patchew
  2. QEMU
  3. [RFC PATCH 0/3] Fix some TCG RISC-V backend bugs
  4. View patch

[RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st

LIU Zhiwei posted 3 patches 3 years, 3 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Richard Henderson <richard.henderson@linaro.org>
[RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st
Posted by LIU Zhiwei 3 years, 3 months ago 
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;
     }
     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
-- 
2.25.1
Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st
Posted by Philippe Mathieu-Daudé 3 years, 3 months ago 
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.
Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st
Posted by LIU Zhiwei 3 years, 3 months ago 
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
Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st
Posted by Richard Henderson 3 years, 3 months ago 
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~
Re: [RFC PATCH 1/3] tcg/riscv: Fix base regsiter for qemu_ld/st
Posted by LIU Zhiwei 3 years, 3 months ago 
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~

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.