Re: [PATCH v8 01/18] target/s390x: Fix wrong address handling in address loops

[Harald Freudenberger <freude@linux.ibm.com>]

On 2026-06-25 12:37, Philippe Mathieu-Daudé wrote:
> On 25/6/26 09:31, Harald Freudenberger wrote:
>> On 2026-06-24 12:05, Philippe Mathieu-Daudé wrote:
>>> Hi Harald,
>>> 
>>> On 24/6/26 10:09, Harald Freudenberger wrote:
>>>> With the introduction of the address wrapping function
>>>> wrap_address() the result can't be used to walk the
>>>> source address any more. So introduce a new local variable
>>>> to hold the wrapped address to avoid mixing source and
>>>> wrapped address value.
>>>> 
>>>> Fixes: fcc2699d41 ("target/s390x: Have MSA helper pass a mmu_idx 
>>>> argument")
>>> 
>>> This commit merely added an argument. I suppose you meant
>>> 9f17bfdab42 ("target/s390x: support SHA-512 extensions").
>>> 
>> 
>> No. Commit fcc2699d41 ("target/s390x: Have MSA helper pass a mmu_idx 
>> argument")
>> introduces the mmu_idx with the wrap_address() function or macro.
> 
> You are saying the same, not really explaining, I'm not following...
> 

I rephrased my wording regarding this fix. Please see v9 coming soon.

>> And the result was something like this:
>> 
>>    for (int i = 0; i < 8; i++, addr += 8) {
>>      addr = wrap_address(env, addr);
>>      a[i] = cpu_ldq_mmu(env, addr, oi, ra);
>>    }
>> 
>> This code reuses the addr variable in the loop
>> by setting addr to the wrapped address but after
>> that the loop adds +8 to this wrapped address.
>> This code only works as long as the wrapped address
>> is equal to the address put put in.
>> Thus introduce another intermediate variable which
>> holds the wrapped address:
>> 
>>    for (int i = 0; i < 8; i++, addr += 8) {
>>      uint64_t _addr = wrap_address(env, addr);
>>      a[i] = cpu_ldq_mmu(env, _addr, oi, ra);
>>    }
>> 
>> Of course it would also work with a direct replacement:
>> 
>>    for (int i = 0; i < 8; i++, addr += 8) {
>>      a[i] = cpu_ldq_mmu(env, wrap_address(env, addr), oi, ra);
>>    }
>> 
>> but this isn't that readable.
>> 
>>>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>>>> ---
>>>>   target/s390x/tcg/crypto_helper.c | 16 ++++++++--------
>>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/target/s390x/tcg/crypto_helper.c b/target/s390x/tcg/ 
>>>> crypto_helper.c
>>>> index ae392bce0e..29ad2aff43 100644
>>>> --- a/target/s390x/tcg/crypto_helper.c
>>>> +++ b/target/s390x/tcg/crypto_helper.c
>>>> @@ -126,8 +126,8 @@ static void sha512_read_icv(CPUS390XState *env, 
>>>> const int mmu_idx,
>>>>       const MemOpIdx oi = make_memop_idx(MO_BE | MO_64 | MO_UNALN, 
>>>> mmu_idx);
>>>>         for (int i = 0; i < 8; i++, addr += 8) {
>>>> -        addr = wrap_address(env, addr);
>>>> -        a[i] = cpu_ldq_mmu(env, addr, oi, ra);
>>>> +        uint64_t _addr = wrap_address(env, addr);
>>> 
>>> s/_addr/wa/ or even directly inline?
>>> 
>>>            a[i] = cpu_ldq_mmu(env, wrap_address(env, addr), oi, ra);
>>> 
>>>> +        a[i] = cpu_ldq_mmu(env, _addr, oi, ra);
>>>>       }
>>>>   }
>>