[PATCH 0/4] Add MTE stubs for aarch64 user mode

[PATCH 0/4] Add MTE stubs for aarch64 user mode

[Gustavo Romero]

This patchset adds the stubs necessary to support GDB memory tagging
commands on QEMU aarch64 user mode.

These new stubs handle the qIsAddressTagged, qMemTag, and QMemTag
packets, which allow GDB memory tagging subcommands 'check',
'print-allocation-tag', and 'set-allocation-tag' to work. The remaining
memory tagging commands ('print-logical-tag' and 'with-logical-tag')
will also work, but they don't rely on any stub because they perform
local operations.

Since the memory tagging stubs are not common to all architectures, this
patchset also introduces three functions: set_query_supported_arch,
set_gdb_gen_query_table_arch, and set_gdb_gen_set_table_arch. These
functions can be used to extend the target-specific 'qSupported' feature
string and the handlers for the 'q' (query) and 'Q' (set) packets. These
new functions are used to add the MTE stubs for the aarch64 gdbstub.
 
Note that this patchset requires a GDB that supports the
qIsAddressTagged packet (recently added to GDB), so the gdbstub MTE
tests introduced by it must be run using GDB's master branch, since the
GDB in the distros hasn't picked up the change yet.

Once GDB is built and installed locally, the tests can be exercised, for
example, this way:

make GDB=~/.local/bin/gdb run-tcg-tests-aarch64-linux-user -j 32


Cheers,
Gustavo

Gustavo Romero (4):
  gdbstub: Add support for target-specific stubs
  gdbstub: Add support for MTE in user mode
  tests: Gently exit from GDB when tests complete
  tests/tcg/aarch64: Add MTE gdbstub tests

 configs/targets/aarch64-linux-user.mak |   2 +-
 gdbstub/gdbstub.c                      | 108 +++++----
 gdbstub/internals.h                    |  22 --
 gdbstub/syscalls.c                     |   1 +
 include/exec/gdbstub.h                 |  86 ++++++-
 target/arm/cpu.c                       |   1 +
 target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
 target/arm/internals.h                 |   2 +
 tests/guest-debug/test_gdbstub.py      |   2 +-
 tests/tcg/aarch64/Makefile.target      |  11 +-
 tests/tcg/aarch64/gdbstub/test-mte.py  |  86 +++++++
 tests/tcg/aarch64/mte-8.c              | 102 ++++++++
 12 files changed, 670 insertions(+), 74 deletions(-)
 create mode 100644 tests/tcg/aarch64/gdbstub/test-mte.py
 create mode 100644 tests/tcg/aarch64/mte-8.c

-- 
2.34.1

[PATCH 1/4] gdbstub: Add support for target-specific stubs

[Gustavo Romero]

Currently, it's not possible to have stubs specific to a given target,
even though there are GDB features which are target-specific, like, for
instance, memory tagging.

This commit introduces set_query_supported_arch,
set_gdb_gen_query_table_arch, and set_gdb_gen_set_table_arch functions
as interfaces to extend the qSupported string, the query handler table,
and set handler table per target, so allowing target-specific stub
implementation.

Besides that, it moves GdbCmdParseEntry struct, its related types, and
gdb_put_packet to include/exec/gdbstub.h so they are also available in
the target-specific stubs.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdbstub/gdbstub.c      | 108 +++++++++++++++++++++++------------------
 gdbstub/internals.h    |  22 ---------
 gdbstub/syscalls.c     |   1 +
 include/exec/gdbstub.h |  86 +++++++++++++++++++++++++++++++-
 4 files changed, 147 insertions(+), 70 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b3574997ea..4996530fde 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -920,43 +920,6 @@ static int cmd_parse_params(const char *data, const char *schema,
     return 0;
 }
 
-typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
-
-/*
- * cmd_startswith -> cmd is compared using startswith
- *
- * allow_stop_reply -> true iff the gdbstub can respond to this command with a
- *   "stop reply" packet. The list of commands that accept such response is
- *   defined at the GDB Remote Serial Protocol documentation. see:
- *   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
- *
- * schema definitions:
- * Each schema parameter entry consists of 2 chars,
- * 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;
-    bool cmd_startswith;
-    const char *schema;
-    bool allow_stop_reply;
-} GdbCmdParseEntry;
-
 static inline int startswith(const char *string, const char *pattern)
 {
   return !strncmp(string, pattern, strlen(pattern));
@@ -1645,6 +1608,13 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
     gdb_put_strbuf();
 }
 
+/* Arch-specific qSupported */
+char *query_supported_arch = NULL;
+void set_query_supported_arch(char *query_supported)
+{
+    query_supported_arch = query_supported;
+}
+
 static void handle_query_supported(GArray *params, void *user_ctx)
 {
     CPUClass *cc;
@@ -1684,6 +1654,11 @@ static void handle_query_supported(GArray *params, void *user_ctx)
     }
 
     g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
+
+    if (query_supported_arch) {
+        g_string_append(gdbserver_state.str_buf, query_supported_arch);
+    }
+
     gdb_put_strbuf();
 }
 
@@ -1765,6 +1740,16 @@ static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
     },
 };
 
+
+/* Arch-specific query table */
+static GdbCmdParseEntry *gdb_gen_query_table_arch = NULL ;
+static int gdb_gen_query_table_arch_size = 0;
+void set_gdb_gen_query_table_arch(GdbCmdParseEntry  *table, int size)
+{
+    gdb_gen_query_table_arch = table;
+    gdb_gen_query_table_arch_size = size;
+}
+
 static const GdbCmdParseEntry gdb_gen_query_table[] = {
     {
         .handler = handle_query_curr_tid,
@@ -1857,6 +1842,15 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
 #endif
 };
 
+/* Arch-specific set table */
+static GdbCmdParseEntry *gdb_gen_set_table_arch = NULL;
+static int gdb_gen_set_table_arch_size = 0;
+void set_gdb_gen_set_table_arch(GdbCmdParseEntry *table, int size)
+{
+    gdb_gen_set_table_arch = table;
+    gdb_gen_set_table_arch_size = size;
+}
+
 static const GdbCmdParseEntry gdb_gen_set_table[] = {
     /* Order is important if has same prefix */
     {
@@ -1889,17 +1883,27 @@ static void handle_gen_query(GArray *params, void *user_ctx)
         return;
     }
 
-    if (!process_string_cmd(get_param(params, 0)->data,
-                            gdb_gen_query_set_common_table,
-                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+    if (process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_query_set_common_table,
+                           ARRAY_SIZE(gdb_gen_query_set_common_table)) == 0) {
         return;
     }
 
     if (process_string_cmd(get_param(params, 0)->data,
                            gdb_gen_query_table,
-                           ARRAY_SIZE(gdb_gen_query_table))) {
-        gdb_put_packet("");
+                           ARRAY_SIZE(gdb_gen_query_table)) == 0) {
+        return;
     }
+
+    if (gdb_gen_query_table_arch &&
+        process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_query_table_arch,
+                           gdb_gen_query_table_arch_size) == 0) {
+        return;
+    }
+
+    /* Can't handle query, return Empty response. */
+    gdb_put_packet("");
 }
 
 static void handle_gen_set(GArray *params, void *user_ctx)
@@ -1908,17 +1912,27 @@ static void handle_gen_set(GArray *params, void *user_ctx)
         return;
     }
 
-    if (!process_string_cmd(get_param(params, 0)->data,
-                            gdb_gen_query_set_common_table,
-                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+    if (process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_query_set_common_table,
+                           ARRAY_SIZE(gdb_gen_query_set_common_table)) == 0) {
         return;
     }
 
     if (process_string_cmd(get_param(params, 0)->data,
                            gdb_gen_set_table,
-                           ARRAY_SIZE(gdb_gen_set_table))) {
-        gdb_put_packet("");
+                           ARRAY_SIZE(gdb_gen_set_table)) == 0) {
+        return;
     }
+
+    if (gdb_gen_set_table_arch &&
+        process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_set_table_arch,
+                           gdb_gen_set_table_arch_size) == 0) {
+        return;
+    }
+
+    /* Can't handle set, return Empty response. */
+    gdb_put_packet("");
 }
 
 static void handle_target_halt(GArray *params, void *user_ctx)
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 32f9f63297..34121dc61a 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -106,7 +106,6 @@ static inline int tohex(int v)
  */
 
 void gdb_put_strbuf(void);
-int gdb_put_packet(const char *buf);
 int gdb_put_packet_binary(const char *buf, int len, bool dump);
 void gdb_hextomem(GByteArray *mem, const char *buf, int len);
 void gdb_memtohex(GString *buf, const uint8_t *mem, int len);
@@ -166,27 +165,6 @@ void gdb_put_buffer(const uint8_t *buf, int len);
  */
 void gdb_init_gdbserver_state(void);
 
-typedef enum GDBThreadIdKind {
-    GDB_ONE_THREAD = 0,
-    GDB_ALL_THREADS,     /* One process, all threads */
-    GDB_ALL_PROCESSES,
-    GDB_READ_THREAD_ERR
-} GDBThreadIdKind;
-
-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;
-
-#define get_param(p, i)    (&g_array_index(p, GdbCmdVariant, i))
-
 void gdb_handle_query_rcmd(GArray *params, void *ctx); /* system */
 void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
 void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c
index 02e3a8f74c..ee9a16495e 100644
--- a/gdbstub/syscalls.c
+++ b/gdbstub/syscalls.c
@@ -18,6 +18,7 @@
 #include "gdbstub/syscalls.h"
 #include "trace.h"
 #include "internals.h"
+#include "exec/gdbstub.h"
 
 /* Syscall specific state */
 typedef struct {
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index eb14b91139..7bf189d7f3 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -144,4 +144,88 @@ void gdb_set_stop_cpu(CPUState *cpu);
 /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
 extern const GDBFeature gdb_static_features[];
 
-#endif
+typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
+
+typedef enum GDBThreadIdKind {
+    GDB_ONE_THREAD = 0,
+    GDB_ALL_THREADS,     /* One process, all threads */
+    GDB_ALL_PROCESSES,
+    GDB_READ_THREAD_ERR
+} GDBThreadIdKind;
+
+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;
+
+#define get_param(p, i)    (&g_array_index(p, GdbCmdVariant, i))
+
+/*
+ * cmd_startswith -> cmd is compared using startswith
+ *
+ * allow_stop_reply -> true iff the gdbstub can respond to this command with a
+ *   "stop reply" packet. The list of commands that accept such response is
+ *   defined at the GDB Remote Serial Protocol documentation. see:
+ *   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
+ *
+ * schema definitions:
+ * Each schema parameter entry consists of 2 chars,
+ * 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;
+    bool cmd_startswith;
+    const char *schema;
+    bool allow_stop_reply;
+} GdbCmdParseEntry;
+
+#define get_cmd_parsers(p) (&g_array_index(p, GdbCmdParseEntry, 0))
+
+/**
+ * set_gdb_gen_query_table_arch() - set a table to handle arch-specific query
+ * packets
+ */
+void set_gdb_gen_query_table_arch(GdbCmdParseEntry *table, int size);
+
+/**
+ * set_gdb_gen_set_table_arch() - set a table to handle arch-specific set
+ * packets
+ */
+void set_gdb_gen_set_table_arch(GdbCmdParseEntry *, int size);
+
+/**
+ * set_query_supported_arch() - set arch-specific features in qSupported
+ * features
+ */
+void set_query_supported_arch(char *);
+
+/**
+ * gdb_put_packet() - put string into gdb server's buffer so it is sent
+ * to the client
+ */
+int gdb_put_packet(const char *buf);
+
+#endif /* GDBSTUB_H */
-- 
2.34.1

[PATCH 2/4] gdbstub: Add support for MTE in user mode

[Gustavo Romero]

This commit implements the stubs to handle the qIsAddressTagged,
qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
subcommands to work with QEMU gdbstub on aarch64 user mode. It also
implements the get/set function for the special GDB MTE register
'tag_ctl', used to control the MTE fault type at runtime.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 configs/targets/aarch64-linux-user.mak |   2 +-
 target/arm/cpu.c                       |   1 +
 target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
 target/arm/internals.h                 |   2 +
 4 files changed, 325 insertions(+), 1 deletion(-)

diff --git a/configs/targets/aarch64-linux-user.mak b/configs/targets/aarch64-linux-user.mak
index ba8bc5fe3f..8f0ed21d76 100644
--- a/configs/targets/aarch64-linux-user.mak
+++ b/configs/targets/aarch64-linux-user.mak
@@ -1,6 +1,6 @@
 TARGET_ARCH=aarch64
 TARGET_BASE_ARCH=arm
-TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml
+TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml gdb-xml/aarch64-mte.xml
 TARGET_HAS_BFLT=y
 CONFIG_SEMIHOSTING=y
 CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 77f8c9c748..29f7b99a88 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2479,6 +2479,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
     register_cp_regs_for_features(cpu);
     arm_cpu_register_gdb_regs_for_features(cpu);
+    arm_cpu_register_gdb_commands(cpu);
 
     init_cpreg_list(cpu);
 
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index a3bb73cfa7..f3897f75b3 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -474,6 +474,317 @@ static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
 #endif
 #endif /* CONFIG_TCG */
 
+#ifdef TARGET_AARCH64
+#ifdef CONFIG_USER_ONLY
+static int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, struct _GByteArray *buf, int reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    uint64_t tcf0;
+
+    assert(reg == 0);
+
+    /* TCF0, bits [39:38]. */
+    tcf0 = extract64(env->cp15.sctlr_el[1], 38, 2);
+
+    return gdb_get_reg64(buf, tcf0);
+}
+
+static int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    assert(reg == 0);
+
+    /* Sanitize TCF0 bits. */
+    *buf &= 0x03;
+
+    if (!isar_feature_aa64_mte3(&cpu->isar) && *buf == 3) {
+        /*
+         * If FEAT_MTE3 is not implemented, the value 0b11 is reserved, hence
+         * ignore setting it.
+         */
+        return 0;
+    }
+
+    /*
+     * 'tag_ctl' register is actually a "pseudo-register" provided by GDB to
+     * expose options that can be controlled at runtime and has the same effect
+     * of prctl() with option PR_SET_TAGGED_ADDR_CTRL,
+     * i.e. prctl(PR_SET_TAGGED_ADDR_CTRL, tcf, 0, 0, 0), hence it controls
+     * the effect of Tag Check Faults (TCF) due to Loads and Stores in EL0.
+     */
+    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, *buf);
+
+    return 1;
+}
+
+static void handle_q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
+{
+    uint64_t addr = get_param(params, 0)->val_ull;
+    uint64_t len = get_param(params, 1)->val_ul;
+    int type = get_param(params, 2)->val_ul;
+
+    uint64_t clean_addr;
+    uint8_t *tags;
+    int granules_index;
+    int granule_index;
+    uint8_t addr_tag;
+
+    g_autoptr(GString) str_buf = g_string_new(NULL);
+
+    /*
+     * GDB does not query tags for a memory range on remote targets, so that's
+     * not supported either by gdbstub.
+     */
+    if (len != 1) {
+        gdb_put_packet("E02");
+    }
+
+    /* GDB never queries a tag different from an allocation tag (type 1). */
+    if (type != 1) {
+        gdb_put_packet("E02");
+    }
+
+    /* Remove any non-addressing bits. */
+    clean_addr = useronly_clean_ptr(addr);
+
+    /*
+     * Get pointer to all tags in the page where the address is. Note that tags
+     * are packed, so there are 2 tags packed in one byte.
+     */
+    tags = page_get_target_data(clean_addr);
+
+    /*
+     * Tags are per granule (16 bytes). 2 tags (4 bits each) are kept in a
+     * single byte for compactness, so first a page tag index for 2 packed
+     * granule tags (1 byte) is found, and then an index for a single granule
+     * tag (nibble) is found, and finally the address tag is obtained.
+     */
+    granules_index = extract32(clean_addr, LOG2_TAG_GRANULE + 1,
+                               TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
+    granule_index = extract32(clean_addr, LOG2_TAG_GRANULE, 1);
+
+    addr_tag = *(tags + granules_index);
+    /* Extract tag from the right nibble. */
+    if (granule_index == 0) {
+        addr_tag &= 0xF;
+    } else {
+        addr_tag >>= 4;
+    }
+
+    g_string_printf(str_buf, "m%.2x", addr_tag);
+
+    gdb_put_packet(str_buf->str);
+}
+
+static void handle_q_isaddresstagged(GArray *params, G_GNUC_UNUSED void *user_ctx)
+{
+    uint64_t addr = get_param(params, 0)->val_ull;
+
+    uint64_t clean_addr;
+    int mflags;
+
+    g_autoptr(GString) str_buf = g_string_new(NULL);
+
+    /* Remove any non-addressing bits. */
+    clean_addr = useronly_clean_ptr(addr);
+
+    mflags = page_get_flags(clean_addr);
+    if (mflags & PAGE_ANON && mflags & PAGE_MTE) {
+        /* Address is tagged. */
+        g_string_printf(str_buf, "%.2x", 1 /* true */);
+    } else {
+        /* Address is not tagged. */
+        g_string_printf(str_buf, "%.2x", 0 /* false */);
+    }
+
+    gdb_put_packet(str_buf->str);
+}
+
+static void handle_Q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
+{
+    uint64_t addr = get_param(params, 0)->val_ull;
+    uint64_t len = get_param(params, 1)->val_ul;
+    int type = get_param(params, 2)->val_ul;
+    char const *new_tags = get_param(params, 3)->data;
+
+    uint64_t clean_addr;
+    int last_addr_index;
+
+    uint64_t start_addr_page;
+    uint64_t end_addr_page;
+
+    uint32_t first_tag_index;
+    uint32_t last_tag_index;
+
+    uint8_t *tags; /* Pointer to the current tags in a page. */
+    int num_new_tags;
+
+    g_autoptr(GString) str_buf = g_string_new(NULL);
+
+    /*
+     * Only the allocation tag (type 1) can be set at the stub side.
+     */
+    if (type != 1) {
+        gdb_put_packet("E02");
+        return;
+    }
+
+    /*
+     * 'len' is always >= 1 and refers to the size of the memory range about to
+     * have its tags updated. However, it's convenient to obtain the index for
+     * the last byte of the memory range for page boundary checks and for
+     * obtaining the indexes for the tags in the page.
+     */
+    last_addr_index = len - 1;
+
+    /* Remove any non-addressing bits. */
+    clean_addr = useronly_clean_ptr(addr);
+
+    start_addr_page = extract64(clean_addr, TARGET_PAGE_BITS,
+                                64 - TARGET_PAGE_BITS);
+    end_addr_page = extract64(clean_addr + last_addr_index, TARGET_PAGE_BITS,
+                              64 - TARGET_PAGE_BITS);
+
+    /*
+     * Check if memory range is within page boundaries.
+     */
+    if (start_addr_page != end_addr_page) {
+        gdb_put_packet("E03");
+        return;
+    }
+
+    /*
+     * Get pointer to all tags in the page where the address is. Note that here
+     * tags are packed, so there are 2 tags packed in one byte.
+     */
+    tags = page_get_target_data(clean_addr);
+
+    /* Tag indices below refer to unpacked tags. */
+    first_tag_index = extract32(clean_addr, LOG2_TAG_GRANULE,
+                                TARGET_PAGE_BITS - LOG2_TAG_GRANULE);
+    last_tag_index = extract32(clean_addr + last_addr_index, LOG2_TAG_GRANULE,
+                               TARGET_PAGE_BITS - LOG2_TAG_GRANULE);
+
+    /*
+     * GDB sends 2 hex digits per tag number, i.e. tags are not represented in
+     * a packed way.
+     */
+    num_new_tags = strlen(new_tags) / 2;
+
+    /*
+     * If the number of tags provided is greater than the number of tags
+     * in the provided memory range, the exceeding tags are ignored. If the
+     * number of tags is less than the number of tags in the provided memory
+     * range, then the provided tags are used as a repeating pattern to fill
+     * the tags in the provided memory range.
+     */
+    for (int i = first_tag_index, j = 0; i <= last_tag_index; i++, j++) {
+        int new_tag_value;
+        int packed_granules_index;
+        int nibble_index;
+
+        sscanf(new_tags + 2 * (j % num_new_tags), "%2x", &new_tag_value);
+        /*
+         * Find packed tag index from unpacked tag index. There are two tags
+         * packed in one packed index. One tag per nibble.
+         */
+        packed_granules_index = i / 2;
+        /* Find nibble index in the packed tag from unpacked tag index. */
+        nibble_index = i % 2;
+
+        if (nibble_index == 0) { /* Update low nibble */
+            *(tags + packed_granules_index) &= 0xF0;
+            *(tags + packed_granules_index) |= (new_tag_value & 0x0F);
+        } else { /* Update high nibble */
+            *(tags + packed_granules_index) &= 0x0F;
+            *(tags + packed_granules_index) |= ((new_tag_value & 0x0F) << 4);
+        }
+    }
+
+    g_string_printf(str_buf, "OK");
+
+    gdb_put_packet(str_buf->str);
+}
+
+enum Packet {
+    qMemTags,
+    qIsAddressTagged,
+    QMemTags,
+    NUM_PACKETS
+};
+
+static GdbCmdParseEntry packet_handler_table[NUM_PACKETS] = {
+    [qMemTags] = {
+        .handler = handle_q_memtag,
+        .cmd_startswith = 1,
+        .cmd = "MemTags:",
+        .schema = "L,l:l0"
+    },
+    [qIsAddressTagged] = {
+        .handler = handle_q_isaddresstagged,
+        .cmd_startswith = 1,
+        .cmd = "IsAddressTagged:",
+        .schema = "L0"
+    },
+    [QMemTags] = {
+        .handler = handle_Q_memtag,
+        .cmd_startswith = 1,
+        .cmd = "MemTags:",
+        .schema = "L,l:l:s0"
+    },
+};
+
+static void add_packet_handler(GArray *handlers, enum Packet packet) {
+    g_array_append_val(handlers, packet_handler_table[packet]);
+}
+#endif /* CONFIG_USER_ONLY */
+#endif /* TARGET_AARCH64 */
+
+void arm_cpu_register_gdb_commands(ARMCPU *cpu)
+{
+    GArray *gdb_gen_query_table_arm =
+        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
+    GArray *gdb_gen_set_table_arm =
+        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
+    GString *supported_features = g_string_new(NULL);
+
+#ifdef TARGET_AARCH64
+#ifdef CONFIG_USER_ONLY
+    /* MTE */
+    if (isar_feature_aa64_mte(&cpu->isar)) {
+        g_string_append(supported_features, ";memory-tagging+");
+
+        add_packet_handler(gdb_gen_query_table_arm, qMemTags);
+        add_packet_handler(gdb_gen_query_table_arm, qIsAddressTagged);
+
+        add_packet_handler(gdb_gen_set_table_arm, QMemTags);
+    }
+#endif
+#endif
+
+    /* Set arch-specific handlers for 'q' commands. */
+    if (gdb_gen_query_table_arm->len) {
+        set_gdb_gen_query_table_arch(&g_array_index(gdb_gen_query_table_arm,
+                                                    GdbCmdParseEntry, 0),
+                                                    gdb_gen_query_table_arm->len);
+    }
+
+    /* Set arch-specific handlers for 'Q' commands. */
+    if (gdb_gen_set_table_arm->len) {
+        set_gdb_gen_set_table_arch(&g_array_index(gdb_gen_set_table_arm,
+                                   GdbCmdParseEntry, 0),
+                                   gdb_gen_set_table_arm->len);
+    }
+
+    /* Set arch-specific qSupported feature. */
+    if (supported_features->len) {
+        set_query_supported_arch(supported_features->str);
+    }
+}
+
 void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
@@ -507,6 +818,16 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
                                      gdb_find_static_feature("aarch64-pauth.xml"),
                                      0);
         }
+
+#ifdef CONFIG_USER_ONLY
+        /* Memory Tagging Extension (MTE) 'tag_ctl' register. */
+        if (isar_feature_aa64_mte(&cpu->isar)) {
+            gdb_register_coprocessor(cs, aarch64_gdb_get_tag_ctl_reg,
+                                     aarch64_gdb_set_tag_ctl_reg,
+                                     gdb_find_static_feature("aarch64-mte.xml"),
+                                     0);
+        }
+#endif
 #endif
     } else {
         if (arm_feature(env, ARM_FEATURE_NEON)) {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index ee3ebd383e..3750591b44 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -358,6 +358,8 @@ void init_cpreg_list(ARMCPU *cpu);
 void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
 void arm_translate_init(void);
 
+void arm_cpu_register_gdb_commands(ARMCPU *cpu);
+
 void arm_restore_state_to_opc(CPUState *cs,
                               const TranslationBlock *tb,
                               const uint64_t *data);
-- 
2.34.1

[PATCH 3/4] tests: Gently exit from GDB when tests complete

[Gustavo Romero]

GDB commit a207f6b3a38 ('Rewrite "python" command exception handling')
changed how exit() called from Python scripts loaded by GDB behave,
turning it into an exception instead of a generic error code that is
returned. This change caused several QEMU tests to crash with the
following exception:

Python Exception <class 'SystemExit'>: 0
Error occurred in Python: 0

This happens because in tests/guest-debug/test_gdbstub.py exit is
called after the tests have completed.

This commit fixes it by politely asking GDB to exit via gdb.execute,
passing the proper fail_count to be reported to 'make', instead of
abruptly calling exit() from the Python script.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 tests/guest-debug/test_gdbstub.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/guest-debug/test_gdbstub.py b/tests/guest-debug/test_gdbstub.py
index 7f71d34da1..46fbf98f0c 100644
--- a/tests/guest-debug/test_gdbstub.py
+++ b/tests/guest-debug/test_gdbstub.py
@@ -57,4 +57,4 @@ def main(test, expected_arch=None):
         pass
 
     print("All tests complete: {} failures".format(fail_count))
-    exit(fail_count)
+    gdb.execute(f"exit {fail_count}")
-- 
2.34.1

[PATCH 4/4] tests/tcg/aarch64: Add MTE gdbstub tests

[Gustavo Romero]

Add tests to exercise the MTE stubs.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 tests/tcg/aarch64/Makefile.target     |  11 ++-
 tests/tcg/aarch64/gdbstub/test-mte.py |  86 ++++++++++++++++++++++
 tests/tcg/aarch64/mte-8.c             | 102 ++++++++++++++++++++++++++
 3 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/aarch64/gdbstub/test-mte.py
 create mode 100644 tests/tcg/aarch64/mte-8.c

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 70d728ae9a..d2e3f251eb 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -62,7 +62,7 @@ AARCH64_TESTS += bti-2
 
 # MTE Tests
 ifneq ($(CROSS_CC_HAS_ARMV8_MTE),)
-AARCH64_TESTS += mte-1 mte-2 mte-3 mte-4 mte-5 mte-6 mte-7
+AARCH64_TESTS += mte-1 mte-2 mte-3 mte-4 mte-5 mte-6 mte-7 mte-8
 mte-%: CFLAGS += -march=armv8.5-a+memtag
 endif
 
@@ -127,7 +127,14 @@ run-gdbstub-sve-ioctls: sve-ioctls
 		--bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \
 	basic gdbstub SVE ZLEN support)
 
-EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
+run-gdbstub-mte: mte-8
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(GDB) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+		--bin "$< -s" --test $(AARCH64_SRC)/gdbstub/test-mte.py, \
+	gdbstub MTE support)
+
+EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls run-gdbstub-mte
 endif
 endif
 
diff --git a/tests/tcg/aarch64/gdbstub/test-mte.py b/tests/tcg/aarch64/gdbstub/test-mte.py
new file mode 100644
index 0000000000..6530f33ad8
--- /dev/null
+++ b/tests/tcg/aarch64/gdbstub/test-mte.py
@@ -0,0 +1,86 @@
+from __future__ import print_function
+#
+# Test GDB memory-tag commands that exercise the stubs for the qIsAddressTagged,
+# qMemTag, and QMemTag packets. Logical tag-only commands rely on local
+# operations, hence don't exercise any stub.
+#
+# The test consists in breaking just after a atag() call (which sets the
+# allocation tag -- see mte-8.c for details) and setting/getting tags in
+# different memory locations and ranges starting at the address of the array
+# 'a'.
+#
+# This is launched via tests/guest-debug/run-test.py
+#
+
+
+import gdb
+import re
+from test_gdbstub import main, report
+
+
+PATTERN_0 = "Memory tags for address 0x[0-9a-f]+ match \(0x[0-9a-f]+\)."
+PATTERN_1 = ".*(0x[0-9a-f]+)"
+
+
+def run_test():
+    gdb.execute("break 94", False, True)
+    gdb.execute("continue", False, True)
+    try:
+        # Test if we can check correctly that the allocation tag for
+        # array 'a' matches the logical tag after atag() is called.
+        co = gdb.execute("memory-tag check a", False, True)
+        tags_match = re.findall(PATTERN_0, co, re.MULTILINE)
+        if tags_match:
+            report(True, "Logical and allocation tags match.")
+        else:
+            report(False, "Logical and allocation tags don't match!")
+
+        # Test allocation tag 'set and print' commands. Commands on logical
+        # tags rely on local operation and so don't exercise any stub.
+
+        # Set the allocation tag for the first granule (16 bytes) of
+        # address starting at 'a' address to a known value, i.e. 0x04.
+        gdb.execute("memory-tag set-allocation-tag a 1 04", False, True)
+
+        # Then set the allocation tag for the second granule to a known
+        # value, i.e. 0x06. This tests that contiguous tag granules are
+        # set correct and don't run over each other.
+        gdb.execute("memory-tag set-allocation-tag a+16 1 06", False, True)
+
+        # Read the known values back and check if they remain the same.
+
+        co = gdb.execute("memory-tag print-allocation-tag a", False, True)
+        first_tag = re.match(PATTERN_1, co)[1]
+
+        co = gdb.execute("memory-tag print-allocation-tag a+16", False, True)
+        second_tag = re.match(PATTERN_1, co)[1]
+
+        if first_tag == "0x4" and second_tag == "0x6":
+            report(True, "Allocation tags are correctly set/printed.")
+        else:
+            report(False, "Can't set/print allocation tags!")
+
+        # Now test fill pattern by setting a whole page with a pattern.
+        gdb.execute("memory-tag set-allocation-tag a 4096 0a0b", False, True)
+
+        # And read back the tags of the last two granules in page so
+        # we also test if the pattern is set correctly up to the end of
+        # the page.
+        co = gdb.execute("memory-tag print-allocation-tag a+4096-32", False, True)
+        tag = re.match(PATTERN_1, co)[1]
+
+        co = gdb.execute("memory-tag print-allocation-tag a+4096-16", False, True)
+        last_tag = re.match(PATTERN_1, co)[1]
+
+        if tag == "0xa" and last_tag == "0xb":
+            report(True, "Fill pattern is ok.")
+        else:
+            report(False, "Fill pattern failed!")
+
+    except gdb.error:
+        # This usually happens because a GDB version that does not
+        # support memory tagging was used to run the test.
+        report(False, "'memory-tag' command failed!")
+
+
+main(run_test, expected_arch="aarch64")
diff --git a/tests/tcg/aarch64/mte-8.c b/tests/tcg/aarch64/mte-8.c
new file mode 100644
index 0000000000..367768e6b6
--- /dev/null
+++ b/tests/tcg/aarch64/mte-8.c
@@ -0,0 +1,102 @@
+/*
+ * To be compiled with -march=armv8.5-a+memtag
+ *
+ * This test is adapted from a Linux test. Please see:
+ *
+ * https://www.kernel.org/doc/html/next/arch/arm64/memory-tagging-extension.html#example-of-correct-usage
+ */
+#include <errno.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/auxv.h>
+#include <sys/mman.h>
+#include <sys/prctl.h>
+#include <string.h>
+/*
+ * From arch/arm64/include/uapi/asm/hwcap.h
+ */
+#define HWCAP2_MTE              (1 << 18)
+
+/*
+ * From arch/arm64/include/uapi/asm/mman.h
+ */
+#define PROT_MTE                 0x20
+
+/*
+ * Insert a random logical tag into the given pointer.
+ */
+#define insert_random_tag(ptr) ({                   \
+    uint64_t __val;                                 \
+    asm("irg %0, %1" : "=r" (__val) : "r" (ptr));   \
+    __val;                                          \
+})
+
+/*
+ * Set the allocation tag on the destination address.
+ */
+#define set_tag(tagged_addr) do {                                      \
+        asm volatile("stg %0, [%0]" : : "r" (tagged_addr) : "memory"); \
+} while (0)
+
+
+int main(int argc, char *argv[])
+{
+    unsigned char *a;
+    unsigned long page_sz = sysconf(_SC_PAGESIZE);
+    unsigned long hwcap2 = getauxval(AT_HWCAP2);
+
+    if (!(argc == 2 && strcmp(argv[1], "-s") == 0)) {
+        return 0;
+    }
+
+    /* check if MTE is present */
+    if (!(hwcap2 & HWCAP2_MTE))
+            return EXIT_FAILURE;
+
+    /*
+     * Enable the tagged address ABI, synchronous or asynchronous MTE
+     * tag check faults (based on per-CPU preference) and allow all
+     * non-zero tags in the randomly generated set.
+     */
+    if (prctl(PR_SET_TAGGED_ADDR_CTRL,
+              PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC | PR_MTE_TCF_ASYNC |
+              (0xfffe << PR_MTE_TAG_SHIFT),
+              0, 0, 0)) {
+        perror("prctl() failed");
+        return EXIT_FAILURE;
+    }
+
+    a = mmap(0, page_sz, PROT_READ | PROT_WRITE,
+             MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (a == MAP_FAILED) {
+        perror("mmap() failed");
+        return EXIT_FAILURE;
+    }
+
+    printf("a[] address is %p\n", a);
+
+    /*
+     * Enable MTE on the above anonymous mmap. The flag could be passed
+     * directly to mmap() and skip this step.
+     */
+    if (mprotect(a, page_sz, PROT_READ | PROT_WRITE | PROT_MTE)) {
+        perror("mprotect() failed");
+        return EXIT_FAILURE;
+    }
+
+    /* access with the default tag (0) */
+    a[0] = 1;
+    a[1] = 2;
+
+    printf("a[0] = %hhu a[1] = %hhu\n", a[0], a[1]);
+
+    /* set the logical and allocation tags */
+    a = (unsigned char *)insert_random_tag(a);
+    set_tag(a);
+
+    printf("%p\n", a);
+
+    return 0;
+}
-- 
2.34.1

Re: [PATCH 3/4] tests: Gently exit from GDB when tests complete

[Philippe Mathieu-Daudé]

On 15/5/24 19:31, Gustavo Romero wrote:
> GDB commit a207f6b3a38 ('Rewrite "python" command exception handling')
> changed how exit() called from Python scripts loaded by GDB behave,
> turning it into an exception instead of a generic error code that is
> returned. This change caused several QEMU tests to crash with the
> following exception:
> 
> Python Exception <class 'SystemExit'>: 0
> Error occurred in Python: 0
> 
> This happens because in tests/guest-debug/test_gdbstub.py exit is
> called after the tests have completed.
> 
> This commit fixes it by politely asking GDB to exit via gdb.execute,
> passing the proper fail_count to be reported to 'make', instead of
> abruptly calling exit() from the Python script.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   tests/guest-debug/test_gdbstub.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/guest-debug/test_gdbstub.py b/tests/guest-debug/test_gdbstub.py
> index 7f71d34da1..46fbf98f0c 100644
> --- a/tests/guest-debug/test_gdbstub.py
> +++ b/tests/guest-debug/test_gdbstub.py
> @@ -57,4 +57,4 @@ def main(test, expected_arch=None):
>           pass
>   
>       print("All tests complete: {} failures".format(fail_count))
> -    exit(fail_count)
> +    gdb.execute(f"exit {fail_count}")

Yay!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/4] gdbstub: Add support for target-specific stubs

[Philippe Mathieu-Daudé]

On 15/5/24 19:31, Gustavo Romero wrote:
> Currently, it's not possible to have stubs specific to a given target,
> even though there are GDB features which are target-specific, like, for
> instance, memory tagging.
> 
> This commit introduces set_query_supported_arch,
> set_gdb_gen_query_table_arch, and set_gdb_gen_set_table_arch functions
> as interfaces to extend the qSupported string, the query handler table,
> and set handler table per target, so allowing target-specific stub
> implementation.
> 
> Besides that, it moves GdbCmdParseEntry struct, its related types, and
> gdb_put_packet to include/exec/gdbstub.h so they are also available in
> the target-specific stubs.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   gdbstub/gdbstub.c      | 108 +++++++++++++++++++++++------------------
>   gdbstub/internals.h    |  22 ---------
>   gdbstub/syscalls.c     |   1 +
>   include/exec/gdbstub.h |  86 +++++++++++++++++++++++++++++++-
>   4 files changed, 147 insertions(+), 70 deletions(-)


> @@ -1645,6 +1608,13 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
>       gdb_put_strbuf();
>   }
>   
> +/* Arch-specific qSupported */
> +char *query_supported_arch = NULL;

Please declare query_supported_arch as static.

> +void set_query_supported_arch(char *query_supported)
> +{
> +    query_supported_arch = query_supported;
> +}
> +
>   static void handle_query_supported(GArray *params, void *user_ctx)
>   {
>       CPUClass *cc;
> @@ -1684,6 +1654,11 @@ static void handle_query_supported(GArray *params, void *user_ctx)
>       }
>   
>       g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
> +
> +    if (query_supported_arch) {
> +        g_string_append(gdbserver_state.str_buf, query_supported_arch);
> +    }
> +
>       gdb_put_strbuf();
>   }

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Although I'd rather split in 3 as:

1/ Expose API

-- >8 --
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 32f9f63297..34121dc61a 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -109 +108,0 @@ void gdb_put_strbuf(void);
-int gdb_put_packet(const char *buf);
@@ -169,21 +167,0 @@ void gdb_init_gdbserver_state(void);
-typedef enum GDBThreadIdKind {
-    GDB_ONE_THREAD = 0,
-    GDB_ALL_THREADS,     /* One process, all threads */
-    GDB_ALL_PROCESSES,
-    GDB_READ_THREAD_ERR
-} GDBThreadIdKind;
-
-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;
-
-#define get_param(p, i)    (&g_array_index(p, GdbCmdVariant, i))
-
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index eb14b91139..7bf189d7f3 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -147 +147,85 @@ extern const GDBFeature gdb_static_features[];
-#endif
+typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
+
+typedef enum GDBThreadIdKind {
+    GDB_ONE_THREAD = 0,
+    GDB_ALL_THREADS,     /* One process, all threads */
+    GDB_ALL_PROCESSES,
+    GDB_READ_THREAD_ERR
+} GDBThreadIdKind;
+
+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;
+
+#define get_param(p, i)    (&g_array_index(p, GdbCmdVariant, i))
+
+/*
+ * cmd_startswith -> cmd is compared using startswith
+ *
+ * allow_stop_reply -> true iff the gdbstub can respond to this command 
with a
+ *   "stop reply" packet. The list of commands that accept such response is
+ *   defined at the GDB Remote Serial Protocol documentation. see:
+ * 
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
+ *
+ * schema definitions:
+ * Each schema parameter entry consists of 2 chars,
+ * 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;
+    bool cmd_startswith;
+    const char *schema;
+    bool allow_stop_reply;
+} GdbCmdParseEntry;
+
+#define get_cmd_parsers(p) (&g_array_index(p, GdbCmdParseEntry, 0))
+
+/**
+ * set_gdb_gen_query_table_arch() - set a table to handle arch-specific 
query
+ * packets
+ */
+void set_gdb_gen_query_table_arch(GdbCmdParseEntry *table, int size);
+
+/**
+ * set_gdb_gen_set_table_arch() - set a table to handle arch-specific set
+ * packets
+ */
+void set_gdb_gen_set_table_arch(GdbCmdParseEntry *, int size);
+
+/**
+ * set_query_supported_arch() - set arch-specific features in qSupported
+ * features
+ */
+void set_query_supported_arch(char *);
+
+/**
+ * gdb_put_packet() - put string into gdb server's buffer so it is sent
+ * to the client
+ */
+int gdb_put_packet(const char *buf);
+
+#endif /* GDBSTUB_H */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b3574997ea..fc72f03c8a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -923,37 +922,0 @@ static int cmd_parse_params(const char *data, const 
char *schema,
-typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
-
-/*
- * cmd_startswith -> cmd is compared using startswith
- *
- * allow_stop_reply -> true iff the gdbstub can respond to this command 
with a
- *   "stop reply" packet. The list of commands that accept such response is
- *   defined at the GDB Remote Serial Protocol documentation. see:
- * 
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
- *
- * schema definitions:
- * Each schema parameter entry consists of 2 chars,
- * 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;
-    bool cmd_startswith;
-    const char *schema;
-    bool allow_stop_reply;
-} GdbCmdParseEntry;
-
diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c
index 02e3a8f74c..ee9a16495e 100644
--- a/gdbstub/syscalls.c
+++ b/gdbstub/syscalls.c
@@ -20,0 +21 @@
+#include "exec/gdbstub.h"
---

2/ Refactor query/set handlers in preparation of target specific

-- >8 --
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index fc72f03c8a..1bb35167d4 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1855,3 +1855,3 @@ static void handle_gen_query(GArray *params, void 
*user_ctx)
-    if (!process_string_cmd(get_param(params, 0)->data,
-                            gdb_gen_query_set_common_table,
-                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+    if (process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_query_set_common_table,
+                           ARRAY_SIZE(gdb_gen_query_set_common_table)) 
== 0) {
@@ -1863,2 +1863,2 @@ static void handle_gen_query(GArray *params, void 
*user_ctx)
-                           ARRAY_SIZE(gdb_gen_query_table))) {
-        gdb_put_packet("");
+                           ARRAY_SIZE(gdb_gen_query_table)) == 0) {
+        return;
@@ -1865,0 +1866,3 @@ static void handle_gen_query(GArray *params, void 
*user_ctx)
+
+    /* Can't handle query, return Empty response. */
+    gdb_put_packet("");
@@ -1874,3 +1877,3 @@ static void handle_gen_set(GArray *params, void 
*user_ctx)
-    if (!process_string_cmd(get_param(params, 0)->data,
-                            gdb_gen_query_set_common_table,
-                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+    if (process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_query_set_common_table,
+                           ARRAY_SIZE(gdb_gen_query_set_common_table)) 
== 0) {
@@ -1882,2 +1885,2 @@ static void handle_gen_set(GArray *params, void 
*user_ctx)
-                           ARRAY_SIZE(gdb_gen_set_table))) {
-        gdb_put_packet("");
+                           ARRAY_SIZE(gdb_gen_set_table)) == 0) {
+        return;
@@ -1884,0 +1888,3 @@ static void handle_gen_set(GArray *params, void 
*user_ctx)
+
+    /* Can't handle set, return Empty response. */
+    gdb_put_packet("");
---

3/ Introduce target specific handlers

-- >8 --
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 1bb35167d4..a6b2200fc3 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1610,0 +1611,7 @@ static void handle_query_thread_extra(GArray 
*params, void *user_ctx)
+/* Arch-specific qSupported */
+static char *query_supported_arch = NULL;
+void set_query_supported_arch(char *query_supported)
+{
+    query_supported_arch = query_supported;
+}
+
@@ -1649,0 +1657,5 @@ static void handle_query_supported(GArray *params, 
void *user_ctx)
+
+    if (query_supported_arch) {
+        g_string_append(gdbserver_state.str_buf, query_supported_arch);
+    }
+
@@ -1730,0 +1743,10 @@ static const GdbCmdParseEntry 
gdb_gen_query_set_common_table[] = {
+
+/* Arch-specific query table */
+static GdbCmdParseEntry *gdb_gen_query_table_arch = NULL ;
+static int gdb_gen_query_table_arch_size = 0;
+void set_gdb_gen_query_table_arch(GdbCmdParseEntry  *table, int size)
+{
+    gdb_gen_query_table_arch = table;
+    gdb_gen_query_table_arch_size = size;
+}
+
@@ -1822,0 +1845,9 @@ static const GdbCmdParseEntry 
gdb_gen_query_table[] = {
+/* Arch-specific set table */
+static GdbCmdParseEntry *gdb_gen_set_table_arch = NULL;
+static int gdb_gen_set_table_arch_size = 0;
+void set_gdb_gen_set_table_arch(GdbCmdParseEntry *table, int size)
+{
+    gdb_gen_set_table_arch = table;
+    gdb_gen_set_table_arch_size = size;
+}
+
@@ -1866,0 +1898,7 @@ static void handle_gen_query(GArray *params, void 
*user_ctx)
+    if (gdb_gen_query_table_arch &&
+        process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_query_table_arch,
+                           gdb_gen_query_table_arch_size) == 0) {
+        return;
+    }
+
@@ -1888,0 +1927,7 @@ static void handle_gen_set(GArray *params, void 
*user_ctx)
+    if (gdb_gen_set_table_arch &&
+        process_string_cmd(get_param(params, 0)->data,
+                           gdb_gen_set_table_arch,
+                           gdb_gen_set_table_arch_size) == 0) {
+        return;
+    }
+
---

Regards,

Phil.

Re: [PATCH 2/4] gdbstub: Add support for MTE in user mode

[Philippe Mathieu-Daudé]

Hi Gustavo,

On 15/5/24 19:31, Gustavo Romero wrote:
> This commit implements the stubs to handle the qIsAddressTagged,
> qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
> subcommands to work with QEMU gdbstub on aarch64 user mode. It also
> implements the get/set function for the special GDB MTE register
> 'tag_ctl', used to control the MTE fault type at runtime.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   configs/targets/aarch64-linux-user.mak |   2 +-
>   target/arm/cpu.c                       |   1 +
>   target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
>   target/arm/internals.h                 |   2 +
>   4 files changed, 325 insertions(+), 1 deletion(-)


> +void arm_cpu_register_gdb_commands(ARMCPU *cpu)
> +{
> +    GArray *gdb_gen_query_table_arm =
> +        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> +    GArray *gdb_gen_set_table_arm =
> +        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> +    GString *supported_features = g_string_new(NULL);
> +
> +#ifdef TARGET_AARCH64
> +#ifdef CONFIG_USER_ONLY
> +    /* MTE */
> +    if (isar_feature_aa64_mte(&cpu->isar)) {

Can we keep this code generic (not guarded by #ifdef'ry)? We
are protected by this isar_feature_aa64_mte() call to register
the MTE feature.

> +        g_string_append(supported_features, ";memory-tagging+");
> +
> +        add_packet_handler(gdb_gen_query_table_arm, qMemTags);
> +        add_packet_handler(gdb_gen_query_table_arm, qIsAddressTagged);
> +
> +        add_packet_handler(gdb_gen_set_table_arm, QMemTags);
> +    }
> +#endif
> +#endif
> +
> +    /* Set arch-specific handlers for 'q' commands. */
> +    if (gdb_gen_query_table_arm->len) {
> +        set_gdb_gen_query_table_arch(&g_array_index(gdb_gen_query_table_arm,
> +                                                    GdbCmdParseEntry, 0),
> +                                                    gdb_gen_query_table_arm->len);
> +    }
> +
> +    /* Set arch-specific handlers for 'Q' commands. */
> +    if (gdb_gen_set_table_arm->len) {
> +        set_gdb_gen_set_table_arch(&g_array_index(gdb_gen_set_table_arm,
> +                                   GdbCmdParseEntry, 0),
> +                                   gdb_gen_set_table_arm->len);
> +    }
> +
> +    /* Set arch-specific qSupported feature. */
> +    if (supported_features->len) {
> +        set_query_supported_arch(supported_features->str);
> +    }
> +}

Re: [PATCH 2/4] gdbstub: Add support for MTE in user mode

[Richard Henderson]

On 5/15/24 19:31, Gustavo Romero wrote:
> +static int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    assert(reg == 0);
> +
> +    /* Sanitize TCF0 bits. */
> +    *buf &= 0x03;
> +
> +    if (!isar_feature_aa64_mte3(&cpu->isar) && *buf == 3) {

cpu_isar_feature(aa64_mte3, cpu)


> +        /*
> +         * If FEAT_MTE3 is not implemented, the value 0b11 is reserved, hence
> +         * ignore setting it.
> +         */
> +        return 0;

That said, we always implement the mte3 behaviour, so perhaps drop this entirely?

> +    }
> +
> +    /*
> +     * 'tag_ctl' register is actually a "pseudo-register" provided by GDB to
> +     * expose options that can be controlled at runtime and has the same effect
> +     * of prctl() with option PR_SET_TAGGED_ADDR_CTRL,
> +     * i.e. prctl(PR_SET_TAGGED_ADDR_CTRL, tcf, 0, 0, 0), hence it controls
> +     * the effect of Tag Check Faults (TCF) due to Loads and Stores in EL0.
> +     */
> +    env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, *buf);
> +
> +    return 1;
> +}
> +
> +static void handle_q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
> +{
> +    uint64_t addr = get_param(params, 0)->val_ull;
> +    uint64_t len = get_param(params, 1)->val_ul;
> +    int type = get_param(params, 2)->val_ul;
> +
> +    uint64_t clean_addr;
> +    uint8_t *tags;
> +    int granules_index;
> +    int granule_index;
> +    uint8_t addr_tag;
> +
> +    g_autoptr(GString) str_buf = g_string_new(NULL);
> +
> +    /*
> +     * GDB does not query tags for a memory range on remote targets, so that's
> +     * not supported either by gdbstub.
> +     */
> +    if (len != 1) {
> +        gdb_put_packet("E02");
> +    }
> +
> +    /* GDB never queries a tag different from an allocation tag (type 1). */
> +    if (type != 1) {
> +        gdb_put_packet("E02");
> +    }
> +
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    /*
> +     * Get pointer to all tags in the page where the address is. Note that tags
> +     * are packed, so there are 2 tags packed in one byte.
> +     */
> +    tags = page_get_target_data(clean_addr);

While you expect gdb will have called isaddresstagged first, you can't guarantee that:
you should verify that the address is valid and tagged first.

> +static void handle_q_isaddresstagged(GArray *params, G_GNUC_UNUSED void *user_ctx)
> +{
> +    uint64_t addr = get_param(params, 0)->val_ull;
> +
> +    uint64_t clean_addr;
> +    int mflags;
> +
> +    g_autoptr(GString) str_buf = g_string_new(NULL);
> +
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    mflags = page_get_flags(clean_addr);
> +    if (mflags & PAGE_ANON && mflags & PAGE_MTE) {
> +        /* Address is tagged. */
> +        g_string_printf(str_buf, "%.2x", 1 /* true */);
> +    } else {
> +        /* Address is not tagged. */
> +        g_string_printf(str_buf, "%.2x", 0 /* false */);
> +    }
> +
> +    gdb_put_packet(str_buf->str);

Overkill with GString?

     const char *result = (mflags & PAGE_ANON && mflags & PAGE_MTE ? "1" : "0");
     gdb_put_packet(result);

?

> +}
> +
> +static void handle_Q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
> +{
> +    uint64_t addr = get_param(params, 0)->val_ull;
> +    uint64_t len = get_param(params, 1)->val_ul;
> +    int type = get_param(params, 2)->val_ul;
> +    char const *new_tags = get_param(params, 3)->data;
> +
> +    uint64_t clean_addr;
> +    int last_addr_index;
> +
> +    uint64_t start_addr_page;
> +    uint64_t end_addr_page;
> +
> +    uint32_t first_tag_index;
> +    uint32_t last_tag_index;
> +
> +    uint8_t *tags; /* Pointer to the current tags in a page. */
> +    int num_new_tags;
> +
> +    g_autoptr(GString) str_buf = g_string_new(NULL);
> +
> +    /*
> +     * Only the allocation tag (type 1) can be set at the stub side.
> +     */
> +    if (type != 1) {
> +        gdb_put_packet("E02");
> +        return;
> +    }
> +
> +    /*
> +     * 'len' is always >= 1 and refers to the size of the memory range about to
> +     * have its tags updated. However, it's convenient to obtain the index for
> +     * the last byte of the memory range for page boundary checks and for
> +     * obtaining the indexes for the tags in the page.
> +     */
> +    last_addr_index = len - 1;
> +
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    start_addr_page = extract64(clean_addr, TARGET_PAGE_BITS,
> +                                64 - TARGET_PAGE_BITS);
> +    end_addr_page = extract64(clean_addr + last_addr_index, TARGET_PAGE_BITS,
> +                              64 - TARGET_PAGE_BITS);
> +
> +    /*
> +     * Check if memory range is within page boundaries.
> +     */
> +    if (start_addr_page != end_addr_page) {
> +        gdb_put_packet("E03");
> +        return;
> +    }
> +
> +    /*
> +     * Get pointer to all tags in the page where the address is. Note that here
> +     * tags are packed, so there are 2 tags packed in one byte.
> +     */
> +    tags = page_get_target_data(clean_addr);

Likewise.

> +
> +    /* Tag indices below refer to unpacked tags. */
> +    first_tag_index = extract32(clean_addr, LOG2_TAG_GRANULE,
> +                                TARGET_PAGE_BITS - LOG2_TAG_GRANULE);
> +    last_tag_index = extract32(clean_addr + last_addr_index, LOG2_TAG_GRANULE,
> +                               TARGET_PAGE_BITS - LOG2_TAG_GRANULE);
> +
> +    /*
> +     * GDB sends 2 hex digits per tag number, i.e. tags are not represented in
> +     * a packed way.
> +     */
> +    num_new_tags = strlen(new_tags) / 2;
> +
> +    /*
> +     * If the number of tags provided is greater than the number of tags
> +     * in the provided memory range, the exceeding tags are ignored. If the
> +     * number of tags is less than the number of tags in the provided memory
> +     * range, then the provided tags are used as a repeating pattern to fill
> +     * the tags in the provided memory range.
> +     */
> +    for (int i = first_tag_index, j = 0; i <= last_tag_index; i++, j++) {
> +        int new_tag_value;
> +        int packed_granules_index;
> +        int nibble_index;
> +
> +        sscanf(new_tags + 2 * (j % num_new_tags), "%2x", &new_tag_value);

Overkill?
While gdb may send 2 digits, only 0[0-9a-fA-F] is valid.


> +        /*
> +         * Find packed tag index from unpacked tag index. There are two tags
> +         * packed in one packed index. One tag per nibble.
> +         */
> +        packed_granules_index = i / 2;
> +        /* Find nibble index in the packed tag from unpacked tag index. */
> +        nibble_index = i % 2;
> +
> +        if (nibble_index == 0) { /* Update low nibble */
> +            *(tags + packed_granules_index) &= 0xF0;
> +            *(tags + packed_granules_index) |= (new_tag_value & 0x0F);
> +        } else { /* Update high nibble */
> +            *(tags + packed_granules_index) &= 0x0F;
> +            *(tags + packed_granules_index) |= ((new_tag_value & 0x0F) << 4);
> +        }

How many tags will gdb typically send with this?
If 1 or 2, it might be worth using memset.
If even, it might be worth pre-computing and using memcpy.


r~

Re: [PATCH 2/4] gdbstub: Add support for MTE in user mode

[Richard Henderson]

On 5/15/24 19:31, Gustavo Romero wrote:
> +    /* Remove any non-addressing bits. */
> +    clean_addr = useronly_clean_ptr(addr);
> +
> +    /*
> +     * Get pointer to all tags in the page where the address is. Note that tags
> +     * are packed, so there are 2 tags packed in one byte.
> +     */
> +    tags = page_get_target_data(clean_addr);
> +
> +    /*
> +     * Tags are per granule (16 bytes). 2 tags (4 bits each) are kept in a
> +     * single byte for compactness, so first a page tag index for 2 packed
> +     * granule tags (1 byte) is found, and then an index for a single granule
> +     * tag (nibble) is found, and finally the address tag is obtained.
> +     */
> +    granules_index = extract32(clean_addr, LOG2_TAG_GRANULE + 1,
> +                               TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
> +    granule_index = extract32(clean_addr, LOG2_TAG_GRANULE, 1);
> +
> +    addr_tag = *(tags + granules_index);
> +    /* Extract tag from the right nibble. */
> +    if (granule_index == 0) {
> +        addr_tag &= 0xF;
> +    } else {
> +        addr_tag >>= 4;
> +    }
> +

I think I would prefer the body of all three of these gdb commands to be split out into 
separate functions.  I think they should use use allocation_tag_mem_probe, load_tag1, 
store_tag1 from mte_helper.c.  I am undecided as to whether the gdb helpers should be 
placed in mte_helper.c, or if the existing mte_helper.c functions should be exported.


r~

Re: [PATCH 0/4] Add MTE stubs for aarch64 user mode

[Alex Bennée]

Gustavo Romero <gustavo.romero@linaro.org> writes:

> This patchset adds the stubs necessary to support GDB memory tagging
> commands on QEMU aarch64 user mode.

On application I'm getting the following failure on configure which
makes me think something is missing:

  Program scripts/undefsym.py found: YES (/home/alex/lsrc/qemu.git/builds/all/pyvenv/bin/python3 /home/alex/lsrc/qemu.git/scripts/undefsym.py)
  Program scripts/feature_to_c.py found: YES (/home/alex/lsrc/qemu.git/builds/all/pyvenv/bin/python3 /home/alex/lsrc/qemu.git/scripts/feature_to_c.py)

  ../../meson.build:3851:4: ERROR: File gdb-xml/aarch64-mte.xml does not exist.

  A full log can be found at /home/alex/lsrc/qemu.git/builds/all/meson-logs/meson-log.txt
  ninja: error: rebuilding 'build.ninja': subcommand failed
  FAILED: build.ninja 
  /home/alex/lsrc/qemu.git/builds/all/pyvenv/bin/meson --internal regenerate /home/alex/lsrc/qemu.git /home/alex/lsrc/qemu.git/builds/all
  make: *** [Makefile:167: run-ninja] Error 1

  Compilation exited abnormally with code 2 at Tue May 28 16:59:05

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/4] gdbstub: Add support for target-specific stubs

[Alex Bennée]

Gustavo Romero <gustavo.romero@linaro.org> writes:

> Currently, it's not possible to have stubs specific to a given target,
> even though there are GDB features which are target-specific, like, for
> instance, memory tagging.
>
> This commit introduces set_query_supported_arch,
> set_gdb_gen_query_table_arch, and set_gdb_gen_set_table_arch functions
> as interfaces to extend the qSupported string, the query handler table,
> and set handler table per target, so allowing target-specific stub
> implementation.

Subsystem functions exposed to the wider QEMU should be prefixed by the
subsystem name (e.g. gdb_*).

> Besides that, it moves GdbCmdParseEntry struct, its related types, and
> gdb_put_packet to include/exec/gdbstub.h so they are also available in
> the target-specific stubs.

Generally I'm trying to reduce the amount of stuff that gets dumped in
here because it is included widely and if your not careful you start
winding yourself into knots.

Could we put the command specific stuff into include/gdbstub/commands.h
and only include it when needed?

In general I think we could make the series a little more granular:

  - move GdbCmdParseEntry into headers
  - clean-up process_string_cmd
  - introduce new gdb_ APIs

That will reduce the size of the individual patches and make review a
bit easier.

>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdbstub/gdbstub.c      | 108 +++++++++++++++++++++++------------------
>  gdbstub/internals.h    |  22 ---------
>  gdbstub/syscalls.c     |   1 +
>  include/exec/gdbstub.h |  86 +++++++++++++++++++++++++++++++-
>  4 files changed, 147 insertions(+), 70 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b3574997ea..4996530fde 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -920,43 +920,6 @@ static int cmd_parse_params(const char *data, const char *schema,
>      return 0;
>  }
>  
> -typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
> -
> -/*
> - * cmd_startswith -> cmd is compared using startswith
> - *
> - * allow_stop_reply -> true iff the gdbstub can respond to this command with a
> - *   "stop reply" packet. The list of commands that accept such response is
> - *   defined at the GDB Remote Serial Protocol documentation. see:
> - *   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
> - *
> - * schema definitions:
> - * Each schema parameter entry consists of 2 chars,
> - * 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;
> -    bool cmd_startswith;
> -    const char *schema;
> -    bool allow_stop_reply;
> -} GdbCmdParseEntry;
> -
>  static inline int startswith(const char *string, const char *pattern)
>  {
>    return !strncmp(string, pattern, strlen(pattern));
> @@ -1645,6 +1608,13 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
>      gdb_put_strbuf();
>  }
>  
> +/* Arch-specific qSupported */
> +char *query_supported_arch = NULL;

This should be static.

> +void set_query_supported_arch(char *query_supported)
> +{
> +    query_supported_arch = query_supported;
> +}
> +

Maybe gdb_extended_qsupported_features?

>  static void handle_query_supported(GArray *params, void *user_ctx)
>  {
>      CPUClass *cc;
> @@ -1684,6 +1654,11 @@ static void handle_query_supported(GArray *params, void *user_ctx)
>      }
>  
>      g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
> +
> +    if (query_supported_arch) {
> +        g_string_append(gdbserver_state.str_buf, query_supported_arch);
> +    }
> +
>      gdb_put_strbuf();
>  }
>  
> @@ -1765,6 +1740,16 @@ static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
>      },
>  };
>  
> +
> +/* Arch-specific query table */
> +static GdbCmdParseEntry *gdb_gen_query_table_arch = NULL ;
> +static int gdb_gen_query_table_arch_size = 0;

For local statics you don't need prefixes if this saves on variables
getting too large. Also you don't need init to NULL/0 for statics.

> +void set_gdb_gen_query_table_arch(GdbCmdParseEntry  *table, int size)
> +{
> +    gdb_gen_query_table_arch = table;
> +    gdb_gen_query_table_arch_size = size;
> +}
> +
>  static const GdbCmdParseEntry gdb_gen_query_table[] = {
>      {
>          .handler = handle_query_curr_tid,
> @@ -1857,6 +1842,15 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>  #endif
>  };
>  
> +/* Arch-specific set table */
> +static GdbCmdParseEntry *gdb_gen_set_table_arch = NULL;
> +static int gdb_gen_set_table_arch_size = 0;
> +void set_gdb_gen_set_table_arch(GdbCmdParseEntry *table, int size)
> +{
> +    gdb_gen_set_table_arch = table;
> +    gdb_gen_set_table_arch_size = size;
> +}
> +
>  static const GdbCmdParseEntry gdb_gen_set_table[] = {
>      /* Order is important if has same prefix */
>      {
> @@ -1889,17 +1883,27 @@ static void handle_gen_query(GArray *params, void *user_ctx)
>          return;
>      }
>  
> -    if (!process_string_cmd(get_param(params, 0)->data,
> -                            gdb_gen_query_set_common_table,
> -                            ARRAY_SIZE(gdb_gen_query_set_common_table))) {
> +    if (process_string_cmd(get_param(params, 0)->data,
> +                           gdb_gen_query_set_common_table,
> +                           ARRAY_SIZE(gdb_gen_query_set_common_table)) == 0) {
>          return;
>      }

I think changing the process_string_cmd to return bool on success could
be a pre-cursor patch.

>  
>      if (process_string_cmd(get_param(params, 0)->data,
>                             gdb_gen_query_table,
> -                           ARRAY_SIZE(gdb_gen_query_table))) {
> -        gdb_put_packet("");
> +                           ARRAY_SIZE(gdb_gen_query_table)) == 0) {
> +        return;
>      }
> +
> +    if (gdb_gen_query_table_arch &&
> +        process_string_cmd(get_param(params, 0)->data,
> +                           gdb_gen_query_table_arch,
> +                           gdb_gen_query_table_arch_size) == 0) {
> +        return;
> +    }
> +
> +    /* Can't handle query, return Empty response. */
> +    gdb_put_packet("");
>  }
>  
<snip>
> diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c
> index 02e3a8f74c..ee9a16495e 100644
> --- a/gdbstub/syscalls.c
> +++ b/gdbstub/syscalls.c
> @@ -18,6 +18,7 @@
>  #include "gdbstub/syscalls.h"
>  #include "trace.h"
>  #include "internals.h"
> +#include "exec/gdbstub.h"

This looks like a stray - it would be clearer once split out anyway.

>  
>  /* Syscall specific state */
>  typedef struct {
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index eb14b91139..7bf189d7f3 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -144,4 +144,88 @@ void gdb_set_stop_cpu(CPUState *cpu);
>  /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
>  extern const GDBFeature gdb_static_features[];
>  
> -#endif
> +typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
> +
> +typedef enum GDBThreadIdKind {
> +    GDB_ONE_THREAD = 0,
> +    GDB_ALL_THREADS,     /* One process, all threads */
> +    GDB_ALL_PROCESSES,
> +    GDB_READ_THREAD_ERR
> +} GDBThreadIdKind;
> +
> +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;
> +
> +#define get_param(p, i)    (&g_array_index(p, GdbCmdVariant, i))

This will need renaming if its public outside of gdbstub. gdb_get_cmd_param?

> +
> +/*

If this is moving to a public API could you kdoc the description when
you move it please?

> + * cmd_startswith -> cmd is compared using startswith
> + *
> + * allow_stop_reply -> true iff the gdbstub can respond to this command with a
> + *   "stop reply" packet. The list of commands that accept such response is
> + *   defined at the GDB Remote Serial Protocol documentation. see:
> + *   https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
> + *
> + * schema definitions:
> + * Each schema parameter entry consists of 2 chars,
> + * 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;
> +    bool cmd_startswith;
> +    const char *schema;
> +    bool allow_stop_reply;
> +} GdbCmdParseEntry;
> +
> +#define get_cmd_parsers(p) (&g_array_index(p, GdbCmdParseEntry, 0))
> +
> +/**
> + * set_gdb_gen_query_table_arch() - set a table to handle arch-specific query
> + * packets
> + */
> +void set_gdb_gen_query_table_arch(GdbCmdParseEntry *table, int size);
> +
> +/**
> + * set_gdb_gen_set_table_arch() - set a table to handle arch-specific set
> + * packets
> + */
> +void set_gdb_gen_set_table_arch(GdbCmdParseEntry *, int size);
> +
> +/**
> + * set_query_supported_arch() - set arch-specific features in qSupported
> + * features
> + */
> +void set_query_supported_arch(char *);
> +
> +/**
> + * gdb_put_packet() - put string into gdb server's buffer so it is sent
> + * to the client
> + */
> +int gdb_put_packet(const char *buf);
> +
> +#endif /* GDBSTUB_H */

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/4] gdbstub: Add support for MTE in user mode

[Alex Bennée]

Gustavo Romero <gustavo.romero@linaro.org> writes:

> This commit implements the stubs to handle the qIsAddressTagged,
> qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
> subcommands to work with QEMU gdbstub on aarch64 user mode. It also
> implements the get/set function for the special GDB MTE register
> 'tag_ctl', used to control the MTE fault type at runtime.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  configs/targets/aarch64-linux-user.mak |   2 +-
>  target/arm/cpu.c                       |   1 +
>  target/arm/gdbstub.c                   | 321 +++++++++++++++++++++++++
>  target/arm/internals.h                 |   2 +
>  4 files changed, 325 insertions(+), 1 deletion(-)
>
> diff --git a/configs/targets/aarch64-linux-user.mak b/configs/targets/aarch64-linux-user.mak
> index ba8bc5fe3f..8f0ed21d76 100644
> --- a/configs/targets/aarch64-linux-user.mak
> +++ b/configs/targets/aarch64-linux-user.mak
> @@ -1,6 +1,6 @@
>  TARGET_ARCH=aarch64
>  TARGET_BASE_ARCH=arm
> -TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml
> +TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml gdb-xml/aarch64-mte.xml

Ahh there it is, this got missed from the commit

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro