[PATCH v2 RESEND 0/7] Hexagon: add lldb support

[PATCH v2 RESEND 0/7] Hexagon: add lldb support

[Matheus Tavares Bernardino]

This series allows hexagon programs to be debugged under qemu user-mode
through LLDB and qemu's gdbstub. LLDB implements the GDB remote serial
protocol, so most of the necessary changes are in the Hexagon part itself.
However, one fix is needed at the arch-independent side too.

Changes in v2:
- Rebased on current master
- Added Taylor's Reviewed-by at patch 5

v1: https://lore.kernel.org/qemu-devel/cover.1680808943.git.quic_mathbern@quicinc.com/

Brian Cain (3):
  gdbstub: add support for the qRegisterInfo query
  Hexagon: support qRegisterInfo at gdbstub
  Hexagon (gdbstub): fix p3:0 read and write via stub

Matheus Tavares Bernardino (2):
  gdbstub: only send stop-reply packets when allowed to
  gdbstub: add test for untimely stop-reply packets

Taylor Simpson (2):
  Hexagon (gdbstub): add HVX support
  Hexagon (linux-user/hexagon): handle breakpoints

 gdbstub/internals.h                           |   5 +
 include/hw/core/cpu.h                         |   4 +
 target/hexagon/gdb_qreginfo.h                 | 124 ++++++++++++++++++
 gdbstub/gdbstub.c                             |  64 +++++++--
 gdbstub/softmmu.c                             |  13 +-
 gdbstub/user.c                                |  17 ++-
 linux-user/hexagon/cpu_loop.c                 |  10 ++
 target/hexagon/cpu.c                          |   3 +
 target/hexagon/gdbstub.c                      |  76 +++++++++++
 tests/guest-debug/run-test.py                 |  16 ++-
 .../multiarch/system/Makefile.softmmu-target  |  16 ++-
 11 files changed, 326 insertions(+), 22 deletions(-)
 create mode 100644 target/hexagon/gdb_qreginfo.h

-- 
2.39.1

[PATCH v2 RESEND 1/7] gdbstub: only send stop-reply packets when allowed to

[Matheus Tavares Bernardino]

GDB's remote serial protocol allows stop-reply messages to be sent by
the stub either as a notification packet or as a reply to a GDB command
(provided that the cmd accepts such a response). QEMU currently does not
implement notification packets, so it should only send stop-replies
synchronously and when requested. Nevertheless, it still issues
unsolicited stop messages through gdb_vm_state_change().

Although this behavior doesn't seem to cause problems with GDB itself
(the messages are just ignored), it can impact other debuggers that
implement the GDB remote serial protocol, like hexagon-lldb. Let's
change the gdbstub to send stop messages only as a response to a
previous GDB command that accepts such a reply.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 gdbstub/internals.h |  5 +++++
 gdbstub/gdbstub.c   | 37 ++++++++++++++++++++++++++++---------
 gdbstub/softmmu.c   | 13 +++++++++++--
 gdbstub/user.c      | 17 +++++++++++------
 4 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 94ddff4495..33d21d6488 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -65,6 +65,11 @@ typedef struct GDBState {
     GByteArray *mem_buf;
     int sstep_flags;
     int supported_sstep_flags;
+    /*
+     * Whether we are allowed to send a stop reply packet at this moment.
+     * Must be set off after sending the stop reply itself.
+     */
+    bool allow_stop_reply;
 } GDBState;
 
 /* lives in main gdbstub.c */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 0760d78685..be18568d0a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -777,6 +777,10 @@ typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
 /*
  * cmd_startswith -> cmd is compared using startswith
  *
+ * allow_stop_reply -> true iff the gdbstub can respond to this command with a
+ *   "stop reply" packet. The list of commands that accept such response is
+ *   defined at the GDB Remote Serial Protocol documentation. see:
+ *   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
  *
  * schema definitions:
  * Each schema parameter entry consists of 2 chars,
@@ -802,6 +806,7 @@ typedef struct GdbCmdParseEntry {
     const char *cmd;
     bool cmd_startswith;
     const char *schema;
+    bool allow_stop_reply;
 } GdbCmdParseEntry;
 
 static inline int startswith(const char *string, const char *pattern)
@@ -835,6 +840,7 @@ static int process_string_cmd(void *user_ctx, const char *data,
             }
         }
 
+        gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
         cmd->handler(params, user_ctx);
         return 0;
     }
@@ -1283,11 +1289,14 @@ static void handle_v_attach(GArray *params, void *user_ctx)
     gdbserver_state.g_cpu = cpu;
     gdbserver_state.c_cpu = cpu;
 
-    g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-    gdb_append_thread_id(cpu, gdbserver_state.str_buf);
-    g_string_append_c(gdbserver_state.str_buf, ';');
+    if (gdbserver_state.allow_stop_reply) {
+        g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
+        gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+        g_string_append_c(gdbserver_state.str_buf, ';');
+        gdbserver_state.allow_stop_reply = false;
 cleanup:
-    gdb_put_strbuf();
+        gdb_put_strbuf();
+    }
 }
 
 static void handle_v_kill(GArray *params, void *user_ctx)
@@ -1310,12 +1319,14 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
         .handler = handle_v_cont,
         .cmd = "Cont",
         .cmd_startswith = 1,
+        .allow_stop_reply = true,
         .schema = "s0"
     },
     {
         .handler = handle_v_attach,
         .cmd = "Attach;",
         .cmd_startswith = 1,
+        .allow_stop_reply = true,
         .schema = "l0"
     },
     {
@@ -1698,10 +1709,13 @@ static void handle_gen_set(GArray *params, void *user_ctx)
 
 static void handle_target_halt(GArray *params, void *user_ctx)
 {
-    g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-    gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
-    g_string_append_c(gdbserver_state.str_buf, ';');
-    gdb_put_strbuf();
+    if (gdbserver_state.allow_stop_reply) {
+        g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
+        gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
+        g_string_append_c(gdbserver_state.str_buf, ';');
+        gdb_put_strbuf();
+        gdbserver_state.allow_stop_reply = false;
+    }
     /*
      * Remove all the breakpoints when this query is issued,
      * because gdb is doing an initial connect and the state
@@ -1725,7 +1739,8 @@ static int gdb_handle_packet(const char *line_buf)
             static const GdbCmdParseEntry target_halted_cmd_desc = {
                 .handler = handle_target_halt,
                 .cmd = "?",
-                .cmd_startswith = 1
+                .cmd_startswith = 1,
+                .allow_stop_reply = true,
             };
             cmd_parser = &target_halted_cmd_desc;
         }
@@ -1736,6 +1751,7 @@ static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_continue,
                 .cmd = "c",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "L0"
             };
             cmd_parser = &continue_cmd_desc;
@@ -1747,6 +1763,7 @@ static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_cont_with_sig,
                 .cmd = "C",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "l0"
             };
             cmd_parser = &cont_with_sig_cmd_desc;
@@ -1785,6 +1802,7 @@ static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_step,
                 .cmd = "s",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "L0"
             };
             cmd_parser = &step_cmd_desc;
@@ -1976,6 +1994,7 @@ void gdb_read_byte(uint8_t ch)
 {
     uint8_t reply;
 
+    gdbserver_state.allow_stop_reply = false;
 #ifndef CONFIG_USER_ONLY
     if (gdbserver_state.last_packet->len) {
         /* Waiting for a response to the last packet.  If we see the start
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 22ecd09d04..99d994e6bf 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -43,6 +43,7 @@ static void reset_gdbserver_state(void)
     g_free(gdbserver_state.processes);
     gdbserver_state.processes = NULL;
     gdbserver_state.process_num = 0;
+    gdbserver_state.allow_stop_reply = false;
 }
 
 /*
@@ -139,6 +140,10 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
         return;
     }
 
+    if (!gdbserver_state.allow_stop_reply) {
+        return;
+    }
+
     gdb_append_thread_id(cpu, tid);
 
     switch (state) {
@@ -205,6 +210,7 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
 
 send_packet:
     gdb_put_packet(buf->str);
+    gdbserver_state.allow_stop_reply = false;
 
     /* disable single step if it was enabled */
     cpu_single_step(cpu, 0);
@@ -422,8 +428,11 @@ void gdb_exit(int code)
 
     trace_gdbstub_op_exiting((uint8_t)code);
 
-    snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
-    gdb_put_packet(buf);
+    if (gdbserver_state.allow_stop_reply) {
+        snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
+        gdb_put_packet(buf);
+        gdbserver_state.allow_stop_reply = false;
+    }
 
     qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
 }
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 80488b6bb9..bb03622c83 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -127,11 +127,14 @@ int gdb_handlesig(CPUState *cpu, int sig)
 
     if (sig != 0) {
         gdb_set_stop_cpu(cpu);
-        g_string_printf(gdbserver_state.str_buf,
-                        "T%02xthread:", gdb_target_signal_to_gdb(sig));
-        gdb_append_thread_id(cpu, gdbserver_state.str_buf);
-        g_string_append_c(gdbserver_state.str_buf, ';');
-        gdb_put_strbuf();
+        if (gdbserver_state.allow_stop_reply) {
+            g_string_printf(gdbserver_state.str_buf,
+                            "T%02xthread:", gdb_target_signal_to_gdb(sig));
+            gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+            g_string_append_c(gdbserver_state.str_buf, ';');
+            gdb_put_strbuf();
+            gdbserver_state.allow_stop_reply = false;
+        }
     }
     /*
      * gdb_put_packet() might have detected that the peer terminated the
@@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
 {
     char buf[4];
 
-    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
+        !gdbserver_state.allow_stop_reply) {
         return;
     }
 
     snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
     gdb_put_packet(buf);
+    gdbserver_state.allow_stop_reply = false;
 }
 
 static void gdb_accept_init(int fd)
-- 
2.39.1

Re: [PATCH v2 RESEND 1/7] gdbstub: only send stop-reply packets when allowed to

[Alex Bennée]

Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:

> GDB's remote serial protocol allows stop-reply messages to be sent by
> the stub either as a notification packet or as a reply to a GDB command
> (provided that the cmd accepts such a response). QEMU currently does not
> implement notification packets, so it should only send stop-replies
> synchronously and when requested. Nevertheless, it still issues
> unsolicited stop messages through gdb_vm_state_change().
>
> Although this behavior doesn't seem to cause problems with GDB itself
> (the messages are just ignored), it can impact other debuggers that
> implement the GDB remote serial protocol, like hexagon-lldb. Let's
> change the gdbstub to send stop messages only as a response to a
> previous GDB command that accepts such a reply.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  gdbstub/internals.h |  5 +++++
>  gdbstub/gdbstub.c   | 37 ++++++++++++++++++++++++++++---------
>  gdbstub/softmmu.c   | 13 +++++++++++--
>  gdbstub/user.c      | 17 +++++++++++------
>  4 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 94ddff4495..33d21d6488 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -65,6 +65,11 @@ typedef struct GDBState {
>      GByteArray *mem_buf;
>      int sstep_flags;
>      int supported_sstep_flags;
> +    /*
> +     * Whether we are allowed to send a stop reply packet at this moment.
> +     * Must be set off after sending the stop reply itself.
> +     */
> +    bool allow_stop_reply;
>  } GDBState;
>  
>  /* lives in main gdbstub.c */
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 0760d78685..be18568d0a 100644
<snip>
>  /*
> @@ -139,6 +140,10 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
>          return;
>      }
>  
> +    if (!gdbserver_state.allow_stop_reply) {
> +        return;
> +    }
> +
>      gdb_append_thread_id(cpu, tid);
>  
>      switch (state) {
> @@ -205,6 +210,7 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
>  
>  send_packet:
>      gdb_put_packet(buf->str);
> +    gdbserver_state.allow_stop_reply = false;
>  
>      /* disable single step if it was enabled */
>      cpu_single_step(cpu, 0);
> @@ -422,8 +428,11 @@ void gdb_exit(int code)
>  
>      trace_gdbstub_op_exiting((uint8_t)code);
>  
> -    snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
> -    gdb_put_packet(buf);
> +    if (gdbserver_state.allow_stop_reply) {
> +        snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
> +        gdb_put_packet(buf);
> +        gdbserver_state.allow_stop_reply = false;
> +    }
>  
>      qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
>  }
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index 80488b6bb9..bb03622c83 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -127,11 +127,14 @@ int gdb_handlesig(CPUState *cpu, int sig)
>  
>      if (sig != 0) {
>          gdb_set_stop_cpu(cpu);
> -        g_string_printf(gdbserver_state.str_buf,
> -                        "T%02xthread:", gdb_target_signal_to_gdb(sig));
> -        gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> -        g_string_append_c(gdbserver_state.str_buf, ';');
> -        gdb_put_strbuf();
> +        if (gdbserver_state.allow_stop_reply) {
> +            g_string_printf(gdbserver_state.str_buf,
> +                            "T%02xthread:", gdb_target_signal_to_gdb(sig));
> +            gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> +            g_string_append_c(gdbserver_state.str_buf, ';');
> +            gdb_put_strbuf();
> +            gdbserver_state.allow_stop_reply = false;
> +        }
>      }
>      /*
>       * gdb_put_packet() might have detected that the peer terminated the
> @@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
>  {
>      char buf[4];
>  
> -    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
> +        !gdbserver_state.allow_stop_reply) {
>          return;
>      }
>  
>      snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
>      gdb_put_packet(buf);
> +    gdbserver_state.allow_stop_reply = false;

Did I miss an equivalent for softmmu mode here?

Anyway:

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 RESEND 1/7] gdbstub: only send stop-reply packets when allowed to

[Matheus Tavares Bernardino]

Alex Bennée <alex.bennee@linaro.org> wrote:
>
> > Matheus Tavares <quic_mathbern@quicinc.com> wrote:
> >
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 80488b6bb9..bb03622c83 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
> >  {
> >      char buf[4];
> >  
> > -    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> > +    if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
> > +        !gdbserver_state.allow_stop_reply) {
> >          return;
> >      }
> >  
> >      snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
> >      gdb_put_packet(buf);
> > +    gdbserver_state.allow_stop_reply = false;
> 
> Did I miss an equivalent for softmmu mode here?

Hmm, there doesn't seem to be other "X aa" stop-replies sent from
softmmu in our gdbstub. However, I just realize I did miss another spot
of "W aa" at gdbstub/user.c:gdb_exit(). I'll add the allow_stop_reply
guard there for the next iteration.

[PATCH v2 RESEND 2/7] gdbstub: add test for untimely stop-reply packets

[Matheus Tavares Bernardino]

In the previous commit, we modified gdbstub.c to only send stop-reply
packets as a response to GDB commands that accept it. Now, let's add a
test for this intended behavior. Running this test before the fix from
the previous commit fails as QEMU sends a stop-reply packet
asynchronously, when GDB was in fact waiting an ACK.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 tests/guest-debug/run-test.py                    | 16 ++++++++++++----
 .../tcg/multiarch/system/Makefile.softmmu-target | 16 +++++++++++++++-
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index d865e46ecd..de6106a5e5 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -26,11 +26,12 @@ def get_args():
     parser.add_argument("--qargs", help="Qemu arguments for test")
     parser.add_argument("--binary", help="Binary to debug",
                         required=True)
-    parser.add_argument("--test", help="GDB test script",
-                        required=True)
+    parser.add_argument("--test", help="GDB test script")
     parser.add_argument("--gdb", help="The gdb binary to use",
                         default=None)
+    parser.add_argument("--gdb-args", help="Additional gdb arguments")
     parser.add_argument("--output", help="A file to redirect output to")
+    parser.add_argument("--stderr", help="A file to redirect stderr to")
 
     return parser.parse_args()
 
@@ -58,6 +59,10 @@ def log(output, msg):
         output = open(args.output, "w")
     else:
         output = None
+    if args.stderr:
+        stderr = open(args.stderr, "w")
+    else:
+        stderr = None
 
     socket_dir = TemporaryDirectory("qemu-gdbstub")
     socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
@@ -77,6 +82,8 @@ def log(output, msg):
 
     # Now launch gdb with our test and collect the result
     gdb_cmd = "%s %s" % (args.gdb, args.binary)
+    if args.gdb_args:
+        gdb_cmd += " %s" % (args.gdb_args)
     # run quietly and ignore .gdbinit
     gdb_cmd += " -q -n -batch"
     # disable prompts in case of crash
@@ -84,13 +91,14 @@ def log(output, msg):
     # connect to remote
     gdb_cmd += " -ex 'target remote %s'" % (socket_name)
     # finally the test script itself
-    gdb_cmd += " -x %s" % (args.test)
+    if args.test:
+        gdb_cmd += " -x %s" % (args.test)
 
 
     sleep(1)
     log(output, "GDB CMD: %s" % (gdb_cmd))
 
-    result = subprocess.call(gdb_cmd, shell=True, stdout=output)
+    result = subprocess.call(gdb_cmd, shell=True, stdout=output, stderr=stderr)
 
     # A result of greater than 128 indicates a fatal signal (likely a
     # crash due to gdb internal failure). That's a problem for GDB and
diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target b/tests/tcg/multiarch/system/Makefile.softmmu-target
index 5f432c95f3..fe40195d39 100644
--- a/tests/tcg/multiarch/system/Makefile.softmmu-target
+++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
@@ -27,6 +27,20 @@ run-gdbstub-memory: memory
 		"-monitor none -display none -chardev file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
 		--bin $< --test $(MULTIARCH_SRC)/gdbstub/memory.py, \
 	softmmu gdbstub support)
+
+run-gdbstub-untimely-packet: hello
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(HAVE_GDB_BIN) \
+		--gdb-args "-ex 'set debug remote 1'" \
+		--output untimely-packet.gdb.out \
+		--stderr untimely-packet.gdb.err \
+		--qemu $(QEMU) \
+		--bin $< --qargs \
+		"-monitor none -display none -chardev file$(COMMA)path=untimely-packet.out$(COMMA)id=output $(QEMU_OPTS)", \
+	"softmmu gdbstub untimely packets")
+	$(call quiet-command, \
+		(! grep -Fq 'Packet instead of Ack, ignoring it' untimely-packet.gdb.err), \
+		"GREP", "file  untimely-packet.gdb.err")
 else
 run-gdbstub-%:
 	$(call skip-test, "gdbstub test $*", "no guest arch support")
@@ -36,4 +50,4 @@ run-gdbstub-%:
 	$(call skip-test, "gdbstub test $*", "need working gdb")
 endif
 
-MULTIARCH_RUNS += run-gdbstub-memory
+MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-untimely-packet
-- 
2.39.1

Re: [PATCH v2 RESEND 2/7] gdbstub: add test for untimely stop-reply packets

[Alex Bennée]

Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:

> In the previous commit, we modified gdbstub.c to only send stop-reply
> packets as a response to GDB commands that accept it. Now, let's add a
> test for this intended behavior. Running this test before the fix from
> the previous commit fails as QEMU sends a stop-reply packet
> asynchronously, when GDB was in fact waiting an ACK.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>

Excellent work adding some more gdb testing ;-)

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query

[Matheus Tavares Bernardino]

From: Brian Cain <bcain@quicinc.com>

Signed-off-by: Brian Cain <bcain@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 include/hw/core/cpu.h |  4 ++++
 gdbstub/gdbstub.c     | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 397fd3ac68..cfdf5514d9 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -124,6 +124,8 @@ struct SysemuCPUOps;
  *       its Harvard architecture split code and data.
  * @gdb_num_core_regs: Number of core registers accessible to GDB.
  * @gdb_core_xml_file: File name for core registers GDB XML description.
+ * @gdb_qreg_info_lines: Array of lines of registers qRegisterInfo description.
+ * @gdb_qreg_info_line_count: Count of lines for @gdb_qreg_info_lines.
  * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
  *           before the insn which triggers a watchpoint rather than after it.
  * @gdb_arch_name: Optional callback that returns the architecture name known
@@ -159,6 +161,8 @@ struct CPUClass {
     vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
 
     const char *gdb_core_xml_file;
+    const char **gdb_qreg_info_lines;
+    int gdb_qreg_info_line_count;
     gchar * (*gdb_arch_name)(CPUState *cpu);
     const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
 
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be18568d0a..f19f8c58c3 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1409,6 +1409,27 @@ static void handle_query_curr_tid(GArray *params, void *user_ctx)
     gdb_put_strbuf();
 }
 
+static void handle_query_regs(GArray *params, void *user_ctx)
+{
+    if (!params->len) {
+        return;
+    }
+
+    CPUClass *cc = CPU_GET_CLASS(gdbserver_state.g_cpu);
+    if (!cc->gdb_qreg_info_lines) {
+        gdb_put_packet("");
+        return;
+    }
+
+    int reg_num = get_param(params, 0)->val_ul;
+    if (reg_num >= cc->gdb_qreg_info_line_count) {
+        gdb_put_packet("");
+        return;
+    }
+
+    gdb_put_packet(cc->gdb_qreg_info_lines[reg_num]);
+}
+
 static void handle_query_threads(GArray *params, void *user_ctx)
 {
     if (!gdbserver_state.query_cpu) {
@@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
         .handler = handle_query_curr_tid,
         .cmd = "C",
     },
+    {
+        .handler = handle_query_regs,
+        .cmd = "RegisterInfo",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
     {
         .handler = handle_query_threads,
         .cmd = "sThreadInfo",
-- 
2.39.1

Re: [PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query

[Alex Bennée]

Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:

> From: Brian Cain <bcain@quicinc.com>
>
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  include/hw/core/cpu.h |  4 ++++
>  gdbstub/gdbstub.c     | 27 +++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 397fd3ac68..cfdf5514d9 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -124,6 +124,8 @@ struct SysemuCPUOps;
>   *       its Harvard architecture split code and data.
>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>   * @gdb_core_xml_file: File name for core registers GDB XML description.
> + * @gdb_qreg_info_lines: Array of lines of registers qRegisterInfo description.
> + * @gdb_qreg_info_line_count: Count of lines for @gdb_qreg_info_lines.
>   * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
>   *           before the insn which triggers a watchpoint rather than after it.
>   * @gdb_arch_name: Optional callback that returns the architecture name known
> @@ -159,6 +161,8 @@ struct CPUClass {
>      vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
>  
>      const char *gdb_core_xml_file;
> +    const char **gdb_qreg_info_lines;
> +    int gdb_qreg_info_line_count;
>      gchar * (*gdb_arch_name)(CPUState *cpu);
>      const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
>  
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index be18568d0a..f19f8c58c3 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1409,6 +1409,27 @@ static void handle_query_curr_tid(GArray *params, void *user_ctx)
>      gdb_put_strbuf();
>  }
>  
> +static void handle_query_regs(GArray *params, void *user_ctx)
> +{
> +    if (!params->len) {
> +        return;
> +    }
> +
> +    CPUClass *cc = CPU_GET_CLASS(gdbserver_state.g_cpu);
> +    if (!cc->gdb_qreg_info_lines) {
> +        gdb_put_packet("");
> +        return;
> +    }
> +
> +    int reg_num = get_param(params, 0)->val_ul;
> +    if (reg_num >= cc->gdb_qreg_info_line_count) {
> +        gdb_put_packet("");
> +        return;
> +    }
> +
> +    gdb_put_packet(cc->gdb_qreg_info_lines[reg_num]);
> +}
> +
>  static void handle_query_threads(GArray *params, void *user_ctx)
>  {
>      if (!gdbserver_state.query_cpu) {
> @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>          .handler = handle_query_curr_tid,
>          .cmd = "C",
>      },
> +    {
> +        .handler = handle_query_regs,
> +        .cmd = "RegisterInfo",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },

Where is this defined in the protocol spec, I can't see it in:

  https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets

and it seems to be information that is handled by the xml register
description. Is there a reason that isn't used for Hexagon?

>      {
>          .handler = handle_query_threads,
>          .cmd = "sThreadInfo",


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query

[Matheus Tavares Bernardino]

Alex Bennée <alex.bennee@linaro.org> wrote:
>
> > Matheus Tavares <quic_mathbern@quicinc.com> wrote:
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index be18568d0a..f19f8c58c3 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
> >          .handler = handle_query_curr_tid,
> >          .cmd = "C",
> >      },
> > +    {
> > +        .handler = handle_query_regs,
> > +        .cmd = "RegisterInfo",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
> 
> Where is this defined in the protocol spec, I can't see it in:
> 
>   https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets
> 
> and it seems to be information that is handled by the xml register
> description. Is there a reason that isn't used for Hexagon?

Good point. It's actually an lldb extension to the protocol:
https://github.com/llvm/llvm-project/blob/main/lldb/docs/lldb-gdb-remote.txt#L573

But indeed, lldb should be able to use the xml register description as
well. I'll take a look and try to do that instead.

Thanks!

Re: [PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query

[Alex Bennée]

Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:

> Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> > Matheus Tavares <quic_mathbern@quicinc.com> wrote:
>> >
>> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> > index be18568d0a..f19f8c58c3 100644
>> > --- a/gdbstub/gdbstub.c
>> > +++ b/gdbstub/gdbstub.c
>> > @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>> >          .handler = handle_query_curr_tid,
>> >          .cmd = "C",
>> >      },
>> > +    {
>> > +        .handler = handle_query_regs,
>> > +        .cmd = "RegisterInfo",
>> > +        .cmd_startswith = 1,
>> > +        .schema = "l0"
>> > +    },
>> 
>> Where is this defined in the protocol spec, I can't see it in:
>> 
>>   https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets
>> 
>> and it seems to be information that is handled by the xml register
>> description. Is there a reason that isn't used for Hexagon?
>
> Good point. It's actually an lldb extension to the protocol:
> https://github.com/llvm/llvm-project/blob/main/lldb/docs/lldb-gdb-remote.txt#L573
>
> But indeed, lldb should be able to use the xml register description as
> well. I'll take a look and try to do that instead.

There may be an argument for supporting both but only if the details of
the xml/RegisterInfo would be sorted out by gdbstub or some other common
code rather than each front-end growing special support.

For now see how the XML does for you.

>
> Thanks!


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query

[Philippe Mathieu-Daudé]

On 20/4/23 14:31, Matheus Tavares Bernardino wrote:
> From: Brian Cain <bcain@quicinc.com>
> 
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>   include/hw/core/cpu.h |  4 ++++
>   gdbstub/gdbstub.c     | 27 +++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+)


> @@ -159,6 +161,8 @@ struct CPUClass {
>       vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
>   
>       const char *gdb_core_xml_file;
> +    const char **gdb_qreg_info_lines;

Shouldn't this be 'const char *const *gdb_qreg_info_lines;'?

> +    int gdb_qreg_info_line_count;
>       gchar * (*gdb_arch_name)(CPUState *cpu);
>       const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);

Re: [PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query

[Philippe Mathieu-Daudé]

On 20/4/23 16:04, Philippe Mathieu-Daudé wrote:
> On 20/4/23 14:31, Matheus Tavares Bernardino wrote:
>> From: Brian Cain <bcain@quicinc.com>
>>
>> Signed-off-by: Brian Cain <bcain@quicinc.com>
>> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
>> ---
>>   include/hw/core/cpu.h |  4 ++++
>>   gdbstub/gdbstub.c     | 27 +++++++++++++++++++++++++++
>>   2 files changed, 31 insertions(+)
> 
> 
>> @@ -159,6 +161,8 @@ struct CPUClass {
>>       vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
>>       const char *gdb_core_xml_file;
>> +    const char **gdb_qreg_info_lines;
> 
> Shouldn't this be 'const char *const *gdb_qreg_info_lines;'?
> 
>> +    int gdb_qreg_info_line_count;

Also, unsigned or size_t?

>>       gchar * (*gdb_arch_name)(CPUState *cpu);
>>       const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char 
>> *xmlname);

[PATCH v2 RESEND 4/7] Hexagon: support qRegisterInfo at gdbstub

[Matheus Tavares Bernardino]

From: Brian Cain <bcain@quicinc.com>

Signed-off-by: Brian Cain <bcain@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 target/hexagon/gdb_qreginfo.h | 124 ++++++++++++++++++++++++++++++++++
 target/hexagon/cpu.c          |   3 +
 2 files changed, 127 insertions(+)
 create mode 100644 target/hexagon/gdb_qreginfo.h

diff --git a/target/hexagon/gdb_qreginfo.h b/target/hexagon/gdb_qreginfo.h
new file mode 100644
index 0000000000..64631ddd58
--- /dev/null
+++ b/target/hexagon/gdb_qreginfo.h
@@ -0,0 +1,124 @@
+/*
+ *  Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HEXAGON_GDB_QREGINFO_H
+#define HEXAGON_GDB_QREGINFO_H
+
+const char * const hexagon_qreg_descs[] = {
+    "name:r00;alt-name:r0;bitsize:32;offset=0;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:0;generic:r00;",
+    "name:r01;alt-name:r1;bitsize:32;offset=4;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:1;generic:r01;",
+    "name:r02;alt-name:r2;bitsize:32;offset=8;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:2;generic:r02;",
+    "name:r03;alt-name:r3;bitsize:32;offset=12;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:3;generic:r03;",
+    "name:r04;alt-name:r4;bitsize:32;offset=16;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:4;generic:r04;",
+    "name:r05;alt-name:r5;bitsize:32;offset=20;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:5;generic:r05;",
+    "name:r06;alt-name:r6;bitsize:32;offset=24;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:6;generic:r06;",
+    "name:r07;alt-name:r7;bitsize:32;offset=28;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:7;generic:r07;",
+    "name:r08;alt-name:r8;bitsize:32;offset=32;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:8;generic:r08;",
+    "name:r09;alt-name:r9;bitsize:32;offset=36;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:9;generic:r09;",
+    "name:r10;alt-name:;bitsize:32;offset=40;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:10;generic:;",
+    "name:r11;alt-name:;bitsize:32;offset=44;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:11;generic:;",
+    "name:r12;alt-name:;bitsize:32;offset=48;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:12;generic:;",
+    "name:r13;alt-name:;bitsize:32;offset=52;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:13;generic:;",
+    "name:r14;alt-name:;bitsize:32;offset=56;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:14;generic:;",
+    "name:r15;alt-name:;bitsize:32;offset=60;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:15;generic:;",
+    "name:r16;alt-name:;bitsize:32;offset=64;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:16;generic:;",
+    "name:r17;alt-name:;bitsize:32;offset=68;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:17;generic:;",
+    "name:r18;alt-name:;bitsize:32;offset=72;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:18;generic:;",
+    "name:r19;alt-name:;bitsize:32;offset=76;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:19;generic:;",
+    "name:r20;alt-name:;bitsize:32;offset=80;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:20;generic:;",
+    "name:r21;alt-name:;bitsize:32;offset=84;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:21;generic:;",
+    "name:r22;alt-name:;bitsize:32;offset=88;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:22;generic:;",
+    "name:r23;alt-name:;bitsize:32;offset=92;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:23;generic:;",
+    "name:r24;alt-name:;bitsize:32;offset=96;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:24;generic:;",
+    "name:r25;alt-name:;bitsize:32;offset=100;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:25;generic:;",
+    "name:r26;alt-name:;bitsize:32;offset=104;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:26;generic:;",
+    "name:r27;alt-name:;bitsize:32;offset=108;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:27;generic:;",
+    "name:r28;alt-name:;bitsize:32;offset=112;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:28;generic:;",
+    "name:r29;alt-name:sp;bitsize:32;offset=116;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:29;generic:sp;",
+    "name:r30;alt-name:fp;bitsize:32;offset=120;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:30;generic:fp;",
+    "name:r31;alt-name:ra;bitsize:32;offset=124;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:31;generic:ra;",
+    "name:sa0;alt-name:;bitsize:32;offset=128;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:32;generic:;",
+    "name:lc0;alt-name:;bitsize:32;offset=132;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:33;generic:;",
+    "name:sa1;alt-name:;bitsize:32;offset=136;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:34;generic:;",
+    "name:lc1;alt-name:;bitsize:32;offset=140;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:35;generic:;",
+    "name:p3_0;alt-name:;bitsize:32;offset=144;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:36;generic:;",
+    "name:c5;alt-name:;bitsize:32;offset=148;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:37;generic:;",
+    "name:m0;alt-name:;bitsize:32;offset=152;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:38;generic:;",
+    "name:m1;alt-name:;bitsize:32;offset=156;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:39;generic:;",
+    "name:usr;alt-name:;bitsize:32;offset=160;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:40;generic:;",
+    "name:pc;alt-name:pc;bitsize:32;offset=164;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:41;generic:pc;",
+    "name:ugp;alt-name:;bitsize:32;offset=168;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:42;generic:;",
+    "name:gp;alt-name:;bitsize:32;offset=172;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:43;generic:;",
+    "name:cs0;alt-name:;bitsize:32;offset=176;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:44;generic:;",
+    "name:cs1;alt-name:;bitsize:32;offset=180;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:45;generic:;",
+    "name:upcyclelo;alt-name:;bitsize:32;offset=184;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:46;generic:;",
+    "name:upcyclehi;alt-name:;bitsize:32;offset=188;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:47;generic:;",
+    "name:framelimit;alt-name:;bitsize:32;offset=192;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:48;generic:;",
+    "name:framekey;alt-name:;bitsize:32;offset=196;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:49;generic:;",
+    "name:pktcountlo;alt-name:;bitsize:32;offset=200;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:50;generic:;",
+    "name:pktcounthi;alt-name:;bitsize:32;offset=204;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:51;generic:;",
+    "name:pkt_cnt;alt-name:;bitsize:32;offset=208;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:52;generic:;",
+    "name:insn_cnt;alt-name:;bitsize:32;offset=212;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:53;generic:;",
+    "name:hvx_cnt;alt-name:;bitsize:32;offset=216;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:54;generic:;",
+    "name:c23;alt-name:;bitsize:32;offset=220;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:55;generic:;",
+    "name:c24;alt-name:;bitsize:32;offset=224;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:56;generic:;",
+    "name:c25;alt-name:;bitsize:32;offset=228;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:57;generic:;",
+    "name:c26;alt-name:;bitsize:32;offset=232;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:58;generic:;",
+    "name:c27;alt-name:;bitsize:32;offset=236;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:59;generic:;",
+    "name:c28;alt-name:;bitsize:32;offset=240;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:60;generic:;",
+    "name:c29;alt-name:;bitsize:32;offset=244;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:61;generic:;",
+    "name:utimerlo;alt-name:;bitsize:32;offset=248;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:62;generic:;",
+    "name:utimerhi;alt-name:;bitsize:32;offset=252;variable-size:0;encoding:uint;format:hex;set:Thread Registers;dwarf:63;generic:;",
+    "name:v0;alt-name:;bitsize:1024;offset=256;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:88;generic:;",
+    "name:v1;alt-name:;bitsize:1024;offset=384;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:89;generic:;",
+    "name:v2;alt-name:;bitsize:1024;offset=512;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:90;generic:;",
+    "name:v3;alt-name:;bitsize:1024;offset=640;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:91;generic:;",
+    "name:v4;alt-name:;bitsize:1024;offset=768;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:92;generic:;",
+    "name:v5;alt-name:;bitsize:1024;offset=896;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:93;generic:;",
+    "name:v6;alt-name:;bitsize:1024;offset=1024;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:94;generic:;",
+    "name:v7;alt-name:;bitsize:1024;offset=1152;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:95;generic:;",
+    "name:v8;alt-name:;bitsize:1024;offset=1280;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:96;generic:;",
+    "name:v9;alt-name:;bitsize:1024;offset=1408;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:97;generic:;",
+    "name:v10;alt-name:;bitsize:1024;offset=1536;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:98;generic:;",
+    "name:v11;alt-name:;bitsize:1024;offset=1664;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:99;generic:;",
+    "name:v12;alt-name:;bitsize:1024;offset=1792;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:100;generic:;",
+    "name:v13;alt-name:;bitsize:1024;offset=1920;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:101;generic:;",
+    "name:v14;alt-name:;bitsize:1024;offset=2048;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:102;generic:;",
+    "name:v15;alt-name:;bitsize:1024;offset=2176;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:103;generic:;",
+    "name:v16;alt-name:;bitsize:1024;offset=2304;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:104;generic:;",
+    "name:v17;alt-name:;bitsize:1024;offset=2432;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:105;generic:;",
+    "name:v18;alt-name:;bitsize:1024;offset=2560;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:106;generic:;",
+    "name:v19;alt-name:;bitsize:1024;offset=2688;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:107;generic:;",
+    "name:v20;alt-name:;bitsize:1024;offset=2816;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:108;generic:;",
+    "name:v21;alt-name:;bitsize:1024;offset=2944;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:109;generic:;",
+    "name:v22;alt-name:;bitsize:1024;offset=3072;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:110;generic:;",
+    "name:v23;alt-name:;bitsize:1024;offset=3200;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:111;generic:;",
+    "name:v24;alt-name:;bitsize:1024;offset=3328;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:112;generic:;",
+    "name:v25;alt-name:;bitsize:1024;offset=3456;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:113;generic:;",
+    "name:v26;alt-name:;bitsize:1024;offset=3584;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:114;generic:;",
+    "name:v27;alt-name:;bitsize:1024;offset=3712;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:115;generic:;",
+    "name:v28;alt-name:;bitsize:1024;offset=3840;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:116;generic:;",
+    "name:v29;alt-name:;bitsize:1024;offset=3968;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:117;generic:;",
+    "name:v30;alt-name:;bitsize:1024;offset=4096;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:118;generic:;",
+    "name:v31;alt-name:;bitsize:1024;offset=4224;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:119;generic:;",
+    "name:q0;alt-name:;bitsize:128;offset=4352;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:120;generic:;",
+    "name:q1;alt-name:;bitsize:128;offset=4368;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:121;generic:;",
+    "name:q2;alt-name:;bitsize:128;offset=4384;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:122;generic:;",
+    "name:q3;alt-name:;bitsize:128;offset=4400;variable-size:1;encoding:vector;format:hex;set:HVX Vector Registers;dwarf:123;generic:;",
+};
+
+#endif /* HEXAGON_GDB_QREGINFO_H */
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index ab40cfc283..aa470d9ee4 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -19,6 +19,7 @@
 #include "qemu/qemu-print.h"
 #include "cpu.h"
 #include "internal.h"
+#include "gdb_qreginfo.h"
 #include "exec/exec-all.h"
 #include "qapi/error.h"
 #include "hw/qdev-properties.h"
@@ -358,6 +359,8 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
     cc->get_pc = hexagon_cpu_get_pc;
     cc->gdb_read_register = hexagon_gdb_read_register;
     cc->gdb_write_register = hexagon_gdb_write_register;
+    cc->gdb_qreg_info_lines = (const char **)hexagon_qreg_descs;
+    cc->gdb_qreg_info_line_count = ARRAY_SIZE(hexagon_qreg_descs);
     cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS + NUM_VREGS + NUM_QREGS;
     cc->gdb_stop_before_watchpoint = true;
     cc->disas_set_info = hexagon_cpu_disas_set_info;
-- 
2.39.1

Re: [PATCH v2 RESEND 4/7] Hexagon: support qRegisterInfo at gdbstub

[Philippe Mathieu-Daudé]

On 20/4/23 14:31, Matheus Tavares Bernardino wrote:
> From: Brian Cain <bcain@quicinc.com>
> 
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>   target/hexagon/gdb_qreginfo.h | 124 ++++++++++++++++++++++++++++++++++
>   target/hexagon/cpu.c          |   3 +
>   2 files changed, 127 insertions(+)
>   create mode 100644 target/hexagon/gdb_qreginfo.h
> 
> diff --git a/target/hexagon/gdb_qreginfo.h b/target/hexagon/gdb_qreginfo.h
> new file mode 100644
> index 0000000000..64631ddd58
> --- /dev/null
> +++ b/target/hexagon/gdb_qreginfo.h

This should be gdb_qreginfo.h.inc

> +#ifndef HEXAGON_GDB_QREGINFO_H
> +#define HEXAGON_GDB_QREGINFO_H
> +
> +const char * const hexagon_qreg_descs[] = {

...

> +};
> +
> +#endif /* HEXAGON_GDB_QREGINFO_H */
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index ab40cfc283..aa470d9ee4 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -19,6 +19,7 @@
>   #include "qemu/qemu-print.h"
>   #include "cpu.h"
>   #include "internal.h"
> +#include "gdb_qreginfo.h"
>   #include "exec/exec-all.h"
>   #include "qapi/error.h"
>   #include "hw/qdev-properties.h"
> @@ -358,6 +359,8 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
>       cc->get_pc = hexagon_cpu_get_pc;
>       cc->gdb_read_register = hexagon_gdb_read_register;
>       cc->gdb_write_register = hexagon_gdb_write_register;
> +    cc->gdb_qreg_info_lines = (const char **)hexagon_qreg_descs;

No need to cast if fixing gdb_qreg_info_lines's prototype
(see previous patch review).

> +    cc->gdb_qreg_info_line_count = ARRAY_SIZE(hexagon_qreg_descs);
>       cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS + NUM_VREGS + NUM_QREGS;
>       cc->gdb_stop_before_watchpoint = true;
>       cc->disas_set_info = hexagon_cpu_disas_set_info;

Re: [PATCH v2 RESEND 4/7] Hexagon: support qRegisterInfo at gdbstub

[Matheus Tavares Bernardino]

Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> > Matheus Tavares <quic_mathbern@quicinc.com> wrote:
> > 
> > diff --git a/target/hexagon/gdb_qreginfo.h b/target/hexagon/gdb_qreginfo.h
> > new file mode 100644
> > index 0000000000..64631ddd58
> > --- /dev/null
> > +++ b/target/hexagon/gdb_qreginfo.h
> 
> This should be gdb_qreginfo.h.inc

Ok, thanks.

> > @@ -358,6 +359,8 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
> >       cc->get_pc = hexagon_cpu_get_pc;
> >       cc->gdb_read_register = hexagon_gdb_read_register;
> >       cc->gdb_write_register = hexagon_gdb_write_register;
> > +    cc->gdb_qreg_info_lines = (const char **)hexagon_qreg_descs;
> 
> No need to cast if fixing gdb_qreg_info_lines's prototype
> (see previous patch review).

Ah, good call, thanks. I'll try to avoid the whole qRegisterInfo
implemenation in the next round, as Alex suggested [1]. But if not
possible, I'll make sure to add this changes to the types and cast.
Thanks!

[1]: https://lore.kernel.org/qemu-devel/20230421113420.67122-1-quic_mathbern@quicinc.com/

[PATCH v2 RESEND 5/7] Hexagon (gdbstub): fix p3:0 read and write via stub

[Matheus Tavares Bernardino]

From: Brian Cain <bcain@quicinc.com>

Co-authored-by: Sid Manning <sidneym@quicinc.com>
Signed-off-by: Sid Manning <sidneym@quicinc.com>
Signed-off-by: Brian Cain <bcain@quicinc.com>
Co-authored-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/gdbstub.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index 46083da620..a06fed9f18 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -25,6 +25,14 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     HexagonCPU *cpu = HEXAGON_CPU(cs);
     CPUHexagonState *env = &cpu->env;
 
+    if (n == HEX_REG_P3_0_ALIASED) {
+        uint32_t p3_0 = 0;
+        for (int i = 0; i < NUM_PREGS; i++) {
+            p3_0 = deposit32(p3_0, i * 8, 8, env->pred[i]);
+        }
+        return gdb_get_regl(mem_buf, p3_0);
+    }
+
     if (n < TOTAL_PER_THREAD_REGS) {
         return gdb_get_regl(mem_buf, env->gpr[n]);
     }
@@ -37,6 +45,14 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     HexagonCPU *cpu = HEXAGON_CPU(cs);
     CPUHexagonState *env = &cpu->env;
 
+    if (n == HEX_REG_P3_0_ALIASED) {
+        uint32_t p3_0 = ldtul_p(mem_buf);
+        for (int i = 0; i < NUM_PREGS; i++) {
+            env->pred[i] = extract32(p3_0, i * 8, 8);
+        }
+        return sizeof(target_ulong);
+    }
+
     if (n < TOTAL_PER_THREAD_REGS) {
         env->gpr[n] = ldtul_p(mem_buf);
         return sizeof(target_ulong);
-- 
2.39.1

[PATCH v2 RESEND 6/7] Hexagon (gdbstub): add HVX support

[Matheus Tavares Bernardino]

From: Taylor Simpson <tsimpson@quicinc.com>

Co-authored-by: Brian Cain <bcain@quicinc.com>
Signed-off-by: Brian Cain <bcain@quicinc.com>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
Co-authored-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 target/hexagon/gdbstub.c | 60 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index a06fed9f18..6c5a15da68 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -20,6 +20,26 @@
 #include "cpu.h"
 #include "internal.h"
 
+static int gdb_get_vreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+    int total = 0;
+    int i;
+    for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+        total += gdb_get_regl(mem_buf, env->VRegs[n].uw[i]);
+    }
+    return total;
+}
+
+static int gdb_get_qreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+    int total = 0;
+    int i;
+    for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+        total += gdb_get_regl(mem_buf, env->QRegs[n].uw[i]);
+    }
+    return total;
+}
+
 int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 {
     HexagonCPU *cpu = HEXAGON_CPU(cs);
@@ -36,10 +56,40 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     if (n < TOTAL_PER_THREAD_REGS) {
         return gdb_get_regl(mem_buf, env->gpr[n]);
     }
+    n -= TOTAL_PER_THREAD_REGS;
+
+    if (n < NUM_VREGS) {
+        return gdb_get_vreg(env, mem_buf, n);
+    }
+    n -= NUM_VREGS;
+
+    if (n < NUM_QREGS) {
+        return gdb_get_qreg(env, mem_buf, n);
+    }
 
     g_assert_not_reached();
 }
 
+static int gdb_put_vreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+    int i;
+    for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+        env->VRegs[n].uw[i] = ldtul_p(mem_buf);
+        mem_buf += 4;
+    }
+    return MAX_VEC_SIZE_BYTES;
+}
+
+static int gdb_put_qreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+    int i;
+    for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+        env->QRegs[n].uw[i] = ldtul_p(mem_buf);
+        mem_buf += 4;
+    }
+    return MAX_VEC_SIZE_BYTES / 8;
+}
+
 int hexagon_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 {
     HexagonCPU *cpu = HEXAGON_CPU(cs);
@@ -57,6 +107,16 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         env->gpr[n] = ldtul_p(mem_buf);
         return sizeof(target_ulong);
     }
+    n -= TOTAL_PER_THREAD_REGS;
+
+    if (n < NUM_VREGS) {
+        return gdb_put_vreg(env, mem_buf, n);
+    }
+    n -= NUM_VREGS;
+
+    if (n < NUM_QREGS) {
+        return gdb_put_qreg(env, mem_buf, n);
+    }
 
     g_assert_not_reached();
 }
-- 
2.39.1

[PATCH v2 RESEND 7/7] Hexagon (linux-user/hexagon): handle breakpoints

[Matheus Tavares Bernardino]

From: Taylor Simpson <tsimpson@quicinc.com>

This enables LLDB to work with hexagon linux-user mode through the GDB
remote protocol.

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 linux-user/hexagon/cpu_loop.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index b84e25bf71..00c12bbb55 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -33,6 +33,7 @@ void cpu_loop(CPUHexagonState *env)
     target_ulong ret;
 
     for (;;) {
+        target_siginfo_t info;
         cpu_exec_start(cs);
         trapnr = cpu_exec(cs);
         cpu_exec_end(cs);
@@ -63,6 +64,15 @@ void cpu_loop(CPUHexagonState *env)
         case EXCP_ATOMIC:
             cpu_exec_step_atomic(cs);
             break;
+        case EXCP_DEBUG:
+            info = (target_siginfo_t) {
+                .si_signo = TARGET_SIGTRAP,
+                .si_errno = 0,
+                .si_code = TARGET_TRAP_BRKPT,
+                ._sifields._sigfault._addr = 0
+            };
+            queue_signal(env, info.si_signo, QEMU_SI_KILL, &info);
+            break;
         default:
             EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n",
                      trapnr);
-- 
2.39.1

Re: [PATCH v2 RESEND 7/7] Hexagon (linux-user/hexagon): handle breakpoints

[Richard Henderson]

On 4/20/23 13:31, Matheus Tavares Bernardino wrote:
> From: Taylor Simpson <tsimpson@quicinc.com>
> 
> This enables LLDB to work with hexagon linux-user mode through the GDB
> remote protocol.
> 
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>   linux-user/hexagon/cpu_loop.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
> index b84e25bf71..00c12bbb55 100644
> --- a/linux-user/hexagon/cpu_loop.c
> +++ b/linux-user/hexagon/cpu_loop.c
> @@ -33,6 +33,7 @@ void cpu_loop(CPUHexagonState *env)
>       target_ulong ret;
>   
>       for (;;) {
> +        target_siginfo_t info;
>           cpu_exec_start(cs);
>           trapnr = cpu_exec(cs);
>           cpu_exec_end(cs);
> @@ -63,6 +64,15 @@ void cpu_loop(CPUHexagonState *env)
>           case EXCP_ATOMIC:
>               cpu_exec_step_atomic(cs);
>               break;
> +        case EXCP_DEBUG:
> +            info = (target_siginfo_t) {
> +                .si_signo = TARGET_SIGTRAP,
> +                .si_errno = 0,
> +                .si_code = TARGET_TRAP_BRKPT,
> +                ._sifields._sigfault._addr = 0
> +            };
> +            queue_signal(env, info.si_signo, QEMU_SI_KILL, &info);

Use force_sig_fault.


r~