[PATCH 0/2] linux-user/s390x: Fix single-stepping SVC

[PATCH 0/2] linux-user/s390x: Fix single-stepping SVC

[Ilya Leoshkevich]

Hi,

I noticed that single-stepping SVC runs two instructions instead of
one. The reason is that EXCP_SVC masks EXCP_DEBUG.
Patch 1 fixes this problem, patch 2 adds a test.

Btw, there is at least one more problem in that area, namely
single-stepping instructions that cause e.g. SIGILL. Using the
existing signals-s390x test as an example:

    (gdb) x/i $pc
    => 0x1001740 <illegal_op>:      .long   0x000007fe

    (gdb) si
    Program received signal SIGILL, Illegal instruction.
    (gdb) x/i $pc
    => 0x1001742 <after_illegal_op>:        br      %r14
    # So far so good.

    (gdb) si
    (gdb) x/i $pc
    => 0x10017b6 <handle_signal+6>: lay     %r15,-344(%r15)
    # Missed the first signal handler instruction!

I'm not sure what to do about it - the trivial fix to add
gdb_handlesig(cpu, 0) to the end of handle_pending_signal() caused GDB
to hang, and I haven't looked further yet.

Best regards,
Ilya

Ilya Leoshkevich (2):
  linux-user/s390x: Fix single-stepping SVC
  tests/tcg/s390x: Test single-stepping SVC

 linux-user/s390x/cpu_loop.c         |  9 ++++
 tests/tcg/s390x/Makefile.target     | 11 ++++-
 tests/tcg/s390x/gdbstub/test-svc.py | 64 +++++++++++++++++++++++++++++
 tests/tcg/s390x/hello-s390x-asm.S   | 20 +++++++++
 4 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/s390x/gdbstub/test-svc.py
 create mode 100644 tests/tcg/s390x/hello-s390x-asm.S

-- 
2.40.1

[PATCH 1/2] linux-user/s390x: Fix single-stepping SVC

[Ilya Leoshkevich]

Currently single-stepping SVC executes two instructions. The reason is
that EXCP_DEBUG for the SVC instruction itself is masked by EXCP_SVC.
Fix by re-raising EXCP_DEBUG.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/s390x/cpu_loop.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
index 285bc60071a..8b7ac2879ef 100644
--- a/linux-user/s390x/cpu_loop.c
+++ b/linux-user/s390x/cpu_loop.c
@@ -86,6 +86,15 @@ void cpu_loop(CPUS390XState *env)
             } else if (ret != -QEMU_ESIGRETURN) {
                 env->regs[2] = ret;
             }
+
+            if (unlikely(cs->singlestep_enabled)) {
+                /*
+                 * cpu_tb_exec() did not raise EXCP_DEBUG, because it has seen
+                 * that EXCP_SVC was already pending.
+                 */
+                cs->exception_index = EXCP_DEBUG;
+            }
+
             break;
 
         case EXCP_DEBUG:
-- 
2.40.1

Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC

[Michael Tokarev]

11.05.2023 02:02, Ilya Leoshkevich wrote:
> Currently single-stepping SVC executes two instructions. The reason is
> that EXCP_DEBUG for the SVC instruction itself is masked by EXCP_SVC.
> Fix by re-raising EXCP_DEBUG.

Is it a -stable material?

/mjt

Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC

[Ilya Leoshkevich]

On Thu, 2023-05-11 at 13:55 +0300, Michael Tokarev wrote:
> 11.05.2023 02:02, Ilya Leoshkevich wrote:
> > Currently single-stepping SVC executes two instructions. The reason
> > is
> > that EXCP_DEBUG for the SVC instruction itself is masked by
> > EXCP_SVC.
> > Fix by re-raising EXCP_DEBUG.
> 
> Is it a -stable material?
> 
> /mjt

While I would personally love to see this in -stable, I don't think it
fits the official criteria - it's not a security fix and it's not a
regression.

Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC

[Michael Tokarev]

11.05.2023 14:20, Ilya Leoshkevich wrote:
> On Thu, 2023-05-11 at 13:55 +0300, Michael Tokarev wrote:

>> Is it a -stable material?

> While I would personally love to see this in -stable, I don't think it
> fits the official criteria - it's not a security fix and it's not a
> regression.

I'm not sure I follow. It's definitely okay to include a bug fix into
-stable, this has been done countless times in the past..

/mjt

Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC

[Ilya Leoshkevich]

On Thu, 2023-05-11 at 15:32 +0300, Michael Tokarev wrote:
> 11.05.2023 14:20, Ilya Leoshkevich wrote:
> > On Thu, 2023-05-11 at 13:55 +0300, Michael Tokarev wrote:
> 
> > > Is it a -stable material?
> 
> > While I would personally love to see this in -stable, I don't think
> > it
> > fits the official criteria - it's not a security fix and it's not a
> > regression.
> 
> I'm not sure I follow. It's definitely okay to include a bug fix into
> -stable, this has been done countless times in the past..
> 
> /mjt

Okay, then let's include it into -stable.

It's just that I'm not too familiar with the QEMU -stable process, so
I read [1], and it sounded quite strict.

[1] https://www.qemu.org/docs/master/devel/stable-process.html

Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC

[Michael Tokarev]

11.05.2023 16:45, Ilya Leoshkevich wrote:
>> 11.05.2023 14:20, Ilya Leoshkevich wrote:

>>> While I would personally love to see this in -stable, I don't think it
>>> fits the official criteria - it's not a security fix and it's not a
>>> regression.

> Okay, then let's include it into -stable.
> 
> It's just that I'm not too familiar with the QEMU -stable process, so
> I read [1], and it sounded quite strict.
> 
> [1] https://www.qemu.org/docs/master/devel/stable-process.html

The text there reads:

    If you think the patch would be important for users of
    the current release (or for a distribution picking fixes),
    it is usually a good candidate for stable.

:)

Please Cc: qemu-stable@nongnu.org the next time you think
something is good to have there.

/mjt

[PATCH 2/2] tests/tcg/s390x: Test single-stepping SVC

[Ilya Leoshkevich]

Add a small test to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target     | 11 ++++-
 tests/tcg/s390x/gdbstub/test-svc.py | 64 +++++++++++++++++++++++++++++
 tests/tcg/s390x/hello-s390x-asm.S   | 20 +++++++++
 3 files changed, 94 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/s390x/gdbstub/test-svc.py
 create mode 100644 tests/tcg/s390x/hello-s390x-asm.S

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 0031868b136..bd435e4dd63 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -74,7 +74,16 @@ run-gdbstub-signals-s390x: signals-s390x
 		--bin $< --test $(S390X_SRC)/gdbstub/test-signals-s390x.py, \
 	mixing signals and debugging)
 
-EXTRA_RUNS += run-gdbstub-signals-s390x
+hello-s390x-asm: CFLAGS+=-nostdlib
+
+run-gdbstub-svc: hello-s390x-asm
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(HAVE_GDB_BIN) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+		--bin $< --test $(S390X_SRC)/gdbstub/test-svc.py, \
+	single-stepping svc)
+
+EXTRA_RUNS += run-gdbstub-signals-s390x run-gdbstub-svc
 endif
 
 # MVX versions of sha512
diff --git a/tests/tcg/s390x/gdbstub/test-svc.py b/tests/tcg/s390x/gdbstub/test-svc.py
new file mode 100644
index 00000000000..7851ca72846
--- /dev/null
+++ b/tests/tcg/s390x/gdbstub/test-svc.py
@@ -0,0 +1,64 @@
+"""Test single-stepping SVC.
+
+This runs as a sourced script (via -x, via run-test.py)."""
+from __future__ import print_function
+import gdb
+import sys
+
+
+n_failures = 0
+
+
+def report(cond, msg):
+    """Report success/fail of a test"""
+    if cond:
+        print("PASS: {}".format(msg))
+    else:
+        print("FAIL: {}".format(msg))
+        global n_failures
+        n_failures += 1
+
+
+def run_test():
+    """Run through the tests one by one"""
+    report("lghi\t" in gdb.execute("x/i $pc", False, True), "insn #1")
+    gdb.execute("si")
+    report("larl\t" in gdb.execute("x/i $pc", False, True), "insn #2")
+    gdb.execute("si")
+    report("lghi\t" in gdb.execute("x/i $pc", False, True), "insn #3")
+    gdb.execute("si")
+    report("svc\t" in gdb.execute("x/i $pc", False, True), "insn #4")
+    gdb.execute("si")
+    report("xgr\t" in gdb.execute("x/i $pc", False, True), "insn #5")
+    gdb.execute("si")
+    report("svc\t" in gdb.execute("x/i $pc", False, True), "insn #6")
+    gdb.execute("si")
+
+
+def main():
+    """Prepare the environment and run through the tests"""
+    try:
+        inferior = gdb.selected_inferior()
+        print("ATTACHED: {}".format(inferior.architecture().name()))
+    except (gdb.error, AttributeError):
+        print("SKIPPING (not connected)")
+        exit(0)
+
+    if gdb.parse_and_eval('$pc') == 0:
+        print("SKIP: PC not set")
+        exit(0)
+
+    try:
+        # These are not very useful in scripts
+        gdb.execute("set pagination off")
+        gdb.execute("set confirm off")
+
+        # Run the actual tests
+        run_test()
+    except gdb.error:
+        report(False, "GDB Exception: {}".format(sys.exc_info()[0]))
+    print("All tests complete: %d failures" % n_failures)
+    exit(n_failures)
+
+
+main()
diff --git a/tests/tcg/s390x/hello-s390x-asm.S b/tests/tcg/s390x/hello-s390x-asm.S
new file mode 100644
index 00000000000..2e9faa16047
--- /dev/null
+++ b/tests/tcg/s390x/hello-s390x-asm.S
@@ -0,0 +1,20 @@
+/*
+ * Hello, World! in assembly.
+ */
+
+.globl _start
+_start:
+
+/* puts("Hello, World!"); */
+lghi %r2,1
+larl %r3,foo
+lghi %r4,foo_end-foo
+svc 4
+
+/* exit(0); */
+xgr %r2,%r2
+svc 1
+
+.align 2
+foo: .asciz "Hello, World!\n"
+foo_end:
-- 
2.40.1

Re: [PATCH 2/2] tests/tcg/s390x: Test single-stepping SVC

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Add a small test to prevent regressions.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/2] tests/tcg/s390x: Test single-stepping SVC

[Thomas Huth]

On 11/05/2023 01.02, Ilya Leoshkevich wrote:
> Add a small test to prevent regressions.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
...
> diff --git a/tests/tcg/s390x/hello-s390x-asm.S b/tests/tcg/s390x/hello-s390x-asm.S
> new file mode 100644
> index 00000000000..2e9faa16047
> --- /dev/null
> +++ b/tests/tcg/s390x/hello-s390x-asm.S
> @@ -0,0 +1,20 @@
> +/*
> + * Hello, World! in assembly.
> + */
> +
> +.globl _start
> +_start:
> +
> +/* puts("Hello, World!"); */
> +lghi %r2,1
> +larl %r3,foo
> +lghi %r4,foo_end-foo

  Hi Ilya!

While testing your other currently pending s390x TCG patches, I noticed that 
this is failing with Clang (v16.0.1):

tests/tcg/s390x/hello-s390x-asm.S:11:10: error: invalid operand for instruction
lghi %r4,foo_end-foo
          ^
make[1]: *** [Makefile:121: hello-s390x-asm] Error 1

Any ideas how to fix this?

  Thomas

Re: [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC

[Thomas Huth]

On 11/05/2023 01.02, Ilya Leoshkevich wrote:
> Hi,
> 
> I noticed that single-stepping SVC runs two instructions instead of
> one. The reason is that EXCP_SVC masks EXCP_DEBUG.
> Patch 1 fixes this problem, patch 2 adds a test.
> 
> Btw, there is at least one more problem in that area, namely
> single-stepping instructions that cause e.g. SIGILL. Using the
> existing signals-s390x test as an example:
> 
>      (gdb) x/i $pc
>      => 0x1001740 <illegal_op>:      .long   0x000007fe
> 
>      (gdb) si
>      Program received signal SIGILL, Illegal instruction.
>      (gdb) x/i $pc
>      => 0x1001742 <after_illegal_op>:        br      %r14
>      # So far so good.
> 
>      (gdb) si
>      (gdb) x/i $pc
>      => 0x10017b6 <handle_signal+6>: lay     %r15,-344(%r15)
>      # Missed the first signal handler instruction!
> 
> I'm not sure what to do about it - the trivial fix to add
> gdb_handlesig(cpu, 0) to the end of handle_pending_signal() caused GDB
> to hang, and I haven't looked further yet.
> 
> Best regards,
> Ilya
> 
> Ilya Leoshkevich (2):
>    linux-user/s390x: Fix single-stepping SVC
>    tests/tcg/s390x: Test single-stepping SVC

If there are no disagreements, I can take this through my s390x tree. Queued 
it for my next pull request now.

  Thomas