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

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

On 2026-06-30 16:19, Philippe Mathieu-Daudé wrote:
> On 30/6/26 12:54, Ilya Leoshkevich wrote:
>> 
>> 
>> 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;
>>          }
> 
>            bool wrap32 = env->psw.mask & PSW_MASK_32;
>            a = extract64(a, 0, wrap32 ? 31 : 24);
>>      }
>>      return a;
>> }
>> 
>> and I believe in this specific case the order doesn't matter.
>> 

Hm, however a wrap_address() function may return whatever
wrapping address calculation may be. A caller should not
rely on the returned address being 'the same' as the address
given in. So using separate variables for the address to
wrap and the wrapped address is wise in my opinion.