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

[Ilya Leoshkevich <iii@linux.ibm.com>]

On 6/30/26 12:23, Harald Freudenberger wrote:
> 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.

Ah, now I see where we were talking past each other, thanks for the
clarification. You assume that wrap_address() may do arbitrary
transformations, like addition in your example above. But today it does
only bitwise "and", as defined by the architecture:

static inline uint64_t wrap_address(CPUS390XState *env, uint64_t a)
{
     if (!(env->psw.mask & PSW_MASK_64)) {
         if (!(env->psw.mask & PSW_MASK_32)) {
             /* 24-Bit mode */
             a &= 0x00ffffff;
         } else {
             /* 31-Bit mode */
             a &= 0x7fffffff;
         }
     }
     return a;
}

and I believe in this specific case the order doesn't matter.