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

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

On 2026-06-30 11:33, Ilya Leoshkevich wrote:
> On 6/29/26 14:57, Harald Freudenberger wrote:
>> On 2026-06-24 14:56, Ilya Leoshkevich wrote:
>>> On 6/24/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")
>>>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>>>> ---
>>>>   target/s390x/tcg/crypto_helper.c | 16 ++++++++--------
>>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>> 
>>> If I take as an example AMODE 24 and look at the third loop 
>>> iteration,
>>> with the current code in master I would get:
>>> 
>>>     ((((addr & 0xffffff) + 8) & 0xffffff) + 8) & 0xffffff
>>> 
>>> and with your patch it would be:
>>> 
>>>     (addr + 8 + 8) & 0xffffff
>>> 
>>> which is undeniably more elegant, but otherwise looks equivalent to 
>>> me.
>>> 
>>> What is the functional issue here?
>>> 
>>>> 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);
>>>> +        a[i] = cpu_ldq_mmu(env, _addr, oi, ra);
>>>>       }
>>>>   }
>>> 
>>> [...]
>> 
>> It is a fix, not an improvement. The original code
>> used the addr variable to hold the wrapped address.
>> But as addr is also used as the loop variable there
>> is a mixing between unwrapped and wrapped address here
>> and the unfixed code only works when wrap_address does
>> return the very same address as output as it became as
>> input. My fix corrects this.
> 
> What does the patch change w.r.t. wrapping?
> Suppose we have amode 24 and addr = 0xfffff0:
> 
> Before:
> 
>   i = 0; addr = wrap(0xfffff0) = 0xfffff0;
>          a[0] = ldq(0xfffff0);
>   i = 1; addr = wrap(0xfffff0 + 8) = 0xfffff8;
>          a[1] = ldq(0xfffff8);
>   i = 2; addr = wrap(0xfffff8 + 8) = 0;
>          a[2] = ldq(0);
>   i = 3; addr = wrap(0 + 8) = 8;
>          a[3] = ldq(8);
> 
> After:
> 
>   i = 0; addr = 0xfffff0;
>          _addr = wrap(0xfffff0) = 0xfffff0;
>          a[0] = ldq(0xfffff0)
>   i = 1; addr = 0xfffff0 + 8;
>          _addr = wrap(0xfffff8) = 0xfffff8;
>          a[1] = ldq(0xfffff8);
>   i = 2; addr = 0xfffff8 + 8 = 0x1000000
>          _addr = wrap(0x1000000) = 0;
>          a[2] = ldq(0);
>   i = 3; addr = 0x1000000 + 8 = 0x1000008;
>          _addr = wrap(0x1000008) = 8;
>          a[3] = ldq(8);
> 
> The behavior look identical to me.
> What am I missing?

Lets assume addr is some 0x1000 and wrap does just add some offset, e.g. 
0x8000

for (int i = 0; i < 8; i++, addr += 8) {
     addr = wrap_address(env, addr);
     fetch or write something at the wrapped address addr;
}

Then the idea of this loop is to run through addr 0x1000 ... 0x103F.
1. loop with i=0 and addr 0x1000
2. loop with i=1 and addr 0x1000 + 8
3. loop with i=2 and addr 0x1000 + 8 + 8
4. loop with i=3 and addr 0x1000 + 8 + 8 + 8
5. loop with i=4 and addr 0x1000 + 8 + 8 + 8 + 8
6. loop with i=5 and addr 0x1000 + 8 + 8 + 8 + 8 + 8
7. loop with i=6 and addr 0x1000 + 8 + 8 + 8 + 8 + 8 + 8
8. loop with i=7 and addr 0x1000 + 8 + 8 + 8 + 8 + 8 + 8 + 8

BUT the given code does something different. As the addr is used in the 
loop
to hold the wrapped address this loop in fact does:
1. loop with i=0 and addr 0x1000 - all fine, but as addr = 
wrap_address() at the
    end of the loop now addr is 0x1000 + 0x8000 = 0x9000, so next loop 
starts with
2. loop with i=1 and addr 0x9000 + 8
    and at the end of the loop addr is 0x9008 + 0x8000 = 0x11008 and next 
loop starts
3. loop with i=2 and addr 0x11008 + 8
    ...

So introducing an intermediate variable which holds the wrapped address
makes sure the surrounding addr walk in the loop does as intended.