[Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler

[Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler

[Jon Doron]

This patch series refactors the old gdbstub command packets handler
with a new infrastructure which should ease extending and adding new
and missing gdb command packets.

version 9 changes:
- checkpatch fixes

version 8 changes:
- Add new command to display the Supported qemu generic query/sets
- kvm: Add API to read/write a MSR
- Add new commands specific for qemu:
  * Command to swap the memory GDB sees to be the physical memory
  * Commands to read and write a MSR

version 7 changes:
- Fixed few checkpatch complaints
- Feedback from Alex Bennee

version 4-6 changes:
- mostly feedback from Richard Henderson

version 3 changes
- Split the single patch to many individual patches for easier reviewing

version 2 changes
- Code convention fixes

Jon Doron (27):
  gdbstub: Add infrastructure to parse cmd packets
  gdbstub: Implement deatch (D pkt) with new infra
  gdbstub: Implement thread_alive (T pkt) with new infra
  gdbstub: Implement continue (c pkt) with new infra
  gdbstub: Implement continue with signal (C pkt) with new infra
  gdbstub: Implement set_thread (H pkt) with new infra
  gdbstub: Implement insert breakpoint (Z pkt) with new infra
  gdbstub: Implement remove breakpoint (z pkt) with new infra
  gdbstub: Implement set register (P pkt) with new infra
  gdbstub: Implement get register (p pkt) with new infra
  gdbstub: Implement write memory (M pkt) with new infra
  gdbstub: Implement read memory (m pkt) with new infra
  gdbstub: Implement write all registers (G pkt) with new infra
  gdbstub: Implement read all registers (g pkt) with new infra
  gdbstub: Implement file io (F pkt) with new infra
  gdbstub: Implement step (s pkt) with new infra
  gdbstub: Implement v commands with new infra
  gdbstub: Implement generic query (q pkt) with new infra
  gdbstub: Implement generic set (Q pkt) with new infra
  gdbstub: Implement target halted (? pkt) with new infra
  gdbstub: Clear unused variables in gdb_handle_packet
  gdbstub: Implement generic query qemu.Supported
  gdbstub: Implement qemu physical memory mode
  gdbstub: Add another handler for setting qemu.sstep
  kvm: Add API to read/write a CPU MSR value
  gdbstub: Add support to read a MSR for KVM target
  gdbstub: Add support to write a MSR for KVM target

 accel/kvm/kvm-all.c  |   39 +
 gdbstub.c            | 1807 ++++++++++++++++++++++++++++++------------
 include/sysemu/kvm.h |    2 +
 3 files changed, 1359 insertions(+), 489 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v9 01/27] gdbstub: Add infrastructure to parse cmd packets

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 200 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index d54abd17cc..d5e0f3878a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1268,6 +1268,206 @@ out:
     return res;
 }
 
+typedef union GdbCmdVariant {
+    const char *data;
+    uint8_t opcode;
+    unsigned long val_ul;
+    unsigned long long val_ull;
+    struct {
+        GDBThreadIdKind kind;
+        uint32_t pid;
+        uint32_t tid;
+    } thread_id;
+} GdbCmdVariant;
+
+static const char *cmd_next_param(const char *param, const char delimiter)
+{
+    static const char all_delimiters[] = ",;:=";
+    char curr_delimiters[2] = {0};
+    const char *delimiters;
+
+    if (delimiter == '?') {
+        delimiters = all_delimiters;
+    } else if (delimiter == '0') {
+        return strchr(param, '\0');
+    } else if (delimiter == '.' && *param) {
+        return param + 1;
+    } else {
+        curr_delimiters[0] = delimiter;
+        delimiters = curr_delimiters;
+    }
+
+    param += strcspn(param, delimiters);
+    if (*param) {
+        param++;
+    }
+    return param;
+}
+
+static int cmd_parse_params(const char *data, const char *schema,
+                            GdbCmdVariant *params, int *num_params)
+{
+    int curr_param;
+    const char *curr_schema, *curr_data;
+
+    *num_params = 0;
+
+    if (!schema) {
+        return 0;
+    }
+
+    curr_schema = schema;
+    curr_param = 0;
+    curr_data = data;
+    while (curr_schema[0] && curr_schema[1] && *curr_data) {
+        switch (curr_schema[0]) {
+        case 'l':
+            if (qemu_strtoul(curr_data, &curr_data, 16,
+                             &params[curr_param].val_ul)) {
+                return -EINVAL;
+            }
+            curr_param++;
+            curr_data = cmd_next_param(curr_data, curr_schema[1]);
+            break;
+        case 'L':
+            if (qemu_strtou64(curr_data, &curr_data, 16,
+                              (uint64_t *)&params[curr_param].val_ull)) {
+                return -EINVAL;
+            }
+            curr_param++;
+            curr_data = cmd_next_param(curr_data, curr_schema[1]);
+            break;
+        case 's':
+            params[curr_param].data = curr_data;
+            curr_param++;
+            curr_data = cmd_next_param(curr_data, curr_schema[1]);
+            break;
+        case 'o':
+            params[curr_param].opcode = *(uint8_t *)curr_data;
+            curr_param++;
+            curr_data = cmd_next_param(curr_data, curr_schema[1]);
+            break;
+        case 't':
+            params[curr_param].thread_id.kind =
+                read_thread_id(curr_data, &curr_data,
+                               &params[curr_param].thread_id.pid,
+                               &params[curr_param].thread_id.tid);
+            curr_param++;
+            curr_data = cmd_next_param(curr_data, curr_schema[1]);
+            break;
+        case '?':
+            curr_data = cmd_next_param(curr_data, curr_schema[1]);
+            break;
+        default:
+            return -EINVAL;
+        }
+        curr_schema += 2;
+    }
+
+    *num_params = curr_param;
+    return 0;
+}
+
+typedef struct GdbCmdContext {
+    GDBState *s;
+    GdbCmdVariant *params;
+    int num_params;
+    uint8_t mem_buf[MAX_PACKET_LENGTH];
+    char str_buf[MAX_PACKET_LENGTH + 1];
+} GdbCmdContext;
+
+typedef void (*GdbCmdHandler)(GdbCmdContext *gdb_ctx, void *user_ctx);
+
+/*
+ * cmd_startswith -> cmd is compared using startswith
+ *
+ *
+ * schema definitions:
+ * Each schema parameter entry consists of 2 chars,
+ * the first char represents the parameter type handling
+ * the second char represents the delimiter for the next parameter
+ *
+ * Currently supported schema types:
+ * 'l' -> unsigned long (stored in .val_ul)
+ * 'L' -> unsigned long long (stored in .val_ull)
+ * 's' -> string (stored in .data)
+ * 'o' -> single char (stored in .opcode)
+ * 't' -> thread id (stored in .thread_id)
+ * '?' -> skip according to delimiter
+ *
+ * Currently supported delimiters:
+ * '?' -> Stop at any delimiter (",;:=\0")
+ * '0' -> Stop at "\0"
+ * '.' -> Skip 1 char unless reached "\0"
+ * Any other value is treated as the delimiter value itself
+ */
+typedef struct GdbCmdParseEntry {
+    GdbCmdHandler handler;
+    const char *cmd;
+    union {
+        int flags;
+        struct {
+            int cmd_startswith:1;
+        };
+    };
+    const char *schema;
+} GdbCmdParseEntry;
+
+static inline int startswith(const char *string, const char *pattern)
+{
+  return !strncmp(string, pattern, strlen(pattern));
+}
+
+static int process_string_cmd(
+        GDBState *s, void *user_ctx, const char *data,
+        const GdbCmdParseEntry *cmds, int num_cmds)
+        __attribute__((unused));
+
+static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
+                              const GdbCmdParseEntry *cmds, int num_cmds)
+{
+    int i, schema_len, max_num_params = 0;
+    GdbCmdContext gdb_ctx;
+
+    if (!cmds) {
+        return -1;
+    }
+
+    for (i = 0; i < num_cmds; i++) {
+        const GdbCmdParseEntry *cmd = &cmds[i];
+        g_assert(cmd->handler && cmd->cmd);
+
+        if ((cmd->cmd_startswith && !startswith(data, cmd->cmd)) ||
+            (!cmd->cmd_startswith && strcmp(cmd->cmd, data))) {
+            continue;
+        }
+
+        if (cmd->schema) {
+            schema_len = strlen(cmd->schema);
+            if (schema_len % 2) {
+                return -2;
+            }
+
+            max_num_params = schema_len / 2;
+        }
+
+        gdb_ctx.params =
+            (GdbCmdVariant *)alloca(sizeof(*gdb_ctx.params) * max_num_params);
+        memset(gdb_ctx.params, 0, sizeof(*gdb_ctx.params) * max_num_params);
+
+        if (cmd_parse_params(&data[strlen(cmd->cmd)], cmd->schema,
+                             gdb_ctx.params, &gdb_ctx.num_params)) {
+            return -1;
+        }
+
+        gdb_ctx.s = s;
+        cmd->handler(&gdb_ctx, user_ctx);
+        return 0;
+    }
+
+    return -1;
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
-- 
2.20.1

[Qemu-devel] [PATCH v9 02/27] gdbstub: Implement deatch (D pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 90 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 40 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index d5e0f3878a..621d689868 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern)
   return !strncmp(string, pattern, strlen(pattern));
 }
 
-static int process_string_cmd(
-        GDBState *s, void *user_ctx, const char *data,
-        const GdbCmdParseEntry *cmds, int num_cmds)
-        __attribute__((unused));
-
 static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
                               const GdbCmdParseEntry *cmds, int num_cmds)
 {
@@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
     return -1;
 }
 
+static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    GDBProcess *process;
+    GDBState *s = gdb_ctx->s;
+    uint32_t pid = 1;
+
+    if (s->multiprocess) {
+        if (!gdb_ctx->num_params) {
+            put_packet(s, "E22");
+            return;
+        }
+
+        pid = gdb_ctx->params[0].val_ul;
+    }
+
+    process = gdb_get_process(s, pid);
+    gdb_process_breakpoint_remove_all(s, process);
+    process->attached = false;
+
+    if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
+        s->c_cpu = gdb_first_attached_cpu(s);
+    }
+
+    if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
+        s->g_cpu = gdb_first_attached_cpu(s);
+    }
+
+    if (!s->c_cpu) {
+        /* No more process attached */
+        gdb_syscall_mode = GDB_SYS_DISABLED;
+        gdb_continue(s);
+    }
+    put_packet(s, "OK");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     uint8_t *registers;
     target_ulong addr, len;
     GDBThreadIdKind thread_kind;
+    const GdbCmdParseEntry *cmd_parser = NULL;
 
     trace_gdbstub_io_command(line_buf);
 
@@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         error_report("QEMU: Terminated via GDBstub");
         exit(0);
     case 'D':
-        /* Detach packet */
-        pid = 1;
-
-        if (s->multiprocess) {
-            unsigned long lpid;
-            if (*p != ';') {
-                put_packet(s, "E22");
-                break;
-            }
-
-            if (qemu_strtoul(p + 1, &p, 16, &lpid)) {
-                put_packet(s, "E22");
-                break;
-            }
-
-            pid = lpid;
-        }
-
-        process = gdb_get_process(s, pid);
-        gdb_process_breakpoint_remove_all(s, process);
-        process->attached = false;
-
-        if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
-            s->c_cpu = gdb_first_attached_cpu(s);
-        }
-
-        if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
-            s->g_cpu = gdb_first_attached_cpu(s);
-        }
-
-        if (s->c_cpu == NULL) {
-            /* No more process attached */
-            gdb_syscall_mode = GDB_SYS_DISABLED;
-            gdb_continue(s);
+        {
+            static const GdbCmdParseEntry detach_cmd_desc = {
+                .handler = handle_detach,
+                .cmd = "D",
+                .cmd_startswith = 1,
+                .schema = "?.l0"
+            };
+            cmd_parser = &detach_cmd_desc;
         }
-        put_packet(s, "OK");
         break;
     case 's':
         if (*p != '\0') {
@@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         put_packet(s, buf);
         break;
     }
+
+    if (cmd_parser &&
+        process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) {
+        put_packet(s, "");
+    }
+
     return RS_IDLE;
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v9 03/27] gdbstub: Implement thread_alive (T pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 621d689868..c47ef7dd9c 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1498,6 +1498,30 @@ static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(s, "OK");
 }
 
+static void handle_thread_alive(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    CPUState *cpu;
+
+    if (!gdb_ctx->num_params) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (gdb_ctx->params[0].thread_id.kind == GDB_READ_THREAD_ERR) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[0].thread_id.pid,
+                      gdb_ctx->params[0].thread_id.tid);
+    if (!cpu) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    put_packet(gdb_ctx->s, "OK");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1798,17 +1822,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'T':
-        thread_kind = read_thread_id(p, &p, &pid, &tid);
-        if (thread_kind == GDB_READ_THREAD_ERR) {
-            put_packet(s, "E22");
-            break;
-        }
-        cpu = gdb_get_cpu(s, pid, tid);
-
-        if (cpu != NULL) {
-            put_packet(s, "OK");
-        } else {
-            put_packet(s, "E22");
+        {
+            static const GdbCmdParseEntry thread_alive_cmd_desc = {
+                .handler = handle_thread_alive,
+                .cmd = "T",
+                .cmd_startswith = 1,
+                .schema = "t0"
+            };
+            cmd_parser = &thread_alive_cmd_desc;
         }
         break;
     case 'q':
-- 
2.20.1

[Qemu-devel] [PATCH v9 04/27] gdbstub: Implement continue (c pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index c47ef7dd9c..89f1ab6524 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1522,6 +1522,16 @@ static void handle_thread_alive(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "OK");
 }
 
+static void handle_continue(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (gdb_ctx->num_params) {
+        gdb_set_cpu_pc(gdb_ctx->s, gdb_ctx->params[0].val_ull);
+    }
+
+    gdb_ctx->s->signal = 0;
+    gdb_continue(gdb_ctx->s);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1558,13 +1568,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         gdb_breakpoint_remove_all();
         break;
     case 'c':
-        if (*p != '\0') {
-            addr = strtoull(p, (char **)&p, 16);
-            gdb_set_cpu_pc(s, addr);
+        {
+            static const GdbCmdParseEntry continue_cmd_desc = {
+                .handler = handle_continue,
+                .cmd = "c",
+                .cmd_startswith = 1,
+                .schema = "L0"
+            };
+            cmd_parser = &continue_cmd_desc;
         }
-        s->signal = 0;
-        gdb_continue(s);
-        return RS_IDLE;
+        break;
     case 'C':
         s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
         if (s->signal == -1)
-- 
2.20.1

[Qemu-devel] [PATCH v9 05/27] gdbstub: Implement continue with signal (C pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 89f1ab6524..469aaeb875 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1532,6 +1532,21 @@ static void handle_continue(GdbCmdContext *gdb_ctx, void *user_ctx)
     gdb_continue(gdb_ctx->s);
 }
 
+static void handle_cont_with_sig(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    unsigned long signal = 0;
+
+    if (gdb_ctx->num_params) {
+        signal = gdb_ctx->params[0].val_ul;
+    }
+
+    gdb_ctx->s->signal = gdb_signal_to_target(signal);
+    if (gdb_ctx->s->signal == -1) {
+        gdb_ctx->s->signal = 0;
+    }
+    gdb_continue(gdb_ctx->s);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1579,11 +1594,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'C':
-        s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
-        if (s->signal == -1)
-            s->signal = 0;
-        gdb_continue(s);
-        return RS_IDLE;
+        {
+            static const GdbCmdParseEntry cont_with_sig_cmd_desc = {
+                .handler = handle_cont_with_sig,
+                .cmd = "C",
+                .cmd_startswith = 1,
+                .schema = "l0"
+            };
+            cmd_parser = &cont_with_sig_cmd_desc;
+        }
+        break;
     case 'v':
         if (strncmp(p, "Cont", 4) == 0) {
             p += 4;
-- 
2.20.1

[Qemu-devel] [PATCH v9 06/27] gdbstub: Implement set_thread (H pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 79 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 30 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 469aaeb875..21cdaf4678 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1547,6 +1547,47 @@ static void handle_cont_with_sig(GdbCmdContext *gdb_ctx, void *user_ctx)
     gdb_continue(gdb_ctx->s);
 }
 
+static void handle_set_thread(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    CPUState *cpu;
+
+    if (gdb_ctx->num_params < 2) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (gdb_ctx->params[1].thread_id.kind == GDB_READ_THREAD_ERR) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (gdb_ctx->params[1].thread_id.kind != GDB_ONE_THREAD) {
+        put_packet(gdb_ctx->s, "OK");
+        return;
+    }
+
+    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[1].thread_id.pid,
+                      gdb_ctx->params[1].thread_id.tid);
+    if (!cpu) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    switch (gdb_ctx->params[0].opcode) {
+    case 'c':
+        gdb_ctx->s->c_cpu = cpu;
+        put_packet(gdb_ctx->s, "OK");
+        break;
+    case 'g':
+        gdb_ctx->s->g_cpu = cpu;
+        put_packet(gdb_ctx->s, "OK");
+        break;
+    default:
+        put_packet(gdb_ctx->s, "E22");
+        break;
+    }
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1560,7 +1601,6 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     char thread_id[16];
     uint8_t *registers;
     target_ulong addr, len;
-    GDBThreadIdKind thread_kind;
     const GdbCmdParseEntry *cmd_parser = NULL;
 
     trace_gdbstub_io_command(line_buf);
@@ -1823,35 +1863,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             put_packet(s, "E22");
         break;
     case 'H':
-        type = *p++;
-
-        thread_kind = read_thread_id(p, &p, &pid, &tid);
-        if (thread_kind == GDB_READ_THREAD_ERR) {
-            put_packet(s, "E22");
-            break;
-        }
-
-        if (thread_kind != GDB_ONE_THREAD) {
-            put_packet(s, "OK");
-            break;
-        }
-        cpu = gdb_get_cpu(s, pid, tid);
-        if (cpu == NULL) {
-            put_packet(s, "E22");
-            break;
-        }
-        switch (type) {
-        case 'c':
-            s->c_cpu = cpu;
-            put_packet(s, "OK");
-            break;
-        case 'g':
-            s->g_cpu = cpu;
-            put_packet(s, "OK");
-            break;
-        default:
-             put_packet(s, "E22");
-             break;
+        {
+            static const GdbCmdParseEntry set_thread_cmd_desc = {
+                .handler = handle_set_thread,
+                .cmd = "H",
+                .cmd_startswith = 1,
+                .schema = "o.t0"
+            };
+            cmd_parser = &set_thread_cmd_desc;
         }
         break;
     case 'T':
-- 
2.20.1

[Qemu-devel] [PATCH v9 07/27] gdbstub: Implement insert breakpoint (Z pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 21cdaf4678..36c7353a22 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1588,6 +1588,29 @@ static void handle_set_thread(GdbCmdContext *gdb_ctx, void *user_ctx)
     }
 }
 
+static void handle_insert_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int res;
+
+    if (gdb_ctx->num_params < 3) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    res = gdb_breakpoint_insert(gdb_ctx->params[1].val_ull,
+                                gdb_ctx->params[2].val_ull,
+                                gdb_ctx->params[0].val_ul);
+    if (res >= 0) {
+        put_packet(gdb_ctx->s, "OK");
+        return;
+    } else if (res == -ENOSYS) {
+        put_packet(gdb_ctx->s, "");
+        return;
+    }
+
+    put_packet(gdb_ctx->s, "E22");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1843,6 +1866,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         put_packet(s, "OK");
         break;
     case 'Z':
+        {
+            static const GdbCmdParseEntry insert_bp_cmd_desc = {
+                .handler = handle_insert_bp,
+                .cmd = "Z",
+                .cmd_startswith = 1,
+                .schema = "l?L?L0"
+            };
+            cmd_parser = &insert_bp_cmd_desc;
+        }
+        break;
     case 'z':
         type = strtoul(p, (char **)&p, 16);
         if (*p == ',')
-- 
2.20.1

[Qemu-devel] [PATCH v9 08/27] gdbstub: Implement remove breakpoint (z pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 36c7353a22..b42425b24c 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1611,6 +1611,29 @@ static void handle_insert_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "E22");
 }
 
+static void handle_remove_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int res;
+
+    if (gdb_ctx->num_params < 3) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    res = gdb_breakpoint_remove(gdb_ctx->params[1].val_ull,
+                                gdb_ctx->params[2].val_ull,
+                                gdb_ctx->params[0].val_ul);
+    if (res >= 0) {
+        put_packet(gdb_ctx->s, "OK");
+        return;
+    } else if (res == -ENOSYS) {
+        put_packet(gdb_ctx->s, "");
+        return;
+    }
+
+    put_packet(gdb_ctx->s, "E22");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1877,23 +1900,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'z':
-        type = strtoul(p, (char **)&p, 16);
-        if (*p == ',')
-            p++;
-        addr = strtoull(p, (char **)&p, 16);
-        if (*p == ',')
-            p++;
-        len = strtoull(p, (char **)&p, 16);
-        if (ch == 'Z')
-            res = gdb_breakpoint_insert(addr, len, type);
-        else
-            res = gdb_breakpoint_remove(addr, len, type);
-        if (res >= 0)
-             put_packet(s, "OK");
-        else if (res == -ENOSYS)
-            put_packet(s, "");
-        else
-            put_packet(s, "E22");
+        {
+            static const GdbCmdParseEntry remove_bp_cmd_desc = {
+                .handler = handle_remove_bp,
+                .cmd = "z",
+                .cmd_startswith = 1,
+                .schema = "l?L?L0"
+            };
+            cmd_parser = &remove_bp_cmd_desc;
+        }
         break;
     case 'H':
         {
-- 
2.20.1

[Qemu-devel] [PATCH v9 09/27] gdbstub: Implement set register (P pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index b42425b24c..10e3f12a68 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1634,6 +1634,27 @@ static void handle_remove_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "E22");
 }
 
+static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int reg_size;
+
+    if (!gdb_has_xml) {
+        put_packet(gdb_ctx->s, "");
+        return;
+    }
+
+    if (gdb_ctx->num_params < 2) {
+        put_packet(gdb_ctx->s, "");
+        return;
+    }
+
+    reg_size = strlen(gdb_ctx->params[1].data) / 2;
+    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[1].data, reg_size);
+    gdb_write_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf,
+                       gdb_ctx->params[0].val_ull);
+    put_packet(gdb_ctx->s, "OK");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1878,15 +1899,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'P':
-        if (!gdb_has_xml)
-            goto unknown_command;
-        addr = strtoull(p, (char **)&p, 16);
-        if (*p == '=')
-            p++;
-        reg_size = strlen(p) / 2;
-        hextomem(mem_buf, p, reg_size);
-        gdb_write_register(s->g_cpu, mem_buf, addr);
-        put_packet(s, "OK");
+        {
+            static const GdbCmdParseEntry set_reg_cmd_desc = {
+                .handler = handle_set_reg,
+                .cmd = "P",
+                .cmd_startswith = 1,
+                .schema = "L?s0"
+            };
+            cmd_parser = &set_reg_cmd_desc;
+        }
         break;
     case 'Z':
         {
-- 
2.20.1

[Qemu-devel] [PATCH v9 10/27] gdbstub: Implement get register (p pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 10e3f12a68..e9a3d0c2bc 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1655,6 +1655,36 @@ static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "OK");
 }
 
+static void handle_get_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int reg_size;
+
+    /*
+     * Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
+     * This works, but can be very slow.  Anything new enough to
+     * understand XML also knows how to use this properly.
+     */
+    if (!gdb_has_xml) {
+        put_packet(gdb_ctx->s, "");
+        return;
+    }
+
+    if (!gdb_ctx->num_params) {
+        put_packet(gdb_ctx->s, "E14");
+        return;
+    }
+
+    reg_size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf,
+                                 gdb_ctx->params[0].val_ull);
+    if (!reg_size) {
+        put_packet(gdb_ctx->s, "E14");
+        return;
+    }
+
+    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, reg_size);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1884,18 +1914,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'p':
-        /* Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
-           This works, but can be very slow.  Anything new enough to
-           understand XML also knows how to use this properly.  */
-        if (!gdb_has_xml)
-            goto unknown_command;
-        addr = strtoull(p, (char **)&p, 16);
-        reg_size = gdb_read_register(s->g_cpu, mem_buf, addr);
-        if (reg_size) {
-            memtohex(buf, mem_buf, reg_size);
-            put_packet(s, buf);
-        } else {
-            put_packet(s, "E14");
+        {
+            static const GdbCmdParseEntry get_reg_cmd_desc = {
+                .handler = handle_get_reg,
+                .cmd = "p",
+                .cmd_startswith = 1,
+                .schema = "L0"
+            };
+            cmd_parser = &get_reg_cmd_desc;
         }
         break;
     case 'P':
-- 
2.20.1

[Qemu-devel] [PATCH v9 11/27] gdbstub: Implement write memory (M pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 51 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index e9a3d0c2bc..8dc2e1d507 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1685,6 +1685,31 @@ static void handle_get_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, gdb_ctx->str_buf);
 }
 
+static void handle_write_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (gdb_ctx->num_params < 3) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    /* hextomem() reads 2*len bytes */
+    if (gdb_ctx->params[1].val_ull > strlen(gdb_ctx->params[2].data) / 2) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[2].data,
+             gdb_ctx->params[1].val_ull);
+    if (target_memory_rw_debug(gdb_ctx->s->g_cpu, gdb_ctx->params[0].val_ull,
+                               gdb_ctx->mem_buf,
+                               gdb_ctx->params[1].val_ull, true)) {
+        put_packet(gdb_ctx->s, "E14");
+        return;
+    }
+
+    put_packet(gdb_ctx->s, "OK");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1893,24 +1918,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'M':
-        addr = strtoull(p, (char **)&p, 16);
-        if (*p == ',')
-            p++;
-        len = strtoull(p, (char **)&p, 16);
-        if (*p == ':')
-            p++;
-
-        /* hextomem() reads 2*len bytes */
-        if (len > strlen(p) / 2) {
-            put_packet (s, "E22");
-            break;
-        }
-        hextomem(mem_buf, p, len);
-        if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
-                                   true) != 0) {
-            put_packet(s, "E14");
-        } else {
-            put_packet(s, "OK");
+        {
+            static const GdbCmdParseEntry write_mem_cmd_desc = {
+                .handler = handle_write_mem,
+                .cmd = "M",
+                .cmd_startswith = 1,
+                .schema = "L,L:s0"
+            };
+            cmd_parser = &write_mem_cmd_desc;
         }
         break;
     case 'p':
-- 
2.20.1

[Qemu-devel] [PATCH v9 12/27] gdbstub: Implement read memory (m pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 8dc2e1d507..daa602edc3 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1710,6 +1710,30 @@ static void handle_write_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "OK");
 }
 
+static void handle_read_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (gdb_ctx->num_params < 2) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    /* memtohex() doubles the required space */
+    if (gdb_ctx->params[1].val_ull > MAX_PACKET_LENGTH / 2) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (target_memory_rw_debug(gdb_ctx->s->g_cpu, gdb_ctx->params[0].val_ull,
+                               gdb_ctx->mem_buf,
+                               gdb_ctx->params[1].val_ull, false)) {
+        put_packet(gdb_ctx->s, "E14");
+        return;
+    }
+
+    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, gdb_ctx->params[1].val_ull);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1899,22 +1923,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         put_packet(s, "OK");
         break;
     case 'm':
-        addr = strtoull(p, (char **)&p, 16);
-        if (*p == ',')
-            p++;
-        len = strtoull(p, NULL, 16);
-
-        /* memtohex() doubles the required space */
-        if (len > MAX_PACKET_LENGTH / 2) {
-            put_packet (s, "E22");
-            break;
-        }
-
-        if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
-            put_packet (s, "E14");
-        } else {
-            memtohex(buf, mem_buf, len);
-            put_packet(s, buf);
+        {
+            static const GdbCmdParseEntry read_mem_cmd_desc = {
+                .handler = handle_read_mem,
+                .cmd = "m",
+                .cmd_startswith = 1,
+                .schema = "L,L0"
+            };
+            cmd_parser = &read_mem_cmd_desc;
         }
         break;
     case 'M':
-- 
2.20.1

[Qemu-devel] [PATCH v9 13/27] gdbstub: Implement write all registers (G pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index daa602edc3..adfe39b3a3 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1734,6 +1734,29 @@ static void handle_read_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, gdb_ctx->str_buf);
 }
 
+static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    target_ulong addr, len;
+    uint8_t *registers;
+    int reg_size;
+
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    cpu_synchronize_state(gdb_ctx->s->g_cpu);
+    registers = gdb_ctx->mem_buf;
+    len = strlen(gdb_ctx->params[0].data) / 2;
+    hextomem(registers, gdb_ctx->params[0].data, len);
+    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs && len > 0;
+         addr++) {
+        reg_size = gdb_write_register(gdb_ctx->s->g_cpu, registers, addr);
+        len -= reg_size;
+        registers += reg_size;
+    }
+    put_packet(gdb_ctx->s, "OK");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1745,7 +1768,6 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
-    uint8_t *registers;
     target_ulong addr, len;
     const GdbCmdParseEntry *cmd_parser = NULL;
 
@@ -1911,16 +1933,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         put_packet(s, buf);
         break;
     case 'G':
-        cpu_synchronize_state(s->g_cpu);
-        registers = mem_buf;
-        len = strlen(p) / 2;
-        hextomem((uint8_t *)registers, p, len);
-        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs && len > 0; addr++) {
-            reg_size = gdb_write_register(s->g_cpu, registers, addr);
-            len -= reg_size;
-            registers += reg_size;
+        {
+            static const GdbCmdParseEntry write_all_regs_cmd_desc = {
+                .handler = handle_write_all_regs,
+                .cmd = "G",
+                .cmd_startswith = 1,
+                .schema = "s0"
+            };
+            cmd_parser = &write_all_regs_cmd_desc;
         }
-        put_packet(s, "OK");
         break;
     case 'm':
         {
-- 
2.20.1

[Qemu-devel] [PATCH v9 14/27] gdbstub: Implement read all registers (g pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index adfe39b3a3..3478ac778d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1757,6 +1757,21 @@ static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "OK");
 }
 
+static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    target_ulong addr, len;
+
+    cpu_synchronize_state(gdb_ctx->s->g_cpu);
+    len = 0;
+    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
+        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
+                                 addr);
+    }
+
+    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1764,7 +1779,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     CPUClass *cc;
     const char *p;
     uint32_t pid, tid;
-    int ch, reg_size, type, res;
+    int ch, type, res;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
@@ -1923,14 +1938,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'g':
-        cpu_synchronize_state(s->g_cpu);
-        len = 0;
-        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
-            reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
-            len += reg_size;
+        {
+            static const GdbCmdParseEntry read_all_regs_cmd_desc = {
+                .handler = handle_read_all_regs,
+                .cmd = "g",
+                .cmd_startswith = 1
+            };
+            cmd_parser = &read_all_regs_cmd_desc;
         }
-        memtohex(buf, mem_buf, len);
-        put_packet(s, buf);
         break;
     case 'G':
         {
-- 
2.20.1

[Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 62 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 3478ac778d..9fe130f30d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1772,6 +1772,39 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, gdb_ctx->str_buf);
 }
 
+static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int num_syscall_params;
+    GdbCmdVariant syscall_params[3] = {};
+
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    if (cmd_parse_params(gdb_ctx->params[0].data, "L,L,o0", syscall_params,
+                         &num_syscall_params)) {
+        return;
+    }
+
+    if (!num_syscall_params) {
+        return;
+    }
+
+    if (gdb_ctx->s->current_syscall_cb) {
+        gdb_ctx->s->current_syscall_cb(gdb_ctx->s->c_cpu,
+                                       (target_ulong)syscall_params[0].val_ull,
+                                       (target_ulong)syscall_params[1].val_ull);
+        gdb_ctx->s->current_syscall_cb = NULL;
+    }
+
+    if (syscall_params[2].opcode == (uint8_t)'C') {
+        put_packet(gdb_ctx->s, "T02");
+        return;
+    }
+
+    gdb_continue(gdb_ctx->s);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1913,28 +1946,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         return RS_IDLE;
     case 'F':
         {
-            target_ulong ret;
-            target_ulong err;
-
-            ret = strtoull(p, (char **)&p, 16);
-            if (*p == ',') {
-                p++;
-                err = strtoull(p, (char **)&p, 16);
-            } else {
-                err = 0;
-            }
-            if (*p == ',')
-                p++;
-            type = *p;
-            if (s->current_syscall_cb) {
-                s->current_syscall_cb(s->c_cpu, ret, err);
-                s->current_syscall_cb = NULL;
-            }
-            if (type == 'C') {
-                put_packet(s, "T02");
-            } else {
-                gdb_continue(s);
-            }
+            static const GdbCmdParseEntry file_io_cmd_desc = {
+                .handler = handle_file_io,
+                .cmd = "F",
+                .cmd_startswith = 1,
+                .schema = "s0"
+            };
+            cmd_parser = &file_io_cmd_desc;
         }
         break;
     case 'g':
-- 
2.20.1

[Qemu-devel] [PATCH v9 16/27] gdbstub: Implement step (s pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 9fe130f30d..9b0556f8be 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1805,6 +1805,16 @@ static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
     gdb_continue(gdb_ctx->s);
 }
 
+static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (gdb_ctx->num_params) {
+        gdb_set_cpu_pc(gdb_ctx->s, (target_ulong)gdb_ctx->params[0].val_ull);
+    }
+
+    cpu_single_step(gdb_ctx->s->c_cpu, sstep_flags);
+    gdb_continue(gdb_ctx->s);
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1937,13 +1947,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 's':
-        if (*p != '\0') {
-            addr = strtoull(p, (char **)&p, 16);
-            gdb_set_cpu_pc(s, addr);
+        {
+            static const GdbCmdParseEntry step_cmd_desc = {
+                .handler = handle_step,
+                .cmd = "s",
+                .cmd_startswith = 1,
+                .schema = "L0"
+            };
+            cmd_parser = &step_cmd_desc;
         }
-        cpu_single_step(s->c_cpu, sstep_flags);
-        gdb_continue(s);
-        return RS_IDLE;
+        break;
     case 'F':
         {
             static const GdbCmdParseEntry file_io_cmd_desc = {
-- 
2.20.1

[Qemu-devel] [PATCH v9 17/27] gdbstub: Implement v commands with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 170 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 110 insertions(+), 60 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 9b0556f8be..d56d0fd235 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1815,6 +1815,106 @@ static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
     gdb_continue(gdb_ctx->s);
 }
 
+static void handle_v_cont_query(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    put_packet(gdb_ctx->s, "vCont;c;C;s;S");
+}
+
+static void handle_v_cont(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int res;
+
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    res = gdb_handle_vcont(gdb_ctx->s, gdb_ctx->params[0].data);
+    if ((res == -EINVAL) || (res == -ERANGE)) {
+        put_packet(gdb_ctx->s, "E22");
+    } else if (res) {
+        put_packet(gdb_ctx->s, "\0");
+    }
+}
+
+static void handle_v_attach(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    GDBProcess *process;
+    CPUState *cpu;
+    char thread_id[16];
+
+    strcpy(gdb_ctx->str_buf, "E22");
+    if (!gdb_ctx->num_params) {
+        goto cleanup;
+    }
+
+    process = gdb_get_process(gdb_ctx->s, gdb_ctx->params[0].val_ul);
+    if (!process) {
+        goto cleanup;
+    }
+
+    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
+    if (!cpu) {
+        goto cleanup;
+    }
+
+    process->attached = true;
+    gdb_ctx->s->g_cpu = cpu;
+    gdb_ctx->s->c_cpu = cpu;
+
+    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
+             GDB_SIGNAL_TRAP, thread_id);
+cleanup:
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    /* Kill the target */
+    put_packet(gdb_ctx->s, "OK");
+    error_report("QEMU: Terminated via GDBstub");
+    exit(0);
+}
+
+static GdbCmdParseEntry gdb_v_commands_table[] = {
+    /* Order is important if has same prefix */
+    {
+        .handler = handle_v_cont_query,
+        .cmd = "Cont?",
+        .cmd_startswith = 1
+    },
+    {
+        .handler = handle_v_cont,
+        .cmd = "Cont",
+        .cmd_startswith = 1,
+        .schema = "s0"
+    },
+    {
+        .handler = handle_v_attach,
+        .cmd = "Attach;",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
+    {
+        .handler = handle_v_kill,
+        .cmd = "Kill;",
+        .cmd_startswith = 1
+    },
+};
+
+static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
+                           gdb_v_commands_table,
+                           ARRAY_SIZE(gdb_v_commands_table))) {
+        put_packet(gdb_ctx->s, "");
+    }
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1822,7 +1922,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     CPUClass *cc;
     const char *p;
     uint32_t pid, tid;
-    int ch, type, res;
+    int ch, type;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
@@ -1871,66 +1971,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'v':
-        if (strncmp(p, "Cont", 4) == 0) {
-            p += 4;
-            if (*p == '?') {
-                put_packet(s, "vCont;c;C;s;S");
-                break;
-            }
-
-            res = gdb_handle_vcont(s, p);
-
-            if (res) {
-                if ((res == -EINVAL) || (res == -ERANGE)) {
-                    put_packet(s, "E22");
-                    break;
-                }
-                goto unknown_command;
-            }
-            break;
-        } else if (strncmp(p, "Attach;", 7) == 0) {
-            unsigned long pid;
-
-            p += 7;
-
-            if (qemu_strtoul(p, &p, 16, &pid)) {
-                put_packet(s, "E22");
-                break;
-            }
-
-            process = gdb_get_process(s, pid);
-
-            if (process == NULL) {
-                put_packet(s, "E22");
-                break;
-            }
-
-            cpu = get_first_cpu_in_process(s, process);
-
-            if (cpu == NULL) {
-                /* Refuse to attach an empty process */
-                put_packet(s, "E22");
-                break;
-            }
-
-            process->attached = true;
-
-            s->g_cpu = cpu;
-            s->c_cpu = cpu;
-
-            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
-                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
-
-            put_packet(s, buf);
-            break;
-        } else if (strncmp(p, "Kill;", 5) == 0) {
-            /* Kill the target */
-            put_packet(s, "OK");
-            error_report("QEMU: Terminated via GDBstub");
-            exit(0);
-        } else {
-            goto unknown_command;
+        {
+            static const GdbCmdParseEntry v_cmd_desc = {
+                .handler = handle_v_commands,
+                .cmd = "v",
+                .cmd_startswith = 1,
+                .schema = "s0"
+            };
+            cmd_parser = &v_cmd_desc;
         }
+        break;
     case 'k':
         /* Kill the target */
         error_report("QEMU: Terminated via GDBstub");
-- 
2.20.1

[Qemu-devel] [PATCH v9 18/27] gdbstub: Implement generic query (q pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 327 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 327 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index d56d0fd235..83ae8738cc 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1915,6 +1915,323 @@ static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
     }
 }
 
+static void handle_query_qemu_sstepbits(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
+             "ENABLE=%x,NOIRQ=%x,NOTIMER=%x", SSTEP_ENABLE,
+             SSTEP_NOIRQ, SSTEP_NOTIMER);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_set_qemu_sstep(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    sstep_flags = gdb_ctx->params[0].val_ul;
+    put_packet(gdb_ctx->s, "OK");
+}
+
+static void handle_query_qemu_sstep(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "0x%x", sstep_flags);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_query_curr_tid(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    CPUState *cpu;
+    GDBProcess *process;
+    char thread_id[16];
+
+    /*
+     * "Current thread" remains vague in the spec, so always return
+     * the first thread of the current process (gdb returns the
+     * first thread).
+     */
+    process = gdb_get_cpu_process(gdb_ctx->s, gdb_ctx->s->g_cpu);
+    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
+    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "QC%s", thread_id);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_query_threads(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    char thread_id[16];
+
+    if (!gdb_ctx->s->query_cpu) {
+        put_packet(gdb_ctx->s, "l");
+        return;
+    }
+
+    gdb_fmt_thread_id(gdb_ctx->s, gdb_ctx->s->query_cpu, thread_id,
+                      sizeof(thread_id));
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "m%s", thread_id);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+    gdb_ctx->s->query_cpu =
+        gdb_next_attached_cpu(gdb_ctx->s, gdb_ctx->s->query_cpu);
+}
+
+static void handle_query_first_threads(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    gdb_ctx->s->query_cpu = gdb_first_attached_cpu(gdb_ctx->s);
+    handle_query_threads(gdb_ctx, user_ctx);
+}
+
+static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    CPUState *cpu;
+    int len;
+
+    if (!gdb_ctx->num_params ||
+        gdb_ctx->params[0].thread_id.kind == GDB_READ_THREAD_ERR) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[0].thread_id.pid,
+                      gdb_ctx->params[0].thread_id.tid);
+    if (!cpu) {
+        return;
+    }
+
+    cpu_synchronize_state(cpu);
+
+    if (gdb_ctx->s->multiprocess && (gdb_ctx->s->process_num > 1)) {
+        /* Print the CPU model and name in multiprocess mode */
+        ObjectClass *oc = object_get_class(OBJECT(cpu));
+        const char *cpu_model = object_class_get_name(oc);
+        char *cpu_name = object_get_canonical_path_component(OBJECT(cpu));
+        len = snprintf((char *)gdb_ctx->mem_buf, sizeof(gdb_ctx->str_buf) / 2,
+                       "%s %s [%s]", cpu_model, cpu_name,
+                       cpu->halted ? "halted " : "running");
+        g_free(cpu_name);
+    } else {
+        /* memtohex() doubles the required space */
+        len = snprintf((char *)gdb_ctx->mem_buf, sizeof(gdb_ctx->str_buf) / 2,
+                        "CPU#%d [%s]", cpu->cpu_index,
+                        cpu->halted ? "halted " : "running");
+    }
+    trace_gdbstub_op_extra_info((char *)gdb_ctx->mem_buf);
+    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+#ifdef CONFIG_USER_ONLY
+static void handle_query_offsets(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    TaskState *ts;
+
+    ts = gdb_ctx->s->c_cpu->opaque;
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
+             "Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
+             ";Bss=" TARGET_ABI_FMT_lx,
+             ts->info->code_offset,
+             ts->info->data_offset,
+             ts->info->data_offset);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+#else
+static void handle_query_rcmd(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int len;
+
+    if (!gdb_ctx->num_params) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    len = strlen(gdb_ctx->params[0].data);
+    if (len % 2) {
+        put_packet(gdb_ctx->s, "E01");
+        return;
+    }
+
+    len = len / 2;
+    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[0].data, len);
+    gdb_ctx->mem_buf[len++] = 0;
+    qemu_chr_be_write(gdb_ctx->s->mon_chr, gdb_ctx->mem_buf, len);
+    put_packet(gdb_ctx->s, "OK");
+
+}
+#endif
+
+static void handle_query_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    CPUClass *cc;
+
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "PacketSize=%x",
+             MAX_PACKET_LENGTH);
+    cc = CPU_GET_CLASS(first_cpu);
+    if (cc->gdb_core_xml_file) {
+        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
+                ";qXfer:features:read+");
+    }
+
+    if (gdb_ctx->num_params &&
+        strstr(gdb_ctx->params[0].data, "multiprocess+")) {
+        gdb_ctx->s->multiprocess = true;
+    }
+
+    pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";multiprocess+");
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_query_xfer_features(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    GDBProcess *process;
+    CPUClass *cc;
+    unsigned long len, total_len, addr;
+    const char *xml;
+    const char *p;
+
+    if (gdb_ctx->num_params < 3) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    process = gdb_get_cpu_process(gdb_ctx->s, gdb_ctx->s->g_cpu);
+    cc = CPU_GET_CLASS(gdb_ctx->s->g_cpu);
+    if (!cc->gdb_core_xml_file) {
+        put_packet(gdb_ctx->s, "");
+        return;
+    }
+
+    gdb_has_xml = true;
+    p = gdb_ctx->params[0].data;
+    xml = get_feature_xml(gdb_ctx->s, p, &p, process);
+    if (!xml) {
+        put_packet(gdb_ctx->s, "E00");
+        return;
+    }
+
+    addr = gdb_ctx->params[1].val_ul;
+    len = gdb_ctx->params[2].val_ul;
+    total_len = strlen(xml);
+    if (addr > total_len) {
+        put_packet(gdb_ctx->s, "E00");
+        return;
+    }
+
+    if (len > (MAX_PACKET_LENGTH - 5) / 2) {
+        len = (MAX_PACKET_LENGTH - 5) / 2;
+    }
+
+    if (len < total_len - addr) {
+        gdb_ctx->str_buf[0] = 'm';
+        len = memtox(gdb_ctx->str_buf + 1, xml + addr, len);
+    } else {
+        gdb_ctx->str_buf[0] = 'l';
+        len = memtox(gdb_ctx->str_buf + 1, xml + addr, total_len - addr);
+    }
+
+    put_packet_binary(gdb_ctx->s, gdb_ctx->str_buf, len + 1, true);
+}
+
+static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    put_packet(gdb_ctx->s, GDB_ATTACHED);
+}
+
+static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
+    /* Order is important if has same prefix */
+    {
+        .handler = handle_query_qemu_sstepbits,
+        .cmd = "qemu.sstepbits",
+    },
+    {
+        .handler = handle_query_qemu_sstep,
+        .cmd = "qemu.sstep",
+    },
+    {
+        .handler = handle_set_qemu_sstep,
+        .cmd = "qemu.sstep=",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
+};
+
+static GdbCmdParseEntry gdb_gen_query_table[] = {
+    {
+        .handler = handle_query_curr_tid,
+        .cmd = "C",
+    },
+    {
+        .handler = handle_query_threads,
+        .cmd = "sThreadInfo",
+    },
+    {
+        .handler = handle_query_first_threads,
+        .cmd = "fThreadInfo",
+    },
+    {
+        .handler = handle_query_thread_extra,
+        .cmd = "ThreadExtraInfo,",
+        .cmd_startswith = 1,
+        .schema = "t0"
+    },
+#ifdef CONFIG_USER_ONLY
+    {
+        .handler = handle_query_offsets,
+        .cmd = "Offsets",
+    },
+#else
+    {
+        .handler = handle_query_rcmd,
+        .cmd = "Rcmd,",
+        .cmd_startswith = 1,
+        .schema = "s0"
+    },
+#endif
+    {
+        .handler = handle_query_supported,
+        .cmd = "Supported:",
+        .cmd_startswith = 1,
+        .schema = "s0"
+    },
+    {
+        .handler = handle_query_supported,
+        .cmd = "Supported",
+        .schema = "s0"
+    },
+    {
+        .handler = handle_query_xfer_features,
+        .cmd = "Xfer:features:read:",
+        .cmd_startswith = 1,
+        .schema = "s:l,l0"
+    },
+    {
+        .handler = handle_query_attached,
+        .cmd = "Attached:",
+        .cmd_startswith = 1
+    },
+    {
+        .handler = handle_query_attached,
+        .cmd = "Attached",
+    },
+};
+
+static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    if (!process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
+                            gdb_gen_query_set_common_table,
+                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+        return;
+    }
+
+    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
+                           gdb_gen_query_table,
+                           ARRAY_SIZE(gdb_gen_query_table))) {
+        put_packet(gdb_ctx->s, "");
+    }
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -2128,6 +2445,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'q':
+        {
+            static const GdbCmdParseEntry gen_query_cmd_desc = {
+                .handler = handle_gen_query,
+                .cmd = "q",
+                .cmd_startswith = 1,
+                .schema = "s0"
+            };
+            cmd_parser = &gen_query_cmd_desc;
+        }
+        break;
     case 'Q':
         /* parse any 'q' packets here */
         if (!strcmp(p,"qemu.sstepbits")) {
-- 
2.20.1

[Qemu-devel] [PATCH v9 19/27] gdbstub: Implement generic set (Q pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 213 +++++++-----------------------------------------------
 1 file changed, 25 insertions(+), 188 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 83ae8738cc..2fd0d66f4d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1130,14 +1130,6 @@ static GDBThreadIdKind read_thread_id(const char *buf, const char **end_buf,
     return GDB_ONE_THREAD;
 }
 
-static int is_query_packet(const char *p, const char *query, char separator)
-{
-    unsigned int query_len = strlen(query);
-
-    return strncmp(p, query, query_len) == 0 &&
-        (p[query_len] == '\0' || p[query_len] == separator);
-}
-
 /**
  * gdb_handle_vcont - Parses and handles a vCont packet.
  * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if there is
@@ -2232,18 +2224,28 @@ static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
     }
 }
 
+static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    if (!process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
+                            gdb_gen_query_set_common_table,
+                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+        return;
+    }
+
+    put_packet(gdb_ctx->s, "");
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
-    CPUState *cpu;
-    GDBProcess *process;
-    CPUClass *cc;
     const char *p;
-    uint32_t pid, tid;
-    int ch, type;
+    int ch;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
-    target_ulong addr, len;
     const GdbCmdParseEntry *cmd_parser = NULL;
 
     trace_gdbstub_io_command(line_buf);
@@ -2456,182 +2458,17 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'Q':
-        /* parse any 'q' packets here */
-        if (!strcmp(p,"qemu.sstepbits")) {
-            /* Query Breakpoint bit definitions */
-            snprintf(buf, sizeof(buf), "ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
-                     SSTEP_ENABLE,
-                     SSTEP_NOIRQ,
-                     SSTEP_NOTIMER);
-            put_packet(s, buf);
-            break;
-        } else if (is_query_packet(p, "qemu.sstep", '=')) {
-            /* Display or change the sstep_flags */
-            p += 10;
-            if (*p != '=') {
-                /* Display current setting */
-                snprintf(buf, sizeof(buf), "0x%x", sstep_flags);
-                put_packet(s, buf);
-                break;
-            }
-            p++;
-            type = strtoul(p, (char **)&p, 16);
-            sstep_flags = type;
-            put_packet(s, "OK");
-            break;
-        } else if (strcmp(p,"C") == 0) {
-            /*
-             * "Current thread" remains vague in the spec, so always return
-             * the first thread of the current process (gdb returns the
-             * first thread).
-             */
-            cpu = get_first_cpu_in_process(s, gdb_get_cpu_process(s, s->g_cpu));
-            snprintf(buf, sizeof(buf), "QC%s",
-                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
-            put_packet(s, buf);
-            break;
-        } else if (strcmp(p,"fThreadInfo") == 0) {
-            s->query_cpu = gdb_first_attached_cpu(s);
-            goto report_cpuinfo;
-        } else if (strcmp(p,"sThreadInfo") == 0) {
-        report_cpuinfo:
-            if (s->query_cpu) {
-                snprintf(buf, sizeof(buf), "m%s",
-                         gdb_fmt_thread_id(s, s->query_cpu,
-                                       thread_id, sizeof(thread_id)));
-                put_packet(s, buf);
-                s->query_cpu = gdb_next_attached_cpu(s, s->query_cpu);
-            } else
-                put_packet(s, "l");
-            break;
-        } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
-            if (read_thread_id(p + 16, &p, &pid, &tid) == GDB_READ_THREAD_ERR) {
-                put_packet(s, "E22");
-                break;
-            }
-            cpu = gdb_get_cpu(s, pid, tid);
-            if (cpu != NULL) {
-                cpu_synchronize_state(cpu);
-
-                if (s->multiprocess && (s->process_num > 1)) {
-                    /* Print the CPU model and name in multiprocess mode */
-                    ObjectClass *oc = object_get_class(OBJECT(cpu));
-                    const char *cpu_model = object_class_get_name(oc);
-                    char *cpu_name =
-                        object_get_canonical_path_component(OBJECT(cpu));
-                    len = snprintf((char *)mem_buf, sizeof(buf) / 2,
-                                   "%s %s [%s]", cpu_model, cpu_name,
-                                   cpu->halted ? "halted " : "running");
-                    g_free(cpu_name);
-                } else {
-                    /* memtohex() doubles the required space */
-                    len = snprintf((char *)mem_buf, sizeof(buf) / 2,
-                                   "CPU#%d [%s]", cpu->cpu_index,
-                                   cpu->halted ? "halted " : "running");
-                }
-                trace_gdbstub_op_extra_info((char *)mem_buf);
-                memtohex(buf, mem_buf, len);
-                put_packet(s, buf);
-            }
-            break;
-        }
-#ifdef CONFIG_USER_ONLY
-        else if (strcmp(p, "Offsets") == 0) {
-            TaskState *ts = s->c_cpu->opaque;
-
-            snprintf(buf, sizeof(buf),
-                     "Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
-                     ";Bss=" TARGET_ABI_FMT_lx,
-                     ts->info->code_offset,
-                     ts->info->data_offset,
-                     ts->info->data_offset);
-            put_packet(s, buf);
-            break;
-        }
-#else /* !CONFIG_USER_ONLY */
-        else if (strncmp(p, "Rcmd,", 5) == 0) {
-            int len = strlen(p + 5);
-
-            if ((len % 2) != 0) {
-                put_packet(s, "E01");
-                break;
-            }
-            len = len / 2;
-            hextomem(mem_buf, p + 5, len);
-            mem_buf[len++] = 0;
-            qemu_chr_be_write(s->mon_chr, mem_buf, len);
-            put_packet(s, "OK");
-            break;
-        }
-#endif /* !CONFIG_USER_ONLY */
-        if (is_query_packet(p, "Supported", ':')) {
-            snprintf(buf, sizeof(buf), "PacketSize=%x", MAX_PACKET_LENGTH);
-            cc = CPU_GET_CLASS(first_cpu);
-            if (cc->gdb_core_xml_file != NULL) {
-                pstrcat(buf, sizeof(buf), ";qXfer:features:read+");
-            }
-
-            if (strstr(p, "multiprocess+")) {
-                s->multiprocess = true;
-            }
-            pstrcat(buf, sizeof(buf), ";multiprocess+");
-
-            put_packet(s, buf);
-            break;
-        }
-        if (strncmp(p, "Xfer:features:read:", 19) == 0) {
-            const char *xml;
-            target_ulong total_len;
-
-            process = gdb_get_cpu_process(s, s->g_cpu);
-            cc = CPU_GET_CLASS(s->g_cpu);
-            if (cc->gdb_core_xml_file == NULL) {
-                goto unknown_command;
-            }
-
-            gdb_has_xml = true;
-            p += 19;
-            xml = get_feature_xml(s, p, &p, process);
-            if (!xml) {
-                snprintf(buf, sizeof(buf), "E00");
-                put_packet(s, buf);
-                break;
-            }
-
-            if (*p == ':')
-                p++;
-            addr = strtoul(p, (char **)&p, 16);
-            if (*p == ',')
-                p++;
-            len = strtoul(p, (char **)&p, 16);
-
-            total_len = strlen(xml);
-            if (addr > total_len) {
-                snprintf(buf, sizeof(buf), "E00");
-                put_packet(s, buf);
-                break;
-            }
-            if (len > (MAX_PACKET_LENGTH - 5) / 2)
-                len = (MAX_PACKET_LENGTH - 5) / 2;
-            if (len < total_len - addr) {
-                buf[0] = 'm';
-                len = memtox(buf + 1, xml + addr, len);
-            } else {
-                buf[0] = 'l';
-                len = memtox(buf + 1, xml + addr, total_len - addr);
-            }
-            put_packet_binary(s, buf, len + 1, true);
-            break;
-        }
-        if (is_query_packet(p, "Attached", ':')) {
-            put_packet(s, GDB_ATTACHED);
-            break;
+        {
+            static const GdbCmdParseEntry gen_set_cmd_desc = {
+                .handler = handle_gen_set,
+                .cmd = "Q",
+                .cmd_startswith = 1,
+                .schema = "s0"
+            };
+            cmd_parser = &gen_set_cmd_desc;
         }
-        /* Unrecognised 'q' command.  */
-        goto unknown_command;
-
+        break;
     default:
-    unknown_command:
         /* put empty packet */
         buf[0] = '\0';
         put_packet(s, buf);
-- 
2.20.1

[Qemu-devel] [PATCH v9 20/27] gdbstub: Implement target halted (? pkt) with new infra

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 2fd0d66f4d..d678191705 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2239,13 +2239,30 @@ static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "");
 }
 
+static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    char thread_id[16];
+
+    /* TODO: Make this return the correct value for user-mode.  */
+    gdb_fmt_thread_id(gdb_ctx->s, gdb_ctx->s->c_cpu, thread_id,
+                      sizeof(thread_id));
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
+             GDB_SIGNAL_TRAP, thread_id);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+    /*
+     * Remove all the breakpoints when this query is issued,
+     * because gdb is doing and initial connect and the state
+     * should be cleaned up.
+     */
+    gdb_breakpoint_remove_all();
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     const char *p;
     int ch;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
-    char thread_id[16];
     const GdbCmdParseEntry *cmd_parser = NULL;
 
     trace_gdbstub_io_command(line_buf);
@@ -2257,15 +2274,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         put_packet(s, "OK");
         break;
     case '?':
-        /* TODO: Make this return the correct value for user-mode.  */
-        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
-                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
-        put_packet(s, buf);
-        /* Remove all the breakpoints when this query is issued,
-         * because gdb is doing and initial connect and the state
-         * should be cleaned up.
-         */
-        gdb_breakpoint_remove_all();
+        {
+            static const GdbCmdParseEntry target_halted_cmd_desc = {
+                .handler = handle_target_halt,
+                .cmd = "?",
+                .cmd_startswith = 1
+            };
+            cmd_parser = &target_halted_cmd_desc;
+        }
         break;
     case 'c':
         {
-- 
2.20.1

[Qemu-devel] [PATCH v9 21/27] gdbstub: Clear unused variables in gdb_handle_packet

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index d678191705..8bdfae4b29 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2259,17 +2259,11 @@ static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
 
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
-    const char *p;
-    int ch;
-    uint8_t mem_buf[MAX_PACKET_LENGTH];
-    char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     const GdbCmdParseEntry *cmd_parser = NULL;
 
     trace_gdbstub_io_command(line_buf);
 
-    p = line_buf;
-    ch = *p++;
-    switch(ch) {
+    switch (line_buf[0]) {
     case '!':
         put_packet(s, "OK");
         break;
@@ -2486,8 +2480,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         break;
     default:
         /* put empty packet */
-        buf[0] = '\0';
-        put_packet(s, buf);
+        put_packet(s, "");
         break;
     }
 
-- 
2.20.1

[Qemu-devel] [PATCH v9 22/27] gdbstub: Implement generic query qemu.Supported

[Jon Doron]

qemu.Supported query reply back with the supported qemu query/set
commands (commands are seperated with a semicolon from each other).

gdb> maint packet qqemu.Supported

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 8bdfae4b29..00c07d6ec0 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2127,6 +2127,11 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, GDB_ATTACHED);
 }
 
+static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    put_packet(gdb_ctx->s, "sstepbits;sstep");
+}
+
 static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
     /* Order is important if has same prefix */
     {
@@ -2203,6 +2208,10 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
         .handler = handle_query_attached,
         .cmd = "Attached",
     },
+    {
+        .handler = handle_query_qemu_supported,
+        .cmd = "qemu.Supported",
+    },
 };
 
 static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
-- 
2.20.1

[Qemu-devel] [PATCH v9 23/27] gdbstub: Implement qemu physical memory mode

[Jon Doron]

Add a new query/set which changes the memory GDB sees to physical memory
only.

gdb> maint packet qqemu.PhyMemMode
will reply the current phy_mem_mode state (1 for enabled, 0 for disabled)
gdb> maint packet Qqemu.PhyMemMode:1
Will make GDB read/write only to physical memory, set to 0 to disable

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 00c07d6ec0..88ff6224e6 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -46,11 +46,23 @@
 #define GDB_ATTACHED "1"
 #endif
 
+static int phy_memory_mode;
+
 static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
                                          uint8_t *buf, int len, bool is_write)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
+    CPUClass *cc;
 
+    if (phy_memory_mode) {
+        if (is_write) {
+            cpu_physical_memory_write(addr, buf, len);
+        } else {
+            cpu_physical_memory_read(addr, buf, len);
+        }
+        return 0;
+    }
+
+    cc = CPU_GET_CLASS(cpu);
     if (cc->memory_rw_debug) {
         return cc->memory_rw_debug(cpu, addr, buf, len, is_write);
     }
@@ -2129,7 +2141,29 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
 
 static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
 {
-    put_packet(gdb_ctx->s, "sstepbits;sstep");
+    put_packet(gdb_ctx->s, "sstepbits;sstep;PhyMemMode");
+}
+
+static void handle_query_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx,
+                                           void *user_ctx)
+{
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "%d", phy_memory_mode);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!gdb_ctx->num_params) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (!gdb_ctx->params[0].val_ul) {
+        phy_memory_mode = 0;
+    } else {
+        phy_memory_mode = 1;
+    }
+    put_packet(gdb_ctx->s, "OK");
 }
 
 static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
@@ -2212,6 +2246,20 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
         .handler = handle_query_qemu_supported,
         .cmd = "qemu.Supported",
     },
+    {
+        .handler = handle_query_qemu_phy_mem_mode,
+        .cmd = "qemu.PhyMemMode",
+    },
+};
+
+static GdbCmdParseEntry gdb_gen_set_table[] = {
+    /* Order is important if has same prefix */
+    {
+        .handler = handle_set_qemu_phy_mem_mode,
+        .cmd = "qemu.PhyMemMode:",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
 };
 
 static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
@@ -2245,7 +2293,11 @@ static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
         return;
     }
 
-    put_packet(gdb_ctx->s, "");
+    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
+                           gdb_gen_set_table,
+                           ARRAY_SIZE(gdb_gen_set_table))) {
+        put_packet(gdb_ctx->s, "");
+    }
 }
 
 static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
-- 
2.20.1

[Qemu-devel] [PATCH v9 24/27] gdbstub: Add another handler for setting qemu.sstep

[Jon Doron]

Follow GDB general query/set packet conventions, qemu.sstep can now
be set with the following command as well:
gdb> maint packet Qqemu.sstep:Value

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 88ff6224e6..34da10260d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2260,6 +2260,12 @@ static GdbCmdParseEntry gdb_gen_set_table[] = {
         .cmd_startswith = 1,
         .schema = "l0"
     },
+    {
+        .handler = handle_set_qemu_sstep,
+        .cmd = "qemu.sstep:",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
 };
 
 static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
-- 
2.20.1

[Qemu-devel] [PATCH v9 25/27] kvm: Add API to read/write a CPU MSR value

[Jon Doron]

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 accel/kvm/kvm-all.c  | 39 +++++++++++++++++++++++++++++++++++++++
 include/sysemu/kvm.h |  2 ++
 2 files changed, 41 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 524c4ddfbd..35207d910b 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2444,6 +2444,45 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
 }
 #endif /* !KVM_CAP_SET_GUEST_DEBUG */
 
+int kvm_arch_read_msr(CPUState *cpu, uint32_t index, uint64_t *value)
+{
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[1];
+    } msr_data;
+    int ret;
+
+    msr_data.info.nmsrs = 1;
+    msr_data.entries[0].index = index;
+    ret = kvm_vcpu_ioctl(cpu, KVM_GET_MSRS, &msr_data);
+    if (ret < 0) {
+        return ret;
+    }
+
+    *value = msr_data.entries[0].data;
+    return 0;
+}
+
+int kvm_arch_write_msr(CPUState *cpu, uint32_t index, uint64_t value)
+{
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[1];
+    } msr_data;
+    int ret;
+
+    msr_data.info.nmsrs = 1;
+    msr_data.entries[0].index = index;
+    msr_data.entries[0].reserved = 0;
+    msr_data.entries[0].data = value;
+    ret = kvm_vcpu_ioctl(cpu, KVM_SET_MSRS, &msr_data);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return 0;
+}
+
 static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
 {
     KVMState *s = kvm_state;
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a6d1cd190f..409b1a5444 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -462,6 +462,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
+int kvm_arch_read_msr(CPUState *cpu, uint32_t index, uint64_t *value);
+int kvm_arch_write_msr(CPUState *cpu, uint32_t index, uint64_t value);
 
 
 void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
-- 
2.20.1

[Qemu-devel] [PATCH v9 26/27] gdbstub: Add support to read a MSR for KVM target

[Jon Doron]

gdb> maint packet qqemu.kvm.Rdmsr:MsrIndex

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index 34da10260d..f48c3a2b5f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2141,7 +2141,14 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
 
 static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
 {
-    put_packet(gdb_ctx->s, "sstepbits;sstep;PhyMemMode");
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
+             "sstepbits;sstep;PhyMemMode");
+
+    if (kvm_enabled()) {
+        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";kvm.Rdmsr");
+    }
+
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
 }
 
 static void handle_query_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx,
@@ -2166,6 +2173,29 @@ static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "OK");
 }
 
+static void handle_query_kvm_read_msr(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    uint64_t msr_val;
+
+    if (!kvm_enabled()) {
+        return;
+    }
+
+    if (!gdb_ctx->num_params) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (kvm_arch_read_msr(gdbserver_state->c_cpu, gdb_ctx->params[0].val_ul,
+                          &msr_val)) {
+        put_packet(gdb_ctx->s, "E00");
+        return;
+    }
+
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "0x%" PRIx64, msr_val);
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
 static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
     /* Order is important if has same prefix */
     {
@@ -2250,6 +2280,12 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
         .handler = handle_query_qemu_phy_mem_mode,
         .cmd = "qemu.PhyMemMode",
     },
+    {
+        .handler = handle_query_kvm_read_msr,
+        .cmd = "qemu.kvm.Rdmsr:",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
 };
 
 static GdbCmdParseEntry gdb_gen_set_table[] = {
-- 
2.20.1

[Qemu-devel] [PATCH v9 27/27] gdbstub: Add support to write a MSR for KVM target

[Jon Doron]

gdb> maint packet Qqemu.kvm.Wrmsr:MsrIndex,Value

Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index f48c3a2b5f..a434a3749e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2145,7 +2145,8 @@ static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
              "sstepbits;sstep;PhyMemMode");
 
     if (kvm_enabled()) {
-        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";kvm.Rdmsr");
+        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
+                ";kvm.Rdmsr;kvm.Wrmsr");
     }
 
     put_packet(gdb_ctx->s, gdb_ctx->str_buf);
@@ -2196,6 +2197,26 @@ static void handle_query_kvm_read_msr(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, gdb_ctx->str_buf);
 }
 
+static void handle_set_kvm_write_msr(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!kvm_enabled()) {
+        return;
+    }
+
+    if (gdb_ctx->num_params < 2) {
+        put_packet(gdb_ctx->s, "E22");
+        return;
+    }
+
+    if (kvm_arch_write_msr(gdbserver_state->c_cpu, gdb_ctx->params[0].val_ul,
+                           gdb_ctx->params[1].val_ull)) {
+        put_packet(gdb_ctx->s, "E00");
+        return;
+    }
+
+    put_packet(gdb_ctx->s, "OK");
+}
+
 static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
     /* Order is important if has same prefix */
     {
@@ -2302,6 +2323,12 @@ static GdbCmdParseEntry gdb_gen_set_table[] = {
         .cmd_startswith = 1,
         .schema = "l0"
     },
+    {
+        .handler = handle_set_kvm_write_msr,
+        .cmd = "qemu.kvm.Wrmsr:",
+        .cmd_startswith = 1,
+        .schema = "l,L0"
+    },
 };
 
 static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v9 01/27] gdbstub: Add infrastructure to parse cmd packets

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
<snip>
> +
> +/*
> + * cmd_startswith -> cmd is compared using startswith
> + *
> + *
> + * schema definitions:
> + * Each schema parameter entry consists of 2 chars,
> + * the first char represents the parameter type handling
> + * the second char represents the delimiter for the next parameter
> + *
> + * Currently supported schema types:
> + * 'l' -> unsigned long (stored in .val_ul)
> + * 'L' -> unsigned long long (stored in .val_ull)
> + * 's' -> string (stored in .data)
> + * 'o' -> single char (stored in .opcode)
> + * 't' -> thread id (stored in .thread_id)
> + * '?' -> skip according to delimiter
> + *
> + * Currently supported delimiters:
> + * '?' -> Stop at any delimiter (",;:=\0")
> + * '0' -> Stop at "\0"
> + * '.' -> Skip 1 char unless reached "\0"
> + * Any other value is treated as the delimiter value itself
> + */
> +typedef struct GdbCmdParseEntry {
> +    GdbCmdHandler handler;
> +    const char *cmd;
> +    union {
> +        int flags;
> +        struct {
> +            int cmd_startswith:1;
> +        };
> +    };

This union seems a little over the top given flags isn't used AFAICT.
Why not just have a bool cmd_startswith for now? You can always expand
the structure later if you need to.

Otherwise:

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

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 02/27] gdbstub: Implement deatch (D pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 90 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d5e0f3878a..621d689868 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern)
>    return !strncmp(string, pattern, strlen(pattern));
>  }
>
> -static int process_string_cmd(
> -        GDBState *s, void *user_ctx, const char *data,
> -        const GdbCmdParseEntry *cmds, int num_cmds)
> -        __attribute__((unused));
> -
>  static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
>                                const GdbCmdParseEntry *cmds, int num_cmds)
>  {
> @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
>      return -1;
>  }
>
> +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    GDBProcess *process;
> +    GDBState *s = gdb_ctx->s;
> +    uint32_t pid = 1;
> +
> +    if (s->multiprocess) {
> +        if (!gdb_ctx->num_params) {
> +            put_packet(s, "E22");
> +            return;
> +        }
> +
> +        pid = gdb_ctx->params[0].val_ul;
> +    }
> +
> +    process = gdb_get_process(s, pid);
> +    gdb_process_breakpoint_remove_all(s, process);
> +    process->attached = false;
> +
> +    if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
> +        s->c_cpu = gdb_first_attached_cpu(s);
> +    }
> +
> +    if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
> +        s->g_cpu = gdb_first_attached_cpu(s);
> +    }
> +
> +    if (!s->c_cpu) {
> +        /* No more process attached */
> +        gdb_syscall_mode = GDB_SYS_DISABLED;
> +        gdb_continue(s);
> +    }
> +    put_packet(s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      uint8_t *registers;
>      target_ulong addr, len;
>      GDBThreadIdKind thread_kind;
> +    const GdbCmdParseEntry *cmd_parser = NULL;
>
>      trace_gdbstub_io_command(line_buf);
>
> @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          error_report("QEMU: Terminated via GDBstub");
>          exit(0);
>      case 'D':
> -        /* Detach packet */
> -        pid = 1;
> -
> -        if (s->multiprocess) {
> -            unsigned long lpid;
> -            if (*p != ';') {
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            if (qemu_strtoul(p + 1, &p, 16, &lpid)) {
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            pid = lpid;
> -        }
> -
> -        process = gdb_get_process(s, pid);
> -        gdb_process_breakpoint_remove_all(s, process);
> -        process->attached = false;
> -
> -        if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
> -            s->c_cpu = gdb_first_attached_cpu(s);
> -        }
> -
> -        if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
> -            s->g_cpu = gdb_first_attached_cpu(s);
> -        }
> -
> -        if (s->c_cpu == NULL) {
> -            /* No more process attached */
> -            gdb_syscall_mode = GDB_SYS_DISABLED;
> -            gdb_continue(s);
> +        {
> +            static const GdbCmdParseEntry detach_cmd_desc = {
> +                .handler = handle_detach,
> +                .cmd = "D",
> +                .cmd_startswith = 1,
> +                .schema = "?.l0"
> +            };
> +            cmd_parser = &detach_cmd_desc;
>          }
> -        put_packet(s, "OK");
>          break;
>      case 's':
>          if (*p != '\0') {
> @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          put_packet(s, buf);
>          break;
>      }
> +
> +    if (cmd_parser &&
> +        process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) {
> +        put_packet(s, "");

Why this null put_packet at the end? You've passed the handling of the
OK reply back to your handler so this seems superfluous.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 03/27] gdbstub: Implement thread_alive (T pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

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

> ---
>  gdbstub.c | 43 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 621d689868..c47ef7dd9c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1498,6 +1498,30 @@ static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(s, "OK");
>  }
>
> +static void handle_thread_alive(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    CPUState *cpu;
> +
> +    if (!gdb_ctx->num_params) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (gdb_ctx->params[0].thread_id.kind == GDB_READ_THREAD_ERR) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[0].thread_id.pid,
> +                      gdb_ctx->params[0].thread_id.tid);
> +    if (!cpu) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1798,17 +1822,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'T':
> -        thread_kind = read_thread_id(p, &p, &pid, &tid);
> -        if (thread_kind == GDB_READ_THREAD_ERR) {
> -            put_packet(s, "E22");
> -            break;
> -        }
> -        cpu = gdb_get_cpu(s, pid, tid);
> -
> -        if (cpu != NULL) {
> -            put_packet(s, "OK");
> -        } else {
> -            put_packet(s, "E22");
> +        {
> +            static const GdbCmdParseEntry thread_alive_cmd_desc = {
> +                .handler = handle_thread_alive,
> +                .cmd = "T",
> +                .cmd_startswith = 1,
> +                .schema = "t0"
> +            };
> +            cmd_parser = &thread_alive_cmd_desc;
>          }
>          break;
>      case 'q':


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 03/27] gdbstub: Implement thread_alive (T pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

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

> ---
>  gdbstub.c | 43 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 621d689868..c47ef7dd9c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1498,6 +1498,30 @@ static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(s, "OK");
>  }
>
> +static void handle_thread_alive(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    CPUState *cpu;
> +
> +    if (!gdb_ctx->num_params) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (gdb_ctx->params[0].thread_id.kind == GDB_READ_THREAD_ERR) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[0].thread_id.pid,
> +                      gdb_ctx->params[0].thread_id.tid);
> +    if (!cpu) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1798,17 +1822,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'T':
> -        thread_kind = read_thread_id(p, &p, &pid, &tid);
> -        if (thread_kind == GDB_READ_THREAD_ERR) {
> -            put_packet(s, "E22");
> -            break;
> -        }
> -        cpu = gdb_get_cpu(s, pid, tid);
> -
> -        if (cpu != NULL) {
> -            put_packet(s, "OK");
> -        } else {
> -            put_packet(s, "E22");
> +        {
> +            static const GdbCmdParseEntry thread_alive_cmd_desc = {
> +                .handler = handle_thread_alive,
> +                .cmd = "T",
> +                .cmd_startswith = 1,
> +                .schema = "t0"
> +            };
> +            cmd_parser = &thread_alive_cmd_desc;
>          }
>          break;
>      case 'q':


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 04/27] gdbstub: Implement continue (c pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

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

> ---
>  gdbstub.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index c47ef7dd9c..89f1ab6524 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1522,6 +1522,16 @@ static void handle_thread_alive(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "OK");
>  }
>
> +static void handle_continue(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (gdb_ctx->num_params) {
> +        gdb_set_cpu_pc(gdb_ctx->s, gdb_ctx->params[0].val_ull);
> +    }
> +
> +    gdb_ctx->s->signal = 0;
> +    gdb_continue(gdb_ctx->s);
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1558,13 +1568,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          gdb_breakpoint_remove_all();
>          break;
>      case 'c':
> -        if (*p != '\0') {
> -            addr = strtoull(p, (char **)&p, 16);
> -            gdb_set_cpu_pc(s, addr);
> +        {
> +            static const GdbCmdParseEntry continue_cmd_desc = {
> +                .handler = handle_continue,
> +                .cmd = "c",
> +                .cmd_startswith = 1,
> +                .schema = "L0"
> +            };
> +            cmd_parser = &continue_cmd_desc;
>          }
> -        s->signal = 0;
> -        gdb_continue(s);
> -        return RS_IDLE;
> +        break;
>      case 'C':
>          s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
>          if (s->signal == -1)


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 05/27] gdbstub: Implement continue with signal (C pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 89f1ab6524..469aaeb875 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1532,6 +1532,21 @@ static void handle_continue(GdbCmdContext *gdb_ctx, void *user_ctx)
>      gdb_continue(gdb_ctx->s);
>  }

It might be worth adding a comment that we don't currently support the:

  C sig;[addr]

form of continue packet here, which we didn't before so:

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

>
> +static void handle_cont_with_sig(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    unsigned long signal = 0;
> +
> +    if (gdb_ctx->num_params) {
> +        signal = gdb_ctx->params[0].val_ul;
> +    }
> +
> +    gdb_ctx->s->signal = gdb_signal_to_target(signal);
> +    if (gdb_ctx->s->signal == -1) {
> +        gdb_ctx->s->signal = 0;
> +    }
> +    gdb_continue(gdb_ctx->s);
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1579,11 +1594,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'C':
> -        s->signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
> -        if (s->signal == -1)
> -            s->signal = 0;
> -        gdb_continue(s);
> -        return RS_IDLE;
> +        {
> +            static const GdbCmdParseEntry cont_with_sig_cmd_desc = {
> +                .handler = handle_cont_with_sig,
> +                .cmd = "C",
> +                .cmd_startswith = 1,
> +                .schema = "l0"
> +            };
> +            cmd_parser = &cont_with_sig_cmd_desc;
> +        }
> +        break;
>      case 'v':
>          if (strncmp(p, "Cont", 4) == 0) {
>              p += 4;


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 06/27] gdbstub: Implement set_thread (H pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 79 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 469aaeb875..21cdaf4678 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1547,6 +1547,47 @@ static void handle_cont_with_sig(GdbCmdContext *gdb_ctx, void *user_ctx)
>      gdb_continue(gdb_ctx->s);
>  }
>
> +static void handle_set_thread(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    CPUState *cpu;
> +
> +    if (gdb_ctx->num_params < 2) {

Given we should have a fixed number of parameters != 2 perhaps?

> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (gdb_ctx->params[1].thread_id.kind == GDB_READ_THREAD_ERR) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (gdb_ctx->params[1].thread_id.kind != GDB_ONE_THREAD) {
> +        put_packet(gdb_ctx->s, "OK");
> +        return;
> +    }
> +
> +    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[1].thread_id.pid,
> +                      gdb_ctx->params[1].thread_id.tid);
> +    if (!cpu) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    switch (gdb_ctx->params[0].opcode) {


Perhaps a comment here to say this is a legacy command and modern gdb's
should be using vCont?

> +    case 'c':
> +        gdb_ctx->s->c_cpu = cpu;
> +        put_packet(gdb_ctx->s, "OK");
> +        break;
> +    case 'g':
> +        gdb_ctx->s->g_cpu = cpu;
> +        put_packet(gdb_ctx->s, "OK");
> +        break;
> +    default:
> +        put_packet(gdb_ctx->s, "E22");
> +        break;
> +    }
> +}
<snip>

Otherwise:

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

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 07/27] gdbstub: Implement insert breakpoint (Z pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 21cdaf4678..36c7353a22 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1588,6 +1588,29 @@ static void handle_set_thread(GdbCmdContext *gdb_ctx, void *user_ctx)
>      }
>  }
>
> +static void handle_insert_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int res;
> +
> +    if (gdb_ctx->num_params < 3) {

!= 3?

> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    res = gdb_breakpoint_insert(gdb_ctx->params[1].val_ull,
> +                                gdb_ctx->params[2].val_ull,
> +                                gdb_ctx->params[0].val_ul);

I would be tempted to fix the prototype of gdb_breakpoint_insert to
match the order of the packet parameters for cleanliness.

> +    if (res >= 0) {
> +        put_packet(gdb_ctx->s, "OK");
> +        return;
> +    } else if (res == -ENOSYS) {
> +        put_packet(gdb_ctx->s, "");
> +        return;
> +    }
> +
> +    put_packet(gdb_ctx->s, "E22");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1843,6 +1866,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          put_packet(s, "OK");
>          break;
>      case 'Z':
> +        {
> +            static const GdbCmdParseEntry insert_bp_cmd_desc = {
> +                .handler = handle_insert_bp,
> +                .cmd = "Z",
> +                .cmd_startswith = 1,
> +                .schema = "l?L?L0"
> +            };
> +            cmd_parser = &insert_bp_cmd_desc;
> +        }
> +        break;
>      case 'z':
>          type = strtoul(p, (char **)&p, 16);
>          if (*p == ',')

You should also delete the:

        if (ch == 'Z')
            res = gdb_breakpoint_insert(addr, len, type);
case.

In fact I think there is probably a good case just to merge with the
next patch as the two functions are very much related.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 08/27] gdbstub: Implement remove breakpoint (z pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

Merge with previous patch as per comments there.

> ---
>  gdbstub.c | 49 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 36c7353a22..b42425b24c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1611,6 +1611,29 @@ static void handle_insert_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "E22");
>  }
>
> +static void handle_remove_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int res;
> +
> +    if (gdb_ctx->num_params < 3) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    res = gdb_breakpoint_remove(gdb_ctx->params[1].val_ull,
> +                                gdb_ctx->params[2].val_ull,
> +                                gdb_ctx->params[0].val_ul);
> +    if (res >= 0) {
> +        put_packet(gdb_ctx->s, "OK");
> +        return;
> +    } else if (res == -ENOSYS) {
> +        put_packet(gdb_ctx->s, "");
> +        return;
> +    }
> +
> +    put_packet(gdb_ctx->s, "E22");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1877,23 +1900,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'z':
> -        type = strtoul(p, (char **)&p, 16);
> -        if (*p == ',')
> -            p++;
> -        addr = strtoull(p, (char **)&p, 16);
> -        if (*p == ',')
> -            p++;
> -        len = strtoull(p, (char **)&p, 16);
> -        if (ch == 'Z')
> -            res = gdb_breakpoint_insert(addr, len, type);
> -        else
> -            res = gdb_breakpoint_remove(addr, len, type);
> -        if (res >= 0)
> -             put_packet(s, "OK");
> -        else if (res == -ENOSYS)
> -            put_packet(s, "");
> -        else
> -            put_packet(s, "E22");
> +        {
> +            static const GdbCmdParseEntry remove_bp_cmd_desc = {
> +                .handler = handle_remove_bp,
> +                .cmd = "z",
> +                .cmd_startswith = 1,
> +                .schema = "l?L?L0"
> +            };
> +            cmd_parser = &remove_bp_cmd_desc;
> +        }
>          break;
>      case 'H':
>          {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 09/27] gdbstub: Implement set register (P pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index b42425b24c..10e3f12a68 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1634,6 +1634,27 @@ static void handle_remove_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "E22");
>  }
>
> +static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int reg_size;
> +
> +    if (!gdb_has_xml) {
> +        put_packet(gdb_ctx->s, "");
> +        return;
> +    }
> +
> +    if (gdb_ctx->num_params < 2) {

!= 2?

> +        put_packet(gdb_ctx->s, "");
> +        return;
> +    }

I don't understand what these null put_packets have been added for.
Should we not report an E code on failure?

> +
> +    reg_size = strlen(gdb_ctx->params[1].data) / 2;
> +    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[1].data, reg_size);
> +    gdb_write_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf,
> +                       gdb_ctx->params[0].val_ull);
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1878,15 +1899,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'P':
> -        if (!gdb_has_xml)
> -            goto unknown_command;
> -        addr = strtoull(p, (char **)&p, 16);
> -        if (*p == '=')
> -            p++;
> -        reg_size = strlen(p) / 2;
> -        hextomem(mem_buf, p, reg_size);
> -        gdb_write_register(s->g_cpu, mem_buf, addr);
> -        put_packet(s, "OK");
> +        {
> +            static const GdbCmdParseEntry set_reg_cmd_desc = {
> +                .handler = handle_set_reg,
> +                .cmd = "P",
> +                .cmd_startswith = 1,
> +                .schema = "L?s0"
> +            };
> +            cmd_parser = &set_reg_cmd_desc;
> +        }
>          break;
>      case 'Z':
>          {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> This patch series refactors the old gdbstub command packets handler
> with a new infrastructure which should ease extending and adding new
> and missing gdb command packets.

There seems to be some compiler errors and variants that this breaks the
CI on:

  https://travis-ci.org/stsquad/qemu/builds/532410263
  https://app.shippable.com/github/stsquad/qemu/runs/822/summary/console
  https://gitlab.com/stsquad/qemu/pipelines/61291132

You might want to consider setting up CI checks on your system and
running your next series through them before posting:

  https://wiki.qemu.org/Testing/CI

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 23/27] gdbstub: Implement qemu physical memory mode

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Add a new query/set which changes the memory GDB sees to physical memory
> only.
>
> gdb> maint packet qqemu.PhyMemMode
> will reply the current phy_mem_mode state (1 for enabled, 0 for disabled)
> gdb> maint packet Qqemu.PhyMemMode:1
> Will make GDB read/write only to physical memory, set to 0 to disable
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 00c07d6ec0..88ff6224e6 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -46,11 +46,23 @@
>  #define GDB_ATTACHED "1"
>  #endif
>
> +static int phy_memory_mode;
> +
>  static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>                                           uint8_t *buf, int len, bool is_write)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    CPUClass *cc;
>
> +    if (phy_memory_mode) {
> +        if (is_write) {
> +            cpu_physical_memory_write(addr, buf, len);
> +        } else {
> +            cpu_physical_memory_read(addr, buf, len);
> +        }
> +        return 0;
> +    }
> +

I think this is the commit that breaks the build. As the gdbstub can run
for both system and linux-user emulation modes you need to take care to
disable the bits that don't make sense for linux-user emulation. You'll
see other places in the code doing that with #if[n]def CONFIG_USER_ONLY.


> +    cc = CPU_GET_CLASS(cpu);
>      if (cc->memory_rw_debug) {
>          return cc->memory_rw_debug(cpu, addr, buf, len, is_write);
>      }
> @@ -2129,7 +2141,29 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
>
>  static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
>  {
> -    put_packet(gdb_ctx->s, "sstepbits;sstep");
> +    put_packet(gdb_ctx->s, "sstepbits;sstep;PhyMemMode");
> +}
> +
> +static void handle_query_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx,
> +                                           void *user_ctx)
> +{
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "%d", phy_memory_mode);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (!gdb_ctx->num_params) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (!gdb_ctx->params[0].val_ul) {
> +        phy_memory_mode = 0;
> +    } else {
> +        phy_memory_mode = 1;
> +    }
> +    put_packet(gdb_ctx->s, "OK");
>  }
>
>  static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
> @@ -2212,6 +2246,20 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
>          .handler = handle_query_qemu_supported,
>          .cmd = "qemu.Supported",
>      },
> +    {
> +        .handler = handle_query_qemu_phy_mem_mode,
> +        .cmd = "qemu.PhyMemMode",
> +    },
> +};
> +
> +static GdbCmdParseEntry gdb_gen_set_table[] = {
> +    /* Order is important if has same prefix */
> +    {
> +        .handler = handle_set_qemu_phy_mem_mode,
> +        .cmd = "qemu.PhyMemMode:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
>  };
>
>  static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
> @@ -2245,7 +2293,11 @@ static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
>          return;
>      }
>
> -    put_packet(gdb_ctx->s, "");
> +    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> +                           gdb_gen_set_table,
> +                           ARRAY_SIZE(gdb_gen_set_table))) {
> +        put_packet(gdb_ctx->s, "");
> +    }
>  }
>
>  static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 11/27] gdbstub: Implement write memory (M pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

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

> ---
>  gdbstub.c | 51 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index e9a3d0c2bc..8dc2e1d507 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1685,6 +1685,31 @@ static void handle_get_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>  }
>
> +static void handle_write_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (gdb_ctx->num_params < 3) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    /* hextomem() reads 2*len bytes */
> +    if (gdb_ctx->params[1].val_ull > strlen(gdb_ctx->params[2].data) / 2) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[2].data,
> +             gdb_ctx->params[1].val_ull);
> +    if (target_memory_rw_debug(gdb_ctx->s->g_cpu, gdb_ctx->params[0].val_ull,
> +                               gdb_ctx->mem_buf,
> +                               gdb_ctx->params[1].val_ull, true)) {
> +        put_packet(gdb_ctx->s, "E14");
> +        return;
> +    }
> +
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1893,24 +1918,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'M':
> -        addr = strtoull(p, (char **)&p, 16);
> -        if (*p == ',')
> -            p++;
> -        len = strtoull(p, (char **)&p, 16);
> -        if (*p == ':')
> -            p++;
> -
> -        /* hextomem() reads 2*len bytes */
> -        if (len > strlen(p) / 2) {
> -            put_packet (s, "E22");
> -            break;
> -        }
> -        hextomem(mem_buf, p, len);
> -        if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
> -                                   true) != 0) {
> -            put_packet(s, "E14");
> -        } else {
> -            put_packet(s, "OK");
> +        {
> +            static const GdbCmdParseEntry write_mem_cmd_desc = {
> +                .handler = handle_write_mem,
> +                .cmd = "M",
> +                .cmd_startswith = 1,
> +                .schema = "L,L:s0"
> +            };
> +            cmd_parser = &write_mem_cmd_desc;
>          }
>          break;
>      case 'p':


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 12/27] gdbstub: Implement read memory (m pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 48 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 8dc2e1d507..daa602edc3 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1710,6 +1710,30 @@ static void handle_write_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "OK");
>  }
>
> +static void handle_read_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (gdb_ctx->num_params < 2) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }

!=2?

Otherwise:

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


> +
> +    /* memtohex() doubles the required space */
> +    if (gdb_ctx->params[1].val_ull > MAX_PACKET_LENGTH / 2) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (target_memory_rw_debug(gdb_ctx->s->g_cpu, gdb_ctx->params[0].val_ull,
> +                               gdb_ctx->mem_buf,
> +                               gdb_ctx->params[1].val_ull, false)) {
> +        put_packet(gdb_ctx->s, "E14");
> +        return;
> +    }
> +
> +    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, gdb_ctx->params[1].val_ull);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1899,22 +1923,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          put_packet(s, "OK");
>          break;
>      case 'm':
> -        addr = strtoull(p, (char **)&p, 16);
> -        if (*p == ',')
> -            p++;
> -        len = strtoull(p, NULL, 16);
> -
> -        /* memtohex() doubles the required space */
> -        if (len > MAX_PACKET_LENGTH / 2) {
> -            put_packet (s, "E22");
> -            break;
> -        }
> -
> -        if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
> -            put_packet (s, "E14");
> -        } else {
> -            memtohex(buf, mem_buf, len);
> -            put_packet(s, buf);
> +        {
> +            static const GdbCmdParseEntry read_mem_cmd_desc = {
> +                .handler = handle_read_mem,
> +                .cmd = "m",
> +                .cmd_startswith = 1,
> +                .schema = "L,L0"
> +            };
> +            cmd_parser = &read_mem_cmd_desc;
>          }
>          break;
>      case 'M':


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 13/27] gdbstub: Implement write all registers (G pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index daa602edc3..adfe39b3a3 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1734,6 +1734,29 @@ static void handle_read_mem(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>  }
>
> +static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    target_ulong addr, len;
> +    uint8_t *registers;
> +    int reg_size;
> +
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    cpu_synchronize_state(gdb_ctx->s->g_cpu);
> +    registers = gdb_ctx->mem_buf;
> +    len = strlen(gdb_ctx->params[0].data) / 2;
> +    hextomem(registers, gdb_ctx->params[0].data, len);

Admittedly the legacy code didn't check either but we are assuming that
registers (i.e. gdb_ctx->mem_buf) won't overflow. It's probably still
safe as the incoming packets are limited in size. I was trying to find
where MAX_PACKET_LENGTH came from and AFAICT it's an arbitrary number we
made up.

I wonder if it would make sense in another series to convert the various
buffers to GString and GBytes so we can dynamically handle longer
packets without all this inconsistent application of bounds checking.

Anyway:

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


> +    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs && len > 0;
> +         addr++) {
> +        reg_size = gdb_write_register(gdb_ctx->s->g_cpu, registers, addr);
> +        len -= reg_size;
> +        registers += reg_size;
> +    }
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1745,7 +1768,6 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>      char thread_id[16];
> -    uint8_t *registers;
>      target_ulong addr, len;
>      const GdbCmdParseEntry *cmd_parser = NULL;
>
> @@ -1911,16 +1933,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          put_packet(s, buf);
>          break;
>      case 'G':
> -        cpu_synchronize_state(s->g_cpu);
> -        registers = mem_buf;
> -        len = strlen(p) / 2;
> -        hextomem((uint8_t *)registers, p, len);
> -        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs && len > 0; addr++) {
> -            reg_size = gdb_write_register(s->g_cpu, registers, addr);
> -            len -= reg_size;
> -            registers += reg_size;
> +        {
> +            static const GdbCmdParseEntry write_all_regs_cmd_desc = {
> +                .handler = handle_write_all_regs,
> +                .cmd = "G",
> +                .cmd_startswith = 1,
> +                .schema = "s0"
> +            };
> +            cmd_parser = &write_all_regs_cmd_desc;
>          }
> -        put_packet(s, "OK");
>          break;
>      case 'm':
>          {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 14/27] gdbstub: Implement read all registers (g pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index adfe39b3a3..3478ac778d 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1757,6 +1757,21 @@ static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "OK");
>  }
>
> +static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    target_ulong addr, len;
> +
> +    cpu_synchronize_state(gdb_ctx->s->g_cpu);
> +    len = 0;
> +    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
> +        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
> +                                 addr);
> +    }

Again no bounds checking - we get away with it because for hppa:

  (* 8 128 2) = 2048

Anyway:

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

> +
> +    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1764,7 +1779,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      CPUClass *cc;
>      const char *p;
>      uint32_t pid, tid;
> -    int ch, reg_size, type, res;
> +    int ch, type, res;
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>      char thread_id[16];
> @@ -1923,14 +1938,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'g':
> -        cpu_synchronize_state(s->g_cpu);
> -        len = 0;
> -        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
> -            reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
> -            len += reg_size;
> +        {
> +            static const GdbCmdParseEntry read_all_regs_cmd_desc = {
> +                .handler = handle_read_all_regs,
> +                .cmd = "g",
> +                .cmd_startswith = 1
> +            };
> +            cmd_parser = &read_all_regs_cmd_desc;
>          }
> -        memtohex(buf, mem_buf, len);
> -        put_packet(s, buf);
>          break;
>      case 'G':
>          {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

There is a bit more going on here than a simple conversion. I think we
need some additional commentary about the format of the data coming
back.


> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 62 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 3478ac778d..9fe130f30d 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1772,6 +1772,39 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>  }
>
> +static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int num_syscall_params;
> +    GdbCmdVariant syscall_params[3] = {};
> +
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    if (cmd_parse_params(gdb_ctx->params[0].data, "L,L,o0", syscall_params,
> +                         &num_syscall_params)) {
> +        return;
> +    }

What's going on here? I thought the schema was meant to handle the
parsing of data. I see bellow we originally parse the command as a null
terminated string but we actually should handle:

  ‘Fretcode,errno,Ctrl-C flag;call-specific attachment’

I see the argument for dealing with the call-specific attachment here
but shouldn't the generic parsing code be able to split everything
apart?

> +
> +    if (!num_syscall_params) {
> +        return;
> +    }
> +
> +    if (gdb_ctx->s->current_syscall_cb) {
> +        gdb_ctx->s->current_syscall_cb(gdb_ctx->s->c_cpu,
> +                                       (target_ulong)syscall_params[0].val_ull,
> +                                       (target_ulong)syscall_params[1].val_ull);
> +        gdb_ctx->s->current_syscall_cb = NULL;
> +    }



> +
> +    if (syscall_params[2].opcode == (uint8_t)'C') {
> +        put_packet(gdb_ctx->s, "T02");
> +        return;
> +    }
> +
> +    gdb_continue(gdb_ctx->s);
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1913,28 +1946,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          return RS_IDLE;
>      case 'F':
>          {
> -            target_ulong ret;
> -            target_ulong err;
> -
> -            ret = strtoull(p, (char **)&p, 16);
> -            if (*p == ',') {
> -                p++;
> -                err = strtoull(p, (char **)&p, 16);
> -            } else {
> -                err = 0;
> -            }
> -            if (*p == ',')
> -                p++;
> -            type = *p;
> -            if (s->current_syscall_cb) {
> -                s->current_syscall_cb(s->c_cpu, ret, err);
> -                s->current_syscall_cb = NULL;
> -            }
> -            if (type == 'C') {
> -                put_packet(s, "T02");
> -            } else {
> -                gdb_continue(s);
> -            }
> +            static const GdbCmdParseEntry file_io_cmd_desc = {
> +                .handler = handle_file_io,
> +                .cmd = "F",
> +                .cmd_startswith = 1,
> +                .schema = "s0"
> +            };
> +            cmd_parser = &file_io_cmd_desc;
>          }
>          break;
>      case 'g':


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 16/27] gdbstub: Implement step (s pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

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

> ---
>  gdbstub.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 9fe130f30d..9b0556f8be 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1805,6 +1805,16 @@ static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
>      gdb_continue(gdb_ctx->s);
>  }
>
> +static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (gdb_ctx->num_params) {
> +        gdb_set_cpu_pc(gdb_ctx->s, (target_ulong)gdb_ctx->params[0].val_ull);
> +    }
> +
> +    cpu_single_step(gdb_ctx->s->c_cpu, sstep_flags);
> +    gdb_continue(gdb_ctx->s);
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1937,13 +1947,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 's':
> -        if (*p != '\0') {
> -            addr = strtoull(p, (char **)&p, 16);
> -            gdb_set_cpu_pc(s, addr);
> +        {
> +            static const GdbCmdParseEntry step_cmd_desc = {
> +                .handler = handle_step,
> +                .cmd = "s",
> +                .cmd_startswith = 1,
> +                .schema = "L0"
> +            };
> +            cmd_parser = &step_cmd_desc;
>          }
> -        cpu_single_step(s->c_cpu, sstep_flags);
> -        gdb_continue(s);
> -        return RS_IDLE;
> +        break;
>      case 'F':
>          {
>              static const GdbCmdParseEntry file_io_cmd_desc = {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 17/27] gdbstub: Implement v commands with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 170 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 110 insertions(+), 60 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 9b0556f8be..d56d0fd235 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1815,6 +1815,106 @@ static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
>      gdb_continue(gdb_ctx->s);
>  }
>
> +static void handle_v_cont_query(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    put_packet(gdb_ctx->s, "vCont;c;C;s;S");
> +}
> +
> +static void handle_v_cont(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int res;
> +
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    res = gdb_handle_vcont(gdb_ctx->s, gdb_ctx->params[0].data);
> +    if ((res == -EINVAL) || (res == -ERANGE)) {
> +        put_packet(gdb_ctx->s, "E22");
> +    } else if (res) {
> +        put_packet(gdb_ctx->s, "\0");

Isn't this just ""?

Either way my reading of the spec say the response needs to be a "Stop
Reply Packet" which I don't think includes empty or E codes.

> +    }
> +}
> +
> +static void handle_v_attach(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    GDBProcess *process;
> +    CPUState *cpu;
> +    char thread_id[16];
> +
> +    strcpy(gdb_ctx->str_buf, "E22");

pstrcpy (see HACKING about strncpy) but...

> +    if (!gdb_ctx->num_params) {
> +        goto cleanup;
> +    }
> +
> +    process = gdb_get_process(gdb_ctx->s, gdb_ctx->params[0].val_ul);
> +    if (!process) {
> +        goto cleanup;
> +    }
> +
> +    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
> +    if (!cpu) {
> +        goto cleanup;
> +    }
> +
> +    process->attached = true;
> +    gdb_ctx->s->g_cpu = cpu;
> +    gdb_ctx->s->c_cpu = cpu;
> +
> +    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
> +             GDB_SIGNAL_TRAP, thread_id);

again this would be an argument for using GString to build-up our reply packets.

> +cleanup:
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    /* Kill the target */
> +    put_packet(gdb_ctx->s, "OK");
> +    error_report("QEMU: Terminated via GDBstub");
> +    exit(0);
> +}
> +
> +static GdbCmdParseEntry gdb_v_commands_table[] = {
> +    /* Order is important if has same prefix */
> +    {
> +        .handler = handle_v_cont_query,
> +        .cmd = "Cont?",
> +        .cmd_startswith = 1
> +    },
> +    {
> +        .handler = handle_v_cont,
> +        .cmd = "Cont",
> +        .cmd_startswith = 1,
> +        .schema = "s0"
> +    },
> +    {
> +        .handler = handle_v_attach,
> +        .cmd = "Attach;",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    {
> +        .handler = handle_v_kill,
> +        .cmd = "Kill;",
> +        .cmd_startswith = 1
> +    },
> +};
> +
> +static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> +                           gdb_v_commands_table,
> +                           ARRAY_SIZE(gdb_v_commands_table))) {
> +        put_packet(gdb_ctx->s, "");
> +    }
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1822,7 +1922,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      CPUClass *cc;
>      const char *p;
>      uint32_t pid, tid;
> -    int ch, type, res;
> +    int ch, type;
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>      char thread_id[16];
> @@ -1871,66 +1971,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'v':
> -        if (strncmp(p, "Cont", 4) == 0) {
> -            p += 4;
> -            if (*p == '?') {
> -                put_packet(s, "vCont;c;C;s;S");
> -                break;
> -            }
> -
> -            res = gdb_handle_vcont(s, p);
> -
> -            if (res) {
> -                if ((res == -EINVAL) || (res == -ERANGE)) {
> -                    put_packet(s, "E22");
> -                    break;
> -                }
> -                goto unknown_command;
> -            }
> -            break;
> -        } else if (strncmp(p, "Attach;", 7) == 0) {
> -            unsigned long pid;
> -
> -            p += 7;
> -
> -            if (qemu_strtoul(p, &p, 16, &pid)) {
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            process = gdb_get_process(s, pid);
> -
> -            if (process == NULL) {
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            cpu = get_first_cpu_in_process(s, process);
> -
> -            if (cpu == NULL) {
> -                /* Refuse to attach an empty process */
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            process->attached = true;
> -
> -            s->g_cpu = cpu;
> -            s->c_cpu = cpu;
> -
> -            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> -                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
> -
> -            put_packet(s, buf);
> -            break;
> -        } else if (strncmp(p, "Kill;", 5) == 0) {
> -            /* Kill the target */
> -            put_packet(s, "OK");
> -            error_report("QEMU: Terminated via GDBstub");
> -            exit(0);
> -        } else {
> -            goto unknown_command;
> +        {
> +            static const GdbCmdParseEntry v_cmd_desc = {
> +                .handler = handle_v_commands,
> +                .cmd = "v",
> +                .cmd_startswith = 1,
> +                .schema = "s0"
> +            };
> +            cmd_parser = &v_cmd_desc;
>          }
> +        break;
>      case 'k':
>          /* Kill the target */
>          error_report("QEMU: Terminated via GDBstub");


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 18/27] gdbstub: Implement generic query (q pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

A bit more for the commit message here as there seems to be a fair
amount going on.

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 327 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 327 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d56d0fd235..83ae8738cc 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1915,6 +1915,323 @@ static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
>      }
>  }
>
> +static void handle_query_qemu_sstepbits(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
> +             "ENABLE=%x,NOIRQ=%x,NOTIMER=%x", SSTEP_ENABLE,
> +             SSTEP_NOIRQ, SSTEP_NOTIMER);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_set_qemu_sstep(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    sstep_flags = gdb_ctx->params[0].val_ul;
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
> +static void handle_query_qemu_sstep(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "0x%x", sstep_flags);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_query_curr_tid(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    CPUState *cpu;
> +    GDBProcess *process;
> +    char thread_id[16];
> +
> +    /*
> +     * "Current thread" remains vague in the spec, so always return
> +     * the first thread of the current process (gdb returns the
> +     * first thread).
> +     */
> +    process = gdb_get_cpu_process(gdb_ctx->s, gdb_ctx->s->g_cpu);
> +    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
> +    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "QC%s", thread_id);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_query_threads(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    char thread_id[16];
> +
> +    if (!gdb_ctx->s->query_cpu) {
> +        put_packet(gdb_ctx->s, "l");
> +        return;
> +    }
> +
> +    gdb_fmt_thread_id(gdb_ctx->s, gdb_ctx->s->query_cpu, thread_id,
> +                      sizeof(thread_id));
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "m%s", thread_id);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +    gdb_ctx->s->query_cpu =
> +        gdb_next_attached_cpu(gdb_ctx->s, gdb_ctx->s->query_cpu);
> +}
> +
> +static void handle_query_first_threads(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    gdb_ctx->s->query_cpu = gdb_first_attached_cpu(gdb_ctx->s);
> +    handle_query_threads(gdb_ctx, user_ctx);
> +}
> +
> +static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    CPUState *cpu;
> +    int len;
> +
> +    if (!gdb_ctx->num_params ||
> +        gdb_ctx->params[0].thread_id.kind == GDB_READ_THREAD_ERR) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    cpu = gdb_get_cpu(gdb_ctx->s, gdb_ctx->params[0].thread_id.pid,
> +                      gdb_ctx->params[0].thread_id.tid);
> +    if (!cpu) {
> +        return;
> +    }
> +
> +    cpu_synchronize_state(cpu);
> +
> +    if (gdb_ctx->s->multiprocess && (gdb_ctx->s->process_num > 1)) {
> +        /* Print the CPU model and name in multiprocess mode */
> +        ObjectClass *oc = object_get_class(OBJECT(cpu));
> +        const char *cpu_model = object_class_get_name(oc);
> +        char *cpu_name = object_get_canonical_path_component(OBJECT(cpu));
> +        len = snprintf((char *)gdb_ctx->mem_buf, sizeof(gdb_ctx->str_buf) / 2,
> +                       "%s %s [%s]", cpu_model, cpu_name,
> +                       cpu->halted ? "halted " : "running");
> +        g_free(cpu_name);
> +    } else {
> +        /* memtohex() doubles the required space */
> +        len = snprintf((char *)gdb_ctx->mem_buf, sizeof(gdb_ctx->str_buf) / 2,
> +                        "CPU#%d [%s]", cpu->cpu_index,
> +                        cpu->halted ? "halted " : "running");
> +    }
> +    trace_gdbstub_op_extra_info((char *)gdb_ctx->mem_buf);
> +    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +#ifdef CONFIG_USER_ONLY
> +static void handle_query_offsets(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    TaskState *ts;
> +
> +    ts = gdb_ctx->s->c_cpu->opaque;
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
> +             "Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
> +             ";Bss=" TARGET_ABI_FMT_lx,
> +             ts->info->code_offset,
> +             ts->info->data_offset,
> +             ts->info->data_offset);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +#else
> +static void handle_query_rcmd(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int len;
> +
> +    if (!gdb_ctx->num_params) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    len = strlen(gdb_ctx->params[0].data);
> +    if (len % 2) {
> +        put_packet(gdb_ctx->s, "E01");
> +        return;
> +    }
> +
> +    len = len / 2;
> +    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[0].data, len);
> +    gdb_ctx->mem_buf[len++] = 0;
> +    qemu_chr_be_write(gdb_ctx->s->mon_chr, gdb_ctx->mem_buf, len);
> +    put_packet(gdb_ctx->s, "OK");
> +
> +}
> +#endif
> +
> +static void handle_query_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    CPUClass *cc;
> +
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "PacketSize=%x",
> +             MAX_PACKET_LENGTH);
> +    cc = CPU_GET_CLASS(first_cpu);
> +    if (cc->gdb_core_xml_file) {
> +        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
> +                ";qXfer:features:read+");
> +    }
> +
> +    if (gdb_ctx->num_params &&
> +        strstr(gdb_ctx->params[0].data, "multiprocess+")) {
> +        gdb_ctx->s->multiprocess = true;
> +    }
> +
> +    pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";multiprocess+");
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_query_xfer_features(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    GDBProcess *process;
> +    CPUClass *cc;
> +    unsigned long len, total_len, addr;
> +    const char *xml;
> +    const char *p;
> +
> +    if (gdb_ctx->num_params < 3) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    process = gdb_get_cpu_process(gdb_ctx->s, gdb_ctx->s->g_cpu);
> +    cc = CPU_GET_CLASS(gdb_ctx->s->g_cpu);
> +    if (!cc->gdb_core_xml_file) {
> +        put_packet(gdb_ctx->s, "");
> +        return;
> +    }
> +
> +    gdb_has_xml = true;
> +    p = gdb_ctx->params[0].data;
> +    xml = get_feature_xml(gdb_ctx->s, p, &p, process);
> +    if (!xml) {
> +        put_packet(gdb_ctx->s, "E00");
> +        return;
> +    }
> +
> +    addr = gdb_ctx->params[1].val_ul;
> +    len = gdb_ctx->params[2].val_ul;
> +    total_len = strlen(xml);
> +    if (addr > total_len) {
> +        put_packet(gdb_ctx->s, "E00");
> +        return;
> +    }
> +
> +    if (len > (MAX_PACKET_LENGTH - 5) / 2) {
> +        len = (MAX_PACKET_LENGTH - 5) / 2;
> +    }
> +
> +    if (len < total_len - addr) {
> +        gdb_ctx->str_buf[0] = 'm';
> +        len = memtox(gdb_ctx->str_buf + 1, xml + addr, len);
> +    } else {
> +        gdb_ctx->str_buf[0] = 'l';
> +        len = memtox(gdb_ctx->str_buf + 1, xml + addr, total_len - addr);
> +    }
> +
> +    put_packet_binary(gdb_ctx->s, gdb_ctx->str_buf, len + 1, true);
> +}
> +
> +static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    put_packet(gdb_ctx->s, GDB_ATTACHED);
> +}
> +
> +static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
> +    /* Order is important if has same prefix */
> +    {
> +        .handler = handle_query_qemu_sstepbits,
> +        .cmd = "qemu.sstepbits",
> +    },
> +    {
> +        .handler = handle_query_qemu_sstep,
> +        .cmd = "qemu.sstep",
> +    },
> +    {
> +        .handler = handle_set_qemu_sstep,
> +        .cmd = "qemu.sstep=",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +};
> +
> +static GdbCmdParseEntry gdb_gen_query_table[] = {
> +    {
> +        .handler = handle_query_curr_tid,
> +        .cmd = "C",
> +    },
> +    {
> +        .handler = handle_query_threads,
> +        .cmd = "sThreadInfo",
> +    },
> +    {
> +        .handler = handle_query_first_threads,
> +        .cmd = "fThreadInfo",
> +    },
> +    {
> +        .handler = handle_query_thread_extra,
> +        .cmd = "ThreadExtraInfo,",
> +        .cmd_startswith = 1,
> +        .schema = "t0"
> +    },
> +#ifdef CONFIG_USER_ONLY
> +    {
> +        .handler = handle_query_offsets,
> +        .cmd = "Offsets",
> +    },
> +#else
> +    {
> +        .handler = handle_query_rcmd,
> +        .cmd = "Rcmd,",
> +        .cmd_startswith = 1,
> +        .schema = "s0"
> +    },
> +#endif
> +    {
> +        .handler = handle_query_supported,
> +        .cmd = "Supported:",
> +        .cmd_startswith = 1,
> +        .schema = "s0"
> +    },
> +    {
> +        .handler = handle_query_supported,
> +        .cmd = "Supported",
> +        .schema = "s0"
> +    },
> +    {
> +        .handler = handle_query_xfer_features,
> +        .cmd = "Xfer:features:read:",
> +        .cmd_startswith = 1,
> +        .schema = "s:l,l0"
> +    },
> +    {
> +        .handler = handle_query_attached,
> +        .cmd = "Attached:",
> +        .cmd_startswith = 1
> +    },
> +    {
> +        .handler = handle_query_attached,
> +        .cmd = "Attached",
> +    },
> +};
> +
> +static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    if (!process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> +                            gdb_gen_query_set_common_table,
> +                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
> +        return;
> +    }
> +
> +    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> +                           gdb_gen_query_table,
> +                           ARRAY_SIZE(gdb_gen_query_table))) {
> +        put_packet(gdb_ctx->s, "");
> +    }
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -2128,6 +2445,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'q':
> +        {
> +            static const GdbCmdParseEntry gen_query_cmd_desc = {
> +                .handler = handle_gen_query,
> +                .cmd = "q",
> +                .cmd_startswith = 1,
> +                .schema = "s0"
> +            };
> +            cmd_parser = &gen_query_cmd_desc;
> +        }
> +        break;

The fact we haven't delete any code bellow makes me think you probably
want to merge the two q patch commits.

>      case 'Q':
>          /* parse any 'q' packets here */
>          if (!strcmp(p,"qemu.sstepbits")) {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 20/27] gdbstub: Implement target halted (? pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 2fd0d66f4d..d678191705 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2239,13 +2239,30 @@ static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "");
>  }
>
> +static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    char thread_id[16];
> +
> +    /* TODO: Make this return the correct value for user-mode.  */

Can this be cleaned up as we convert?

> +    gdb_fmt_thread_id(gdb_ctx->s, gdb_ctx->s->c_cpu, thread_id,
> +                      sizeof(thread_id));
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
> +             GDB_SIGNAL_TRAP, thread_id);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +    /*
> +     * Remove all the breakpoints when this query is issued,
> +     * because gdb is doing and initial connect and the state

s/and/an/

> +     * should be cleaned up.
> +     */
> +    gdb_breakpoint_remove_all();
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      const char *p;
>      int ch;
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> -    char thread_id[16];
>      const GdbCmdParseEntry *cmd_parser = NULL;
>
>      trace_gdbstub_io_command(line_buf);
> @@ -2257,15 +2274,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          put_packet(s, "OK");
>          break;
>      case '?':
> -        /* TODO: Make this return the correct value for user-mode.  */
> -        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> -                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
> -        put_packet(s, buf);
> -        /* Remove all the breakpoints when this query is issued,
> -         * because gdb is doing and initial connect and the state
> -         * should be cleaned up.
> -         */
> -        gdb_breakpoint_remove_all();
> +        {
> +            static const GdbCmdParseEntry target_halted_cmd_desc = {
> +                .handler = handle_target_halt,
> +                .cmd = "?",
> +                .cmd_startswith = 1
> +            };
> +            cmd_parser = &target_halted_cmd_desc;
> +        }
>          break;
>      case 'c':
>          {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 21/27] gdbstub: Clear unused variables in gdb_handle_packet

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>

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

> ---
>  gdbstub.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d678191705..8bdfae4b29 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2259,17 +2259,11 @@ static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
>
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
> -    const char *p;
> -    int ch;
> -    uint8_t mem_buf[MAX_PACKET_LENGTH];
> -    char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>      const GdbCmdParseEntry *cmd_parser = NULL;
>
>      trace_gdbstub_io_command(line_buf);
>
> -    p = line_buf;
> -    ch = *p++;
> -    switch(ch) {
> +    switch (line_buf[0]) {
>      case '!':
>          put_packet(s, "OK");
>          break;
> @@ -2486,8 +2480,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          break;
>      default:
>          /* put empty packet */
> -        buf[0] = '\0';
> -        put_packet(s, buf);
> +        put_packet(s, "");
>          break;
>      }


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 22/27] gdbstub: Implement generic query qemu.Supported

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> qemu.Supported query reply back with the supported qemu query/set
> commands (commands are seperated with a semicolon from each other).
>
> gdb> maint packet qqemu.Supported
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 8bdfae4b29..00c07d6ec0 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2127,6 +2127,11 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, GDB_ATTACHED);
>  }
>
> +static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    put_packet(gdb_ctx->s, "sstepbits;sstep");

To maintain bisectability this response should be extended as each
feature is added.

> +}
> +
>  static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
>      /* Order is important if has same prefix */
>      {
> @@ -2203,6 +2208,10 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
>          .handler = handle_query_attached,
>          .cmd = "Attached",
>      },
> +    {
> +        .handler = handle_query_qemu_supported,
> +        .cmd = "qemu.Supported",
> +    },
>  };
>
>  static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 24/27] gdbstub: Add another handler for setting qemu.sstep

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Follow GDB general query/set packet conventions, qemu.sstep can now
> be set with the following command as well:
> gdb> maint packet Qqemu.sstep:Value

I;m not sure about exposing internal values to a protocol like this.
Maybe text based flags would be better?

>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 88ff6224e6..34da10260d 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2260,6 +2260,12 @@ static GdbCmdParseEntry gdb_gen_set_table[] = {
>          .cmd_startswith = 1,
>          .schema = "l0"
>      },
> +    {
> +        .handler = handle_set_qemu_sstep,
> +        .cmd = "qemu.sstep:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },

Hmm the implementation seems to have gone in earlier. These should be
together as a feature patch (along with changing the query/probe
responses).

>  };
>
>  static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 26/27] gdbstub: Add support to read a MSR for KVM target

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> gdb> maint packet qqemu.kvm.Rdmsr:MsrIndex

gdbserver already has a mechanism for exposing system registers see:

  commit 200bf5b7ffea635079cc05fdfb363372b9544ce7
  Author: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
  Date:   Fri May 18 17:48:07 2018 +0100

for an example. As MSR's are very specific to x86 all this should be
handled via target/i386/gdbstub and kept out of the generic code.

>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 34da10260d..f48c3a2b5f 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2141,7 +2141,14 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
>
>  static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
>  {
> -    put_packet(gdb_ctx->s, "sstepbits;sstep;PhyMemMode");
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
> +             "sstepbits;sstep;PhyMemMode");
> +
> +    if (kvm_enabled()) {
> +        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";kvm.Rdmsr");
> +    }
> +
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>  }
>
>  static void handle_query_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx,
> @@ -2166,6 +2173,29 @@ static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "OK");
>  }
>
> +static void handle_query_kvm_read_msr(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    uint64_t msr_val;
> +
> +    if (!kvm_enabled()) {
> +        return;
> +    }
> +
> +    if (!gdb_ctx->num_params) {
> +        put_packet(gdb_ctx->s, "E22");
> +        return;
> +    }
> +
> +    if (kvm_arch_read_msr(gdbserver_state->c_cpu, gdb_ctx->params[0].val_ul,
> +                          &msr_val)) {
> +        put_packet(gdb_ctx->s, "E00");
> +        return;
> +    }
> +
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "0x%" PRIx64, msr_val);
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
>  static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
>      /* Order is important if has same prefix */
>      {
> @@ -2250,6 +2280,12 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
>          .handler = handle_query_qemu_phy_mem_mode,
>          .cmd = "qemu.PhyMemMode",
>      },
> +    {
> +        .handler = handle_query_kvm_read_msr,
> +        .cmd = "qemu.kvm.Rdmsr:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
>  };
>
>  static GdbCmdParseEntry gdb_gen_set_table[] = {


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> This patch series refactors the old gdbstub command packets handler
> with a new infrastructure which should ease extending and adding new
> and missing gdb command packets.

Jon,

I've finished my review and things are looking pretty good. The code is
a good clean-up and makes adding new features a lot easier. Thanks for
the examples of extensions - they were worth it to see how this might be
used although we shouldn't include them in the first merge. As they
extend the gdbserver ABI we'll want to think carefully about exactly
what we want to expose before we include it in master.

Going forwards aside from the various comments on each patch it would be
worth making sure the branch has gone through at least one CI run to
make sure the non-x86 builds (and disable-tcg and other exotica) haven't
been broken.

It would be nice if we could extend the testing of the gdbserver. Have
you been testing this with the gdb test suite or just manually? Now we
have system test and linux-user binaries being built we could probably
do better than the manually run tests/guest-debug/test-gdbstub.py.

Finally it would be nice if we could modernise the membuf and strbuf
handling with a more robust glib based approach. I understand if you
don't want to do that now and I'll happily accept the patches without it
but I did notice you can send the gdbserver a bit loopy if you send it
some very long maint packets so it would be nice to have that a bit
safer.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler

[Alex Bennée]

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

> Jon Doron <arilou@gmail.com> writes:
>
>> This patch series refactors the old gdbstub command packets handler
>> with a new infrastructure which should ease extending and adding new
>> and missing gdb command packets.
>
> Jon,
>
<snip>
>
> Finally it would be nice if we could modernise the membuf and strbuf
> handling with a more robust glib based approach. I understand if you
> don't want to do that now and I'll happily accept the patches without it
> but I did notice you can send the gdbserver a bit loopy if you send it
> some very long maint packets so it would be nice to have that a bit
> safer.

Actually I had a go at this this morning and it turned out to be quite
fiddly so perhaps this is something best left to a follow-up series once
the re-factoring is in.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 09/27] gdbstub: Implement set register (P pkt) with new infra

[Jon Doron]

You are right but I was under the impression you want the patches to
have the exact same logic of the original code, that why i kept
adding those null packets and the parameter checks to be < N rather than !=

Now that I understand you prefer to fix the implementation ill review
all the patches and add error replays accordingly

-- Jon.

On Wed, May 15, 2019 at 3:14 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 39 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index b42425b24c..10e3f12a68 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1634,6 +1634,27 @@ static void handle_remove_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      put_packet(gdb_ctx->s, "E22");
> >  }
> >
> > +static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    int reg_size;
> > +
> > +    if (!gdb_has_xml) {
> > +        put_packet(gdb_ctx->s, "");
> > +        return;
> > +    }
> > +
> > +    if (gdb_ctx->num_params < 2) {
>
> != 2?
>
> > +        put_packet(gdb_ctx->s, "");
> > +        return;
> > +    }
>
> I don't understand what these null put_packets have been added for.
> Should we not report an E code on failure?
>
> > +
> > +    reg_size = strlen(gdb_ctx->params[1].data) / 2;
> > +    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[1].data, reg_size);
> > +    gdb_write_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf,
> > +                       gdb_ctx->params[0].val_ull);
> > +    put_packet(gdb_ctx->s, "OK");
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
> > @@ -1878,15 +1899,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          }
> >          break;
> >      case 'P':
> > -        if (!gdb_has_xml)
> > -            goto unknown_command;
> > -        addr = strtoull(p, (char **)&p, 16);
> > -        if (*p == '=')
> > -            p++;
> > -        reg_size = strlen(p) / 2;
> > -        hextomem(mem_buf, p, reg_size);
> > -        gdb_write_register(s->g_cpu, mem_buf, addr);
> > -        put_packet(s, "OK");
> > +        {
> > +            static const GdbCmdParseEntry set_reg_cmd_desc = {
> > +                .handler = handle_set_reg,
> > +                .cmd = "P",
> > +                .cmd_startswith = 1,
> > +                .schema = "L?s0"
> > +            };
> > +            cmd_parser = &set_reg_cmd_desc;
> > +        }
> >          break;
> >      case 'Z':
> >          {
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 14/27] gdbstub: Implement read all registers (g pkt) with new infra

[Jon Doron]

I agree but I guess to really fix it we need to change
gdb_read_register implementation to support returning the size of the
register for mem_buffer = NULL
Let's leave it for another patchset?

On Wed, May 15, 2019 at 7:10 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index adfe39b3a3..3478ac778d 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1757,6 +1757,21 @@ static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      put_packet(gdb_ctx->s, "OK");
> >  }
> >
> > +static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    target_ulong addr, len;
> > +
> > +    cpu_synchronize_state(gdb_ctx->s->g_cpu);
> > +    len = 0;
> > +    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
> > +        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
> > +                                 addr);
> > +    }
>
> Again no bounds checking - we get away with it because for hppa:
>
>   (* 8 128 2) = 2048
>
> Anyway:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> > +
> > +    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
> > @@ -1764,7 +1779,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >      CPUClass *cc;
> >      const char *p;
> >      uint32_t pid, tid;
> > -    int ch, reg_size, type, res;
> > +    int ch, type, res;
> >      uint8_t mem_buf[MAX_PACKET_LENGTH];
> >      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> >      char thread_id[16];
> > @@ -1923,14 +1938,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          }
> >          break;
> >      case 'g':
> > -        cpu_synchronize_state(s->g_cpu);
> > -        len = 0;
> > -        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
> > -            reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
> > -            len += reg_size;
> > +        {
> > +            static const GdbCmdParseEntry read_all_regs_cmd_desc = {
> > +                .handler = handle_read_all_regs,
> > +                .cmd = "g",
> > +                .cmd_startswith = 1
> > +            };
> > +            cmd_parser = &read_all_regs_cmd_desc;
> >          }
> > -        memtohex(buf, mem_buf, len);
> > -        put_packet(s, buf);
> >          break;
> >      case 'G':
> >          {
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra

[Jon Doron]

Hi Alex, I did have some issues with the 'F' packet as it's not really
well documented, I suggest changing the schema to:
"L,L,o0"
so basically no support for anything after the first C in the Ctrl-C,
if you have a sample or a documentation that really implements
the F packet fully ill take a look at it and see how the schema should
really look like.

-- Jon.

On Wed, May 15, 2019 at 7:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> There is a bit more going on here than a simple conversion. I think we
> need some additional commentary about the format of the data coming
> back.
>
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 62 +++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 40 insertions(+), 22 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 3478ac778d..9fe130f30d 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1772,6 +1772,39 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> >  }
> >
> > +static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    int num_syscall_params;
> > +    GdbCmdVariant syscall_params[3] = {};
> > +
> > +    if (!gdb_ctx->num_params) {
> > +        return;
> > +    }
> > +
> > +    if (cmd_parse_params(gdb_ctx->params[0].data, "L,L,o0", syscall_params,
> > +                         &num_syscall_params)) {
> > +        return;
> > +    }
>
> What's going on here? I thought the schema was meant to handle the
> parsing of data. I see bellow we originally parse the command as a null
> terminated string but we actually should handle:
>
>   ‘Fretcode,errno,Ctrl-C flag;call-specific attachment’
>
> I see the argument for dealing with the call-specific attachment here
> but shouldn't the generic parsing code be able to split everything
> apart?
>
> > +
> > +    if (!num_syscall_params) {
> > +        return;
> > +    }
> > +
> > +    if (gdb_ctx->s->current_syscall_cb) {
> > +        gdb_ctx->s->current_syscall_cb(gdb_ctx->s->c_cpu,
> > +                                       (target_ulong)syscall_params[0].val_ull,
> > +                                       (target_ulong)syscall_params[1].val_ull);
> > +        gdb_ctx->s->current_syscall_cb = NULL;
> > +    }
>
>
>
> > +
> > +    if (syscall_params[2].opcode == (uint8_t)'C') {
> > +        put_packet(gdb_ctx->s, "T02");
> > +        return;
> > +    }
> > +
> > +    gdb_continue(gdb_ctx->s);
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
> > @@ -1913,28 +1946,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          return RS_IDLE;
> >      case 'F':
> >          {
> > -            target_ulong ret;
> > -            target_ulong err;
> > -
> > -            ret = strtoull(p, (char **)&p, 16);
> > -            if (*p == ',') {
> > -                p++;
> > -                err = strtoull(p, (char **)&p, 16);
> > -            } else {
> > -                err = 0;
> > -            }
> > -            if (*p == ',')
> > -                p++;
> > -            type = *p;
> > -            if (s->current_syscall_cb) {
> > -                s->current_syscall_cb(s->c_cpu, ret, err);
> > -                s->current_syscall_cb = NULL;
> > -            }
> > -            if (type == 'C') {
> > -                put_packet(s, "T02");
> > -            } else {
> > -                gdb_continue(s);
> > -            }
> > +            static const GdbCmdParseEntry file_io_cmd_desc = {
> > +                .handler = handle_file_io,
> > +                .cmd = "F",
> > +                .cmd_startswith = 1,
> > +                .schema = "s0"
> > +            };
> > +            cmd_parser = &file_io_cmd_desc;
> >          }
> >          break;
> >      case 'g':
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Hi Alex, I did have some issues with the 'F' packet as it's not really
> well documented, I suggest changing the schema to:
> "L,L,o0"
> so basically no support for anything after the first C in the Ctrl-C,
> if you have a sample or a documentation that really implements
> the F packet fully ill take a look at it and see how the schema should
> really look like.

I'm only going by whats in the gdbdocs:

  https://sourceware.org/gdb/onlinedocs/gdb/The-F-Request-Packet.html#The-F-Request-Packet

>
> -- Jon.
>
> On Wed, May 15, 2019 at 7:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Jon Doron <arilou@gmail.com> writes:
>>
>> There is a bit more going on here than a simple conversion. I think we
>> need some additional commentary about the format of the data coming
>> back.
>>
>>
>> > Signed-off-by: Jon Doron <arilou@gmail.com>
>> > ---
>> >  gdbstub.c | 62 +++++++++++++++++++++++++++++++++++--------------------
>> >  1 file changed, 40 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index 3478ac778d..9fe130f30d 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -1772,6 +1772,39 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>> >      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>> >  }
>> >
>> > +static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
>> > +{
>> > +    int num_syscall_params;
>> > +    GdbCmdVariant syscall_params[3] = {};
>> > +
>> > +    if (!gdb_ctx->num_params) {
>> > +        return;
>> > +    }
>> > +
>> > +    if (cmd_parse_params(gdb_ctx->params[0].data, "L,L,o0", syscall_params,
>> > +                         &num_syscall_params)) {
>> > +        return;
>> > +    }
>>
>> What's going on here? I thought the schema was meant to handle the
>> parsing of data. I see bellow we originally parse the command as a null
>> terminated string but we actually should handle:
>>
>>   ‘Fretcode,errno,Ctrl-C flag;call-specific attachment’
>>
>> I see the argument for dealing with the call-specific attachment here
>> but shouldn't the generic parsing code be able to split everything
>> apart?
>>
>> > +
>> > +    if (!num_syscall_params) {
>> > +        return;
>> > +    }
>> > +
>> > +    if (gdb_ctx->s->current_syscall_cb) {
>> > +        gdb_ctx->s->current_syscall_cb(gdb_ctx->s->c_cpu,
>> > +                                       (target_ulong)syscall_params[0].val_ull,
>> > +                                       (target_ulong)syscall_params[1].val_ull);
>> > +        gdb_ctx->s->current_syscall_cb = NULL;
>> > +    }
>>
>>
>>
>> > +
>> > +    if (syscall_params[2].opcode == (uint8_t)'C') {
>> > +        put_packet(gdb_ctx->s, "T02");
>> > +        return;
>> > +    }
>> > +
>> > +    gdb_continue(gdb_ctx->s);
>> > +}
>> > +
>> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >  {
>> >      CPUState *cpu;
>> > @@ -1913,28 +1946,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >          return RS_IDLE;
>> >      case 'F':
>> >          {
>> > -            target_ulong ret;
>> > -            target_ulong err;
>> > -
>> > -            ret = strtoull(p, (char **)&p, 16);
>> > -            if (*p == ',') {
>> > -                p++;
>> > -                err = strtoull(p, (char **)&p, 16);
>> > -            } else {
>> > -                err = 0;
>> > -            }
>> > -            if (*p == ',')
>> > -                p++;
>> > -            type = *p;
>> > -            if (s->current_syscall_cb) {
>> > -                s->current_syscall_cb(s->c_cpu, ret, err);
>> > -                s->current_syscall_cb = NULL;
>> > -            }
>> > -            if (type == 'C') {
>> > -                put_packet(s, "T02");
>> > -            } else {
>> > -                gdb_continue(s);
>> > -            }
>> > +            static const GdbCmdParseEntry file_io_cmd_desc = {
>> > +                .handler = handle_file_io,
>> > +                .cmd = "F",
>> > +                .cmd_startswith = 1,
>> > +                .schema = "s0"
>> > +            };
>> > +            cmd_parser = &file_io_cmd_desc;
>> >          }
>> >          break;
>> >      case 'g':
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 14/27] gdbstub: Implement read all registers (g pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> I agree but I guess to really fix it we need to change
> gdb_read_register implementation to support returning the size of the
> register for mem_buffer = NULL
> Let's leave it for another patchset?

Sure

>
> On Wed, May 15, 2019 at 7:10 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Jon Doron <arilou@gmail.com> writes:
>>
>> > Signed-off-by: Jon Doron <arilou@gmail.com>
>> > ---
>> >  gdbstub.c | 31 +++++++++++++++++++++++--------
>> >  1 file changed, 23 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index adfe39b3a3..3478ac778d 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -1757,6 +1757,21 @@ static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>> >      put_packet(gdb_ctx->s, "OK");
>> >  }
>> >
>> > +static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>> > +{
>> > +    target_ulong addr, len;
>> > +
>> > +    cpu_synchronize_state(gdb_ctx->s->g_cpu);
>> > +    len = 0;
>> > +    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>> > +        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>> > +                                 addr);
>> > +    }
>>
>> Again no bounds checking - we get away with it because for hppa:
>>
>>   (* 8 128 2) = 2048
>>
>> Anyway:
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> > +
>> > +    memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
>> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>> > +}
>> > +
>> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >  {
>> >      CPUState *cpu;
>> > @@ -1764,7 +1779,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >      CPUClass *cc;
>> >      const char *p;
>> >      uint32_t pid, tid;
>> > -    int ch, reg_size, type, res;
>> > +    int ch, type, res;
>> >      uint8_t mem_buf[MAX_PACKET_LENGTH];
>> >      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>> >      char thread_id[16];
>> > @@ -1923,14 +1938,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >          }
>> >          break;
>> >      case 'g':
>> > -        cpu_synchronize_state(s->g_cpu);
>> > -        len = 0;
>> > -        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
>> > -            reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
>> > -            len += reg_size;
>> > +        {
>> > +            static const GdbCmdParseEntry read_all_regs_cmd_desc = {
>> > +                .handler = handle_read_all_regs,
>> > +                .cmd = "g",
>> > +                .cmd_startswith = 1
>> > +            };
>> > +            cmd_parser = &read_all_regs_cmd_desc;
>> >          }
>> > -        memtohex(buf, mem_buf, len);
>> > -        put_packet(s, buf);
>> >          break;
>> >      case 'G':
>> >          {
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 17/27] gdbstub: Implement v commands with new infra

[Jon Doron]

On Wed, May 15, 2019 at 8:06 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 170 +++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 110 insertions(+), 60 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 9b0556f8be..d56d0fd235 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1815,6 +1815,106 @@ static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      gdb_continue(gdb_ctx->s);
> >  }
> >
> > +static void handle_v_cont_query(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    put_packet(gdb_ctx->s, "vCont;c;C;s;S");
> > +}
> > +
> > +static void handle_v_cont(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    int res;
> > +
> > +    if (!gdb_ctx->num_params) {
> > +        return;
> > +    }
> > +
> > +    res = gdb_handle_vcont(gdb_ctx->s, gdb_ctx->params[0].data);
> > +    if ((res == -EINVAL) || (res == -ERANGE)) {
> > +        put_packet(gdb_ctx->s, "E22");
> > +    } else if (res) {
> > +        put_packet(gdb_ctx->s, "\0");
>
> Isn't this just ""?
>
> Either way my reading of the spec say the response needs to be a "Stop
> Reply Packet" which I don't think includes empty or E codes.
>

From my understanding reading the spec and the gdbserver
implementation in binutils a null packet tells the client
the command is unsupported, so it makes sense to reply with this null
packet if handle_vcont replied with something
we dont know (i.e -ENOTSUP)

> > +    }
> > +}
> > +
> > +static void handle_v_attach(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    GDBProcess *process;
> > +    CPUState *cpu;
> > +    char thread_id[16];
> > +
> > +    strcpy(gdb_ctx->str_buf, "E22");
>
> pstrcpy (see HACKING about strncpy) but...
>
> > +    if (!gdb_ctx->num_params) {
> > +        goto cleanup;
> > +    }
> > +
> > +    process = gdb_get_process(gdb_ctx->s, gdb_ctx->params[0].val_ul);
> > +    if (!process) {
> > +        goto cleanup;
> > +    }
> > +
> > +    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
> > +    if (!cpu) {
> > +        goto cleanup;
> > +    }
> > +
> > +    process->attached = true;
> > +    gdb_ctx->s->g_cpu = cpu;
> > +    gdb_ctx->s->c_cpu = cpu;
> > +
> > +    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
> > +             GDB_SIGNAL_TRAP, thread_id);
>
> again this would be an argument for using GString to build-up our reply packets.
>
Perhaps we will need to make another patchset which fixes all the
strings/buffers stuff and move to Glib
but like you said probably too much for this patchset
> > +cleanup:
> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> > +}
> > +
> > +static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    /* Kill the target */
> > +    put_packet(gdb_ctx->s, "OK");
> > +    error_report("QEMU: Terminated via GDBstub");
> > +    exit(0);
> > +}
> > +
> > +static GdbCmdParseEntry gdb_v_commands_table[] = {
> > +    /* Order is important if has same prefix */
> > +    {
> > +        .handler = handle_v_cont_query,
> > +        .cmd = "Cont?",
> > +        .cmd_startswith = 1
> > +    },
> > +    {
> > +        .handler = handle_v_cont,
> > +        .cmd = "Cont",
> > +        .cmd_startswith = 1,
> > +        .schema = "s0"
> > +    },
> > +    {
> > +        .handler = handle_v_attach,
> > +        .cmd = "Attach;",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
> > +    {
> > +        .handler = handle_v_kill,
> > +        .cmd = "Kill;",
> > +        .cmd_startswith = 1
> > +    },
> > +};
> > +
> > +static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    if (!gdb_ctx->num_params) {
> > +        return;
> > +    }
> > +
> > +    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> > +                           gdb_v_commands_table,
> > +                           ARRAY_SIZE(gdb_v_commands_table))) {
> > +        put_packet(gdb_ctx->s, "");
> > +    }
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
> > @@ -1822,7 +1922,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >      CPUClass *cc;
> >      const char *p;
> >      uint32_t pid, tid;
> > -    int ch, type, res;
> > +    int ch, type;
> >      uint8_t mem_buf[MAX_PACKET_LENGTH];
> >      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> >      char thread_id[16];
> > @@ -1871,66 +1971,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          }
> >          break;
> >      case 'v':
> > -        if (strncmp(p, "Cont", 4) == 0) {
> > -            p += 4;
> > -            if (*p == '?') {
> > -                put_packet(s, "vCont;c;C;s;S");
> > -                break;
> > -            }
> > -
> > -            res = gdb_handle_vcont(s, p);
> > -
> > -            if (res) {
> > -                if ((res == -EINVAL) || (res == -ERANGE)) {
> > -                    put_packet(s, "E22");
> > -                    break;
> > -                }
> > -                goto unknown_command;
> > -            }
> > -            break;
> > -        } else if (strncmp(p, "Attach;", 7) == 0) {
> > -            unsigned long pid;
> > -
> > -            p += 7;
> > -
> > -            if (qemu_strtoul(p, &p, 16, &pid)) {
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            process = gdb_get_process(s, pid);
> > -
> > -            if (process == NULL) {
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            cpu = get_first_cpu_in_process(s, process);
> > -
> > -            if (cpu == NULL) {
> > -                /* Refuse to attach an empty process */
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            process->attached = true;
> > -
> > -            s->g_cpu = cpu;
> > -            s->c_cpu = cpu;
> > -
> > -            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> > -                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
> > -
> > -            put_packet(s, buf);
> > -            break;
> > -        } else if (strncmp(p, "Kill;", 5) == 0) {
> > -            /* Kill the target */
> > -            put_packet(s, "OK");
> > -            error_report("QEMU: Terminated via GDBstub");
> > -            exit(0);
> > -        } else {
> > -            goto unknown_command;
> > +        {
> > +            static const GdbCmdParseEntry v_cmd_desc = {
> > +                .handler = handle_v_commands,
> > +                .cmd = "v",
> > +                .cmd_startswith = 1,
> > +                .schema = "s0"
> > +            };
> > +            cmd_parser = &v_cmd_desc;
> >          }
> > +        break;
> >      case 'k':
> >          /* Kill the target */
> >          error_report("QEMU: Terminated via GDBstub");
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 22/27] gdbstub: Implement generic query qemu.Supported

[Jon Doron]

I suggest then that I'll squash this commit into the commit that
refactors the the Q/q packets and will add the required documentation
about this
in the commit message.

Do you agree?
-- Jon.

On Wed, May 15, 2019 at 8:41 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > qemu.Supported query reply back with the supported qemu query/set
> > commands (commands are seperated with a semicolon from each other).
> >
> > gdb> maint packet qqemu.Supported
> >
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 8bdfae4b29..00c07d6ec0 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -2127,6 +2127,11 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      put_packet(gdb_ctx->s, GDB_ATTACHED);
> >  }
> >
> > +static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    put_packet(gdb_ctx->s, "sstepbits;sstep");
>
> To maintain bisectability this response should be extended as each
> feature is added.
>
> > +}
> > +
> >  static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
> >      /* Order is important if has same prefix */
> >      {
> > @@ -2203,6 +2208,10 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
> >          .handler = handle_query_attached,
> >          .cmd = "Attached",
> >      },
> > +    {
> > +        .handler = handle_query_qemu_supported,
> > +        .cmd = "qemu.Supported",
> > +    },
> >  };
> >
> >  static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 24/27] gdbstub: Add another handler for setting qemu.sstep

[Jon Doron]

On Wed, May 15, 2019 at 8:44 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Follow GDB general query/set packet conventions, qemu.sstep can now
> > be set with the following command as well:
> > gdb> maint packet Qqemu.sstep:Value
>
> I;m not sure about exposing internal values to a protocol like this.
> Maybe text based flags would be better?
>

We kinda have to at this point as this was the original implementation
or we might end up breaking up the "API"
see commit: 60897d369f10b464720d8a6de4553c47943ea927

> >
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 88ff6224e6..34da10260d 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -2260,6 +2260,12 @@ static GdbCmdParseEntry gdb_gen_set_table[] = {
> >          .cmd_startswith = 1,
> >          .schema = "l0"
> >      },
> > +    {
> > +        .handler = handle_set_qemu_sstep,
> > +        .cmd = "qemu.sstep:",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
>
> Hmm the implementation seems to have gone in earlier. These should be
> together as a feature patch (along with changing the query/probe
> responses).
>
Done
> >  };
> >
> >  static void handle_gen_query(GdbCmdContext *gdb_ctx, void *user_ctx)
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 26/27] gdbstub: Add support to read a MSR for KVM target

[Jon Doron]

Ah cool did not know about that I will look into it and perhaps can do
a different patchset just for this no need to add it on top of this
patchset

On Wed, May 15, 2019 at 8:48 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > gdb> maint packet qqemu.kvm.Rdmsr:MsrIndex
>
> gdbserver already has a mechanism for exposing system registers see:
>
>   commit 200bf5b7ffea635079cc05fdfb363372b9544ce7
>   Author: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
>   Date:   Fri May 18 17:48:07 2018 +0100
>
> for an example. As MSR's are very specific to x86 all this should be
> handled via target/i386/gdbstub and kept out of the generic code.
>
> >
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 34da10260d..f48c3a2b5f 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -2141,7 +2141,14 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
> >
> >  static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
> >  {
> > -    put_packet(gdb_ctx->s, "sstepbits;sstep;PhyMemMode");
> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
> > +             "sstepbits;sstep;PhyMemMode");
> > +
> > +    if (kvm_enabled()) {
> > +        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";kvm.Rdmsr");
> > +    }
> > +
> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> >  }
> >
> >  static void handle_query_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx,
> > @@ -2166,6 +2173,29 @@ static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      put_packet(gdb_ctx->s, "OK");
> >  }
> >
> > +static void handle_query_kvm_read_msr(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    uint64_t msr_val;
> > +
> > +    if (!kvm_enabled()) {
> > +        return;
> > +    }
> > +
> > +    if (!gdb_ctx->num_params) {
> > +        put_packet(gdb_ctx->s, "E22");
> > +        return;
> > +    }
> > +
> > +    if (kvm_arch_read_msr(gdbserver_state->c_cpu, gdb_ctx->params[0].val_ul,
> > +                          &msr_val)) {
> > +        put_packet(gdb_ctx->s, "E00");
> > +        return;
> > +    }
> > +
> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "0x%" PRIx64, msr_val);
> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> > +}
> > +
> >  static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
> >      /* Order is important if has same prefix */
> >      {
> > @@ -2250,6 +2280,12 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
> >          .handler = handle_query_qemu_phy_mem_mode,
> >          .cmd = "qemu.PhyMemMode",
> >      },
> > +    {
> > +        .handler = handle_query_kvm_read_msr,
> > +        .cmd = "qemu.kvm.Rdmsr:",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
> >  };
> >
> >  static GdbCmdParseEntry gdb_gen_set_table[] = {
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 20/27] gdbstub: Implement target halted (? pkt) with new infra

[Jon Doron]

On Wed, May 15, 2019 at 8:20 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 36 ++++++++++++++++++++++++++----------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 2fd0d66f4d..d678191705 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -2239,13 +2239,30 @@ static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      put_packet(gdb_ctx->s, "");
> >  }
> >
> > +static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    char thread_id[16];
> > +
> > +    /* TODO: Make this return the correct value for user-mode.  */
>
> Can this be cleaned up as we convert?
>

To be honest i have no idea what the "correct value" is or how to get
it, can you tell me what it should be and ill add it to the patch?

> > +    gdb_fmt_thread_id(gdb_ctx->s, gdb_ctx->s->c_cpu, thread_id,
> > +                      sizeof(thread_id));
> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
> > +             GDB_SIGNAL_TRAP, thread_id);
> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> > +    /*
> > +     * Remove all the breakpoints when this query is issued,
> > +     * because gdb is doing and initial connect and the state
>
> s/and/an/
>
> > +     * should be cleaned up.
> > +     */
> > +    gdb_breakpoint_remove_all();
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      const char *p;
> >      int ch;
> >      uint8_t mem_buf[MAX_PACKET_LENGTH];
> >      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> > -    char thread_id[16];
> >      const GdbCmdParseEntry *cmd_parser = NULL;
> >
> >      trace_gdbstub_io_command(line_buf);
> > @@ -2257,15 +2274,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          put_packet(s, "OK");
> >          break;
> >      case '?':
> > -        /* TODO: Make this return the correct value for user-mode.  */
> > -        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> > -                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
> > -        put_packet(s, buf);
> > -        /* Remove all the breakpoints when this query is issued,
> > -         * because gdb is doing and initial connect and the state
> > -         * should be cleaned up.
> > -         */
> > -        gdb_breakpoint_remove_all();
> > +        {
> > +            static const GdbCmdParseEntry target_halted_cmd_desc = {
> > +                .handler = handle_target_halt,
> > +                .cmd = "?",
> > +                .cmd_startswith = 1
> > +            };
> > +            cmd_parser = &target_halted_cmd_desc;
> > +        }
> >          break;
> >      case 'c':
> >          {
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 24/27] gdbstub: Add another handler for setting qemu.sstep

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> On Wed, May 15, 2019 at 8:44 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Jon Doron <arilou@gmail.com> writes:
>>
>> > Follow GDB general query/set packet conventions, qemu.sstep can now
>> > be set with the following command as well:
>> > gdb> maint packet Qqemu.sstep:Value
>>
>> I;m not sure about exposing internal values to a protocol like this.
>> Maybe text based flags would be better?
>>
>
> We kinda have to at this point as this was the original implementation
> or we might end up breaking up the "API"
> see commit: 60897d369f10b464720d8a6de4553c47943ea927

Ahh yes I see. TIL I guess ;-)

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 26/27] gdbstub: Add support to read a MSR for KVM target

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Ah cool did not know about that I will look into it and perhaps can do
> a different patchset just for this no need to add it on top of this
> patchset

Yes just drop these arch specific patches for your next iteration while
you rework them for the target/ approach. Hopefully we'll have the
re-factor merged before you've finished.

>
> On Wed, May 15, 2019 at 8:48 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Jon Doron <arilou@gmail.com> writes:
>>
>> > gdb> maint packet qqemu.kvm.Rdmsr:MsrIndex
>>
>> gdbserver already has a mechanism for exposing system registers see:
>>
>>   commit 200bf5b7ffea635079cc05fdfb363372b9544ce7
>>   Author: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
>>   Date:   Fri May 18 17:48:07 2018 +0100
>>
>> for an example. As MSR's are very specific to x86 all this should be
>> handled via target/i386/gdbstub and kept out of the generic code.
>>
>> >
>> > Signed-off-by: Jon Doron <arilou@gmail.com>
>> > ---
>> >  gdbstub.c | 38 +++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 37 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index 34da10260d..f48c3a2b5f 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -2141,7 +2141,14 @@ static void handle_query_attached(GdbCmdContext *gdb_ctx, void *user_ctx)
>> >
>> >  static void handle_query_qemu_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
>> >  {
>> > -    put_packet(gdb_ctx->s, "sstepbits;sstep;PhyMemMode");
>> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf),
>> > +             "sstepbits;sstep;PhyMemMode");
>> > +
>> > +    if (kvm_enabled()) {
>> > +        pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";kvm.Rdmsr");
>> > +    }
>> > +
>> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>> >  }
>> >
>> >  static void handle_query_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx,
>> > @@ -2166,6 +2173,29 @@ static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
>> >      put_packet(gdb_ctx->s, "OK");
>> >  }
>> >
>> > +static void handle_query_kvm_read_msr(GdbCmdContext *gdb_ctx, void *user_ctx)
>> > +{
>> > +    uint64_t msr_val;
>> > +
>> > +    if (!kvm_enabled()) {
>> > +        return;
>> > +    }
>> > +
>> > +    if (!gdb_ctx->num_params) {
>> > +        put_packet(gdb_ctx->s, "E22");
>> > +        return;
>> > +    }
>> > +
>> > +    if (kvm_arch_read_msr(gdbserver_state->c_cpu, gdb_ctx->params[0].val_ul,
>> > +                          &msr_val)) {
>> > +        put_packet(gdb_ctx->s, "E00");
>> > +        return;
>> > +    }
>> > +
>> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "0x%" PRIx64, msr_val);
>> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>> > +}
>> > +
>> >  static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
>> >      /* Order is important if has same prefix */
>> >      {
>> > @@ -2250,6 +2280,12 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
>> >          .handler = handle_query_qemu_phy_mem_mode,
>> >          .cmd = "qemu.PhyMemMode",
>> >      },
>> > +    {
>> > +        .handler = handle_query_kvm_read_msr,
>> > +        .cmd = "qemu.kvm.Rdmsr:",
>> > +        .cmd_startswith = 1,
>> > +        .schema = "l0"
>> > +    },
>> >  };
>> >
>> >  static GdbCmdParseEntry gdb_gen_set_table[] = {
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 20/27] gdbstub: Implement target halted (? pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> On Wed, May 15, 2019 at 8:20 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Jon Doron <arilou@gmail.com> writes:
>>
>> > Signed-off-by: Jon Doron <arilou@gmail.com>
>> > ---
>> >  gdbstub.c | 36 ++++++++++++++++++++++++++----------
>> >  1 file changed, 26 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index 2fd0d66f4d..d678191705 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -2239,13 +2239,30 @@ static void handle_gen_set(GdbCmdContext *gdb_ctx, void *user_ctx)
>> >      put_packet(gdb_ctx->s, "");
>> >  }
>> >
>> > +static void handle_target_halt(GdbCmdContext *gdb_ctx, void *user_ctx)
>> > +{
>> > +    char thread_id[16];
>> > +
>> > +    /* TODO: Make this return the correct value for user-mode.  */
>>
>> Can this be cleaned up as we convert?
>>
>
> To be honest i have no idea what the "correct value" is or how to get
> it, can you tell me what it should be and ill add it to the patch?

Actually I think you can delete the comment and mention the thread-id
has been correctly reported in usermode since bd88c780e6

>
>> > +    gdb_fmt_thread_id(gdb_ctx->s, gdb_ctx->s->c_cpu, thread_id,
>> > +                      sizeof(thread_id));
>> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
>> > +             GDB_SIGNAL_TRAP, thread_id);
>> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>> > +    /*
>> > +     * Remove all the breakpoints when this query is issued,
>> > +     * because gdb is doing and initial connect and the state
>>
>> s/and/an/
>>
>> > +     * should be cleaned up.
>> > +     */
>> > +    gdb_breakpoint_remove_all();
>> > +}
>> > +
>> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >  {
>> >      const char *p;
>> >      int ch;
>> >      uint8_t mem_buf[MAX_PACKET_LENGTH];
>> >      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>> > -    char thread_id[16];
>> >      const GdbCmdParseEntry *cmd_parser = NULL;
>> >
>> >      trace_gdbstub_io_command(line_buf);
>> > @@ -2257,15 +2274,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >          put_packet(s, "OK");
>> >          break;
>> >      case '?':
>> > -        /* TODO: Make this return the correct value for user-mode.  */
>> > -        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
>> > -                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
>> > -        put_packet(s, buf);
>> > -        /* Remove all the breakpoints when this query is issued,
>> > -         * because gdb is doing and initial connect and the state
>> > -         * should be cleaned up.
>> > -         */
>> > -        gdb_breakpoint_remove_all();
>> > +        {
>> > +            static const GdbCmdParseEntry target_halted_cmd_desc = {
>> > +                .handler = handle_target_halt,
>> > +                .cmd = "?",
>> > +                .cmd_startswith = 1
>> > +            };
>> > +            cmd_parser = &target_halted_cmd_desc;
>> > +        }
>> >          break;
>> >      case 'c':
>> >          {
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 02/27] gdbstub: Implement deatch (D pkt) with new infra

[Jon Doron]

Hi Alex, I implemented this change but i'm having second guesses on
this, basically a NULL packet means the command is not supported (as
far as i understand from the protocol documentation and implementation
of GDB)
That being said I think it's correct to send back a NULL packet if
process_string_cmd fails for any reason, or you would prefer ill just
omit it?

Snippet of the change I did according to your review:
-    if (cmd_parser &&
-        process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) {
-        put_packet(s, "");
+    if (!cmd_parser) {
+        return RS_IDLE;
     }

+    process_string_cmd(s, NULL, line_buf, cmd_parser, 1);

-- Jon.

On Tue, May 14, 2019 at 9:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 90 ++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 50 insertions(+), 40 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index d5e0f3878a..621d689868 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern)
> >    return !strncmp(string, pattern, strlen(pattern));
> >  }
> >
> > -static int process_string_cmd(
> > -        GDBState *s, void *user_ctx, const char *data,
> > -        const GdbCmdParseEntry *cmds, int num_cmds)
> > -        __attribute__((unused));
> > -
> >  static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
> >                                const GdbCmdParseEntry *cmds, int num_cmds)
> >  {
> > @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
> >      return -1;
> >  }
> >
> > +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    GDBProcess *process;
> > +    GDBState *s = gdb_ctx->s;
> > +    uint32_t pid = 1;
> > +
> > +    if (s->multiprocess) {
> > +        if (!gdb_ctx->num_params) {
> > +            put_packet(s, "E22");
> > +            return;
> > +        }
> > +
> > +        pid = gdb_ctx->params[0].val_ul;
> > +    }
> > +
> > +    process = gdb_get_process(s, pid);
> > +    gdb_process_breakpoint_remove_all(s, process);
> > +    process->attached = false;
> > +
> > +    if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
> > +        s->c_cpu = gdb_first_attached_cpu(s);
> > +    }
> > +
> > +    if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
> > +        s->g_cpu = gdb_first_attached_cpu(s);
> > +    }
> > +
> > +    if (!s->c_cpu) {
> > +        /* No more process attached */
> > +        gdb_syscall_mode = GDB_SYS_DISABLED;
> > +        gdb_continue(s);
> > +    }
> > +    put_packet(s, "OK");
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
> > @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >      uint8_t *registers;
> >      target_ulong addr, len;
> >      GDBThreadIdKind thread_kind;
> > +    const GdbCmdParseEntry *cmd_parser = NULL;
> >
> >      trace_gdbstub_io_command(line_buf);
> >
> > @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          error_report("QEMU: Terminated via GDBstub");
> >          exit(0);
> >      case 'D':
> > -        /* Detach packet */
> > -        pid = 1;
> > -
> > -        if (s->multiprocess) {
> > -            unsigned long lpid;
> > -            if (*p != ';') {
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            if (qemu_strtoul(p + 1, &p, 16, &lpid)) {
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            pid = lpid;
> > -        }
> > -
> > -        process = gdb_get_process(s, pid);
> > -        gdb_process_breakpoint_remove_all(s, process);
> > -        process->attached = false;
> > -
> > -        if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
> > -            s->c_cpu = gdb_first_attached_cpu(s);
> > -        }
> > -
> > -        if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
> > -            s->g_cpu = gdb_first_attached_cpu(s);
> > -        }
> > -
> > -        if (s->c_cpu == NULL) {
> > -            /* No more process attached */
> > -            gdb_syscall_mode = GDB_SYS_DISABLED;
> > -            gdb_continue(s);
> > +        {
> > +            static const GdbCmdParseEntry detach_cmd_desc = {
> > +                .handler = handle_detach,
> > +                .cmd = "D",
> > +                .cmd_startswith = 1,
> > +                .schema = "?.l0"
> > +            };
> > +            cmd_parser = &detach_cmd_desc;
> >          }
> > -        put_packet(s, "OK");
> >          break;
> >      case 's':
> >          if (*p != '\0') {
> > @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          put_packet(s, buf);
> >          break;
> >      }
> > +
> > +    if (cmd_parser &&
> > +        process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) {
> > +        put_packet(s, "");
>
> Why this null put_packet at the end? You've passed the handling of the
> OK reply back to your handler so this seems superfluous.
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH v9 02/27] gdbstub: Implement deatch (D pkt) with new infra

[Alex Bennée]

Jon Doron <arilou@gmail.com> writes:

> Hi Alex, I implemented this change but i'm having second guesses on
> this, basically a NULL packet means the command is not supported (as
> far as i understand from the protocol documentation and implementation
> of GDB)
> That being said I think it's correct to send back a NULL packet if
> process_string_cmd fails for any reason, or you would prefer ill just
> omit it?
>
> Snippet of the change I did according to your review:
> -    if (cmd_parser &&
> -        process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) {
> -        put_packet(s, "");
> +    if (!cmd_parser) {
> +        return RS_IDLE;
>      }
>
> +    process_string_cmd(s, NULL, line_buf, cmd_parser, 1);

OK I see your reasoning. So perhaps:

   if (cmd_parser) {
      /* helper will respond */
      process_string_cmd(s, NULL, line_buf, cmd_parser, 1)
   } else {
      /* unknown command, empty response */
      put_packet(s, "");
   }

   return RS_IDLE;
>
> -- Jon.
>
> On Tue, May 14, 2019 at 9:54 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Jon Doron <arilou@gmail.com> writes:
>>
>> > Signed-off-by: Jon Doron <arilou@gmail.com>
>> > ---
>> >  gdbstub.c | 90 ++++++++++++++++++++++++++++++-------------------------
>> >  1 file changed, 50 insertions(+), 40 deletions(-)
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index d5e0f3878a..621d689868 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern)
>> >    return !strncmp(string, pattern, strlen(pattern));
>> >  }
>> >
>> > -static int process_string_cmd(
>> > -        GDBState *s, void *user_ctx, const char *data,
>> > -        const GdbCmdParseEntry *cmds, int num_cmds)
>> > -        __attribute__((unused));
>> > -
>> >  static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
>> >                                const GdbCmdParseEntry *cmds, int num_cmds)
>> >  {
>> > @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data,
>> >      return -1;
>> >  }
>> >
>> > +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
>> > +{
>> > +    GDBProcess *process;
>> > +    GDBState *s = gdb_ctx->s;
>> > +    uint32_t pid = 1;
>> > +
>> > +    if (s->multiprocess) {
>> > +        if (!gdb_ctx->num_params) {
>> > +            put_packet(s, "E22");
>> > +            return;
>> > +        }
>> > +
>> > +        pid = gdb_ctx->params[0].val_ul;
>> > +    }
>> > +
>> > +    process = gdb_get_process(s, pid);
>> > +    gdb_process_breakpoint_remove_all(s, process);
>> > +    process->attached = false;
>> > +
>> > +    if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
>> > +        s->c_cpu = gdb_first_attached_cpu(s);
>> > +    }
>> > +
>> > +    if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
>> > +        s->g_cpu = gdb_first_attached_cpu(s);
>> > +    }
>> > +
>> > +    if (!s->c_cpu) {
>> > +        /* No more process attached */
>> > +        gdb_syscall_mode = GDB_SYS_DISABLED;
>> > +        gdb_continue(s);
>> > +    }
>> > +    put_packet(s, "OK");
>> > +}
>> > +
>> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >  {
>> >      CPUState *cpu;
>> > @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >      uint8_t *registers;
>> >      target_ulong addr, len;
>> >      GDBThreadIdKind thread_kind;
>> > +    const GdbCmdParseEntry *cmd_parser = NULL;
>> >
>> >      trace_gdbstub_io_command(line_buf);
>> >
>> > @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >          error_report("QEMU: Terminated via GDBstub");
>> >          exit(0);
>> >      case 'D':
>> > -        /* Detach packet */
>> > -        pid = 1;
>> > -
>> > -        if (s->multiprocess) {
>> > -            unsigned long lpid;
>> > -            if (*p != ';') {
>> > -                put_packet(s, "E22");
>> > -                break;
>> > -            }
>> > -
>> > -            if (qemu_strtoul(p + 1, &p, 16, &lpid)) {
>> > -                put_packet(s, "E22");
>> > -                break;
>> > -            }
>> > -
>> > -            pid = lpid;
>> > -        }
>> > -
>> > -        process = gdb_get_process(s, pid);
>> > -        gdb_process_breakpoint_remove_all(s, process);
>> > -        process->attached = false;
>> > -
>> > -        if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
>> > -            s->c_cpu = gdb_first_attached_cpu(s);
>> > -        }
>> > -
>> > -        if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
>> > -            s->g_cpu = gdb_first_attached_cpu(s);
>> > -        }
>> > -
>> > -        if (s->c_cpu == NULL) {
>> > -            /* No more process attached */
>> > -            gdb_syscall_mode = GDB_SYS_DISABLED;
>> > -            gdb_continue(s);
>> > +        {
>> > +            static const GdbCmdParseEntry detach_cmd_desc = {
>> > +                .handler = handle_detach,
>> > +                .cmd = "D",
>> > +                .cmd_startswith = 1,
>> > +                .schema = "?.l0"
>> > +            };
>> > +            cmd_parser = &detach_cmd_desc;
>> >          }
>> > -        put_packet(s, "OK");
>> >          break;
>> >      case 's':
>> >          if (*p != '\0') {
>> > @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >          put_packet(s, buf);
>> >          break;
>> >      }
>> > +
>> > +    if (cmd_parser &&
>> > +        process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) {
>> > +        put_packet(s, "");
>>
>> Why this null put_packet at the end? You've passed the handling of the
>> OK reply back to your handler so this seems superfluous.
>>
>> --
>> Alex Bennée


--
Alex Bennée