* [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC @ 2023-05-10 23:02 Ilya Leoshkevich 2023-05-10 23:02 ` [PATCH 1/2] " Ilya Leoshkevich ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ilya Leoshkevich @ 2023-05-10 23:02 UTC (permalink / raw) To: Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC 2023-05-10 23:02 [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC Ilya Leoshkevich @ 2023-05-10 23:02 ` Ilya Leoshkevich 2023-05-11 10:55 ` Michael Tokarev 2023-05-10 23:02 ` [PATCH 2/2] tests/tcg/s390x: Test " Ilya Leoshkevich 2023-05-31 14:37 ` [PATCH 0/2] linux-user/s390x: Fix " Thomas Huth 2 siblings, 1 reply; 11+ messages in thread From: Ilya Leoshkevich @ 2023-05-10 23:02 UTC (permalink / raw) To: Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC 2023-05-10 23:02 ` [PATCH 1/2] " Ilya Leoshkevich @ 2023-05-11 10:55 ` Michael Tokarev 2023-05-11 11:20 ` Ilya Leoshkevich 0 siblings, 1 reply; 11+ messages in thread From: Michael Tokarev @ 2023-05-11 10:55 UTC (permalink / raw) To: Ilya Leoshkevich, Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC 2023-05-11 10:55 ` Michael Tokarev @ 2023-05-11 11:20 ` Ilya Leoshkevich 2023-05-11 12:32 ` Michael Tokarev 0 siblings, 1 reply; 11+ messages in thread From: Ilya Leoshkevich @ 2023-05-11 11:20 UTC (permalink / raw) To: Michael Tokarev, Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC 2023-05-11 11:20 ` Ilya Leoshkevich @ 2023-05-11 12:32 ` Michael Tokarev 2023-05-11 13:45 ` Ilya Leoshkevich 0 siblings, 1 reply; 11+ messages in thread From: Michael Tokarev @ 2023-05-11 12:32 UTC (permalink / raw) To: Ilya Leoshkevich, Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC 2023-05-11 12:32 ` Michael Tokarev @ 2023-05-11 13:45 ` Ilya Leoshkevich 2023-05-11 13:50 ` Michael Tokarev 0 siblings, 1 reply; 11+ messages in thread From: Ilya Leoshkevich @ 2023-05-11 13:45 UTC (permalink / raw) To: Michael Tokarev, Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] linux-user/s390x: Fix single-stepping SVC 2023-05-11 13:45 ` Ilya Leoshkevich @ 2023-05-11 13:50 ` Michael Tokarev 0 siblings, 0 replies; 11+ messages in thread From: Michael Tokarev @ 2023-05-11 13:50 UTC (permalink / raw) To: Ilya Leoshkevich, Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tests/tcg/s390x: Test single-stepping SVC 2023-05-10 23:02 [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC Ilya Leoshkevich 2023-05-10 23:02 ` [PATCH 1/2] " Ilya Leoshkevich @ 2023-05-10 23:02 ` Ilya Leoshkevich 2023-05-11 10:51 ` Alex Bennée 2023-07-07 8:37 ` Thomas Huth 2023-05-31 14:37 ` [PATCH 0/2] linux-user/s390x: Fix " Thomas Huth 2 siblings, 2 replies; 11+ messages in thread From: Ilya Leoshkevich @ 2023-05-10 23:02 UTC (permalink / raw) To: Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth Cc: qemu-devel, qemu-s390x, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tests/tcg/s390x: Test single-stepping SVC 2023-05-10 23:02 ` [PATCH 2/2] tests/tcg/s390x: Test " Ilya Leoshkevich @ 2023-05-11 10:51 ` Alex Bennée 2023-07-07 8:37 ` Thomas Huth 1 sibling, 0 replies; 11+ messages in thread From: Alex Bennée @ 2023-05-11 10:51 UTC (permalink / raw) To: Ilya Leoshkevich Cc: Laurent Vivier, Richard Henderson, David Hildenbrand, Thomas Huth, qemu-s390x, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tests/tcg/s390x: Test single-stepping SVC 2023-05-10 23:02 ` [PATCH 2/2] tests/tcg/s390x: Test " Ilya Leoshkevich 2023-05-11 10:51 ` Alex Bennée @ 2023-07-07 8:37 ` Thomas Huth 1 sibling, 0 replies; 11+ messages in thread From: Thomas Huth @ 2023-07-07 8:37 UTC (permalink / raw) To: Ilya Leoshkevich Cc: qemu-devel, qemu-s390x, Laurent Vivier, Richard Henderson, David Hildenbrand, Michael Tokarev, Andreas Krebbel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC 2023-05-10 23:02 [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC Ilya Leoshkevich 2023-05-10 23:02 ` [PATCH 1/2] " Ilya Leoshkevich 2023-05-10 23:02 ` [PATCH 2/2] tests/tcg/s390x: Test " Ilya Leoshkevich @ 2023-05-31 14:37 ` Thomas Huth 2 siblings, 0 replies; 11+ messages in thread From: Thomas Huth @ 2023-05-31 14:37 UTC (permalink / raw) To: Ilya Leoshkevich, Laurent Vivier, Richard Henderson, David Hildenbrand Cc: qemu-devel, qemu-s390x 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-07-07 8:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-10 23:02 [PATCH 0/2] linux-user/s390x: Fix single-stepping SVC Ilya Leoshkevich 2023-05-10 23:02 ` [PATCH 1/2] " Ilya Leoshkevich 2023-05-11 10:55 ` Michael Tokarev 2023-05-11 11:20 ` Ilya Leoshkevich 2023-05-11 12:32 ` Michael Tokarev 2023-05-11 13:45 ` Ilya Leoshkevich 2023-05-11 13:50 ` Michael Tokarev 2023-05-10 23:02 ` [PATCH 2/2] tests/tcg/s390x: Test " Ilya Leoshkevich 2023-05-11 10:51 ` Alex Bennée 2023-07-07 8:37 ` Thomas Huth 2023-05-31 14:37 ` [PATCH 0/2] linux-user/s390x: Fix " Thomas Huth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.