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

[Nick Desaulniers]

On Thu, Jan 7, 2021 at 3:27 PM David Hildenbrand <dhildenb@redhat.com> wrote:
>
>
> > 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"
> >> 20210107202135.52379-1-david@redhat.com">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.

Hmm...so I don't think clang can build a Linux kernel image with
CONFIG_MARCH_Z13=y just yet; just defconfig.  Otherwise looks like
clang barfs on some of the inline asm constraints.

It looks like with your patch applied we get further into the boot!
I'm not seeing any output with:
$ /android0/qemu/build/qemu-system-s390x -cpu qemu -append
'conmode=sclp console=ttyS0' -display none -initrd
/<path/to>/boot-utils/images/s390/rootfs.cpio -kernel
arch/s390/boot/bzImage -m 512m -nodefaults -serial mon:stdio

(Based on a quick skim through
https://www.ibm.com/support/knowledgecenter/en/linuxonibm/com.ibm.linux.z.ludd/ludd_r_lmtkernelparameter.html).
Do I have all of those right?

If I attach GDB to QEMU running that kernel image, I was able to view
the print banner once via `lx-dmesg` gdb macro in the kernel, but it
seems on subsequent runs control flow gets diverted unexpected post
entry to start_kernel() always to `s390_base_pgm_handler` ...errr..at
least when I try to single step in GDB.  Tried with linux-5.10.y,
mainline, and linux-next.

qemu: 470dd6bd360782f5137f7e3376af6a44658eb1d3 + your patch
llvm: 106e66f3f555c8f887e82c5f04c3e77bdaf345e8
linux-5.10.y: d1988041d19dc8b532579bdbb7c4a978391c0011
linux: 71c061d2443814de15e177489d5cc00a4a253ef3
linux-next: f87684f6470f5f02bd47d4afb900366e5d2f31b6


(gdb) hbreak setup_arch
Hardware assisted breakpoint 1 at 0x142229e: file
arch/s390/kernel/setup.c, line 1091.
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00000000014222a0 in setup_arch (cmdline_p=0x11d7ed8) at
arch/s390/kernel/setup.c:1091
1091            if (MACHINE_IS_VM)
(gdb) lx-dmesg
[    0.376351] Linux version 5.11.0-rc2-00157-ga2885c701c30
(ndesaulniers@ndesaulniers1.mtv.corp.google.com) (Nick Desaulniers
clang version 12.0.0 (git@github.com:llvm/llvm-project.git
e75fec2b238f0e26cfb7645f2208baebe3440d41), GNU ld (GNU Binutils for
Debian) 2.35.1) #81 SMP Thu Jan 7 17:57:34 PST 2021

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

D'oh, I can never spot single character differences!
-- 
Thanks,
~Nick Desaulniers