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

[Nick Desaulniers via <qemu-devel@nongnu.org>]

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.

>
> ---
>  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?

> +    /* For RISBLG/RISBHG, the wrapping is limited to the high/low doubleword. */
>      mask &= pmask;
>
>      /* IMASK is the set of bits to be kept from R1.  In the case of the high/low
> @@ -3843,9 +3844,6 @@ static DisasJumpType op_risbg(DisasContext *s, DisasOps *o)
>      len = i4 - i3 + 1;
>      pos = 63 - i4;
>      rot = i5 & 63;
> -    if (s->fields.op2 == 0x5d) {
> -        pos += 32;
> -    }
>
>      /* In some cases we can implement this with extract.  */
>      if (imask == 0 && pos == 0 && len > 0 && len <= rot) {
> --
> 2.29.2
>


-- 
Thanks,
~Nick Desaulniers