accel/tcg: Fix cross-page pointer wrapping issue

[PATCH 10/12] target/s390x: Fill in TCGCPUOps.pointer_wrap

Posted by Richard Henderson 7 months, 2 weeks ago

Use the existing wrap_address function.

Cc: qemu-s390x@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/cpu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 9c1158ebcc..f05ce317da 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -347,6 +347,14 @@ static TCGTBCPUState s390x_get_tb_cpu_state(CPUState *cs)
     };
 }
 
+#ifndef CONFIG_USER_ONLY
+static vaddr s390_pointer_wrap(CPUState *cs, int mmu_idx,
+                               vaddr result, vaddr base)
+{
+    return wrap_address(cpu_env(cs), result);
+}
+#endif
+
 static const TCGCPUOps s390_tcg_ops = {
     .mttcg_supported = true,
     .precise_smc = true,
@@ -367,6 +375,7 @@ static const TCGCPUOps s390_tcg_ops = {
     .record_sigbus = s390_cpu_record_sigbus,
 #else
     .tlb_fill = s390_cpu_tlb_fill,
+    .pointer_wrap = s390_pointer_wrap,
     .cpu_exec_interrupt = s390_cpu_exec_interrupt,
     .cpu_exec_halt = s390_cpu_has_work,
     .cpu_exec_reset = cpu_reset,
-- 
2.43.0

Re: [PATCH 10/12] target/s390x: Fill in TCGCPUOps.pointer_wrap

Posted by Philippe Mathieu-Daudé 7 months, 2 weeks ago

On 4/5/25 22:57, Richard Henderson wrote:
> Use the existing wrap_address function.
> 
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/cpu.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 9c1158ebcc..f05ce317da 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -347,6 +347,14 @@ static TCGTBCPUState s390x_get_tb_cpu_state(CPUState *cs)
>       };
>   }
>   
> +#ifndef CONFIG_USER_ONLY
> +static vaddr s390_pointer_wrap(CPUState *cs, int mmu_idx,
> +                               vaddr result, vaddr base)
> +{
> +    return wrap_address(cpu_env(cs), result);
> +}
> +#endif
> +
>   static const TCGCPUOps s390_tcg_ops = {
>       .mttcg_supported = true,
>       .precise_smc = true,
> @@ -367,6 +375,7 @@ static const TCGCPUOps s390_tcg_ops = {
>       .record_sigbus = s390_cpu_record_sigbus,
>   #else
>       .tlb_fill = s390_cpu_tlb_fill,
> +    .pointer_wrap = s390_pointer_wrap,

As future cleanup, we might now remove the wrap_address() calls
in target/s390x/tcg/crypto_helper.c, and target/s390x/tcg/vec_helper.c,
is that correct?

Also some uses in target/s390x/tcg/mem_helper.c.

Meanwhile for this patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 10/12] target/s390x: Fill in TCGCPUOps.pointer_wrap

Posted by Richard Henderson 7 months, 2 weeks ago

On 5/5/25 07:41, Philippe Mathieu-Daudé wrote:
> On 4/5/25 22:57, Richard Henderson wrote:
>> Use the existing wrap_address function.
>>
>> Cc: qemu-s390x@nongnu.org
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/s390x/cpu.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 9c1158ebcc..f05ce317da 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -347,6 +347,14 @@ static TCGTBCPUState s390x_get_tb_cpu_state(CPUState *cs)
>>       };
>>   }
>> +#ifndef CONFIG_USER_ONLY
>> +static vaddr s390_pointer_wrap(CPUState *cs, int mmu_idx,
>> +                               vaddr result, vaddr base)
>> +{
>> +    return wrap_address(cpu_env(cs), result);
>> +}
>> +#endif
>> +
>>   static const TCGCPUOps s390_tcg_ops = {
>>       .mttcg_supported = true,
>>       .precise_smc = true,
>> @@ -367,6 +375,7 @@ static const TCGCPUOps s390_tcg_ops = {
>>       .record_sigbus = s390_cpu_record_sigbus,
>>   #else
>>       .tlb_fill = s390_cpu_tlb_fill,
>> +    .pointer_wrap = s390_pointer_wrap,
> 
> As future cleanup, we might now remove the wrap_address() calls
> in target/s390x/tcg/crypto_helper.c, and target/s390x/tcg/vec_helper.c,
> is that correct?
> 
> Also some uses in target/s390x/tcg/mem_helper.c.

No, not correct.

The new pointer_wrap hook is only used for unaligned accesses that cross page boundaries. 
It does not apply to a sequence of individual accesses like we have in the s390x helpers.


r~

Re: [PATCH 10/12] target/s390x: Fill in TCGCPUOps.pointer_wrap

Posted by Philippe Mathieu-Daudé 6 months, 3 weeks ago

On 5/5/25 18:16, Richard Henderson wrote:
> On 5/5/25 07:41, Philippe Mathieu-Daudé wrote:
>> On 4/5/25 22:57, Richard Henderson wrote:
>>> Use the existing wrap_address function.
>>>
>>> Cc: qemu-s390x@nongnu.org
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>   target/s390x/cpu.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>>> index 9c1158ebcc..f05ce317da 100644
>>> --- a/target/s390x/cpu.c
>>> +++ b/target/s390x/cpu.c
>>> @@ -347,6 +347,14 @@ static TCGTBCPUState 
>>> s390x_get_tb_cpu_state(CPUState *cs)
>>>       };
>>>   }
>>> +#ifndef CONFIG_USER_ONLY
>>> +static vaddr s390_pointer_wrap(CPUState *cs, int mmu_idx,
>>> +                               vaddr result, vaddr base)
>>> +{
>>> +    return wrap_address(cpu_env(cs), result);
>>> +}
>>> +#endif
>>> +
>>>   static const TCGCPUOps s390_tcg_ops = {
>>>       .mttcg_supported = true,
>>>       .precise_smc = true,
>>> @@ -367,6 +375,7 @@ static const TCGCPUOps s390_tcg_ops = {
>>>       .record_sigbus = s390_cpu_record_sigbus,
>>>   #else
>>>       .tlb_fill = s390_cpu_tlb_fill,
>>> +    .pointer_wrap = s390_pointer_wrap,
>>
>> As future cleanup, we might now remove the wrap_address() calls
>> in target/s390x/tcg/crypto_helper.c, and target/s390x/tcg/vec_helper.c,
>> is that correct?
>>
>> Also some uses in target/s390x/tcg/mem_helper.c.
> 
> No, not correct.
> 
> The new pointer_wrap hook is only used for unaligned accesses that cross 
> page boundaries. It does not apply to a sequence of individual accesses 
> like we have in the s390x helpers.

Ah, now I understand, thanks!