[PATCH v3 0/5] s390x/tcg: fix booting Linux kernels compiled with clang-11 and clang-12

[PATCH v3 0/5] s390x/tcg: fix booting Linux kernels compiled with clang-11 and clang-12

[David Hildenbrand]

This series fixes booting current upstream Linux kernel compiled by
clang-11 and clang-12 under TCG.

Latest version of the patches available at:
git@github.com:davidhildenbrand/qemu.git clang

v2 -> v3:
- Add 'tests/tcg/s390x: Fix EXRL tests'
-- "make check-tcg" with v2 revealed two buggy tests
- Added RB's/Tested-by's

v1 -> v2:
- Add 's390x/tcg: Don't ignore content in r0 when not specified via "b" or
  "x"'
- Add 's390x/tcg: Ignore register content if b1/b2 is zero when handling
  EXEUTE'
- "s390x/tcg: Fix ALGSI"
-- Fixup subject
- "s390x/tcg: Fix RISBHG"
-- Rephrase description, stating that it fixes clang-11

David Hildenbrand (5):
  s390x/tcg: Fix ALGSI
  s390x/tcg: Fix RISBHG
  s390x/tcg: Don't ignore content in r0 when not specified via "b" or
    "x"
  tests/tcg/s390x: Fix EXRL tests
  s390x/tcg: Ignore register content if b1/b2 is zero when handling
    EXECUTE

 target/s390x/insn-data.def  | 10 +++++-----
 target/s390x/mem_helper.c   |  4 ++--
 target/s390x/translate.c    | 33 +++++++++++++++++----------------
 tests/tcg/s390x/exrl-trt.c  |  8 ++++----
 tests/tcg/s390x/exrl-trtr.c |  8 ++++----
 5 files changed, 32 insertions(+), 31 deletions(-)

-- 
2.29.2

[PATCH v3 1/5] s390x/tcg: Fix ALGSI

[David Hildenbrand]

Looks like something went wrong whiel touching that line. Instead of "r1"
we need a new temporary. Also, we have to pass MO_TEQ, to indicate that
we are working with 64-bit values. Let's revert these changes.

Fixes: ff26d287bddc ("target/s390x: Improve cc computation for ADD LOGICAL")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/insn-data.def | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 26badb663a..eac5136ee5 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -76,7 +76,7 @@
 /* ADD LOGICAL WITH SIGNED IMMEDIATE */
     D(0xeb6e, ALSI,    SIY,   GIE, la1, i2_32u, new, 0, asi, addu32, MO_TEUL)
     C(0xecda, ALHSIK,  RIE_d, DO,  r3_32u, i2_32u, new, r1_32, add, addu32)
-    C(0xeb7e, ALGSI,   SIY,   GIE, la1, i2, r1, 0, asiu64, addu64)
+    D(0xeb7e, ALGSI,   SIY,   GIE, la1, i2, new, 0, asiu64, addu64, MO_TEQ)
     C(0xecdb, ALGHSIK, RIE_d, DO,  r3, i2, r1, 0, addu64, addu64)
 /* ADD LOGICAL WITH SIGNED IMMEDIATE HIGH */
     C(0xcc0a, ALSIH,   RIL_a, HW,  r1_sr32, i2_32u, new, r1_32h, add, addu32)
-- 
2.29.2

[PATCH v3 2/5] s390x/tcg: Fix RISBHG

[David Hildenbrand]

RISBHG is broken and currently hinders clang-11 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.11-rc2 kernel built with
clang-11, and gcc builds keep on working.

Fixes: 2d6a869833d9 ("target-s390: Implement RISBG")
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Tested-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>
---
 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));
     }
+    /* 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

[PATCH v3 3/5] s390x/tcg: Don't ignore content in r0 when not specified via "b" or "x"

[David Hildenbrand]

Using get_address() with register identifiers comming from an "r" field
is wrong: if the "r" field designates "r0", we don't read the content
and instead assume 0 - which should only be applied when the register
was specified via "b" or "x".

PoP 5-11 "Operand-Address Generation":
  "A zero in any of the B1, B2, X2, B3, or B4 fields indicates the absence
   of the corresponding address component. For the absent component, a zero
   is used in forming the intermediate sum, regardless of the contents of
   general register 0. A displacement of zero has no special significance."

This BUG became visible for CSPG as generated by LLVM-12 in the upstream
Linux kernel (v5.11-rc2), used while creating the linear mapping in
vmem_map_init(): Trying to store to address 0 results in a Low Address
Protection exception.

Debugging this was more complicated than it could have been: The program
interrupt handler in the kernel will try to crash the kernel: doing so, it
will enable DAT. As the linear mapping is not created yet (asce=0), we run
into an addressing exception while tring to walk non-existant DAT tables,
resulting in a program exception loop.

This allows for booting upstream Linux kernels compiled by clang-12. Most
of these cases seem to be broken forever.

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/insn-data.def |  8 ++++----
 target/s390x/translate.c   | 15 +++++++++------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index eac5136ee5..e5b6efabf3 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1290,8 +1290,8 @@
     F(0xe313, LRAY,    RXY_a, LD,  0, a2, r1, 0, lra, 0, IF_PRIV)
     F(0xe303, LRAG,    RXY_a, Z,   0, a2, r1, 0, lra, 0, IF_PRIV)
 /* LOAD USING REAL ADDRESS */
-    E(0xb24b, LURA,    RRE,   Z,   0, 0, new, r1_32, lura, 0, MO_TEUL, IF_PRIV)
-    E(0xb905, LURAG,   RRE,   Z,   0, 0, r1, 0, lura, 0, MO_TEQ, IF_PRIV)
+    E(0xb24b, LURA,    RRE,   Z,   0, ra2, new, r1_32, lura, 0, MO_TEUL, IF_PRIV)
+    E(0xb905, LURAG,   RRE,   Z,   0, ra2, r1, 0, lura, 0, MO_TEQ, IF_PRIV)
 /* MOVE TO PRIMARY */
     F(0xda00, MVCP,    SS_d,  Z,   la1, a2, 0, 0, mvcp, 0, IF_PRIV)
 /* MOVE TO SECONDARY */
@@ -1344,8 +1344,8 @@
 /* STORE THEN OR SYSTEM MASK */
     F(0xad00, STOSM,   SI,    Z,   la1, 0, 0, 0, stnosm, 0, IF_PRIV)
 /* STORE USING REAL ADDRESS */
-    E(0xb246, STURA,   RRE,   Z,   r1_o, 0, 0, 0, stura, 0, MO_TEUL, IF_PRIV)
-    E(0xb925, STURG,   RRE,   Z,   r1_o, 0, 0, 0, stura, 0, MO_TEQ, IF_PRIV)
+    E(0xb246, STURA,   RRE,   Z,   r1_o, ra2, 0, 0, stura, 0, MO_TEUL, IF_PRIV)
+    E(0xb925, STURG,   RRE,   Z,   r1_o, ra2, 0, 0, stura, 0, MO_TEQ, IF_PRIV)
 /* TEST BLOCK */
     F(0xb22c, TB,      RRE,   Z,   0, r2_o, 0, 0, testblock, 0, IF_PRIV)
 /* TEST PROTECTION */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 39e33eeb67..61dd0341e4 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3285,8 +3285,7 @@ static DisasJumpType op_lpq(DisasContext *s, DisasOps *o)
 #ifndef CONFIG_USER_ONLY
 static DisasJumpType op_lura(DisasContext *s, DisasOps *o)
 {
-    o->addr1 = get_address(s, 0, get_field(s, r2), 0);
-    tcg_gen_qemu_ld_tl(o->out, o->addr1, MMU_REAL_IDX, s->insn->data);
+    tcg_gen_qemu_ld_tl(o->out, o->in2, MMU_REAL_IDX, s->insn->data);
     return DISAS_NEXT;
 }
 #endif
@@ -4234,7 +4233,8 @@ static DisasJumpType op_ectg(DisasContext *s, DisasOps *o)
     tcg_gen_addi_i64(o->in1, regs[b1], d1);
     o->in2 = tcg_temp_new_i64();
     tcg_gen_addi_i64(o->in2, regs[b2], d2);
-    o->addr1 = get_address(s, 0, r3, 0);
+    o->addr1 = tcg_temp_new_i64();
+    gen_addi_and_wrap_i64(s, o->addr1, regs[r3], 0);
 
     /* load the third operand into r3 before modifying anything */
     tcg_gen_qemu_ld64(regs[r3], o->addr1, get_mem_index(s));
@@ -4539,8 +4539,7 @@ static DisasJumpType op_stnosm(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_stura(DisasContext *s, DisasOps *o)
 {
-    o->addr1 = get_address(s, 0, get_field(s, r2), 0);
-    tcg_gen_qemu_st_tl(o->in1, o->addr1, MMU_REAL_IDX, s->insn->data);
+    tcg_gen_qemu_st_tl(o->in1, o->in2, MMU_REAL_IDX, s->insn->data);
 
     if (s->base.tb->flags & FLAG_MASK_PER) {
         update_psw_addr(s);
@@ -5923,7 +5922,11 @@ static void in2_x2l(DisasContext *s, DisasOps *o)
 
 static void in2_ra2(DisasContext *s, DisasOps *o)
 {
-    o->in2 = get_address(s, 0, get_field(s, r2), 0);
+    int r2 = get_field(s, r2);
+
+    /* Note: *don't* treat !r2 as 0, use the reg value. */
+    o->in2 = tcg_temp_new_i64();
+    gen_addi_and_wrap_i64(s, o->in2, regs[r2], 0);
 }
 #define SPEC_in2_ra2 0
 
-- 
2.29.2

[PATCH v3 4/5] tests/tcg/s390x: Fix EXRL tests

[David Hildenbrand]

The current EXRL tests crash on real machines: we must not use r0 as a base
register for trt/trtr, otherwise the content gets ignored. Also, we must
not use r0 for exrl, otherwise it gets ignored.

Let's use the "a" constraint so we get a general purpose register != r0.
For op2, we can simply specify a memory operand directly via "Q" (Memory
reference without index register and with short displacement).

Fixes: ad8c851d2e77 ("target/s390x: add EX support for TRT and TRTR")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 tests/tcg/s390x/exrl-trt.c  | 8 ++++----
 tests/tcg/s390x/exrl-trtr.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/tcg/s390x/exrl-trt.c b/tests/tcg/s390x/exrl-trt.c
index 3c5323aecb..16711a3181 100644
--- a/tests/tcg/s390x/exrl-trt.c
+++ b/tests/tcg/s390x/exrl-trt.c
@@ -19,7 +19,7 @@ int main(void)
     }
     asm volatile(
         "    j 2f\n"
-        "1:  trt 0(1,%[op1]),0(%[op2])\n"
+        "1:  trt 0(1,%[op1]),%[op2]\n"
         "2:  exrl %[op1_len],1b\n"
         "    lgr %[r1],%%r1\n"
         "    lgr %[r2],%%r2\n"
@@ -27,9 +27,9 @@ int main(void)
         : [r1] "+r" (r1),
           [r2] "+r" (r2),
           [cc] "=r" (cc)
-        : [op1] "r" (&op1),
-          [op1_len] "r" (5),
-          [op2] "r" (&op2)
+        : [op1] "a" (&op1),
+          [op1_len] "a" (5),
+          [op2] "Q" (op2)
         : "r1", "r2", "cc");
     cc = (cc >> 28) & 3;
     if (cc != 2) {
diff --git a/tests/tcg/s390x/exrl-trtr.c b/tests/tcg/s390x/exrl-trtr.c
index c33153ad7e..5f30cda6bd 100644
--- a/tests/tcg/s390x/exrl-trtr.c
+++ b/tests/tcg/s390x/exrl-trtr.c
@@ -19,7 +19,7 @@ int main(void)
     }
     asm volatile(
         "    j 2f\n"
-        "1:  trtr 3(1,%[op1]),0(%[op2])\n"
+        "1:  trtr 3(1,%[op1]),%[op2]\n"
         "2:  exrl %[op1_len],1b\n"
         "    lgr %[r1],%%r1\n"
         "    lgr %[r2],%%r2\n"
@@ -27,9 +27,9 @@ int main(void)
         : [r1] "+r" (r1),
           [r2] "+r" (r2),
           [cc] "=r" (cc)
-        : [op1] "r" (&op1),
-          [op1_len] "r" (3),
-          [op2] "r" (&op2)
+        : [op1] "a" (&op1),
+          [op1_len] "a" (3),
+          [op2] "Q" (op2)
         : "r1", "r2", "cc");
     cc = (cc >> 28) & 3;
     if (cc != 1) {
-- 
2.29.2

Re: [PATCH v3 4/5] tests/tcg/s390x: Fix EXRL tests

[Thomas Huth]

On 11/01/2021 17.38, David Hildenbrand wrote:
> The current EXRL tests crash on real machines: we must not use r0 as a base
> register for trt/trtr, otherwise the content gets ignored. Also, we must
> not use r0 for exrl, otherwise it gets ignored.
> 
> Let's use the "a" constraint so we get a general purpose register != r0.
> For op2, we can simply specify a memory operand directly via "Q" (Memory
> reference without index register and with short displacement).
> 
> Fixes: ad8c851d2e77 ("target/s390x: add EX support for TRT and TRTR")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   tests/tcg/s390x/exrl-trt.c  | 8 ++++----
>   tests/tcg/s390x/exrl-trtr.c | 8 ++++----
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/tcg/s390x/exrl-trt.c b/tests/tcg/s390x/exrl-trt.c
> index 3c5323aecb..16711a3181 100644
> --- a/tests/tcg/s390x/exrl-trt.c
> +++ b/tests/tcg/s390x/exrl-trt.c
> @@ -19,7 +19,7 @@ int main(void)
>       }
>       asm volatile(
>           "    j 2f\n"
> -        "1:  trt 0(1,%[op1]),0(%[op2])\n"
> +        "1:  trt 0(1,%[op1]),%[op2]\n"
>           "2:  exrl %[op1_len],1b\n"
>           "    lgr %[r1],%%r1\n"
>           "    lgr %[r2],%%r2\n"
> @@ -27,9 +27,9 @@ int main(void)
>           : [r1] "+r" (r1),
>             [r2] "+r" (r2),
>             [cc] "=r" (cc)
> -        : [op1] "r" (&op1),
> -          [op1_len] "r" (5),
> -          [op2] "r" (&op2)
> +        : [op1] "a" (&op1),
> +          [op1_len] "a" (5),

I think op1_len could still stay with "r" instead of "a" ... OTOH "a" also 
does not hurt here, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 4/5] tests/tcg/s390x: Fix EXRL tests

[David Hildenbrand]

> Am 12.01.2021 um 08:41 schrieb Thomas Huth <thuth@redhat.com>:
> 
> On 11/01/2021 17.38, David Hildenbrand wrote:
>> The current EXRL tests crash on real machines: we must not use r0 as a base
>> register for trt/trtr, otherwise the content gets ignored. Also, we must
>> not use r0 for exrl, otherwise it gets ignored.
>> Let's use the "a" constraint so we get a general purpose register != r0.
>> For op2, we can simply specify a memory operand directly via "Q" (Memory
>> reference without index register and with short displacement).
>> Fixes: ad8c851d2e77 ("target/s390x: add EX support for TRT and TRTR")
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  tests/tcg/s390x/exrl-trt.c  | 8 ++++----
>>  tests/tcg/s390x/exrl-trtr.c | 8 ++++----
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>> diff --git a/tests/tcg/s390x/exrl-trt.c b/tests/tcg/s390x/exrl-trt.c
>> index 3c5323aecb..16711a3181 100644
>> --- a/tests/tcg/s390x/exrl-trt.c
>> +++ b/tests/tcg/s390x/exrl-trt.c
>> @@ -19,7 +19,7 @@ int main(void)
>>      }
>>      asm volatile(
>>          "    j 2f\n"
>> -        "1:  trt 0(1,%[op1]),0(%[op2])\n"
>> +        "1:  trt 0(1,%[op1]),%[op2]\n"
>>          "2:  exrl %[op1_len],1b\n"
>>          "    lgr %[r1],%%r1\n"
>>          "    lgr %[r2],%%r2\n"
>> @@ -27,9 +27,9 @@ int main(void)
>>          : [r1] "+r" (r1),
>>            [r2] "+r" (r2),
>>            [cc] "=r" (cc)
>> -        : [op1] "r" (&op1),
>> -          [op1_len] "r" (5),
>> -          [op2] "r" (&op2)
>> +        : [op1] "a" (&op1),
>> +          [op1_len] "a" (5),
> 
> I think op1_len could still stay with "r" instead of "a" ... OTOH "a" also does not hurt here, so:
> 

No, otherwise exrl ignores the register content  if it ends up being r0.

Thanks!


> Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 4/5] tests/tcg/s390x: Fix EXRL tests

[Thomas Huth]

On 12/01/2021 08.47, David Hildenbrand wrote:
> 
>> Am 12.01.2021 um 08:41 schrieb Thomas Huth <thuth@redhat.com>:
>>
>> On 11/01/2021 17.38, David Hildenbrand wrote:
>>> The current EXRL tests crash on real machines: we must not use r0 as a base
>>> register for trt/trtr, otherwise the content gets ignored. Also, we must
>>> not use r0 for exrl, otherwise it gets ignored.
>>> Let's use the "a" constraint so we get a general purpose register != r0.
>>> For op2, we can simply specify a memory operand directly via "Q" (Memory
>>> reference without index register and with short displacement).
>>> Fixes: ad8c851d2e77 ("target/s390x: add EX support for TRT and TRTR")
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   tests/tcg/s390x/exrl-trt.c  | 8 ++++----
>>>   tests/tcg/s390x/exrl-trtr.c | 8 ++++----
>>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>> diff --git a/tests/tcg/s390x/exrl-trt.c b/tests/tcg/s390x/exrl-trt.c
>>> index 3c5323aecb..16711a3181 100644
>>> --- a/tests/tcg/s390x/exrl-trt.c
>>> +++ b/tests/tcg/s390x/exrl-trt.c
>>> @@ -19,7 +19,7 @@ int main(void)
>>>       }
>>>       asm volatile(
>>>           "    j 2f\n"
>>> -        "1:  trt 0(1,%[op1]),0(%[op2])\n"
>>> +        "1:  trt 0(1,%[op1]),%[op2]\n"
>>>           "2:  exrl %[op1_len],1b\n"
>>>           "    lgr %[r1],%%r1\n"
>>>           "    lgr %[r2],%%r2\n"
>>> @@ -27,9 +27,9 @@ int main(void)
>>>           : [r1] "+r" (r1),
>>>             [r2] "+r" (r2),
>>>             [cc] "=r" (cc)
>>> -        : [op1] "r" (&op1),
>>> -          [op1_len] "r" (5),
>>> -          [op2] "r" (&op2)
>>> +        : [op1] "a" (&op1),
>>> +          [op1_len] "a" (5),
>>
>> I think op1_len could still stay with "r" instead of "a" ... OTOH "a" also does not hurt here, so:
>>
> 
> No, otherwise exrl ignores the register content  if it ends up being r0.

Ah, well, sorry, I've got fooled by the description of "EXECUTE RELATIVE 
LONG" in the Principles of Operation since it is talking about "R1" and not 
"B" there ... but you're right, the detailed description there then talks 
about "When the R1 field is not zero ...", so we need the "a" instead of the 
"r" for op1_len here indeed.

  Thomas

Re: [PATCH v3 4/5] tests/tcg/s390x: Fix EXRL tests

[David Hildenbrand]

On 12.01.21 09:16, Thomas Huth wrote:
> On 12/01/2021 08.47, David Hildenbrand wrote:
>>
>>> Am 12.01.2021 um 08:41 schrieb Thomas Huth <thuth@redhat.com>:
>>>
>>> On 11/01/2021 17.38, David Hildenbrand wrote:
>>>> The current EXRL tests crash on real machines: we must not use r0 as a base
>>>> register for trt/trtr, otherwise the content gets ignored. Also, we must
>>>> not use r0 for exrl, otherwise it gets ignored.
>>>> Let's use the "a" constraint so we get a general purpose register != r0.
>>>> For op2, we can simply specify a memory operand directly via "Q" (Memory
>>>> reference without index register and with short displacement).
>>>> Fixes: ad8c851d2e77 ("target/s390x: add EX support for TRT and TRTR")
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>   tests/tcg/s390x/exrl-trt.c  | 8 ++++----
>>>>   tests/tcg/s390x/exrl-trtr.c | 8 ++++----
>>>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>>> diff --git a/tests/tcg/s390x/exrl-trt.c b/tests/tcg/s390x/exrl-trt.c
>>>> index 3c5323aecb..16711a3181 100644
>>>> --- a/tests/tcg/s390x/exrl-trt.c
>>>> +++ b/tests/tcg/s390x/exrl-trt.c
>>>> @@ -19,7 +19,7 @@ int main(void)
>>>>       }
>>>>       asm volatile(
>>>>           "    j 2f\n"
>>>> -        "1:  trt 0(1,%[op1]),0(%[op2])\n"
>>>> +        "1:  trt 0(1,%[op1]),%[op2]\n"
>>>>           "2:  exrl %[op1_len],1b\n"
>>>>           "    lgr %[r1],%%r1\n"
>>>>           "    lgr %[r2],%%r2\n"
>>>> @@ -27,9 +27,9 @@ int main(void)
>>>>           : [r1] "+r" (r1),
>>>>             [r2] "+r" (r2),
>>>>             [cc] "=r" (cc)
>>>> -        : [op1] "r" (&op1),
>>>> -          [op1_len] "r" (5),
>>>> -          [op2] "r" (&op2)
>>>> +        : [op1] "a" (&op1),
>>>> +          [op1_len] "a" (5),
>>>
>>> I think op1_len could still stay with "r" instead of "a" ... OTOH "a" also does not hurt here, so:
>>>
>>
>> No, otherwise exrl ignores the register content  if it ends up being r0.
> 
> Ah, well, sorry, I've got fooled by the description of "EXECUTE RELATIVE 
> LONG" in the Principles of Operation since it is talking about "R1" and not 
> "B" there ... but you're right, the detailed description there then talks 
> about "When the R1 field is not zero ...", so we need the "a" instead of the 
> "r" for op1_len here indeed.

I actually stumbled over that while fixing the test. Converting op1 and
op2 suddenly made the test fail with "bad cc" instead of segfault. The
compiler decided to use r0 for op1_len when not being able to use it for
op1 and op2 ... :)


-- 
Thanks,

David / dhildenb

[PATCH v3 5/5] s390x/tcg: Ignore register content if b1/b2 is zero when handling EXECUTE

[David Hildenbrand]

In our EXECUTE fast path, we have to ignore the content of r0, if
specified by b1 or b2.

Fixes: d376f123c7de ("target/s390x: Re-implement a few EXECUTE target insns directly")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mem_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 0108611cc9..1901e9dfc7 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -2473,8 +2473,8 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr)
             uint32_t d1 = extract64(insn, 32, 12);
             uint32_t b2 = extract64(insn, 28, 4);
             uint32_t d2 = extract64(insn, 16, 12);
-            uint64_t a1 = wrap_address(env, env->regs[b1] + d1);
-            uint64_t a2 = wrap_address(env, env->regs[b2] + d2);
+            uint64_t a1 = wrap_address(env, (b1 ? env->regs[b1] : 0) + d1);
+            uint64_t a2 = wrap_address(env, (b2 ? env->regs[b2] : 0) + d2);
 
             env->cc_op = helper(env, l, a1, a2, 0);
             env->psw.addr += ilen;
-- 
2.29.2

Re: [PATCH v3 0/5] s390x/tcg: fix booting Linux kernels compiled with clang-11 and clang-12

[Cornelia Huck]

On Mon, 11 Jan 2021 17:38:40 +0100
David Hildenbrand <david@redhat.com> wrote:

> This series fixes booting current upstream Linux kernel compiled by
> clang-11 and clang-12 under TCG.
> 
> Latest version of the patches available at:
> git@github.com:davidhildenbrand/qemu.git clang
> 
> v2 -> v3:
> - Add 'tests/tcg/s390x: Fix EXRL tests'
> -- "make check-tcg" with v2 revealed two buggy tests
> - Added RB's/Tested-by's
> 
> v1 -> v2:
> - Add 's390x/tcg: Don't ignore content in r0 when not specified via "b" or
>   "x"'
> - Add 's390x/tcg: Ignore register content if b1/b2 is zero when handling
>   EXEUTE'
> - "s390x/tcg: Fix ALGSI"
> -- Fixup subject
> - "s390x/tcg: Fix RISBHG"
> -- Rephrase description, stating that it fixes clang-11
> 
> David Hildenbrand (5):
>   s390x/tcg: Fix ALGSI
>   s390x/tcg: Fix RISBHG
>   s390x/tcg: Don't ignore content in r0 when not specified via "b" or
>     "x"
>   tests/tcg/s390x: Fix EXRL tests
>   s390x/tcg: Ignore register content if b1/b2 is zero when handling
>     EXECUTE
> 
>  target/s390x/insn-data.def  | 10 +++++-----
>  target/s390x/mem_helper.c   |  4 ++--
>  target/s390x/translate.c    | 33 +++++++++++++++++----------------
>  tests/tcg/s390x/exrl-trt.c  |  8 ++++----
>  tests/tcg/s390x/exrl-trtr.c |  8 ++++----
>  5 files changed, 32 insertions(+), 31 deletions(-)
> 

Thanks, applied.