[PULL 0/4] tcg/gdbstub late fixes

[PULL 0/4] tcg/gdbstub late fixes

[Richard Henderson]

The following changes since commit 64d3be986f9e2379bc688bf1d0aca0557e0035ca:

  Merge tag 'or1k-pull-request-20230809' of https://github.com/stffrdhrn/qemu into staging (2023-08-09 15:05:02 -0700)

are available in the Git repository at:

  https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20230810

for you to fetch changes up to f1b0f894c8c25f7ed24197ff130c7acb6b9fd6e7:

  gdbstub: don't complain about preemptive ACK chars (2023-08-10 11:04:34 -0700)

----------------------------------------------------------------
accel/tcg: Avoid reading too much in load_atom_{2,4}
tests/tcg: ensure system-mode gdb tests start stopped
gdbstub: more fixes for client Ctrl-C handling

----------------------------------------------------------------
Alex Bennée (3):
      tests/tcg: ensure system-mode gdb tests start stopped
      gdbstub: more fixes for client Ctrl-C handling
      gdbstub: don't complain about preemptive ACK chars

Richard Henderson (1):
      accel/tcg: Avoid reading too much in load_atom_{2,4}

 gdbstub/gdbstub.c                 | 10 ++++++++--
 tests/tcg/aarch64/lse2-fault.c    | 38 ++++++++++++++++++++++++++++++++++++++
 accel/tcg/ldst_atomicity.c.inc    | 10 ++++++++--
 gdbstub/trace-events              |  1 +
 tests/guest-debug/run-test.py     |  9 +++------
 tests/tcg/aarch64/Makefile.target |  2 +-
 6 files changed, 59 insertions(+), 11 deletions(-)
 create mode 100644 tests/tcg/aarch64/lse2-fault.c

[PULL 1/4] accel/tcg: Avoid reading too much in load_atom_{2,4}

[Richard Henderson]

When load_atom_extract_al16_or_al8 is inexpensive, we want to use
it early, in order to avoid the overhead of required_atomicity.
However, we must not read past the end of the page.

If there are more than 8 bytes remaining, then both the "aligned 16"
and "aligned 8" paths align down so that the read has at least
16 bytes remaining on the page.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/aarch64/lse2-fault.c    | 38 +++++++++++++++++++++++++++++++
 accel/tcg/ldst_atomicity.c.inc    | 10 ++++++--
 tests/tcg/aarch64/Makefile.target |  2 +-
 3 files changed, 47 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/aarch64/lse2-fault.c

diff --git a/tests/tcg/aarch64/lse2-fault.c b/tests/tcg/aarch64/lse2-fault.c
new file mode 100644
index 0000000000..2187219a08
--- /dev/null
+++ b/tests/tcg/aarch64/lse2-fault.c
@@ -0,0 +1,38 @@
+#include <sys/mman.h>
+#include <sys/shm.h>
+#include <unistd.h>
+#include <stdio.h>
+
+int main()
+{
+    int psize = getpagesize();
+    int id;
+    void *p;
+
+    /*
+     * We need a shared mapping to enter CF_PARALLEL mode.
+     * The easiest way to get that is shmat.
+     */
+    id = shmget(IPC_PRIVATE, 2 * psize, IPC_CREAT | 0600);
+    if (id < 0) {
+        perror("shmget");
+        return 2;
+    }
+    p = shmat(id, NULL, 0);
+    if (p == MAP_FAILED) {
+        perror("shmat");
+        return 2;
+    }
+
+    /* Protect the second page. */
+    if (mprotect(p + psize, psize, PROT_NONE) < 0) {
+        perror("mprotect");
+        return 2;
+    }
+
+    /*
+     * Load 4 bytes, 6 bytes from the end of the page.
+     * On success this will load 0 from the newly allocated shm.
+     */
+    return *(int *)(p + psize - 6);
+}
diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
index e5c590a499..1b793e6935 100644
--- a/accel/tcg/ldst_atomicity.c.inc
+++ b/accel/tcg/ldst_atomicity.c.inc
@@ -404,7 +404,10 @@ static uint16_t load_atom_2(CPUArchState *env, uintptr_t ra,
         return load_atomic2(pv);
     }
     if (HAVE_ATOMIC128_RO) {
-        return load_atom_extract_al16_or_al8(pv, 2);
+        intptr_t left_in_page = -(pi | TARGET_PAGE_MASK);
+        if (likely(left_in_page > 8)) {
+            return load_atom_extract_al16_or_al8(pv, 2);
+        }
     }
 
     atmax = required_atomicity(env, pi, memop);
@@ -443,7 +446,10 @@ static uint32_t load_atom_4(CPUArchState *env, uintptr_t ra,
         return load_atomic4(pv);
     }
     if (HAVE_ATOMIC128_RO) {
-        return load_atom_extract_al16_or_al8(pv, 4);
+        intptr_t left_in_page = -(pi | TARGET_PAGE_MASK);
+        if (likely(left_in_page > 8)) {
+            return load_atom_extract_al16_or_al8(pv, 4);
+        }
     }
 
     atmax = required_atomicity(env, pi, memop);
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 617f821613..681dfa077c 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -9,7 +9,7 @@ AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
 VPATH 		+= $(AARCH64_SRC)
 
 # Base architecture tests
-AARCH64_TESTS=fcvt pcalign-a64
+AARCH64_TESTS=fcvt pcalign-a64 lse2-fault
 
 fcvt: LDFLAGS+=-lm
 
-- 
2.34.1

[PULL 2/4] tests/tcg: ensure system-mode gdb tests start stopped

[Richard Henderson]

From: Alex Bennée <alex.bennee@linaro.org>

Without -S we run into potential races with tests starting before the
gdbstub attaches. We don't need to worry about user-mode as enabling
the gdbstub implies we wait for the initial connection.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230810153640.1879717-7-alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/guest-debug/run-test.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index de6106a5e5..a032e01f79 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -69,13 +69,10 @@ def log(output, msg):
 
     # Launch QEMU with binary
     if "system" in args.qemu:
-        cmd = "%s %s %s -gdb unix:path=%s,server=on" % (args.qemu,
-                                                        args.qargs,
-                                                        args.binary,
-                                                        socket_name)
+        cmd = f'{args.qemu} {args.qargs} {args.binary}' \
+            f' -S -gdb unix:path={socket_name},server=on'
     else:
-        cmd = "%s %s -g %s %s" % (args.qemu, args.qargs, socket_name,
-                                  args.binary)
+        cmd = f'{args.qemu} {args.qargs} -g {socket_name} {args.binary}'
 
     log(output, "QEMU CMD: %s" % (cmd))
     inferior = subprocess.Popen(shlex.split(cmd))
-- 
2.34.1

[PULL 3/4] gdbstub: more fixes for client Ctrl-C handling

[Richard Henderson]

From: Alex Bennée <alex.bennee@linaro.org>

The original fix caused problems with spurious characters on other
system emulation. So:

  - instead of spamming output make the warning a trace point
  - ensure we only allow a stop reply if it was 0x3

Suggested-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <456ed3318421dd7946bdfb5ceda7e05332da368c.1690910333.git.quic_mathbern@quicinc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230810153640.1879717-8-alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 gdbstub/gdbstub.c    | 5 +++--
 gdbstub/trace-events | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index e74ecc78cc..20b6fe03fb 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2059,9 +2059,10 @@ void gdb_read_byte(uint8_t ch)
          * here, but it does expect a stop reply.
          */
         if (ch != 0x03) {
-            warn_report("gdbstub: client sent packet while target running\n");
+            trace_gdbstub_err_unexpected_runpkt(ch);
+        } else {
+            gdbserver_state.allow_stop_reply = true;
         }
-        gdbserver_state.allow_stop_reply = true;
         vm_stop(RUN_STATE_PAUSED);
     } else
 #endif
diff --git a/gdbstub/trace-events b/gdbstub/trace-events
index 0c18a4d70a..7bc79a73c4 100644
--- a/gdbstub/trace-events
+++ b/gdbstub/trace-events
@@ -26,6 +26,7 @@ gdbstub_err_invalid_repeat(uint8_t ch) "got invalid RLE count: 0x%02x"
 gdbstub_err_invalid_rle(void) "got invalid RLE sequence"
 gdbstub_err_checksum_invalid(uint8_t ch) "got invalid command checksum digit: 0x%02x"
 gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packet with incorrect checksum, expected=0x%02x, received=0x%02x"
+gdbstub_err_unexpected_runpkt(uint8_t ch) "unexpected packet (0x%02x) while target running"
 
 # softmmu.c
 gdbstub_hit_watchpoint(const char *type, int cpu_gdb_index, uint64_t vaddr) "Watchpoint hit, type=\"%s\" cpu=%d, vaddr=0x%" PRIx64 ""
-- 
2.34.1

[PULL 4/4] gdbstub: don't complain about preemptive ACK chars

[Richard Henderson]

From: Alex Bennée <alex.bennee@linaro.org>

When starting a remote connection GDB sends an '+':

  /* Ack any packet which the remote side has already sent.  */
  remote_serial_write ("+", 1);

which gets flagged as a garbage character in the gdbstub state
machine. As gdb does send it out lets be permissive about the handling
so we can better see real issues.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: gdb-patches@sourceware.org
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230810153640.1879717-9-alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 gdbstub/gdbstub.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 20b6fe03fb..5f28d5cf57 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2074,6 +2074,11 @@ void gdb_read_byte(uint8_t ch)
                 gdbserver_state.line_buf_index = 0;
                 gdbserver_state.line_sum = 0;
                 gdbserver_state.state = RS_GETLINE;
+            } else if (ch == '+') {
+                /*
+                 * do nothing, gdb may preemptively send out ACKs on
+                 * initial connection
+                 */
             } else {
                 trace_gdbstub_err_garbage(ch);
             }
-- 
2.34.1

Re: [PULL 0/4] tcg/gdbstub late fixes

[Richard Henderson]

On 8/10/23 11:08, Richard Henderson wrote:
> The following changes since commit 64d3be986f9e2379bc688bf1d0aca0557e0035ca:
> 
>    Merge tag 'or1k-pull-request-20230809' of https://github.com/stffrdhrn/qemu into staging (2023-08-09 15:05:02 -0700)
> 
> are available in the Git repository at:
> 
>    https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20230810
> 
> for you to fetch changes up to f1b0f894c8c25f7ed24197ff130c7acb6b9fd6e7:
> 
>    gdbstub: don't complain about preemptive ACK chars (2023-08-10 11:04:34 -0700)
> 
> ----------------------------------------------------------------
> accel/tcg: Avoid reading too much in load_atom_{2,4}
> tests/tcg: ensure system-mode gdb tests start stopped
> gdbstub: more fixes for client Ctrl-C handling

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate.


r~