[PATCH 1/2] target/s390x: Define TARGET_HAS_PRECISE_SMC

[PATCH 1/2] target/s390x: Define TARGET_HAS_PRECISE_SMC

[Ilya Leoshkevich]

PoP (Sequence of Storage References -> Instruction Fetching) says:

    ... if a store that is conceptually earlier is
    made by the same CPU using the same effective
    address as that by which the instruction is subse-
    quently fetched, the updated information is obtained ...

QEMU already has support for this in the common code; enable it for
s390x.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/cpu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index eb5b65b7d3a..304029e57cf 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -36,6 +36,8 @@
 /* The z/Architecture has a strong memory model with some store-after-load re-ordering */
 #define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
 
+#define TARGET_HAS_PRECISE_SMC
+
 #define TARGET_INSN_START_EXTRA_WORDS 2
 
 #define MMU_USER_IDX 0
-- 
2.41.0

[PATCH 2/2] tests/tcg/s390x: Test precise self-modifying code handling

[Ilya Leoshkevich]

Add small softmmu and user tests to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.softmmu-target |  1 +
 tests/tcg/s390x/Makefile.target         |  1 +
 tests/tcg/s390x/precise-smc-softmmu.S   | 63 +++++++++++++++++++++++++
 tests/tcg/s390x/precise-smc-user.c      | 39 +++++++++++++++
 4 files changed, 104 insertions(+)
 create mode 100644 tests/tcg/s390x/precise-smc-softmmu.S
 create mode 100644 tests/tcg/s390x/precise-smc-user.c

diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/Makefile.softmmu-target
index 76345b6e643..1a1f088b280 100644
--- a/tests/tcg/s390x/Makefile.softmmu-target
+++ b/tests/tcg/s390x/Makefile.softmmu-target
@@ -25,6 +25,7 @@ ASM_TESTS =                                                                    \
     lpswe-early                                                                \
     lra                                                                        \
     mc                                                                         \
+    precise-smc-softmmu                                                        \
     ssm-early                                                                  \
     stosm-early                                                                \
     stpq                                                                       \
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 4f3793532bf..b5f88c20fc6 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -60,6 +60,7 @@ Z13_TESTS+=lcbb
 Z13_TESTS+=locfhr
 Z13_TESTS+=vcksm
 Z13_TESTS+=vstl
+Z13_TESTS+=precise-smc-user
 $(Z13_TESTS): CFLAGS+=-march=z13 -O2
 TESTS+=$(Z13_TESTS)
 
diff --git a/tests/tcg/s390x/precise-smc-softmmu.S b/tests/tcg/s390x/precise-smc-softmmu.S
new file mode 100644
index 00000000000..f7fa57d899d
--- /dev/null
+++ b/tests/tcg/s390x/precise-smc-softmmu.S
@@ -0,0 +1,63 @@
+/*
+ * Test s390x-softmmu precise self-modifying code handling.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+    .org 0x8e
+program_interruption_code:
+    .org 0x150
+program_old_psw:
+    .org 0x1D0                         /* program new PSW */
+    .quad 0x180000000,pgm              /* 64-bit mode */
+    .org 0x200                         /* lowcore padding */
+    .globl _start
+_start:
+    lctlg %c0,%c0,c0
+    lghi %r0,15
+
+    /* Test 1: replace sgr with agr. */
+    lghi %r1,21
+    vl %v0,patch1
+    jg 1f                              /* start a new TB */
+0:
+    .org . + 6                         /* pad patched code to 16 bytes */
+1:
+    vstl %v0,%r0,0b                    /* start writing before TB */
+    sgr %r1,%r1                        /* this becomes `agr %r1,%r1` */
+    cgijne %r1,42,failure
+
+    /* Test 2: replace agr with division by zero. */
+    vl %v0,patch2
+    jg 1f                              /* start a new TB */
+0:
+    .org . + 6                         /* pad patched code to 16 bytes */
+1:
+    vstl %v0,%r0,0b                    /* start writing before TB */
+    sgr %r1,%r1                        /* this becomes `d %r0,zero` */
+failure:
+    lpswe failure_psw
+
+pgm:
+    chhsi program_interruption_code,0x9          /* divide exception? */
+    jne failure
+    clc program_old_psw(16),expected_old_psw2    /* correct old PSW? */
+    jne failure
+    lpswe success_psw
+
+patch1:
+    .fill 12                           /* replaces padding and stpq */
+    agr %r1,%r1                        /* replaces sgr */
+patch2:
+    .fill 12                           /* replaces padding and stpq */
+    d %r0,zero                         /* replaces sgr */
+zero:
+    .long 0
+expected_old_psw2:
+    .quad 0x200180000000,failure       /* cc is from addition */
+    .align 8
+c0:
+    .quad 0x60000                      /* AFP, VX */
+success_psw:
+    .quad 0x2000000000000,0xfff        /* see is_special_wait_psw() */
+failure_psw:
+    .quad 0x2000000000000,0            /* disabled wait */
diff --git a/tests/tcg/s390x/precise-smc-user.c b/tests/tcg/s390x/precise-smc-user.c
new file mode 100644
index 00000000000..33a5270865c
--- /dev/null
+++ b/tests/tcg/s390x/precise-smc-user.c
@@ -0,0 +1,39 @@
+/*
+ * Test s390x-linux-user precise self-modifying code handling.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <sys/mman.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+extern __uint128_t __attribute__((__aligned__(1))) smc;
+extern __uint128_t __attribute__((__aligned__(1))) patch;
+
+int main(void)
+{
+    char *aligned_smc = (char *)((uintptr_t)&smc & ~0xFFFULL);
+    char *smc_end = (char *)&smc + sizeof(smc);
+    uint64_t value = 21;
+    int err;
+
+    err = mprotect(aligned_smc, smc_end - aligned_smc,
+                   PROT_READ | PROT_WRITE | PROT_EXEC);
+    assert(err == 0);
+
+    asm("jg 0f\n"                           /* start a new TB */
+        "patch: .byte 0,0,0,0,0,0\n"        /* replaces padding */
+        ".byte 0,0,0,0,0,0\n"               /* replaces vstl */
+        "agr %[value],%[value]\n"           /* replaces sgr */
+        "smc: .org . + 6\n"                 /* pad patched code to 16 bytes */
+        "0: vstl %[patch],%[idx],%[smc]\n"  /* start writing before TB */
+        "sgr %[value],%[value]"             /* this becomes `agr %r0,%r0` */
+        : [smc] "=R" (smc)
+        , [value] "+r" (value)
+        : [patch] "v" (patch)
+        , [idx] "r" (sizeof(patch) - 1)
+        : "cc");
+
+    return value == 42 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
-- 
2.41.0

Re: [PATCH 1/2] target/s390x: Define TARGET_HAS_PRECISE_SMC

[David Hildenbrand]

On 07.08.23 13:48, Ilya Leoshkevich wrote:
> PoP (Sequence of Storage References -> Instruction Fetching) says:
> 
>      ... if a store that is conceptually earlier is
>      made by the same CPU using the same effective
>      address as that by which the instruction is subse-
>      quently fetched, the updated information is obtained ...
> 
> QEMU already has support for this in the common code; enable it for
> s390x.


Figuring out what TARGET_HAS_PRECISE_SMC is all about, I only learned 
from git history

commit d720b93d0bcfe1beb729245b9ed1e5f071a24bd5
Author: Fabrice Bellard <fabrice@bellard.org>
Date:   Sun Apr 25 17:57:43 2004 +0000

     precise self modifying code support


     git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@745 
c046a42c-6fe2-441c-8c8c-71466251a162



AFAIU, precise SMC is stricter compared to what we have right now. So i 
suspect that this patch is actually fixing SMC behavior: for example, 
when a basic block ends up modifying itself.

Were there any BUG reports? (does patch #2 test for that and can 
reproduce the original issue?)

> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/cpu.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index eb5b65b7d3a..304029e57cf 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -36,6 +36,8 @@
>   /* The z/Architecture has a strong memory model with some store-after-load re-ordering */
>   #define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
>   
> +#define TARGET_HAS_PRECISE_SMC
> +
>   #define TARGET_INSN_START_EXTRA_WORDS 2
>   
>   #define MMU_USER_IDX 0

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] target/s390x: Define TARGET_HAS_PRECISE_SMC

[Ilya Leoshkevich]

On Mon, 2023-08-07 at 17:31 +0200, David Hildenbrand wrote:
> On 07.08.23 13:48, Ilya Leoshkevich wrote:
> > PoP (Sequence of Storage References -> Instruction Fetching) says:
> > 
> >      ... if a store that is conceptually earlier is
> >      made by the same CPU using the same effective
> >      address as that by which the instruction is subse-
> >      quently fetched, the updated information is obtained ...
> > 
> > QEMU already has support for this in the common code; enable it for
> > s390x.
> 
> 
> Figuring out what TARGET_HAS_PRECISE_SMC is all about, I only learned
> from git history
> 
> commit d720b93d0bcfe1beb729245b9ed1e5f071a24bd5
> Author: Fabrice Bellard <fabrice@bellard.org>
> Date:   Sun Apr 25 17:57:43 2004 +0000
> 
>      precise self modifying code support
> 
> 
>      git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@745 
> c046a42c-6fe2-441c-8c8c-71466251a162
> 
> 
> 
> AFAIU, precise SMC is stricter compared to what we have right now. So
> i 
> suspect that this patch is actually fixing SMC behavior: for example,
> when a basic block ends up modifying itself.
> 
> Were there any BUG reports? (does patch #2 test for that and can 
> reproduce the original issue?)

There were no bug reports, I found this issue with fuzzing.

Patch #2 tests a TB modifying itself.
Reverting this commit makes the test fail.

[...]

Re: [PATCH 1/2] target/s390x: Define TARGET_HAS_PRECISE_SMC

[David Hildenbrand]

On 07.08.23 18:13, Ilya Leoshkevich wrote:
> On Mon, 2023-08-07 at 17:31 +0200, David Hildenbrand wrote:
>> On 07.08.23 13:48, Ilya Leoshkevich wrote:
>>> PoP (Sequence of Storage References -> Instruction Fetching) says:
>>>
>>>       ... if a store that is conceptually earlier is
>>>       made by the same CPU using the same effective
>>>       address as that by which the instruction is subse-
>>>       quently fetched, the updated information is obtained ...
>>>
>>> QEMU already has support for this in the common code; enable it for
>>> s390x.
>>
>>
>> Figuring out what TARGET_HAS_PRECISE_SMC is all about, I only learned
>> from git history
>>
>> commit d720b93d0bcfe1beb729245b9ed1e5f071a24bd5
>> Author: Fabrice Bellard <fabrice@bellard.org>
>> Date:   Sun Apr 25 17:57:43 2004 +0000
>>
>>       precise self modifying code support
>>
>>
>>       git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@745
>> c046a42c-6fe2-441c-8c8c-71466251a162
>>
>>
>>
>> AFAIU, precise SMC is stricter compared to what we have right now. So
>> i
>> suspect that this patch is actually fixing SMC behavior: for example,
>> when a basic block ends up modifying itself.
>>
>> Were there any BUG reports? (does patch #2 test for that and can
>> reproduce the original issue?)
> 
> There were no bug reports, I found this issue with fuzzing.
> 
> Patch #2 tests a TB modifying itself.
> Reverting this commit makes the test fail.
> 
> [...]
> 

Cool, thanks. Worth adding to the patch description.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb