Re: [PATCH v1] s390x/tcg: Fix RISBHG

[David Hildenbrand <dhildenb@redhat.com>]

> Am 08.01.2021 um 00:21 schrieb Nick Desaulniers <ndesaulniers@google.com>:
> 
> On Thu, Jan 7, 2021 at 3:13 PM David Hildenbrand <david@redhat.com> wrote:
>> 
>> RISBHG is broken and currently hinders clang builds of upstream kernels
>> from booting: the kernel crashes early, while decompressing the image.
>> 
>>  [...]
>>   Kernel fault: interruption code 0005 ilc:2
>>   Kernel random base: 0000000000000000
>>   PSW : 0000200180000000 0000000000017a1e
>>         R:0 T:0 IO:0 EX:0 Key:0 M:0 W:0 P:0 AS:0 CC:2 PM:0 RI:0 EA:3
>>   GPRS: 0000000000000001 0000000c00000000 00000003fffffff4 00000000fffffff0
>>         0000000000000000 00000000fffffff4 000000000000000c 00000000fffffff0
>>         00000000fffffffc 0000000000000000 00000000fffffff8 00000000008e25a8
>>         0000000000000009 0000000000000002 0000000000000008 000000000000bce0
>> 
>> One example of a buggy instruction is:
>> 
>>    17dde:       ec 1e 00 9f 20 5d       risbhg  %r1,%r14,0,159,32
>> 
>> With %r14 = 0x9 and %r1 = 0x7 should result in %r1 = 0x900000007, however,
>> results in %r1 = 0.
>> 
>> Let's interpret values of i3/i4 as documented in the PoP and make
>> computation of "mask" only based on i3 and i4 and use "pmask" only at the
>> very end to make sure wrapping is only applied to the high/low doubleword.
>> 
>> With this patch, I can successfully boot a v5.10 kernel built with
>> clang, and gcc builds keep on working.
>> 
>> Fixes: 2d6a869833d9 ("target-s390: Implement RISBG")
>> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> 
>> This BUG was a nightmare to debug and the code a nightmare to understand.
>> 
>> To make clang/gcc builds boot, the following fix is required as well on
>> top of current master: "[PATCH] target/s390x: Fix ALGSI"
>> https://lkml.kernel.org/r/20210107202135.52379-1-david@redhat.com
> 
> In that case, a huge thank you!!! for this work! ++beers_owed.
> 

:) a kernel build for z13 should work with the (default) „-cpu qemu“ cpu type.

>> 
>> ---
>> target/s390x/translate.c | 18 ++++++++----------
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>> 
>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
>> index 3d5c0d6106..39e33eeb67 100644
>> --- a/target/s390x/translate.c
>> +++ b/target/s390x/translate.c
>> @@ -3815,22 +3815,23 @@ static DisasJumpType op_risbg(DisasContext *s, DisasOps *o)
>>         pmask = 0xffffffff00000000ull;
>>         break;
>>     case 0x51: /* risblg */
>> -        i3 &= 31;
>> -        i4 &= 31;
>> +        i3 = (i3 & 31) + 32;
>> +        i4 = (i4 & 31) + 32;
>>         pmask = 0x00000000ffffffffull;
>>         break;
>>     default:
>>         g_assert_not_reached();
>>     }
>> 
>> -    /* MASK is the set of bits to be inserted from R2.
>> -       Take care for I3/I4 wraparound.  */
>> -    mask = pmask >> i3;
>> +    /* MASK is the set of bits to be inserted from R2. */
>>     if (i3 <= i4) {
>> -        mask ^= pmask >> i4 >> 1;
>> +        /* [0...i3---i4...63] */
>> +        mask = (-1ull >> i3) & (-1ull << (63 - i4));
>>     } else {
>> -        mask |= ~(pmask >> i4 >> 1);
>> +        /* [0---i4...i3---63] */
>> +        mask = (-1ull >> i3) | (-1ull << (63 - i4));
>>     }
> 
> The expression evaluated looks the same to me for both sides of the
> conditional, but the comments differ. Intentional?

& vs |, so the result differs.

Cheers!