On Thu, Jan 7, 2021 at 3:13 PM David Hildenbrand <da...@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 <ndesaulni...@google.com>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Cc: Christian Borntraeger <borntrae...@de.ibm.com>
> Signed-off-by: David Hildenbrand <da...@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-da...@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
