* [PATCH v3 1/9] gdbstub: Clean up process_string_cmd
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-21 0:49 ` Richard Henderson
2024-06-17 6:28 ` [PATCH v3 2/9] gdbstub: Move GdbCmdParseEntry into a new header file Gustavo Romero
` (7 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: gustavo.romero
Change 'process_string_cmd' to return true on success and false on
failure, instead of 0 and -1.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
gdbstub/gdbstub.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b3574997ea..37314b92e5 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -962,14 +962,14 @@ static inline int startswith(const char *string, const char *pattern)
return !strncmp(string, pattern, strlen(pattern));
}
-static int process_string_cmd(const char *data,
- const GdbCmdParseEntry *cmds, int num_cmds)
+static bool process_string_cmd(const char *data,
+ const GdbCmdParseEntry *cmds, int num_cmds)
{
int i;
g_autoptr(GArray) params = g_array_new(false, true, sizeof(GdbCmdVariant));
if (!cmds) {
- return -1;
+ return false;
}
for (i = 0; i < num_cmds; i++) {
@@ -984,16 +984,16 @@ static int process_string_cmd(const char *data,
if (cmd->schema) {
if (cmd_parse_params(&data[strlen(cmd->cmd)],
cmd->schema, params)) {
- return -1;
+ return false;
}
}
gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
cmd->handler(params, NULL);
- return 0;
+ return true;
}
- return -1;
+ return false;
}
static void run_cmd_parser(const char *data, const GdbCmdParseEntry *cmd)
@@ -1007,7 +1007,7 @@ static void run_cmd_parser(const char *data, const GdbCmdParseEntry *cmd)
/* In case there was an error during the command parsing we must
* send a NULL packet to indicate the command is not supported */
- if (process_string_cmd(data, cmd, 1)) {
+ if (!process_string_cmd(data, cmd, 1)) {
gdb_put_packet("");
}
}
@@ -1523,9 +1523,9 @@ static void handle_v_commands(GArray *params, void *user_ctx)
return;
}
- if (process_string_cmd(get_param(params, 0)->data,
- gdb_v_commands_table,
- ARRAY_SIZE(gdb_v_commands_table))) {
+ if (!process_string_cmd(get_param(params, 0)->data,
+ gdb_v_commands_table,
+ ARRAY_SIZE(gdb_v_commands_table))) {
gdb_put_packet("");
}
}
@@ -1889,15 +1889,15 @@ 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))) {
return;
}
- if (process_string_cmd(get_param(params, 0)->data,
- gdb_gen_query_table,
- ARRAY_SIZE(gdb_gen_query_table))) {
+ if (!process_string_cmd(get_param(params, 0)->data,
+ gdb_gen_query_table,
+ ARRAY_SIZE(gdb_gen_query_table))) {
gdb_put_packet("");
}
}
@@ -1908,13 +1908,13 @@ 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))) {
return;
}
- if (process_string_cmd(get_param(params, 0)->data,
+ if (!process_string_cmd(get_param(params, 0)->data,
gdb_gen_set_table,
ARRAY_SIZE(gdb_gen_set_table))) {
gdb_put_packet("");
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 1/9] gdbstub: Clean up process_string_cmd
2024-06-17 6:28 ` [PATCH v3 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
@ 2024-06-21 0:49 ` Richard Henderson
0 siblings, 0 replies; 21+ messages in thread
From: Richard Henderson @ 2024-06-21 0:49 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, peter.maydell, alex.bennee
On 6/16/24 23:28, Gustavo Romero wrote:
> Change 'process_string_cmd' to return true on success and false on
> failure, instead of 0 and -1.
>
> Signed-off-by: Gustavo Romero<gustavo.romero@linaro.org>
> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
> ---
> gdbstub/gdbstub.c | 40 ++++++++++++++++++++--------------------
> 1 file changed, 20 insertions(+), 20 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/9] gdbstub: Move GdbCmdParseEntry into a new header file
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
2024-06-17 6:28 ` [PATCH v3 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-17 6:28 ` [PATCH v3 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
` (6 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: gustavo.romero
Move GdbCmdParseEntry and its associated types into a separate header
file to allow the use of GdbCmdParseEntry and other gdbstub command
functions outside of gdbstub.c.
Since GdbCmdParseEntry and get_param are now public, kdoc
GdbCmdParseEntry and rename get_param to gdb_get_cmd_param.
This commit also makes gdb_put_packet public since is used in gdbstub
command handling.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
gdbstub/gdbstub.c | 134 ++++++++++++++-----------------------
gdbstub/internals.h | 22 ------
gdbstub/syscalls.c | 7 +-
gdbstub/system.c | 7 +-
gdbstub/user-target.c | 25 +++----
gdbstub/user.c | 7 +-
include/gdbstub/commands.h | 74 ++++++++++++++++++++
7 files changed, 148 insertions(+), 128 deletions(-)
create mode 100644 include/gdbstub/commands.h
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 37314b92e5..9ff2f4177d 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -30,6 +30,7 @@
#include "qemu/error-report.h"
#include "trace.h"
#include "exec/gdbstub.h"
+#include "gdbstub/commands.h"
#include "gdbstub/syscalls.h"
#ifdef CONFIG_USER_ONLY
#include "accel/tcg/vcpu-state.h"
@@ -920,43 +921,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));
@@ -1023,7 +987,7 @@ static void handle_detach(GArray *params, void *user_ctx)
return;
}
- pid = get_param(params, 0)->val_ul;
+ pid = gdb_get_cmd_param(params, 0)->val_ul;
}
#ifdef CONFIG_USER_ONLY
@@ -1061,13 +1025,13 @@ static void handle_thread_alive(GArray *params, void *user_ctx)
return;
}
- if (get_param(params, 0)->thread_id.kind == GDB_READ_THREAD_ERR) {
+ if (gdb_get_cmd_param(params, 0)->thread_id.kind == GDB_READ_THREAD_ERR) {
gdb_put_packet("E22");
return;
}
- cpu = gdb_get_cpu(get_param(params, 0)->thread_id.pid,
- get_param(params, 0)->thread_id.tid);
+ cpu = gdb_get_cpu(gdb_get_cmd_param(params, 0)->thread_id.pid,
+ gdb_get_cmd_param(params, 0)->thread_id.tid);
if (!cpu) {
gdb_put_packet("E22");
return;
@@ -1079,7 +1043,7 @@ static void handle_thread_alive(GArray *params, void *user_ctx)
static void handle_continue(GArray *params, void *user_ctx)
{
if (params->len) {
- gdb_set_cpu_pc(get_param(params, 0)->val_ull);
+ gdb_set_cpu_pc(gdb_get_cmd_param(params, 0)->val_ull);
}
gdbserver_state.signal = 0;
@@ -1095,7 +1059,7 @@ static void handle_cont_with_sig(GArray *params, void *user_ctx)
* omit the addr parameter
*/
if (params->len) {
- signal = get_param(params, 0)->val_ul;
+ signal = gdb_get_cmd_param(params, 0)->val_ul;
}
gdbserver_state.signal = gdb_signal_to_target(signal);
@@ -1115,18 +1079,18 @@ static void handle_set_thread(GArray *params, void *user_ctx)
return;
}
- if (get_param(params, 1)->thread_id.kind == GDB_READ_THREAD_ERR) {
+ if (gdb_get_cmd_param(params, 1)->thread_id.kind == GDB_READ_THREAD_ERR) {
gdb_put_packet("E22");
return;
}
- if (get_param(params, 1)->thread_id.kind != GDB_ONE_THREAD) {
+ if (gdb_get_cmd_param(params, 1)->thread_id.kind != GDB_ONE_THREAD) {
gdb_put_packet("OK");
return;
}
- pid = get_param(params, 1)->thread_id.pid;
- tid = get_param(params, 1)->thread_id.tid;
+ pid = gdb_get_cmd_param(params, 1)->thread_id.pid;
+ tid = gdb_get_cmd_param(params, 1)->thread_id.tid;
#ifdef CONFIG_USER_ONLY
if (gdb_handle_set_thread_user(pid, tid)) {
return;
@@ -1142,7 +1106,7 @@ static void handle_set_thread(GArray *params, void *user_ctx)
* Note: This command is deprecated and modern gdb's will be using the
* vCont command instead.
*/
- switch (get_param(params, 0)->opcode) {
+ switch (gdb_get_cmd_param(params, 0)->opcode) {
case 'c':
gdbserver_state.c_cpu = cpu;
gdb_put_packet("OK");
@@ -1167,9 +1131,9 @@ static void handle_insert_bp(GArray *params, void *user_ctx)
}
res = gdb_breakpoint_insert(gdbserver_state.c_cpu,
- get_param(params, 0)->val_ul,
- get_param(params, 1)->val_ull,
- get_param(params, 2)->val_ull);
+ gdb_get_cmd_param(params, 0)->val_ul,
+ gdb_get_cmd_param(params, 1)->val_ull,
+ gdb_get_cmd_param(params, 2)->val_ull);
if (res >= 0) {
gdb_put_packet("OK");
return;
@@ -1191,9 +1155,9 @@ static void handle_remove_bp(GArray *params, void *user_ctx)
}
res = gdb_breakpoint_remove(gdbserver_state.c_cpu,
- get_param(params, 0)->val_ul,
- get_param(params, 1)->val_ull,
- get_param(params, 2)->val_ull);
+ gdb_get_cmd_param(params, 0)->val_ul,
+ gdb_get_cmd_param(params, 1)->val_ull,
+ gdb_get_cmd_param(params, 2)->val_ull);
if (res >= 0) {
gdb_put_packet("OK");
return;
@@ -1225,10 +1189,10 @@ static void handle_set_reg(GArray *params, void *user_ctx)
return;
}
- reg_size = strlen(get_param(params, 1)->data) / 2;
- gdb_hextomem(gdbserver_state.mem_buf, get_param(params, 1)->data, reg_size);
+ reg_size = strlen(gdb_get_cmd_param(params, 1)->data) / 2;
+ gdb_hextomem(gdbserver_state.mem_buf, gdb_get_cmd_param(params, 1)->data, reg_size);
gdb_write_register(gdbserver_state.g_cpu, gdbserver_state.mem_buf->data,
- get_param(params, 0)->val_ull);
+ gdb_get_cmd_param(params, 0)->val_ull);
gdb_put_packet("OK");
}
@@ -1243,7 +1207,7 @@ static void handle_get_reg(GArray *params, void *user_ctx)
reg_size = gdb_read_register(gdbserver_state.g_cpu,
gdbserver_state.mem_buf,
- get_param(params, 0)->val_ull);
+ gdb_get_cmd_param(params, 0)->val_ull);
if (!reg_size) {
gdb_put_packet("E14");
return;
@@ -1264,16 +1228,16 @@ static void handle_write_mem(GArray *params, void *user_ctx)
}
/* gdb_hextomem() reads 2*len bytes */
- if (get_param(params, 1)->val_ull >
- strlen(get_param(params, 2)->data) / 2) {
+ if (gdb_get_cmd_param(params, 1)->val_ull >
+ strlen(gdb_get_cmd_param(params, 2)->data) / 2) {
gdb_put_packet("E22");
return;
}
- gdb_hextomem(gdbserver_state.mem_buf, get_param(params, 2)->data,
- get_param(params, 1)->val_ull);
+ gdb_hextomem(gdbserver_state.mem_buf, gdb_get_cmd_param(params, 2)->data,
+ gdb_get_cmd_param(params, 1)->val_ull);
if (gdb_target_memory_rw_debug(gdbserver_state.g_cpu,
- get_param(params, 0)->val_ull,
+ gdb_get_cmd_param(params, 0)->val_ull,
gdbserver_state.mem_buf->data,
gdbserver_state.mem_buf->len, true)) {
gdb_put_packet("E14");
@@ -1291,16 +1255,16 @@ static void handle_read_mem(GArray *params, void *user_ctx)
}
/* gdb_memtohex() doubles the required space */
- if (get_param(params, 1)->val_ull > MAX_PACKET_LENGTH / 2) {
+ if (gdb_get_cmd_param(params, 1)->val_ull > MAX_PACKET_LENGTH / 2) {
gdb_put_packet("E22");
return;
}
g_byte_array_set_size(gdbserver_state.mem_buf,
- get_param(params, 1)->val_ull);
+ gdb_get_cmd_param(params, 1)->val_ull);
if (gdb_target_memory_rw_debug(gdbserver_state.g_cpu,
- get_param(params, 0)->val_ull,
+ gdb_get_cmd_param(params, 0)->val_ull,
gdbserver_state.mem_buf->data,
gdbserver_state.mem_buf->len, false)) {
gdb_put_packet("E14");
@@ -1324,8 +1288,8 @@ static void handle_write_all_regs(GArray *params, void *user_ctx)
}
cpu_synchronize_state(gdbserver_state.g_cpu);
- len = strlen(get_param(params, 0)->data) / 2;
- gdb_hextomem(gdbserver_state.mem_buf, get_param(params, 0)->data, len);
+ len = strlen(gdb_get_cmd_param(params, 0)->data) / 2;
+ gdb_hextomem(gdbserver_state.mem_buf, gdb_get_cmd_param(params, 0)->data, len);
registers = gdbserver_state.mem_buf->data;
for (reg_id = 0;
reg_id < gdbserver_state.g_cpu->gdb_num_g_regs && len > 0;
@@ -1360,7 +1324,7 @@ static void handle_read_all_regs(GArray *params, void *user_ctx)
static void handle_step(GArray *params, void *user_ctx)
{
if (params->len) {
- gdb_set_cpu_pc(get_param(params, 0)->val_ull);
+ gdb_set_cpu_pc(gdb_get_cmd_param(params, 0)->val_ull);
}
cpu_single_step(gdbserver_state.c_cpu, gdbserver_state.sstep_flags);
@@ -1373,7 +1337,7 @@ static void handle_backward(GArray *params, void *user_ctx)
gdb_put_packet("E22");
}
if (params->len == 1) {
- switch (get_param(params, 0)->opcode) {
+ switch (gdb_get_cmd_param(params, 0)->opcode) {
case 's':
if (replay_reverse_step()) {
gdb_continue();
@@ -1408,7 +1372,7 @@ static void handle_v_cont(GArray *params, void *user_ctx)
return;
}
- res = gdb_handle_vcont(get_param(params, 0)->data);
+ res = gdb_handle_vcont(gdb_get_cmd_param(params, 0)->data);
if ((res == -EINVAL) || (res == -ERANGE)) {
gdb_put_packet("E22");
} else if (res) {
@@ -1426,7 +1390,7 @@ static void handle_v_attach(GArray *params, void *user_ctx)
goto cleanup;
}
- process = gdb_get_process(get_param(params, 0)->val_ul);
+ process = gdb_get_process(gdb_get_cmd_param(params, 0)->val_ul);
if (!process) {
goto cleanup;
}
@@ -1523,7 +1487,7 @@ static void handle_v_commands(GArray *params, void *user_ctx)
return;
}
- if (!process_string_cmd(get_param(params, 0)->data,
+ if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
gdb_v_commands_table,
ARRAY_SIZE(gdb_v_commands_table))) {
gdb_put_packet("");
@@ -1555,7 +1519,7 @@ static void handle_set_qemu_sstep(GArray *params, void *user_ctx)
return;
}
- new_sstep_flags = get_param(params, 0)->val_ul;
+ new_sstep_flags = gdb_get_cmd_param(params, 0)->val_ul;
if (new_sstep_flags & ~gdbserver_state.supported_sstep_flags) {
gdb_put_packet("E22");
@@ -1615,13 +1579,13 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
CPUState *cpu;
if (!params->len ||
- get_param(params, 0)->thread_id.kind == GDB_READ_THREAD_ERR) {
+ gdb_get_cmd_param(params, 0)->thread_id.kind == GDB_READ_THREAD_ERR) {
gdb_put_packet("E22");
return;
}
- cpu = gdb_get_cpu(get_param(params, 0)->thread_id.pid,
- get_param(params, 0)->thread_id.tid);
+ cpu = gdb_get_cpu(gdb_get_cmd_param(params, 0)->thread_id.pid,
+ gdb_get_cmd_param(params, 0)->thread_id.tid);
if (!cpu) {
return;
}
@@ -1673,7 +1637,7 @@ static void handle_query_supported(GArray *params, void *user_ctx)
#endif
if (params->len) {
- const char *gdb_supported = get_param(params, 0)->data;
+ const char *gdb_supported = gdb_get_cmd_param(params, 0)->data;
if (strstr(gdb_supported, "multiprocess+")) {
gdbserver_state.multiprocess = true;
@@ -1707,15 +1671,15 @@ static void handle_query_xfer_features(GArray *params, void *user_ctx)
return;
}
- p = get_param(params, 0)->data;
+ p = gdb_get_cmd_param(params, 0)->data;
xml = get_feature_xml(p, &p, process);
if (!xml) {
gdb_put_packet("E00");
return;
}
- addr = get_param(params, 1)->val_ul;
- len = get_param(params, 2)->val_ul;
+ addr = gdb_get_cmd_param(params, 1)->val_ul;
+ len = gdb_get_cmd_param(params, 2)->val_ul;
total_len = strlen(xml);
if (addr > total_len) {
gdb_put_packet("E00");
@@ -1889,13 +1853,13 @@ static void handle_gen_query(GArray *params, void *user_ctx)
return;
}
- if (process_string_cmd(get_param(params, 0)->data,
+ if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
gdb_gen_query_set_common_table,
ARRAY_SIZE(gdb_gen_query_set_common_table))) {
return;
}
- if (!process_string_cmd(get_param(params, 0)->data,
+ if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
gdb_gen_query_table,
ARRAY_SIZE(gdb_gen_query_table))) {
gdb_put_packet("");
@@ -1908,13 +1872,13 @@ static void handle_gen_set(GArray *params, void *user_ctx)
return;
}
- if (process_string_cmd(get_param(params, 0)->data,
+ if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
gdb_gen_query_set_common_table,
ARRAY_SIZE(gdb_gen_query_set_common_table))) {
return;
}
- if (!process_string_cmd(get_param(params, 0)->data,
+ if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
gdb_gen_set_table,
ARRAY_SIZE(gdb_gen_set_table))) {
gdb_put_packet("");
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..4e1295b782 100644
--- a/gdbstub/syscalls.c
+++ b/gdbstub/syscalls.c
@@ -16,6 +16,7 @@
#include "sysemu/runstate.h"
#include "gdbstub/user.h"
#include "gdbstub/syscalls.h"
+#include "gdbstub/commands.h"
#include "trace.h"
#include "internals.h"
@@ -154,9 +155,9 @@ void gdb_handle_file_io(GArray *params, void *user_ctx)
uint64_t ret;
int err;
- ret = get_param(params, 0)->val_ull;
+ ret = gdb_get_cmd_param(params, 0)->val_ull;
if (params->len >= 2) {
- err = get_param(params, 1)->val_ull;
+ err = gdb_get_cmd_param(params, 1)->val_ull;
} else {
err = 0;
}
@@ -196,7 +197,7 @@ void gdb_handle_file_io(GArray *params, void *user_ctx)
gdbserver_syscall_state.current_syscall_cb = NULL;
}
- if (params->len >= 3 && get_param(params, 2)->opcode == (uint8_t)'C') {
+ if (params->len >= 3 && gdb_get_cmd_param(params, 2)->opcode == (uint8_t)'C') {
gdb_put_packet("T02");
return;
}
diff --git a/gdbstub/system.c b/gdbstub/system.c
index d235403855..1ad87fe7fd 100644
--- a/gdbstub/system.c
+++ b/gdbstub/system.c
@@ -16,6 +16,7 @@
#include "qemu/cutils.h"
#include "exec/gdbstub.h"
#include "gdbstub/syscalls.h"
+#include "gdbstub/commands.h"
#include "exec/hwaddr.h"
#include "exec/tb-flush.h"
#include "sysemu/cpus.h"
@@ -501,7 +502,7 @@ void gdb_handle_set_qemu_phy_mem_mode(GArray *params, void *ctx)
return;
}
- if (!get_param(params, 0)->val_ul) {
+ if (!gdb_get_cmd_param(params, 0)->val_ul) {
phy_memory_mode = 0;
} else {
phy_memory_mode = 1;
@@ -519,7 +520,7 @@ void gdb_handle_query_rcmd(GArray *params, void *ctx)
return;
}
- len = strlen(get_param(params, 0)->data);
+ len = strlen(gdb_get_cmd_param(params, 0)->data);
if (len % 2) {
gdb_put_packet("E01");
return;
@@ -527,7 +528,7 @@ void gdb_handle_query_rcmd(GArray *params, void *ctx)
g_assert(gdbserver_state.mem_buf->len == 0);
len = len / 2;
- gdb_hextomem(gdbserver_state.mem_buf, get_param(params, 0)->data, len);
+ gdb_hextomem(gdbserver_state.mem_buf, gdb_get_cmd_param(params, 0)->data, len);
g_byte_array_append(gdbserver_state.mem_buf, &zero, 1);
qemu_chr_be_write(gdbserver_system_state.mon_chr,
gdbserver_state.mem_buf->data,
diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
index a9c6c64512..b5e01fd8b0 100644
--- a/gdbstub/user-target.c
+++ b/gdbstub/user-target.c
@@ -9,6 +9,7 @@
#include "qemu/osdep.h"
#include "exec/gdbstub.h"
+#include "gdbstub/commands.h"
#include "qemu.h"
#include "internals.h"
#ifdef CONFIG_LINUX
@@ -250,8 +251,8 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
return;
}
- offset = get_param(params, 0)->val_ul;
- len = get_param(params, 1)->val_ul;
+ offset = gdb_get_cmd_param(params, 0)->val_ul;
+ len = gdb_get_cmd_param(params, 1)->val_ul;
ts = get_task_state(gdbserver_state.c_cpu);
saved_auxv = ts->info->saved_auxv;
auxv_len = ts->info->auxv_len;
@@ -288,7 +289,7 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
static const char *get_filename_param(GArray *params, int i)
{
- const char *hex_filename = get_param(params, i)->data;
+ const char *hex_filename = gdb_get_cmd_param(params, i)->data;
gdb_hextomem(gdbserver_state.mem_buf, hex_filename,
strlen(hex_filename) / 2);
g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)"", 1);
@@ -306,8 +307,8 @@ static void hostio_reply_with_data(const void *buf, size_t n)
void gdb_handle_v_file_open(GArray *params, void *user_ctx)
{
const char *filename = get_filename_param(params, 0);
- uint64_t flags = get_param(params, 1)->val_ull;
- uint64_t mode = get_param(params, 2)->val_ull;
+ uint64_t flags = gdb_get_cmd_param(params, 1)->val_ull;
+ uint64_t mode = gdb_get_cmd_param(params, 2)->val_ull;
#ifdef CONFIG_LINUX
int fd = do_guest_openat(cpu_env(gdbserver_state.g_cpu), 0, filename,
@@ -325,7 +326,7 @@ void gdb_handle_v_file_open(GArray *params, void *user_ctx)
void gdb_handle_v_file_close(GArray *params, void *user_ctx)
{
- int fd = get_param(params, 0)->val_ul;
+ int fd = gdb_get_cmd_param(params, 0)->val_ul;
if (close(fd) == -1) {
g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
@@ -338,9 +339,9 @@ void gdb_handle_v_file_close(GArray *params, void *user_ctx)
void gdb_handle_v_file_pread(GArray *params, void *user_ctx)
{
- int fd = get_param(params, 0)->val_ul;
- size_t count = get_param(params, 1)->val_ull;
- off_t offset = get_param(params, 2)->val_ull;
+ int fd = gdb_get_cmd_param(params, 0)->val_ul;
+ size_t count = gdb_get_cmd_param(params, 1)->val_ull;
+ off_t offset = gdb_get_cmd_param(params, 2)->val_ull;
size_t bufsiz = MIN(count, BUFSIZ);
g_autofree char *buf = g_try_malloc(bufsiz);
@@ -383,9 +384,9 @@ void gdb_handle_v_file_readlink(GArray *params, void *user_ctx)
void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
{
- uint32_t pid = get_param(params, 0)->val_ul;
- uint32_t offset = get_param(params, 1)->val_ul;
- uint32_t length = get_param(params, 2)->val_ul;
+ uint32_t pid = gdb_get_cmd_param(params, 0)->val_ul;
+ uint32_t offset = gdb_get_cmd_param(params, 1)->val_ul;
+ uint32_t length = gdb_get_cmd_param(params, 2)->val_ul;
GDBProcess *process = gdb_get_process(pid);
if (!process) {
diff --git a/gdbstub/user.c b/gdbstub/user.c
index e34b58b407..b36033bc7a 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -16,6 +16,7 @@
#include "exec/hwaddr.h"
#include "exec/tb-flush.h"
#include "exec/gdbstub.h"
+#include "gdbstub/commands.h"
#include "gdbstub/syscalls.h"
#include "gdbstub/user.h"
#include "gdbstub/enums.h"
@@ -793,7 +794,7 @@ void gdb_syscall_return(CPUState *cs, int num)
void gdb_handle_set_catch_syscalls(GArray *params, void *user_ctx)
{
- const char *param = get_param(params, 0)->data;
+ const char *param = gdb_get_cmd_param(params, 0)->data;
GDBSyscallsMask catch_syscalls_mask;
bool catch_all_syscalls;
unsigned int num;
@@ -858,8 +859,8 @@ void gdb_handle_query_xfer_siginfo(GArray *params, void *user_ctx)
unsigned long offset, len;
uint8_t *siginfo_offset;
- offset = get_param(params, 0)->val_ul;
- len = get_param(params, 1)->val_ul;
+ offset = gdb_get_cmd_param(params, 0)->val_ul;
+ len = gdb_get_cmd_param(params, 1)->val_ul;
if (offset + len > gdbserver_user_state.siginfo_len) {
/* Invalid offset and/or requested length. */
diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
new file mode 100644
index 0000000000..dd45c38472
--- /dev/null
+++ b/include/gdbstub/commands.h
@@ -0,0 +1,74 @@
+#ifndef GDBSTUB_COMMANDS_H
+#define GDBSTUB
+
+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 gdb_get_cmd_param(p, i) (&g_array_index(p, GdbCmdVariant, i))
+
+/**
+ * typedef GdbCmdParseEntry - gdb command parser
+ *
+ * This structure keeps the information necessary to match a gdb command,
+ * parse it (extract its parameters), and select the correct handler for it.
+ *
+ * @cmd: The command to be matched
+ * @cmd_startswith: If true, @cmd is compared using startswith
+ * @schema: Each schema for the command 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
+ *
+ * @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.
+ */
+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))
+
+/**
+ * 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_COMMANDS_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 3/9] gdbstub: Add support for target-specific stubs
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
2024-06-17 6:28 ` [PATCH v3 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
2024-06-17 6:28 ` [PATCH v3 2/9] gdbstub: Move GdbCmdParseEntry into a new header file Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-21 4:28 ` Richard Henderson
2024-06-21 8:11 ` Alex Bennée
2024-06-17 6:28 ` [PATCH v3 4/9] target/arm: Fix exception case in allocation_tag_mem_probe Gustavo Romero
` (5 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: 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 gdb_extend_qsupported_features,
gdb_extend_query_table, and gdb_extend_set_table functions as interfaces
to extend the qSupported string, the query handler table, and the set
handler table, allowing target-specific stub implementations.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
gdbstub/gdbstub.c | 59 ++++++++++++++++++++++++++++++++++----
include/gdbstub/commands.h | 22 ++++++++++++++
2 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 9ff2f4177d..e69cc5131e 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1609,6 +1609,12 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
gdb_put_strbuf();
}
+static char *extended_qsupported_features;
+void gdb_extend_qsupported_features(char *qsupported_features)
+{
+ extended_qsupported_features = qsupported_features;
+}
+
static void handle_query_supported(GArray *params, void *user_ctx)
{
CPUClass *cc;
@@ -1648,6 +1654,11 @@ static void handle_query_supported(GArray *params, void *user_ctx)
}
g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
+
+ if (extended_qsupported_features) {
+ g_string_append(gdbserver_state.str_buf, extended_qsupported_features);
+ }
+
gdb_put_strbuf();
}
@@ -1729,6 +1740,14 @@ static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
},
};
+static GdbCmdParseEntry *extended_query_table;
+static int extended_query_table_size;
+void gdb_extend_query_table(GdbCmdParseEntry *table, int size)
+{
+ extended_query_table = table;
+ extended_query_table_size = size;
+}
+
static const GdbCmdParseEntry gdb_gen_query_table[] = {
{
.handler = handle_query_curr_tid,
@@ -1821,6 +1840,14 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
#endif
};
+static GdbCmdParseEntry *extended_set_table;
+static int extended_set_table_size;
+void gdb_extend_set_table(GdbCmdParseEntry *table, int size)
+{
+ extended_set_table = table;
+ extended_set_table_size = size;
+}
+
static const GdbCmdParseEntry gdb_gen_set_table[] = {
/* Order is important if has same prefix */
{
@@ -1859,11 +1886,21 @@ static void handle_gen_query(GArray *params, void *user_ctx)
return;
}
- if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
- gdb_gen_query_table,
- ARRAY_SIZE(gdb_gen_query_table))) {
- gdb_put_packet("");
+ if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
+ gdb_gen_query_table,
+ ARRAY_SIZE(gdb_gen_query_table))) {
+ return;
+ }
+
+ if (extended_query_table &&
+ process_string_cmd(gdb_get_cmd_param(params, 0)->data,
+ extended_query_table,
+ extended_query_table_size)) {
+ return;
}
+
+ /* Can't handle query, return Empty response. */
+ gdb_put_packet("");
}
static void handle_gen_set(GArray *params, void *user_ctx)
@@ -1878,11 +1915,21 @@ static void handle_gen_set(GArray *params, void *user_ctx)
return;
}
- if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
+ if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
gdb_gen_set_table,
ARRAY_SIZE(gdb_gen_set_table))) {
- gdb_put_packet("");
+ return;
}
+
+ if (extended_set_table &&
+ process_string_cmd(gdb_get_cmd_param(params, 0)->data,
+ extended_set_table,
+ extended_set_table_size)) {
+ return;
+ }
+
+ /* Can't handle set, return Empty response. */
+ gdb_put_packet("");
}
static void handle_target_halt(GArray *params, void *user_ctx)
diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
index dd45c38472..2204c3ddbe 100644
--- a/include/gdbstub/commands.h
+++ b/include/gdbstub/commands.h
@@ -71,4 +71,26 @@ typedef struct GdbCmdParseEntry {
*/
int gdb_put_packet(const char *buf);
+/**
+ * gdb_extend_query_table() - Extend query table.
+ * @table: The table with the additional query packet handlers.
+ * @size: The number of handlers to be added.
+ */
+void gdb_extend_query_table(GdbCmdParseEntry *table, int size);
+
+/**
+ * gdb_extend_set_table() - Extend set table.
+ * @table: The table with the additional set packet handlers.
+ * @size: The number of handlers to be added.
+ */
+void gdb_extend_set_table(GdbCmdParseEntry *table, int size);
+
+/**
+ * gdb_extend_qsupported_features() - Extend the qSupported features string.
+ * @qsupported_features: The additional qSupported feature(s) string. The string
+ * should start with a semicolon and, if there are more than one feature, the
+ * features should be separate by a semiocolon.
+ */
+void gdb_extend_qsupported_features(char *qsupported_features);
+
#endif /* GDBSTUB_COMMANDS_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 3/9] gdbstub: Add support for target-specific stubs
2024-06-17 6:28 ` [PATCH v3 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
@ 2024-06-21 4:28 ` Richard Henderson
2024-06-21 9:01 ` Alex Bennée
2024-06-21 8:11 ` Alex Bennée
1 sibling, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2024-06-21 4:28 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, peter.maydell, alex.bennee
On 6/16/24 23:28, Gustavo Romero wrote:
> +static char *extended_qsupported_features;
> +void gdb_extend_qsupported_features(char *qsupported_features)
> +{
> + extended_qsupported_features = qsupported_features;
> +}
Assert these functions aren't called twice.
That should be good enough until we need something more complicated.
Speaking of more complicated... do we have a plan for gdb when we get to the point where
the board contains multiple cpu types? Not yet, of course, but we're working on it...
r~
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 3/9] gdbstub: Add support for target-specific stubs
2024-06-21 4:28 ` Richard Henderson
@ 2024-06-21 9:01 ` Alex Bennée
0 siblings, 0 replies; 21+ messages in thread
From: Alex Bennée @ 2024-06-21 9:01 UTC (permalink / raw)
To: Richard Henderson; +Cc: Gustavo Romero, qemu-devel, philmd, peter.maydell
Richard Henderson <richard.henderson@linaro.org> writes:
> On 6/16/24 23:28, Gustavo Romero wrote:
>> +static char *extended_qsupported_features;
>> +void gdb_extend_qsupported_features(char *qsupported_features)
>> +{
>> + extended_qsupported_features = qsupported_features;
>> +}
>
> Assert these functions aren't called twice.
> That should be good enough until we need something more complicated.
They are called more than once (because we go through the vcpu realize
step for each vcpu). But we should assert that if set we are not
changing the feature set.
> Speaking of more complicated... do we have a plan for gdb when we get
> to the point where the board contains multiple cpu types? Not yet, of
> course, but we're working on it...
I think gdb needs to be a little smarter here.
>
>
> r~
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/9] gdbstub: Add support for target-specific stubs
2024-06-17 6:28 ` [PATCH v3 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
2024-06-21 4:28 ` Richard Henderson
@ 2024-06-21 8:11 ` Alex Bennée
2024-06-24 5:35 ` Gustavo Romero
1 sibling, 1 reply; 21+ messages in thread
From: Alex Bennée @ 2024-06-21 8:11 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
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 gdb_extend_qsupported_features,
> gdb_extend_query_table, and gdb_extend_set_table functions as interfaces
> to extend the qSupported string, the query handler table, and the set
> handler table, allowing target-specific stub implementations.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
> gdbstub/gdbstub.c | 59 ++++++++++++++++++++++++++++++++++----
> include/gdbstub/commands.h | 22 ++++++++++++++
> 2 files changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 9ff2f4177d..e69cc5131e 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1609,6 +1609,12 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
> gdb_put_strbuf();
> }
>
> +static char *extended_qsupported_features;
> +void gdb_extend_qsupported_features(char *qsupported_features)
> +{
/*
* We don't support different sets of CPU gdb features on different
* CPUs yet so assert things are the same.
*/
g_assert(!extended_qsupported_features ||
g_strcmp0(extended_qsupported_features, qsupported_features) == 0)
> + extended_qsupported_features = qsupported_features;
> +}
> +
> static void handle_query_supported(GArray *params, void *user_ctx)
> {
> CPUClass *cc;
> @@ -1648,6 +1654,11 @@ static void handle_query_supported(GArray *params, void *user_ctx)
> }
>
> g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
> +
> + if (extended_qsupported_features) {
> + g_string_append(gdbserver_state.str_buf, extended_qsupported_features);
> + }
> +
> gdb_put_strbuf();
> }
>
> @@ -1729,6 +1740,14 @@ static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
> },
> };
>
> +static GdbCmdParseEntry *extended_query_table;
> +static int extended_query_table_size;
> +void gdb_extend_query_table(GdbCmdParseEntry *table, int size)
> +{
> + extended_query_table = table;
> + extended_query_table_size = size;
Similar assert here.
> +}
> +
> static const GdbCmdParseEntry gdb_gen_query_table[] = {
> {
> .handler = handle_query_curr_tid,
> @@ -1821,6 +1840,14 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
> #endif
> };
>
> +static GdbCmdParseEntry *extended_set_table;
> +static int extended_set_table_size;
> +void gdb_extend_set_table(GdbCmdParseEntry *table, int size)
> +{
ditto
> + extended_set_table = table;
> + extended_set_table_size = size;
> +}
> +
> static const GdbCmdParseEntry gdb_gen_set_table[] = {
> /* Order is important if has same prefix */
> {
> @@ -1859,11 +1886,21 @@ static void handle_gen_query(GArray *params, void *user_ctx)
> return;
> }
>
> - if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
> - gdb_gen_query_table,
> - ARRAY_SIZE(gdb_gen_query_table))) {
> - gdb_put_packet("");
> + if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
> + gdb_gen_query_table,
> + ARRAY_SIZE(gdb_gen_query_table))) {
> + return;
> + }
> +
> + if (extended_query_table &&
> + process_string_cmd(gdb_get_cmd_param(params, 0)->data,
> + extended_query_table,
> + extended_query_table_size)) {
> + return;
> }
> +
> + /* Can't handle query, return Empty response. */
> + gdb_put_packet("");
> }
>
> static void handle_gen_set(GArray *params, void *user_ctx)
> @@ -1878,11 +1915,21 @@ static void handle_gen_set(GArray *params, void *user_ctx)
> return;
> }
>
> - if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
> + if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
> gdb_gen_set_table,
> ARRAY_SIZE(gdb_gen_set_table))) {
> - gdb_put_packet("");
> + return;
> }
> +
> + if (extended_set_table &&
> + process_string_cmd(gdb_get_cmd_param(params, 0)->data,
> + extended_set_table,
> + extended_set_table_size)) {
> + return;
> + }
> +
> + /* Can't handle set, return Empty response. */
> + gdb_put_packet("");
> }
>
> static void handle_target_halt(GArray *params, void *user_ctx)
> diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
> index dd45c38472..2204c3ddbe 100644
> --- a/include/gdbstub/commands.h
> +++ b/include/gdbstub/commands.h
> @@ -71,4 +71,26 @@ typedef struct GdbCmdParseEntry {
> */
> int gdb_put_packet(const char *buf);
>
> +/**
> + * gdb_extend_query_table() - Extend query table.
> + * @table: The table with the additional query packet handlers.
> + * @size: The number of handlers to be added.
> + */
> +void gdb_extend_query_table(GdbCmdParseEntry *table, int size);
> +
> +/**
> + * gdb_extend_set_table() - Extend set table.
> + * @table: The table with the additional set packet handlers.
> + * @size: The number of handlers to be added.
> + */
> +void gdb_extend_set_table(GdbCmdParseEntry *table, int size);
> +
> +/**
> + * gdb_extend_qsupported_features() - Extend the qSupported features string.
> + * @qsupported_features: The additional qSupported feature(s) string. The string
> + * should start with a semicolon and, if there are more than one feature, the
> + * features should be separate by a semiocolon.
> + */
> +void gdb_extend_qsupported_features(char *qsupported_features);
> +
> #endif /* GDBSTUB_COMMANDS_H */
Otherwise:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 3/9] gdbstub: Add support for target-specific stubs
2024-06-21 8:11 ` Alex Bennée
@ 2024-06-24 5:35 ` Gustavo Romero
0 siblings, 0 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-24 5:35 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Hi Alex,
On 6/21/24 5:11 AM, Alex Bennée wrote:
> 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 gdb_extend_qsupported_features,
>> gdb_extend_query_table, and gdb_extend_set_table functions as interfaces
>> to extend the qSupported string, the query handler table, and the set
>> handler table, allowing target-specific stub implementations.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>> gdbstub/gdbstub.c | 59 ++++++++++++++++++++++++++++++++++----
>> include/gdbstub/commands.h | 22 ++++++++++++++
>> 2 files changed, 75 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index 9ff2f4177d..e69cc5131e 100644
>> --- a/gdbstub/gdbstub.c
>> +++ b/gdbstub/gdbstub.c
>> @@ -1609,6 +1609,12 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx)
>> gdb_put_strbuf();
>> }
>>
>> +static char *extended_qsupported_features;
>> +void gdb_extend_qsupported_features(char *qsupported_features)
>> +{
>
> /*
> * We don't support different sets of CPU gdb features on different
> * CPUs yet so assert things are the same.
> */
>
> g_assert(!extended_qsupported_features ||
> g_strcmp0(extended_qsupported_features, qsupported_features) == 0)
>
Done in v4. Thanks.
Cheers,
Gustavo
>> + extended_qsupported_features = qsupported_features;
>> +}
>> +
>> static void handle_query_supported(GArray *params, void *user_ctx)
>> {
>> CPUClass *cc;
>> @@ -1648,6 +1654,11 @@ static void handle_query_supported(GArray *params, void *user_ctx)
>> }
>>
>> g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
>> +
>> + if (extended_qsupported_features) {
>> + g_string_append(gdbserver_state.str_buf, extended_qsupported_features);
>> + }
>> +
>> gdb_put_strbuf();
>> }
>>
>> @@ -1729,6 +1740,14 @@ static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
>> },
>> };
>>
>> +static GdbCmdParseEntry *extended_query_table;
>> +static int extended_query_table_size;
>> +void gdb_extend_query_table(GdbCmdParseEntry *table, int size)
>> +{
>> + extended_query_table = table;
>> + extended_query_table_size = size;
>
> Similar assert here.
>
>> +}
>> +
>> static const GdbCmdParseEntry gdb_gen_query_table[] = {
>> {
>> .handler = handle_query_curr_tid,
>> @@ -1821,6 +1840,14 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>> #endif
>> };
>>
>> +static GdbCmdParseEntry *extended_set_table;
>> +static int extended_set_table_size;
>> +void gdb_extend_set_table(GdbCmdParseEntry *table, int size)
>> +{
>
> ditto
>
>> + extended_set_table = table;
>> + extended_set_table_size = size;
>> +}
>> +
>> static const GdbCmdParseEntry gdb_gen_set_table[] = {
>> /* Order is important if has same prefix */
>> {
>> @@ -1859,11 +1886,21 @@ static void handle_gen_query(GArray *params, void *user_ctx)
>> return;
>> }
>>
>> - if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>> - gdb_gen_query_table,
>> - ARRAY_SIZE(gdb_gen_query_table))) {
>> - gdb_put_packet("");
>> + if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>> + gdb_gen_query_table,
>> + ARRAY_SIZE(gdb_gen_query_table))) {
>> + return;
>> + }
>> +
>> + if (extended_query_table &&
>> + process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>> + extended_query_table,
>> + extended_query_table_size)) {
>> + return;
>> }
>> +
>> + /* Can't handle query, return Empty response. */
>> + gdb_put_packet("");
>> }
>>
>> static void handle_gen_set(GArray *params, void *user_ctx)
>> @@ -1878,11 +1915,21 @@ static void handle_gen_set(GArray *params, void *user_ctx)
>> return;
>> }
>>
>> - if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>> + if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>> gdb_gen_set_table,
>> ARRAY_SIZE(gdb_gen_set_table))) {
>> - gdb_put_packet("");
>> + return;
>> }
>> +
>> + if (extended_set_table &&
>> + process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>> + extended_set_table,
>> + extended_set_table_size)) {
>> + return;
>> + }
>> +
>> + /* Can't handle set, return Empty response. */
>> + gdb_put_packet("");
>> }
>>
>> static void handle_target_halt(GArray *params, void *user_ctx)
>> diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
>> index dd45c38472..2204c3ddbe 100644
>> --- a/include/gdbstub/commands.h
>> +++ b/include/gdbstub/commands.h
>> @@ -71,4 +71,26 @@ typedef struct GdbCmdParseEntry {
>> */
>> int gdb_put_packet(const char *buf);
>>
>> +/**
>> + * gdb_extend_query_table() - Extend query table.
>> + * @table: The table with the additional query packet handlers.
>> + * @size: The number of handlers to be added.
>> + */
>> +void gdb_extend_query_table(GdbCmdParseEntry *table, int size);
>> +
>> +/**
>> + * gdb_extend_set_table() - Extend set table.
>> + * @table: The table with the additional set packet handlers.
>> + * @size: The number of handlers to be added.
>> + */
>> +void gdb_extend_set_table(GdbCmdParseEntry *table, int size);
>> +
>> +/**
>> + * gdb_extend_qsupported_features() - Extend the qSupported features string.
>> + * @qsupported_features: The additional qSupported feature(s) string. The string
>> + * should start with a semicolon and, if there are more than one feature, the
>> + * features should be separate by a semiocolon.
>> + */
>> +void gdb_extend_qsupported_features(char *qsupported_features);
>> +
>> #endif /* GDBSTUB_COMMANDS_H */
>
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 4/9] target/arm: Fix exception case in allocation_tag_mem_probe
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (2 preceding siblings ...)
2024-06-17 6:28 ` [PATCH v3 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-21 4:28 ` Richard Henderson
2024-06-17 6:28 ` [PATCH v3 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: gustavo.romero
If page in 'ptr_access' is inaccessible and probe is 'true'
allocation_tag_mem_probe should not throw an exception, but currently it
does, so fix it.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
target/arm/tcg/mte_helper.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
index 037ac6dd60..a50d576294 100644
--- a/target/arm/tcg/mte_helper.c
+++ b/target/arm/tcg/mte_helper.c
@@ -96,6 +96,9 @@ static uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
assert(!(probe && ra));
if (!(flags & (ptr_access == MMU_DATA_STORE ? PAGE_WRITE_ORG : PAGE_READ))) {
+ if (probe) {
+ return NULL;
+ }
cpu_loop_exit_sigsegv(env_cpu(env), ptr, ptr_access,
!(flags & PAGE_VALID), ra);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 5/9] target/arm: Make some MTE helpers widely available
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (3 preceding siblings ...)
2024-06-17 6:28 ` [PATCH v3 4/9] target/arm: Fix exception case in allocation_tag_mem_probe Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-21 4:31 ` Richard Henderson
2024-06-17 6:28 ` [PATCH v3 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: gustavo.romero
Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
available to other subsystems.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
target/arm/tcg/mte_helper.c | 45 +++++---------------------
target/arm/tcg/mte_helper.h | 63 +++++++++++++++++++++++++++++++++++++
2 files changed, 70 insertions(+), 38 deletions(-)
create mode 100644 target/arm/tcg/mte_helper.h
diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
index a50d576294..e3d505e96a 100644
--- a/target/arm/tcg/mte_helper.c
+++ b/target/arm/tcg/mte_helper.c
@@ -29,6 +29,7 @@
#include "hw/core/tcg-cpu-ops.h"
#include "qapi/error.h"
#include "qemu/guest-random.h"
+#include "mte_helper.h"
static int choose_nonexcluded_tag(int tag, int offset, uint16_t exclude)
@@ -50,42 +51,10 @@ static int choose_nonexcluded_tag(int tag, int offset, uint16_t exclude)
return tag;
}
-/**
- * allocation_tag_mem_probe:
- * @env: the cpu environment
- * @ptr_mmu_idx: the addressing regime to use for the virtual address
- * @ptr: the virtual address for which to look up tag memory
- * @ptr_access: the access to use for the virtual address
- * @ptr_size: the number of bytes in the normal memory access
- * @tag_access: the access to use for the tag memory
- * @probe: true to merely probe, never taking an exception
- * @ra: the return address for exception handling
- *
- * Our tag memory is formatted as a sequence of little-endian nibbles.
- * That is, the byte at (addr >> (LOG2_TAG_GRANULE + 1)) contains two
- * tags, with the tag at [3:0] for the lower addr and the tag at [7:4]
- * for the higher addr.
- *
- * Here, resolve the physical address from the virtual address, and return
- * a pointer to the corresponding tag byte.
- *
- * If there is no tag storage corresponding to @ptr, return NULL.
- *
- * If the page is inaccessible for @ptr_access, or has a watchpoint, there are
- * three options:
- * (1) probe = true, ra = 0 : pure probe -- we return NULL if the page is not
- * accessible, and do not take watchpoint traps. The calling code must
- * handle those cases in the right priority compared to MTE traps.
- * (2) probe = false, ra = 0 : probe, no fault expected -- the caller guarantees
- * that the page is going to be accessible. We will take watchpoint traps.
- * (3) probe = false, ra != 0 : non-probe -- we will take both memory access
- * traps and watchpoint traps.
- * (probe = true, ra != 0 is invalid and will assert.)
- */
-static uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
- uint64_t ptr, MMUAccessType ptr_access,
- int ptr_size, MMUAccessType tag_access,
- bool probe, uintptr_t ra)
+uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
+ uint64_t ptr, MMUAccessType ptr_access,
+ int ptr_size, MMUAccessType tag_access,
+ bool probe, uintptr_t ra)
{
#ifdef CONFIG_USER_ONLY
uint64_t clean_ptr = useronly_clean_ptr(ptr);
@@ -287,7 +256,7 @@ uint64_t HELPER(addsubg)(CPUARMState *env, uint64_t ptr,
return address_with_allocation_tag(ptr + offset, rtag);
}
-static int load_tag1(uint64_t ptr, uint8_t *mem)
+inline int load_tag1(uint64_t ptr, uint8_t *mem)
{
int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
return extract32(*mem, ofs, 4);
@@ -321,7 +290,7 @@ static void check_tag_aligned(CPUARMState *env, uint64_t ptr, uintptr_t ra)
}
/* For use in a non-parallel context, store to the given nibble. */
-static void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
+inline void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
{
int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
*mem = deposit32(*mem, ofs, 4, tag);
diff --git a/target/arm/tcg/mte_helper.h b/target/arm/tcg/mte_helper.h
new file mode 100644
index 0000000000..69ad8457f8
--- /dev/null
+++ b/target/arm/tcg/mte_helper.h
@@ -0,0 +1,63 @@
+/*
+ * ARM MemTag operation helpers.
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#ifndef TARGET_ARM_MTE_H
+#define TARGET_ARM_MTE_H
+
+/**
+ * allocation_tag_mem_probe:
+ * @env: the cpu environment
+ * @ptr_mmu_idx: the addressing regime to use for the virtual address
+ * @ptr: the virtual address for which to look up tag memory
+ * @ptr_access: the access to use for the virtual address
+ * @ptr_size: the number of bytes in the normal memory access
+ * @tag_access: the access to use for the tag memory
+ * @probe: true to merely probe, never taking an exception
+ * @ra: the return address for exception handling
+ *
+ * Our tag memory is formatted as a sequence of little-endian nibbles.
+ * That is, the byte at (addr >> (LOG2_TAG_GRANULE + 1)) contains two
+ * tags, with the tag at [3:0] for the lower addr and the tag at [7:4]
+ * for the higher addr.
+ *
+ * Here, resolve the physical address from the virtual address, and return
+ * a pointer to the corresponding tag byte.
+ *
+ * If there is no tag storage corresponding to @ptr, return NULL.
+ *
+ * If the page is inaccessible for @ptr_access, or has a watchpoint, there are
+ * three options:
+ * (1) probe = true, ra = 0 : pure probe -- we return NULL if the page is not
+ * accessible, and do not take watchpoint traps. The calling code must
+ * handle those cases in the right priority compared to MTE traps.
+ * (2) probe = false, ra = 0 : probe, no fault expected -- the caller guarantees
+ * that the page is going to be accessible. We will take watchpoint traps.
+ * (3) probe = false, ra != 0 : non-probe -- we will take both memory access
+ * traps and watchpoint traps.
+ * (probe = true, ra != 0 is invalid and will assert.)
+ */
+uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
+ uint64_t ptr, MMUAccessType ptr_access,
+ int ptr_size, MMUAccessType tag_access,
+ bool probe, uintptr_t ra);
+/**
+ * load_tag1 - Load 1 tag (nibble) from byte
+ * @ptr: The tagged address
+ * @mem: The tag address (packed, 2 tags in byte)
+ */
+int load_tag1(uint64_t ptr, uint8_t *mem);
+
+/**
+ * store_tag1 - Store 1 tag (nibble) into byte
+ * @ptr: The tagged address
+ * @mem: The tag address (packed, 2 tags in byte)
+ * @tag: The tag to be stored in the nibble
+ */
+void store_tag1(uint64_t ptr, uint8_t *mem, int tag);
+
+#endif /* TARGET_ARM_MTE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/9] target/arm: Make some MTE helpers widely available
2024-06-17 6:28 ` [PATCH v3 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
@ 2024-06-21 4:31 ` Richard Henderson
2024-06-24 5:36 ` Gustavo Romero
0 siblings, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2024-06-21 4:31 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, peter.maydell, alex.bennee
On 6/16/24 23:28, Gustavo Romero wrote:
> @@ -287,7 +256,7 @@ uint64_t HELPER(addsubg)(CPUARMState *env, uint64_t ptr,
> return address_with_allocation_tag(ptr + offset, rtag);
> }
>
> -static int load_tag1(uint64_t ptr, uint8_t *mem)
> +inline int load_tag1(uint64_t ptr, uint8_t *mem)
> {
> int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
> return extract32(*mem, ofs, 4);
> @@ -321,7 +290,7 @@ static void check_tag_aligned(CPUARMState *env, uint64_t ptr, uintptr_t ra)
> }
>
> /* For use in a non-parallel context, store to the given nibble. */
> -static void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
> +inline void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
> {
> int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
> *mem = deposit32(*mem, ofs, 4, tag);
Move these two entirely to the header as static inline.
In general, inline without static doesn't mean what you think.
With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> diff --git a/target/arm/tcg/mte_helper.h b/target/arm/tcg/mte_helper.h
> new file mode 100644
> index 0000000000..69ad8457f8
> --- /dev/null
> +++ b/target/arm/tcg/mte_helper.h
> @@ -0,0 +1,63 @@
> +/*
> + * ARM MemTag operation helpers.
> + *
> + * This code is licensed under the GNU GPL v2 or later.
> + *
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#ifndef TARGET_ARM_MTE_H
> +#define TARGET_ARM_MTE_H
> +
> +/**
> + * allocation_tag_mem_probe:
> + * @env: the cpu environment
> + * @ptr_mmu_idx: the addressing regime to use for the virtual address
> + * @ptr: the virtual address for which to look up tag memory
> + * @ptr_access: the access to use for the virtual address
> + * @ptr_size: the number of bytes in the normal memory access
> + * @tag_access: the access to use for the tag memory
> + * @probe: true to merely probe, never taking an exception
> + * @ra: the return address for exception handling
> + *
> + * Our tag memory is formatted as a sequence of little-endian nibbles.
> + * That is, the byte at (addr >> (LOG2_TAG_GRANULE + 1)) contains two
> + * tags, with the tag at [3:0] for the lower addr and the tag at [7:4]
> + * for the higher addr.
> + *
> + * Here, resolve the physical address from the virtual address, and return
> + * a pointer to the corresponding tag byte.
> + *
> + * If there is no tag storage corresponding to @ptr, return NULL.
> + *
> + * If the page is inaccessible for @ptr_access, or has a watchpoint, there are
> + * three options:
> + * (1) probe = true, ra = 0 : pure probe -- we return NULL if the page is not
> + * accessible, and do not take watchpoint traps. The calling code must
> + * handle those cases in the right priority compared to MTE traps.
> + * (2) probe = false, ra = 0 : probe, no fault expected -- the caller guarantees
> + * that the page is going to be accessible. We will take watchpoint traps.
> + * (3) probe = false, ra != 0 : non-probe -- we will take both memory access
> + * traps and watchpoint traps.
> + * (probe = true, ra != 0 is invalid and will assert.)
> + */
> +uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
> + uint64_t ptr, MMUAccessType ptr_access,
> + int ptr_size, MMUAccessType tag_access,
> + bool probe, uintptr_t ra);
> +/**
> + * load_tag1 - Load 1 tag (nibble) from byte
> + * @ptr: The tagged address
> + * @mem: The tag address (packed, 2 tags in byte)
> + */
> +int load_tag1(uint64_t ptr, uint8_t *mem);
> +
> +/**
> + * store_tag1 - Store 1 tag (nibble) into byte
> + * @ptr: The tagged address
> + * @mem: The tag address (packed, 2 tags in byte)
> + * @tag: The tag to be stored in the nibble
> + */
> +void store_tag1(uint64_t ptr, uint8_t *mem, int tag);
> +
> +#endif /* TARGET_ARM_MTE_H */
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/9] target/arm: Make some MTE helpers widely available
2024-06-21 4:31 ` Richard Henderson
@ 2024-06-24 5:36 ` Gustavo Romero
0 siblings, 0 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-24 5:36 UTC (permalink / raw)
To: Richard Henderson, qemu-devel, philmd, peter.maydell, alex.bennee
Hi Richard,
On 6/21/24 1:31 AM, Richard Henderson wrote:
> On 6/16/24 23:28, Gustavo Romero wrote:
>> @@ -287,7 +256,7 @@ uint64_t HELPER(addsubg)(CPUARMState *env, uint64_t ptr,
>> return address_with_allocation_tag(ptr + offset, rtag);
>> }
>> -static int load_tag1(uint64_t ptr, uint8_t *mem)
>> +inline int load_tag1(uint64_t ptr, uint8_t *mem)
>> {
>> int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
>> return extract32(*mem, ofs, 4);
>> @@ -321,7 +290,7 @@ static void check_tag_aligned(CPUARMState *env, uint64_t ptr, uintptr_t ra)
>> }
>> /* For use in a non-parallel context, store to the given nibble. */
>> -static void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
>> +inline void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
>> {
>> int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
>> *mem = deposit32(*mem, ofs, 4, tag);
>
> Move these two entirely to the header as static inline.
> In general, inline without static doesn't mean what you think.
>
> With that,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Done in v4. Thanks.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (4 preceding siblings ...)
2024-06-17 6:28 ` [PATCH v3 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-21 4:35 ` Richard Henderson
2024-06-17 6:28 ` [PATCH v3 7/9] gdbstub: Make get cpu and hex conversion functions non-internal Gustavo Romero
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: gustavo.romero
Factor out the code used for setting the MTE TCF0 field from the prctl
code into a convenient function. Other subsystems, like gdbstub, need to
set this field as well, so keep it as a separate function to avoid
duplication and ensure consistency in how this field is set across the
board.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
linux-user/aarch64/target_prctl.h | 22 ++---------------
target/arm/tcg/mte_user_helper.h | 40 +++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 20 deletions(-)
create mode 100644 target/arm/tcg/mte_user_helper.h
diff --git a/linux-user/aarch64/target_prctl.h b/linux-user/aarch64/target_prctl.h
index aa8e203c15..cfc8567eac 100644
--- a/linux-user/aarch64/target_prctl.h
+++ b/linux-user/aarch64/target_prctl.h
@@ -7,6 +7,7 @@
#define AARCH64_TARGET_PRCTL_H
#include "target/arm/cpu-features.h"
+#include "target/arm/tcg/mte_user_helper.h"
static abi_long do_prctl_sve_get_vl(CPUArchState *env)
{
@@ -173,26 +174,7 @@ static abi_long do_prctl_set_tagged_addr_ctrl(CPUArchState *env, abi_long arg2)
env->tagged_addr_enable = arg2 & PR_TAGGED_ADDR_ENABLE;
if (cpu_isar_feature(aa64_mte, cpu)) {
- /*
- * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
- *
- * The kernel has a per-cpu configuration for the sysadmin,
- * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
- * which qemu does not implement.
- *
- * Because there is no performance difference between the modes, and
- * because SYNC is most useful for debugging MTE errors, choose SYNC
- * as the preferred mode. With this preference, and the way the API
- * uses only two bits, there is no way for the program to select
- * ASYMM mode.
- */
- unsigned tcf = 0;
- if (arg2 & PR_MTE_TCF_SYNC) {
- tcf = 1;
- } else if (arg2 & PR_MTE_TCF_ASYNC) {
- tcf = 2;
- }
- env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
+ arm_set_mte_tcf0(env, arg2);
/*
* Write PR_MTE_TAG to GCR_EL1[Exclude].
diff --git a/target/arm/tcg/mte_user_helper.h b/target/arm/tcg/mte_user_helper.h
new file mode 100644
index 0000000000..dee74d0923
--- /dev/null
+++ b/target/arm/tcg/mte_user_helper.h
@@ -0,0 +1,40 @@
+/*
+ * ARM MemTag convenience functions.
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#ifndef MTE_H
+#define MTE_H
+
+#ifdef CONFIG_USER_ONLY
+#include "sys/prctl.h"
+
+static inline void arm_set_mte_tcf0(CPUArchState *env, abi_long value)
+{
+ /*
+ * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
+ *
+ * The kernel has a per-cpu configuration for the sysadmin,
+ * /sys/devices/system/cpu/cpu<N>/mte_tcf_preferred,
+ * which qemu does not implement.
+ *
+ * Because there is no performance difference between the modes, and
+ * because SYNC is most useful for debugging MTE errors, choose SYNC
+ * as the preferred mode. With this preference, and the way the API
+ * uses only two bits, there is no way for the program to select
+ * ASYMM mode.
+ */
+ unsigned tcf = 0;
+ if (value & PR_MTE_TCF_SYNC) {
+ tcf = 1;
+ } else if (value & PR_MTE_TCF_ASYNC) {
+ tcf = 2;
+ }
+ env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
+}
+#endif /* CONFIG_USER_ONLY */
+
+#endif /* MTE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-17 6:28 ` [PATCH v3 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
@ 2024-06-21 4:35 ` Richard Henderson
2024-06-24 5:38 ` Gustavo Romero
0 siblings, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2024-06-21 4:35 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, peter.maydell, alex.bennee
On 6/16/24 23:28, Gustavo Romero wrote:
> Factor out the code used for setting the MTE TCF0 field from the prctl
> code into a convenient function. Other subsystems, like gdbstub, need to
> set this field as well, so keep it as a separate function to avoid
> duplication and ensure consistency in how this field is set across the
> board.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
> linux-user/aarch64/target_prctl.h | 22 ++---------------
> target/arm/tcg/mte_user_helper.h | 40 +++++++++++++++++++++++++++++++
I'm not keen on this placement, because this is specifically linux syscall semantics.
I'm not sure what the right thing to do here, because it's not like there are any other OS
that support MTE at the moment, and gdb is inheriting linux's ptrace interface.
I think it would be less bad if we put the header in linux-user/aarch64/ and have
target/arm/gdbstub.c include that if CONFIG_USER_ONLY & CONFIG_LINUX.
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-21 4:35 ` Richard Henderson
@ 2024-06-24 5:38 ` Gustavo Romero
0 siblings, 0 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-24 5:38 UTC (permalink / raw)
To: Richard Henderson, qemu-devel, philmd, peter.maydell, alex.bennee
Hi Richard,
On 6/21/24 1:35 AM, Richard Henderson wrote:
> On 6/16/24 23:28, Gustavo Romero wrote:
>> Factor out the code used for setting the MTE TCF0 field from the prctl
>> code into a convenient function. Other subsystems, like gdbstub, need to
>> set this field as well, so keep it as a separate function to avoid
>> duplication and ensure consistency in how this field is set across the
>> board.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>> linux-user/aarch64/target_prctl.h | 22 ++---------------
>> target/arm/tcg/mte_user_helper.h | 40 +++++++++++++++++++++++++++++++
>
> I'm not keen on this placement, because this is specifically linux syscall semantics.
>
> I'm not sure what the right thing to do here, because it's not like there are any other OS that support MTE at the moment, and gdb is inheriting linux's ptrace interface.
>
> I think it would be less bad if we put the header in linux-user/aarch64/ and have target/arm/gdbstub.c include that if CONFIG_USER_ONLY & CONFIG_LINUX.
I think that makes more sense indeed. Done in v4. Thanks.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 7/9] gdbstub: Make get cpu and hex conversion functions non-internal
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (5 preceding siblings ...)
2024-06-17 6:28 ` [PATCH v3 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-21 4:38 ` Richard Henderson
2024-06-17 6:28 ` [PATCH v3 8/9] gdbstub: Add support for MTE in user mode Gustavo Romero
2024-06-17 6:28 ` [PATCH v3 9/9] tests/tcg/aarch64: Add MTE gdbstub tests Gustavo Romero
8 siblings, 1 reply; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: gustavo.romero
Make the gdb_first_attached_cpu and gdb_hextomem non-internal so they
are not confined to use only in gdbstub.c.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
gdbstub/internals.h | 2 --
include/exec/gdbstub.h | 5 +++++
include/gdbstub/commands.h | 6 ++++++
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 34121dc61a..81875abf5f 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -107,7 +107,6 @@ static inline int tohex(int v)
void gdb_put_strbuf(void);
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);
void gdb_memtox(GString *buf, const char *mem, int len);
void gdb_read_byte(uint8_t ch);
@@ -130,7 +129,6 @@ bool gdb_got_immediate_ack(void);
/* utility helpers */
GDBProcess *gdb_get_process(uint32_t pid);
CPUState *gdb_get_first_cpu_in_process(GDBProcess *process);
-CPUState *gdb_first_attached_cpu(void);
void gdb_append_thread_id(CPUState *cpu, GString *buf);
int gdb_get_cpu_index(CPUState *cpu);
unsigned int gdb_get_max_cpus(void); /* both */
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 1bd2c4ec2a..77e5ec9a5b 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -135,4 +135,9 @@ void gdb_set_stop_cpu(CPUState *cpu);
/* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
extern const GDBFeature gdb_static_features[];
+/**
+ * Return the first attached CPU
+ */
+CPUState *gdb_first_attached_cpu(void);
+
#endif /* GDBSTUB_H */
diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
index 2204c3ddbe..914b6d7313 100644
--- a/include/gdbstub/commands.h
+++ b/include/gdbstub/commands.h
@@ -93,4 +93,10 @@ void gdb_extend_set_table(GdbCmdParseEntry *table, int size);
*/
void gdb_extend_qsupported_features(char *qsupported_features);
+/**
+ * Convert a hex string to bytes. Conversion is done per byte, so 2 hex digits
+ * are converted to 1 byte. Invalid hex digits are treated as 0 digits.
+ */
+void gdb_hextomem(GByteArray *mem, const char *buf, int len);
+
#endif /* GDBSTUB_COMMANDS_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 8/9] gdbstub: Add support for MTE in user mode
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (6 preceding siblings ...)
2024-06-17 6:28 ` [PATCH v3 7/9] gdbstub: Make get cpu and hex conversion functions non-internal Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
2024-06-17 6:28 ` [PATCH v3 9/9] tests/tcg/aarch64: Add MTE gdbstub tests Gustavo Romero
8 siblings, 0 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: 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 functions 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 +-
gdb-xml/aarch64-mte.xml | 11 ++
target/arm/cpu.c | 1 +
target/arm/gdbstub.c | 46 ++++++
target/arm/gdbstub64.c | 217 +++++++++++++++++++++++++
target/arm/internals.h | 6 +
6 files changed, 282 insertions(+), 1 deletion(-)
create mode 100644 gdb-xml/aarch64-mte.xml
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/gdb-xml/aarch64-mte.xml b/gdb-xml/aarch64-mte.xml
new file mode 100644
index 0000000000..4b70b4f17a
--- /dev/null
+++ b/gdb-xml/aarch64-mte.xml
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2021-2023 Free Software Foundation, Inc.
+
+ Copying and distribution of this file, with or without modification,
+ are permitted in any medium without royalty provided the copyright
+ notice and this notice are preserved. -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.aarch64.mte">
+ <reg name="tag_ctl" bitsize="64" type="uint64" group="system" save-restore="no"/>
+</feature>
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 35fa281f1b..14d4eca127 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2518,6 +2518,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..c3a9b5eb1e 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -21,6 +21,7 @@
#include "cpu.h"
#include "exec/gdbstub.h"
#include "gdbstub/helpers.h"
+#include "gdbstub/commands.h"
#include "sysemu/tcg.h"
#include "internals.h"
#include "cpu-features.h"
@@ -474,6 +475,41 @@ static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
#endif
#endif /* CONFIG_TCG */
+void arm_cpu_register_gdb_commands(ARMCPU *cpu)
+{
+ GArray *query_table =
+ g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
+ GArray *set_table =
+ g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
+ GString *qsupported_features = g_string_new(NULL);
+
+ if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+ #ifdef TARGET_AARCH64
+ aarch64_cpu_register_gdb_commands(cpu, qsupported_features, query_table,
+ set_table);
+ #endif
+ }
+
+ /* Set arch-specific handlers for 'q' commands. */
+ if (query_table->len) {
+ gdb_extend_query_table(&g_array_index(query_table,
+ GdbCmdParseEntry, 0),
+ query_table->len);
+ }
+
+ /* Set arch-specific handlers for 'Q' commands. */
+ if (set_table->len) {
+ gdb_extend_set_table(&g_array_index(set_table,
+ GdbCmdParseEntry, 0),
+ set_table->len);
+ }
+
+ /* Set arch-specific qSupported feature. */
+ if (qsupported_features->len) {
+ gdb_extend_qsupported_features(qsupported_features->str);
+ }
+}
+
void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
{
CPUState *cs = CPU(cpu);
@@ -507,6 +543,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' pseudo-register. */
+ if (cpu_isar_feature(aa64_mte, cpu)) {
+ 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/gdbstub64.c b/target/arm/gdbstub64.c
index caa31ff3fa..f94a416836 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -21,6 +21,9 @@
#include "cpu.h"
#include "internals.h"
#include "gdbstub/helpers.h"
+#include "gdbstub/commands.h"
+#include "tcg/mte_helper.h"
+#include "tcg/mte_user_helper.h"
int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
{
@@ -381,3 +384,217 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
return &cpu->dyn_svereg_feature.desc;
}
+
+#ifdef CONFIG_USER_ONLY
+int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
+{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+ uint64_t tcf0;
+
+ assert(reg == 0);
+
+ tcf0 = extract64(env->cp15.sctlr_el[1], 38, 2);
+
+ return gdb_get_reg64(buf, tcf0);
+}
+
+int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
+{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
+ uint8_t tcf;
+
+ assert(reg == 0);
+
+ tcf = *buf << PR_MTE_TCF_SHIFT;
+
+ if (!tcf) {
+ return 0;
+ }
+
+ /*
+ * 'tag_ctl' register is actually a "pseudo-register" provided by GDB to
+ * expose options regarding the type of MTE fault that can be controlled at
+ * runtime.
+ */
+ arm_set_mte_tcf0(env, tcf);
+
+ return 1;
+}
+
+static void handle_q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
+{
+ ARMCPU *cpu = ARM_CPU(gdb_first_attached_cpu());
+ CPUARMState *env = &cpu->env;
+
+ uint64_t addr = gdb_get_cmd_param(params, 0)->val_ull;
+ uint64_t len = gdb_get_cmd_param(params, 1)->val_ul;
+ int type = gdb_get_cmd_param(params, 2)->val_ul;
+
+ uint8_t *tags;
+ uint8_t addr_tag;
+
+ g_autoptr(GString) str_buf = g_string_new(NULL);
+
+ /*
+ * GDB does not query multiple 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("E03");
+ }
+
+ /* Note that tags are packed here (2 tags packed in one byte). */
+ tags = allocation_tag_mem_probe(env, 0, addr, MMU_DATA_LOAD, 8 /* 64-bit */,
+ MMU_DATA_LOAD, true, 0);
+ if (!tags) {
+ /* Address is not in a tagged region. */
+ gdb_put_packet("E04");
+ return;
+ }
+
+ /* Unpack tag from byte. */
+ addr_tag = load_tag1(addr, tags);
+ 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)
+{
+ ARMCPU *cpu = ARM_CPU(gdb_first_attached_cpu());
+ CPUARMState *env = &cpu->env;
+
+ uint64_t addr = gdb_get_cmd_param(params, 0)->val_ull;
+
+ uint8_t *tags;
+ const char *reply;
+
+ tags = allocation_tag_mem_probe(env, 0, addr, MMU_DATA_LOAD, 8 /* 64-bit */,
+ MMU_DATA_LOAD, true, 0);
+ reply = tags ? "01" : "00";
+
+ gdb_put_packet(reply);
+}
+
+static void handle_Q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
+{
+ ARMCPU *cpu = ARM_CPU(gdb_first_attached_cpu());
+ CPUARMState *env = &cpu->env;
+
+ uint64_t start_addr = gdb_get_cmd_param(params, 0)->val_ull;
+ uint64_t len = gdb_get_cmd_param(params, 1)->val_ul;
+ int type = gdb_get_cmd_param(params, 2)->val_ul;
+ char const *new_tags_str = gdb_get_cmd_param(params, 3)->data;
+
+ uint64_t end_addr;
+
+ int num_new_tags;
+ uint8_t *tags;
+
+ g_autoptr(GByteArray) new_tags = g_byte_array_new();
+
+ /*
+ * Only the allocation tag (i.e. type 1) can be set at the stub side.
+ */
+ if (type != 1) {
+ gdb_put_packet("E02");
+ return;
+ }
+
+ end_addr = start_addr + (len - 1); /* 'len' is always >= 1 */
+ /* Check if request's memory range does not cross page boundaries. */
+ if ((start_addr ^ end_addr) & TARGET_PAGE_MASK) {
+ gdb_put_packet("E03");
+ return;
+ }
+
+ /*
+ * Get all tags in the page starting from the tag of the start address.
+ * Note that there are two tags packed into a single byte here.
+ */
+ tags = allocation_tag_mem_probe(env, 0, start_addr, MMU_DATA_STORE,
+ 8 /* 64-bit */, MMU_DATA_STORE, true, 0);
+ if (!tags) {
+ /* Address is not in a tagged region. */
+ gdb_put_packet("E04");
+ return;
+ }
+
+ /* Convert tags provided by GDB, 2 hex digits per tag. */
+ num_new_tags = strlen(new_tags_str) / 2;
+ gdb_hextomem(new_tags, new_tags_str, num_new_tags);
+
+ uint64_t address = start_addr;
+ int new_tag_index = 0;
+ while (address <= end_addr) {
+ uint8_t new_tag;
+ int packed_index;
+
+ /*
+ * Find packed tag index from unpacked tag index. There are two tags
+ * in one packed index (one tag per nibble).
+ */
+ packed_index = new_tag_index / 2;
+
+ new_tag = new_tags->data[new_tag_index % num_new_tags];
+ store_tag1(address, tags + packed_index, new_tag);
+
+ address += TAG_GRANULE;
+ new_tag_index++;
+ }
+
+ gdb_put_packet("OK");
+}
+
+enum Command {
+ qMemTags,
+ qIsAddressTagged,
+ QMemTags,
+ NUM_CMDS
+};
+
+static GdbCmdParseEntry cmd_handler_table[NUM_CMDS] = {
+ [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"
+ },
+};
+#endif /* CONFIG_USER_ONLY */
+
+void aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *qsupported,
+ GArray *qtable, GArray *stable)
+{
+#ifdef CONFIG_USER_ONLY
+ /* MTE */
+ if (cpu_isar_feature(aa64_mte, cpu)) {
+ g_string_append(qsupported, ";memory-tagging+");
+
+ g_array_append_val(qtable, cmd_handler_table[qMemTags]);
+ g_array_append_val(qtable, cmd_handler_table[qIsAddressTagged]);
+
+ g_array_append_val(stable, cmd_handler_table[QMemTags]);
+ }
+#endif
+}
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 11b5da2562..e1aa1a63b9 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -358,6 +358,10 @@ 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 aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *, GArray *,
+ GArray *);
+
void arm_restore_state_to_opc(CPUState *cs,
const TranslationBlock *tb,
const uint64_t *data);
@@ -1640,6 +1644,8 @@ int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);
int aarch64_gdb_set_pauth_reg(CPUState *cs, uint8_t *buf, int reg);
+int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg);
+int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg);
void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 9/9] tests/tcg/aarch64: Add MTE gdbstub tests
2024-06-17 6:28 [PATCH v3 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (7 preceding siblings ...)
2024-06-17 6:28 ` [PATCH v3 8/9] gdbstub: Add support for MTE in user mode Gustavo Romero
@ 2024-06-17 6:28 ` Gustavo Romero
8 siblings, 0 replies; 21+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:28 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: 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 | 98 +++++++++++++++++++++++++++
3 files changed, 193 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..2b3fd636be 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 $< --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..2db0663c1a
--- /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 95", 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, f"{tags_match[0]}")
+ 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..9fffd7b737
--- /dev/null
+++ b/tests/tcg/aarch64/mte-8.c
@@ -0,0 +1,98 @@
+/*
+ * 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);
+
+ /* 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
^ permalink raw reply related [flat|nested] 21+ messages in thread