* [PATCH v2 0/9] Add MTE stubs for aarch64 user mode
@ 2024-06-13 17:20 Gustavo Romero
2024-06-13 17:20 ` [PATCH v2 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
` (9 more replies)
0 siblings, 10 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:20 UTC (permalink / raw)
To: qemu-devel, philmd, peter.maydell, alex.bennee, richard.henderson
Cc: 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: gdb_extend_qsupported_features,
gdb_extend_query_table, and gdb_extend_set_table. 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
v2:
- Addressed comments from Richard, Phil, and Alex
- Made the series more granular by splitting it into more patches
- Moved gdbstub command-specific structs and functions into a new header, gdbstub/commands.h
- Fixed exception in allocation_tag_mem_probe()
- Used MTE helpers ({store,load}_tag1 and allocation_tag_mem_probe) in the MTE stubs
- Factored out MTE code to set TCF0, avoiding duplication (both prctl and gdbstub code use it)
- Hoisted sscanf() out of loop in handle_Q_memtag stub and use gdb_hextomem instead
- Rebased this series on Alex's gdb/next branch
Cheers,
Gustavo
Gustavo Romero (9):
gdbstub: Clean up process_string_cmd
gdbstub: Move GdbCmdParseEntry into a new header file
gdbstub: Add support for target-specific stubs
target/arm: Fix exception case in allocation_tag_mem_probe
target/arm: Make some MTE helpers widely available
target/arm: Factor out code for setting MTE TCF0 field
gdbstub: Make get cpu and hex conversion functions non-internal
gdbstub: Add support for MTE in user mode
tests/tcg/aarch64: Add MTE gdbstub tests
configs/targets/aarch64-linux-user.mak | 2 +-
gdb-xml/aarch64-mte.xml | 11 ++
gdbstub/gdbstub.c | 211 +++++++++++----------
gdbstub/internals.h | 24 ---
gdbstub/syscalls.c | 7 +-
gdbstub/system.c | 7 +-
gdbstub/user-target.c | 25 +--
gdbstub/user.c | 7 +-
include/exec/gdbstub.h | 5 +
include/gdbstub/commands.h | 102 ++++++++++
linux-user/aarch64/target_prctl.h | 22 +--
target/arm/cpu.c | 1 +
target/arm/gdbstub.c | 253 +++++++++++++++++++++++++
target/arm/internals.h | 2 +
target/arm/mte.h | 53 ++++++
target/arm/tcg/mte_helper.c | 181 +-----------------
target/arm/tcg/mte_helper.h | 211 +++++++++++++++++++++
tests/tcg/aarch64/Makefile.target | 11 +-
tests/tcg/aarch64/gdbstub/test-mte.py | 86 +++++++++
tests/tcg/aarch64/mte-8.c | 102 ++++++++++
20 files changed, 975 insertions(+), 348 deletions(-)
create mode 100644 gdb-xml/aarch64-mte.xml
create mode 100644 include/gdbstub/commands.h
create mode 100644 target/arm/mte.h
create mode 100644 target/arm/tcg/mte_helper.h
create mode 100644 tests/tcg/aarch64/gdbstub/test-mte.py
create mode 100644 tests/tcg/aarch64/mte-8.c
--
2.34.1
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 1/9] gdbstub: Clean up process_string_cmd
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
@ 2024-06-13 17:20 ` Gustavo Romero
2024-06-14 11:24 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 2/9] gdbstub: Move GdbCmdParseEntry into a new header file Gustavo Romero
` (8 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:20 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>
---
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] 35+ messages in thread
* [PATCH v2 2/9] gdbstub: Move GdbCmdParseEntry into a new header file
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
2024-06-13 17:20 ` [PATCH v2 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
@ 2024-06-13 17:20 ` Gustavo Romero
2024-06-14 11:25 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
` (7 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:20 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>
---
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] 35+ messages in thread
* [PATCH v2 3/9] gdbstub: Add support for target-specific stubs
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
2024-06-13 17:20 ` [PATCH v2 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
2024-06-13 17:20 ` [PATCH v2 2/9] gdbstub: Move GdbCmdParseEntry into a new header file Gustavo Romero
@ 2024-06-13 17:20 ` Gustavo Romero
2024-06-14 11:27 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 4/9] target/arm: Fix exception case in allocation_tag_mem_probe Gustavo Romero
` (6 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:20 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] 35+ messages in thread
* [PATCH v2 4/9] target/arm: Fix exception case in allocation_tag_mem_probe
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (2 preceding siblings ...)
2024-06-13 17:20 ` [PATCH v2 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
@ 2024-06-13 17:20 ` Gustavo Romero
2024-06-14 11:29 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
` (5 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:20 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>
---
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] 35+ messages in thread
* [PATCH v2 5/9] target/arm: Make some MTE helpers widely available
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (3 preceding siblings ...)
2024-06-13 17:20 ` [PATCH v2 4/9] target/arm: Fix exception case in allocation_tag_mem_probe Gustavo Romero
@ 2024-06-13 17:20 ` Gustavo Romero
2024-06-13 17:32 ` Philippe Mathieu-Daudé
2024-06-13 17:21 ` [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
` (4 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:20 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 by moving them from mte_helper.c to a new
header file, mte_helper.h.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
target/arm/tcg/mte_helper.c | 184 +------------------------------
target/arm/tcg/mte_helper.h | 211 ++++++++++++++++++++++++++++++++++++
2 files changed, 212 insertions(+), 183 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..da639282e6 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,176 +51,6 @@ 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)
-{
-#ifdef CONFIG_USER_ONLY
- uint64_t clean_ptr = useronly_clean_ptr(ptr);
- int flags = page_get_flags(clean_ptr);
- uint8_t *tags;
- uintptr_t index;
-
- 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);
- }
-
- /* Require both MAP_ANON and PROT_MTE for the page. */
- if (!(flags & PAGE_ANON) || !(flags & PAGE_MTE)) {
- return NULL;
- }
-
- tags = page_get_target_data(clean_ptr);
-
- index = extract32(ptr, LOG2_TAG_GRANULE + 1,
- TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
- return tags + index;
-#else
- CPUTLBEntryFull *full;
- MemTxAttrs attrs;
- int in_page, flags;
- hwaddr ptr_paddr, tag_paddr, xlat;
- MemoryRegion *mr;
- ARMASIdx tag_asi;
- AddressSpace *tag_as;
- void *host;
-
- /*
- * Probe the first byte of the virtual address. This raises an
- * exception for inaccessible pages, and resolves the virtual address
- * into the softmmu tlb.
- *
- * When RA == 0, this is either a pure probe or a no-fault-expected probe.
- * Indicate to probe_access_flags no-fault, then either return NULL
- * for the pure probe, or assert that we received a valid page for the
- * no-fault-expected probe.
- */
- flags = probe_access_full(env, ptr, 0, ptr_access, ptr_mmu_idx,
- ra == 0, &host, &full, ra);
- if (probe && (flags & TLB_INVALID_MASK)) {
- return NULL;
- }
- assert(!(flags & TLB_INVALID_MASK));
-
- /* If the virtual page MemAttr != Tagged, access unchecked. */
- if (full->extra.arm.pte_attrs != 0xf0) {
- return NULL;
- }
-
- /*
- * If not backed by host ram, there is no tag storage: access unchecked.
- * This is probably a guest os bug though, so log it.
- */
- if (unlikely(flags & TLB_MMIO)) {
- qemu_log_mask(LOG_GUEST_ERROR,
- "Page @ 0x%" PRIx64 " indicates Tagged Normal memory "
- "but is not backed by host ram\n", ptr);
- return NULL;
- }
-
- /*
- * Remember these values across the second lookup below,
- * which may invalidate this pointer via tlb resize.
- */
- ptr_paddr = full->phys_addr | (ptr & ~TARGET_PAGE_MASK);
- attrs = full->attrs;
- full = NULL;
-
- /*
- * The Normal memory access can extend to the next page. E.g. a single
- * 8-byte access to the last byte of a page will check only the last
- * tag on the first page.
- * Any page access exception has priority over tag check exception.
- */
- in_page = -(ptr | TARGET_PAGE_MASK);
- if (unlikely(ptr_size > in_page)) {
- flags |= probe_access_full(env, ptr + in_page, 0, ptr_access,
- ptr_mmu_idx, ra == 0, &host, &full, ra);
- assert(!(flags & TLB_INVALID_MASK));
- }
-
- /* Any debug exception has priority over a tag check exception. */
- if (!probe && unlikely(flags & TLB_WATCHPOINT)) {
- int wp = ptr_access == MMU_DATA_LOAD ? BP_MEM_READ : BP_MEM_WRITE;
- assert(ra != 0);
- cpu_check_watchpoint(env_cpu(env), ptr, ptr_size, attrs, wp, ra);
- }
-
- /* Convert to the physical address in tag space. */
- tag_paddr = ptr_paddr >> (LOG2_TAG_GRANULE + 1);
-
- /* Look up the address in tag space. */
- tag_asi = attrs.secure ? ARMASIdx_TagS : ARMASIdx_TagNS;
- tag_as = cpu_get_address_space(env_cpu(env), tag_asi);
- mr = address_space_translate(tag_as, tag_paddr, &xlat, NULL,
- tag_access == MMU_DATA_STORE, attrs);
-
- /*
- * Note that @mr will never be NULL. If there is nothing in the address
- * space at @tag_paddr, the translation will return the unallocated memory
- * region. For our purposes, the result must be ram.
- */
- if (unlikely(!memory_region_is_ram(mr))) {
- /* ??? Failure is a board configuration error. */
- qemu_log_mask(LOG_UNIMP,
- "Tag Memory @ 0x%" HWADDR_PRIx " not found for "
- "Normal Memory @ 0x%" HWADDR_PRIx "\n",
- tag_paddr, ptr_paddr);
- return NULL;
- }
-
- /*
- * Ensure the tag memory is dirty on write, for migration.
- * Tag memory can never contain code or display memory (vga).
- */
- if (tag_access == MMU_DATA_STORE) {
- ram_addr_t tag_ra = memory_region_get_ram_addr(mr) + xlat;
- cpu_physical_memory_set_dirty_flag(tag_ra, DIRTY_MEMORY_MIGRATION);
- }
-
- return memory_region_get_ram_ptr(mr) + xlat;
-#endif
-}
-
static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx,
uint64_t ptr, MMUAccessType ptr_access,
int ptr_size, MMUAccessType tag_access,
@@ -287,12 +118,6 @@ 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)
-{
- int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
- return extract32(*mem, ofs, 4);
-}
-
uint64_t HELPER(ldg)(CPUARMState *env, uint64_t ptr, uint64_t xt)
{
int mmu_idx = arm_env_mmu_index(env);
@@ -320,13 +145,6 @@ 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)
-{
- int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
- *mem = deposit32(*mem, ofs, 4, tag);
-}
-
/* For use in a parallel context, atomically store to the given nibble. */
static void store_tag1_parallel(uint64_t ptr, uint8_t *mem, int tag)
{
diff --git a/target/arm/tcg/mte_helper.h b/target/arm/tcg/mte_helper.h
new file mode 100644
index 0000000000..2d09345642
--- /dev/null
+++ b/target/arm/tcg/mte_helper.h
@@ -0,0 +1,211 @@
+/*
+ * ARM MemTag Operation Helpers
+ *
+ * Copyright (c) 2024 Linaro, Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef TARGET_ARM_MTE_H
+#define TARGET_ARM_MTE_H
+
+#include "exec/exec-all.h"
+#include "exec/ram_addr.h"
+#include "hw/core/tcg-cpu-ops.h"
+#include "qemu/log.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.)
+ */
+static inline 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);
+ int flags = page_get_flags(clean_ptr);
+ uint8_t *tags;
+ uintptr_t index;
+
+ 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);
+ }
+
+ /* Require both MAP_ANON and PROT_MTE for the page. */
+ if (!(flags & PAGE_ANON) || !(flags & PAGE_MTE)) {
+ return NULL;
+ }
+
+ tags = page_get_target_data(clean_ptr);
+
+ index = extract32(ptr, LOG2_TAG_GRANULE + 1,
+ TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
+ return tags + index;
+#else
+ CPUTLBEntryFull *full;
+ MemTxAttrs attrs;
+ int in_page, flags;
+ hwaddr ptr_paddr, tag_paddr, xlat;
+ MemoryRegion *mr;
+ ARMASIdx tag_asi;
+ AddressSpace *tag_as;
+ void *host;
+
+ /*
+ * Probe the first byte of the virtual address. This raises an
+ * exception for inaccessible pages, and resolves the virtual address
+ * into the softmmu tlb.
+ *
+ * When RA == 0, this is either a pure probe or a no-fault-expected probe.
+ * Indicate to probe_access_flags no-fault, then either return NULL
+ * for the pure probe, or assert that we received a valid page for the
+ * no-fault-expected probe.
+ */
+ flags = probe_access_full(env, ptr, 0, ptr_access, ptr_mmu_idx,
+ ra == 0, &host, &full, ra);
+ if (probe && (flags & TLB_INVALID_MASK)) {
+ return NULL;
+ }
+ assert(!(flags & TLB_INVALID_MASK));
+
+ /* If the virtual page MemAttr != Tagged, access unchecked. */
+ if (full->extra.arm.pte_attrs != 0xf0) {
+ return NULL;
+ }
+
+ /*
+ * If not backed by host ram, there is no tag storage: access unchecked.
+ * This is probably a guest os bug though, so log it.
+ */
+ if (unlikely(flags & TLB_MMIO)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Page @ 0x%" PRIx64 " indicates Tagged Normal memory "
+ "but is not backed by host ram\n", ptr);
+ return NULL;
+ }
+
+ /*
+ * Remember these values across the second lookup below,
+ * which may invalidate this pointer via tlb resize.
+ */
+ ptr_paddr = full->phys_addr | (ptr & ~TARGET_PAGE_MASK);
+ attrs = full->attrs;
+ full = NULL;
+
+ /*
+ * The Normal memory access can extend to the next page. E.g. a single
+ * 8-byte access to the last byte of a page will check only the last
+ * tag on the first page.
+ * Any page access exception has priority over tag check exception.
+ */
+ in_page = -(ptr | TARGET_PAGE_MASK);
+ if (unlikely(ptr_size > in_page)) {
+ flags |= probe_access_full(env, ptr + in_page, 0, ptr_access,
+ ptr_mmu_idx, ra == 0, &host, &full, ra);
+ assert(!(flags & TLB_INVALID_MASK));
+ }
+
+ /* Any debug exception has priority over a tag check exception. */
+ if (!probe && unlikely(flags & TLB_WATCHPOINT)) {
+ int wp = ptr_access == MMU_DATA_LOAD ? BP_MEM_READ : BP_MEM_WRITE;
+ assert(ra != 0);
+ cpu_check_watchpoint(env_cpu(env), ptr, ptr_size, attrs, wp, ra);
+ }
+
+ /* Convert to the physical address in tag space. */
+ tag_paddr = ptr_paddr >> (LOG2_TAG_GRANULE + 1);
+
+ /* Look up the address in tag space. */
+ tag_asi = attrs.secure ? ARMASIdx_TagS : ARMASIdx_TagNS;
+ tag_as = cpu_get_address_space(env_cpu(env), tag_asi);
+ mr = address_space_translate(tag_as, tag_paddr, &xlat, NULL,
+ tag_access == MMU_DATA_STORE, attrs);
+
+ /*
+ * Note that @mr will never be NULL. If there is nothing in the address
+ * space at @tag_paddr, the translation will return the unallocated memory
+ * region. For our purposes, the result must be ram.
+ */
+ if (unlikely(!memory_region_is_ram(mr))) {
+ /* ??? Failure is a board configuration error. */
+ qemu_log_mask(LOG_UNIMP,
+ "Tag Memory @ 0x%" HWADDR_PRIx " not found for "
+ "Normal Memory @ 0x%" HWADDR_PRIx "\n",
+ tag_paddr, ptr_paddr);
+ return NULL;
+ }
+
+ /*
+ * Ensure the tag memory is dirty on write, for migration.
+ * Tag memory can never contain code or display memory (vga).
+ */
+ if (tag_access == MMU_DATA_STORE) {
+ ram_addr_t tag_ra = memory_region_get_ram_addr(mr) + xlat;
+ cpu_physical_memory_set_dirty_flag(tag_ra, DIRTY_MEMORY_MIGRATION);
+ }
+
+ return memory_region_get_ram_ptr(mr) + xlat;
+#endif
+}
+
+static inline int load_tag1(uint64_t ptr, uint8_t *mem)
+{
+ int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
+ return extract32(*mem, ofs, 4);
+}
+
+/* For use in a non-parallel context, store to the given nibble. */
+static 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);
+}
+
+#endif /* TARGET_ARM_MTE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (4 preceding siblings ...)
2024-06-13 17:20 ` [PATCH v2 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
@ 2024-06-13 17:21 ` Gustavo Romero
2024-06-13 17:35 ` Philippe Mathieu-Daudé
2024-06-14 11:21 ` Alex Bennée
2024-06-13 17:21 ` [PATCH v2 7/9] gdbstub: Make get cpu and hex conversion functions non-internal Gustavo Romero
` (3 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:21 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/mte.h | 53 +++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 20 deletions(-)
create mode 100644 target/arm/mte.h
diff --git a/linux-user/aarch64/target_prctl.h b/linux-user/aarch64/target_prctl.h
index aa8e203c15..8a11404222 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/mte.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);
+ set_mte_tcf0(env, arg2);
/*
* Write PR_MTE_TAG to GCR_EL1[Exclude].
diff --git a/target/arm/mte.h b/target/arm/mte.h
new file mode 100644
index 0000000000..89712aad70
--- /dev/null
+++ b/target/arm/mte.h
@@ -0,0 +1,53 @@
+/*
+ * ARM MemTag convenience functions.
+ *
+ * Copyright (c) 2024 Linaro, Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef MTE_H
+#define MTE_H
+
+#ifdef CONFIG_TCG
+#ifdef CONFIG_USER_ONLY
+#include "sys/prctl.h"
+
+static void 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 /* CONFIG_TCG */
+
+#endif /* MTE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 7/9] gdbstub: Make get cpu and hex conversion functions non-internal
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (5 preceding siblings ...)
2024-06-13 17:21 ` [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
@ 2024-06-13 17:21 ` Gustavo Romero
2024-06-14 11:34 ` Alex Bennée
2024-06-13 17:21 ` [PATCH v2 8/9] gdbstub: Add support for MTE in user mode Gustavo Romero
` (2 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:21 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] 35+ messages in thread
* [PATCH v2 8/9] gdbstub: Add support for MTE in user mode
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (6 preceding siblings ...)
2024-06-13 17:21 ` [PATCH v2 7/9] gdbstub: Make get cpu and hex conversion functions non-internal Gustavo Romero
@ 2024-06-13 17:21 ` Gustavo Romero
2024-06-14 10:51 ` Alex Bennée
2024-06-13 17:21 ` [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests Gustavo Romero
2024-06-14 15:49 ` [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Alex Bennée
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:21 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 | 253 +++++++++++++++++++++++++
target/arm/internals.h | 2 +
5 files changed, 268 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..1cbcd6fa98 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -21,10 +21,13 @@
#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"
#include "cpregs.h"
+#include "mte.h"
+#include "tcg/mte_helper.h"
typedef struct RegisterSysregFeatureParam {
CPUState *cs;
@@ -474,6 +477,246 @@ 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 = 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;
+
+ 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.
+ */
+ 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 */
+#endif /* TARGET_AARCH64 */
+
+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);
+
+#ifdef CONFIG_USER_ONLY
+ /* MTE */
+ if (cpu_isar_feature(aa64_mte3, cpu)) {
+ g_string_append(qsupported_features, ";memory-tagging+");
+
+ g_array_append_val(query_table, cmd_handler_table[qMemTags]);
+ g_array_append_val(query_table, cmd_handler_table[qIsAddressTagged]);
+
+ g_array_append_val(set_table, cmd_handler_table[QMemTags]);
+ }
+#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 +750,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/internals.h b/target/arm/internals.h
index 11b5da2562..e27a9fa1e0 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (7 preceding siblings ...)
2024-06-13 17:21 ` [PATCH v2 8/9] gdbstub: Add support for MTE in user mode Gustavo Romero
@ 2024-06-13 17:21 ` Gustavo Romero
2024-06-14 11:42 ` Alex Bennée
2024-06-14 15:49 ` [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Alex Bennée
9 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 17:21 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 | 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..ec49eb8d2b
--- /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 99", 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..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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/9] target/arm: Make some MTE helpers widely available
2024-06-13 17:20 ` [PATCH v2 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
@ 2024-06-13 17:32 ` Philippe Mathieu-Daudé
2024-06-13 18:13 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-13 17:32 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, peter.maydell, alex.bennee,
richard.henderson
Hi Gustavo,
On 13/6/24 19:20, Gustavo Romero wrote:
> Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
> available to other subsystems by moving them from mte_helper.c to a new
> header file, mte_helper.h.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
> target/arm/tcg/mte_helper.c | 184 +------------------------------
> target/arm/tcg/mte_helper.h | 211 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 212 insertions(+), 183 deletions(-)
> create mode 100644 target/arm/tcg/mte_helper.h
> diff --git a/target/arm/tcg/mte_helper.h b/target/arm/tcg/mte_helper.h
> new file mode 100644
> index 0000000000..2d09345642
> --- /dev/null
> +++ b/target/arm/tcg/mte_helper.h
> @@ -0,0 +1,211 @@
> +/*
> + * ARM MemTag Operation Helpers
> + *
> + * Copyright (c) 2024 Linaro, Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
Preferably parsable license tag:
* SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#ifndef TARGET_ARM_MTE_H
> +#define TARGET_ARM_MTE_H
> +
> +#include "exec/exec-all.h"
Why do you need "exec/exec-all.h"?
> +#include "exec/ram_addr.h"
> +#include "hw/core/tcg-cpu-ops.h"
> +#include "qemu/log.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.)
> + */
> +static inline 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)
Do we really need an inlined function? Since it calls non-inlined
methods, I don't really see the point.
> +{
> +#ifdef CONFIG_USER_ONLY
> + uint64_t clean_ptr = useronly_clean_ptr(ptr);
> + int flags = page_get_flags(clean_ptr);
> + uint8_t *tags;
> + uintptr_t index;
> +
> + 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);
> + }
> +
> + /* Require both MAP_ANON and PROT_MTE for the page. */
> + if (!(flags & PAGE_ANON) || !(flags & PAGE_MTE)) {
> + return NULL;
> + }
> +
> + tags = page_get_target_data(clean_ptr);
> +
> + index = extract32(ptr, LOG2_TAG_GRANULE + 1,
> + TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
> + return tags + index;
> +#else
> + CPUTLBEntryFull *full;
> + MemTxAttrs attrs;
> + int in_page, flags;
> + hwaddr ptr_paddr, tag_paddr, xlat;
> + MemoryRegion *mr;
> + ARMASIdx tag_asi;
> + AddressSpace *tag_as;
> + void *host;
> +
> + /*
> + * Probe the first byte of the virtual address. This raises an
> + * exception for inaccessible pages, and resolves the virtual address
> + * into the softmmu tlb.
> + *
> + * When RA == 0, this is either a pure probe or a no-fault-expected probe.
> + * Indicate to probe_access_flags no-fault, then either return NULL
> + * for the pure probe, or assert that we received a valid page for the
> + * no-fault-expected probe.
> + */
> + flags = probe_access_full(env, ptr, 0, ptr_access, ptr_mmu_idx,
> + ra == 0, &host, &full, ra);
> + if (probe && (flags & TLB_INVALID_MASK)) {
> + return NULL;
> + }
> + assert(!(flags & TLB_INVALID_MASK));
> +
> + /* If the virtual page MemAttr != Tagged, access unchecked. */
> + if (full->extra.arm.pte_attrs != 0xf0) {
> + return NULL;
> + }
> +
> + /*
> + * If not backed by host ram, there is no tag storage: access unchecked.
> + * This is probably a guest os bug though, so log it.
> + */
> + if (unlikely(flags & TLB_MMIO)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Page @ 0x%" PRIx64 " indicates Tagged Normal memory "
> + "but is not backed by host ram\n", ptr);
> + return NULL;
> + }
> +
> + /*
> + * Remember these values across the second lookup below,
> + * which may invalidate this pointer via tlb resize.
> + */
> + ptr_paddr = full->phys_addr | (ptr & ~TARGET_PAGE_MASK);
> + attrs = full->attrs;
> + full = NULL;
> +
> + /*
> + * The Normal memory access can extend to the next page. E.g. a single
> + * 8-byte access to the last byte of a page will check only the last
> + * tag on the first page.
> + * Any page access exception has priority over tag check exception.
> + */
> + in_page = -(ptr | TARGET_PAGE_MASK);
> + if (unlikely(ptr_size > in_page)) {
> + flags |= probe_access_full(env, ptr + in_page, 0, ptr_access,
> + ptr_mmu_idx, ra == 0, &host, &full, ra);
> + assert(!(flags & TLB_INVALID_MASK));
> + }
> +
> + /* Any debug exception has priority over a tag check exception. */
> + if (!probe && unlikely(flags & TLB_WATCHPOINT)) {
> + int wp = ptr_access == MMU_DATA_LOAD ? BP_MEM_READ : BP_MEM_WRITE;
> + assert(ra != 0);
> + cpu_check_watchpoint(env_cpu(env), ptr, ptr_size, attrs, wp, ra);
> + }
> +
> + /* Convert to the physical address in tag space. */
> + tag_paddr = ptr_paddr >> (LOG2_TAG_GRANULE + 1);
> +
> + /* Look up the address in tag space. */
> + tag_asi = attrs.secure ? ARMASIdx_TagS : ARMASIdx_TagNS;
> + tag_as = cpu_get_address_space(env_cpu(env), tag_asi);
> + mr = address_space_translate(tag_as, tag_paddr, &xlat, NULL,
> + tag_access == MMU_DATA_STORE, attrs);
> +
> + /*
> + * Note that @mr will never be NULL. If there is nothing in the address
> + * space at @tag_paddr, the translation will return the unallocated memory
> + * region. For our purposes, the result must be ram.
> + */
> + if (unlikely(!memory_region_is_ram(mr))) {
> + /* ??? Failure is a board configuration error. */
> + qemu_log_mask(LOG_UNIMP,
> + "Tag Memory @ 0x%" HWADDR_PRIx " not found for "
> + "Normal Memory @ 0x%" HWADDR_PRIx "\n",
> + tag_paddr, ptr_paddr);
> + return NULL;
> + }
> +
> + /*
> + * Ensure the tag memory is dirty on write, for migration.
> + * Tag memory can never contain code or display memory (vga).
> + */
> + if (tag_access == MMU_DATA_STORE) {
> + ram_addr_t tag_ra = memory_region_get_ram_addr(mr) + xlat;
> + cpu_physical_memory_set_dirty_flag(tag_ra, DIRTY_MEMORY_MIGRATION);
> + }
> +
> + return memory_region_get_ram_ptr(mr) + xlat;
> +#endif
> +}
> +
> +static inline int load_tag1(uint64_t ptr, uint8_t *mem)
> +{
> + int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
> + return extract32(*mem, ofs, 4);
> +}
> +
> +/* For use in a non-parallel context, store to the given nibble. */
> +static 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);
> +}
> +
> +#endif /* TARGET_ARM_MTE_H */
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-13 17:21 ` [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
@ 2024-06-13 17:35 ` Philippe Mathieu-Daudé
2024-06-13 18:15 ` Gustavo Romero
2024-06-14 11:21 ` Alex Bennée
1 sibling, 1 reply; 35+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-13 17:35 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, peter.maydell, alex.bennee,
richard.henderson
On 13/6/24 19:21, 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/mte.h | 53 +++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 20 deletions(-)
> create mode 100644 target/arm/mte.h
> diff --git a/target/arm/mte.h b/target/arm/mte.h
> new file mode 100644
> index 0000000000..89712aad70
> --- /dev/null
> +++ b/target/arm/mte.h
> @@ -0,0 +1,53 @@
> +/*
> + * ARM MemTag convenience functions.
> + *
> + * Copyright (c) 2024 Linaro, Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef MTE_H
> +#define MTE_H
> +
> +#ifdef CONFIG_TCG
> +#ifdef CONFIG_USER_ONLY
> +#include "sys/prctl.h"
> +
> +static void set_mte_tcf0(CPUArchState *env, abi_long value)
Either declare it inlined (otherwise we'll get multiple symbols
declared if this header is included multiple times), or
preferably only expose the prototype.
Also I'd use the 'arm_' prefix.
> +{
> + /*
> + * 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 /* CONFIG_TCG */
> +
> +#endif /* MTE_H */
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/9] target/arm: Make some MTE helpers widely available
2024-06-13 17:32 ` Philippe Mathieu-Daudé
@ 2024-06-13 18:13 ` Gustavo Romero
2024-06-14 12:34 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 18:13 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, peter.maydell,
alex.bennee, richard.henderson
Hi Phil!
On 6/13/24 2:32 PM, Philippe Mathieu-Daudé wrote:
> Hi Gustavo,
>
> On 13/6/24 19:20, Gustavo Romero wrote:
>> Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
>> available to other subsystems by moving them from mte_helper.c to a new
>> header file, mte_helper.h.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>> target/arm/tcg/mte_helper.c | 184 +------------------------------
>> target/arm/tcg/mte_helper.h | 211 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 212 insertions(+), 183 deletions(-)
>> create mode 100644 target/arm/tcg/mte_helper.h
>
>> diff --git a/target/arm/tcg/mte_helper.h b/target/arm/tcg/mte_helper.h
>> new file mode 100644
>> index 0000000000..2d09345642
>> --- /dev/null
>> +++ b/target/arm/tcg/mte_helper.h
>> @@ -0,0 +1,211 @@
>> +/*
>> + * ARM MemTag Operation Helpers
>> + *
>> + * Copyright (c) 2024 Linaro, Ltd.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>
> Preferably parsable license tag:
>
> * SPDX-License-Identifier: LGPL-2.1-or-later
ok.
>> + */
>> +
>> +#ifndef TARGET_ARM_MTE_H
>> +#define TARGET_ARM_MTE_H
>> +
>> +#include "exec/exec-all.h"
>
> Why do you need "exec/exec-all.h"?
Otherwise one gets:
In file included from ../target/arm/gdbstub.c:30:
../target/arm/tcg/mte_helper.h: In function ‘allocation_tag_mem_probe’:
../target/arm/tcg/mte_helper.h:77:9: error: implicit declaration of function ‘cpu_loop_exit_sigsegv’; did you mean ‘cpu_loop_exit_noexc’? [-Werror=implicit-function-declaration]
77 | cpu_loop_exit_sigsegv(env_cpu(env), ptr, ptr_access,
| ^~~~~~~~~~~~~~~~~~~~~
| cpu_loop_exit_noexc
../target/arm/tcg/mte_helper.h:77:9: error: nested extern declaration of ‘cpu_loop_exit_sigsegv’ [-Werror=nested-externs]
Any other idea on how to satisfy it?
>> +#include "exec/ram_addr.h"
>> +#include "hw/core/tcg-cpu-ops.h"
>> +#include "qemu/log.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.)
>> + */
>> +static inline 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)
>
> Do we really need an inlined function? Since it calls non-inlined
> methods, I don't really see the point.
inline is just a hint and I think that in general at least the overhead
for calling this function is reduced, but it's hard to say what the
compile heuristics will do exactly without looking at the compiled code.
But I can remove it for this function and leave it just for {load,store}_tag1.
>> +{
>> +#ifdef CONFIG_USER_ONLY
>> + uint64_t clean_ptr = useronly_clean_ptr(ptr);
>> + int flags = page_get_flags(clean_ptr);
>> + uint8_t *tags;
>> + uintptr_t index;
>> +
>> + 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);
>> + }
>> +
>> + /* Require both MAP_ANON and PROT_MTE for the page. */
>> + if (!(flags & PAGE_ANON) || !(flags & PAGE_MTE)) {
>> + return NULL;
>> + }
>> +
>> + tags = page_get_target_data(clean_ptr);
>> +
>> + index = extract32(ptr, LOG2_TAG_GRANULE + 1,
>> + TARGET_PAGE_BITS - LOG2_TAG_GRANULE - 1);
>> + return tags + index;
>> +#else
>> + CPUTLBEntryFull *full;
>> + MemTxAttrs attrs;
>> + int in_page, flags;
>> + hwaddr ptr_paddr, tag_paddr, xlat;
>> + MemoryRegion *mr;
>> + ARMASIdx tag_asi;
>> + AddressSpace *tag_as;
>> + void *host;
>> +
>> + /*
>> + * Probe the first byte of the virtual address. This raises an
>> + * exception for inaccessible pages, and resolves the virtual address
>> + * into the softmmu tlb.
>> + *
>> + * When RA == 0, this is either a pure probe or a no-fault-expected probe.
>> + * Indicate to probe_access_flags no-fault, then either return NULL
>> + * for the pure probe, or assert that we received a valid page for the
>> + * no-fault-expected probe.
>> + */
>> + flags = probe_access_full(env, ptr, 0, ptr_access, ptr_mmu_idx,
>> + ra == 0, &host, &full, ra);
>> + if (probe && (flags & TLB_INVALID_MASK)) {
>> + return NULL;
>> + }
>> + assert(!(flags & TLB_INVALID_MASK));
>> +
>> + /* If the virtual page MemAttr != Tagged, access unchecked. */
>> + if (full->extra.arm.pte_attrs != 0xf0) {
>> + return NULL;
>> + }
>> +
>> + /*
>> + * If not backed by host ram, there is no tag storage: access unchecked.
>> + * This is probably a guest os bug though, so log it.
>> + */
>> + if (unlikely(flags & TLB_MMIO)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "Page @ 0x%" PRIx64 " indicates Tagged Normal memory "
>> + "but is not backed by host ram\n", ptr);
>> + return NULL;
>> + }
>> +
>> + /*
>> + * Remember these values across the second lookup below,
>> + * which may invalidate this pointer via tlb resize.
>> + */
>> + ptr_paddr = full->phys_addr | (ptr & ~TARGET_PAGE_MASK);
>> + attrs = full->attrs;
>> + full = NULL;
>> +
>> + /*
>> + * The Normal memory access can extend to the next page. E.g. a single
>> + * 8-byte access to the last byte of a page will check only the last
>> + * tag on the first page.
>> + * Any page access exception has priority over tag check exception.
>> + */
>> + in_page = -(ptr | TARGET_PAGE_MASK);
>> + if (unlikely(ptr_size > in_page)) {
>> + flags |= probe_access_full(env, ptr + in_page, 0, ptr_access,
>> + ptr_mmu_idx, ra == 0, &host, &full, ra);
>> + assert(!(flags & TLB_INVALID_MASK));
>> + }
>> +
>> + /* Any debug exception has priority over a tag check exception. */
>> + if (!probe && unlikely(flags & TLB_WATCHPOINT)) {
>> + int wp = ptr_access == MMU_DATA_LOAD ? BP_MEM_READ : BP_MEM_WRITE;
>> + assert(ra != 0);
>> + cpu_check_watchpoint(env_cpu(env), ptr, ptr_size, attrs, wp, ra);
>> + }
>> +
>> + /* Convert to the physical address in tag space. */
>> + tag_paddr = ptr_paddr >> (LOG2_TAG_GRANULE + 1);
>> +
>> + /* Look up the address in tag space. */
>> + tag_asi = attrs.secure ? ARMASIdx_TagS : ARMASIdx_TagNS;
>> + tag_as = cpu_get_address_space(env_cpu(env), tag_asi);
>> + mr = address_space_translate(tag_as, tag_paddr, &xlat, NULL,
>> + tag_access == MMU_DATA_STORE, attrs);
>> +
>> + /*
>> + * Note that @mr will never be NULL. If there is nothing in the address
>> + * space at @tag_paddr, the translation will return the unallocated memory
>> + * region. For our purposes, the result must be ram.
>> + */
>> + if (unlikely(!memory_region_is_ram(mr))) {
>> + /* ??? Failure is a board configuration error. */
>> + qemu_log_mask(LOG_UNIMP,
>> + "Tag Memory @ 0x%" HWADDR_PRIx " not found for "
>> + "Normal Memory @ 0x%" HWADDR_PRIx "\n",
>> + tag_paddr, ptr_paddr);
>> + return NULL;
>> + }
>> +
>> + /*
>> + * Ensure the tag memory is dirty on write, for migration.
>> + * Tag memory can never contain code or display memory (vga).
>> + */
>> + if (tag_access == MMU_DATA_STORE) {
>> + ram_addr_t tag_ra = memory_region_get_ram_addr(mr) + xlat;
>> + cpu_physical_memory_set_dirty_flag(tag_ra, DIRTY_MEMORY_MIGRATION);
>> + }
>> +
>> + return memory_region_get_ram_ptr(mr) + xlat;
>> +#endif
>> +}
>> +
>> +static inline int load_tag1(uint64_t ptr, uint8_t *mem)
>> +{
>> + int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
>> + return extract32(*mem, ofs, 4);
>> +}
>> +
>> +/* For use in a non-parallel context, store to the given nibble. */
>> +static 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);
>> +}
>> +
>> +#endif /* TARGET_ARM_MTE_H */
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-13 17:35 ` Philippe Mathieu-Daudé
@ 2024-06-13 18:15 ` Gustavo Romero
2024-06-14 9:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-13 18:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, peter.maydell,
alex.bennee, richard.henderson
Hi Phil,
On 6/13/24 2:35 PM, Philippe Mathieu-Daudé wrote:
> On 13/6/24 19:21, 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/mte.h | 53 +++++++++++++++++++++++++++++++
>> 2 files changed, 55 insertions(+), 20 deletions(-)
>> create mode 100644 target/arm/mte.h
>
>
>> diff --git a/target/arm/mte.h b/target/arm/mte.h
>> new file mode 100644
>> index 0000000000..89712aad70
>> --- /dev/null
>> +++ b/target/arm/mte.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * ARM MemTag convenience functions.
>> + *
>> + * Copyright (c) 2024 Linaro, Ltd.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef MTE_H
>> +#define MTE_H
>> +
>> +#ifdef CONFIG_TCG
>> +#ifdef CONFIG_USER_ONLY
>> +#include "sys/prctl.h"
>> +
>> +static void set_mte_tcf0(CPUArchState *env, abi_long value)
>
> Either declare it inlined (otherwise we'll get multiple symbols
> declared if this header is included multiple times), or
> preferably only expose the prototype.
>
> Also I'd use the 'arm_' prefix.
Thanks, I forgot to add the inline hint and was really wondering if
I should add the arm_ prefix.
>> +{
>> + /*
>> + * 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 /* CONFIG_TCG */
>> +
>> +#endif /* MTE_H */
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-13 18:15 ` Gustavo Romero
@ 2024-06-14 9:02 ` Philippe Mathieu-Daudé
2024-06-17 6:56 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-14 9:02 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, peter.maydell, alex.bennee,
richard.henderson
On 13/6/24 20:15, Gustavo Romero wrote:
> Hi Phil,
>
> On 6/13/24 2:35 PM, Philippe Mathieu-Daudé wrote:
>> On 13/6/24 19:21, 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/mte.h | 53 +++++++++++++++++++++++++++++++
>>> 2 files changed, 55 insertions(+), 20 deletions(-)
>>> create mode 100644 target/arm/mte.h
>>
>>
>>> diff --git a/target/arm/mte.h b/target/arm/mte.h
>>> new file mode 100644
>>> index 0000000000..89712aad70
>>> --- /dev/null
>>> +++ b/target/arm/mte.h
>>> @@ -0,0 +1,53 @@
>>> +/*
>>> + * ARM MemTag convenience functions.
>>> + *
>>> + * Copyright (c) 2024 Linaro, Ltd.
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see
>>> <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef MTE_H
>>> +#define MTE_H
>>> +
>>> +#ifdef CONFIG_TCG
>>> +#ifdef CONFIG_USER_ONLY
>>> +#include "sys/prctl.h"
>>> +
>>> +static void set_mte_tcf0(CPUArchState *env, abi_long value)
>>
>> Either declare it inlined (otherwise we'll get multiple symbols
>> declared if this header is included multiple times), or
>> preferably only expose the prototype.
>>
>> Also I'd use the 'arm_' prefix.
>
> Thanks, I forgot to add the inline hint and was really wondering if
> I should add the arm_ prefix.
If you expose the prototype, it can be used elsewhere. Here it
will be used by linux-user code. Althought it will be used by ARM
specific code, from this other subsystem PoV it will be clearer
that this method is ARM specific if the prefix is used. But I'm
being picky and it isn't a requirement.
However the question about why do we want this method inlined remains
(usually all inlined functions need a justification).
>>> +{
>>> + /*
>>> + * 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 /* CONFIG_TCG */
>>> +
>>> +#endif /* MTE_H */
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 8/9] gdbstub: Add support for MTE in user mode
2024-06-13 17:21 ` [PATCH v2 8/9] gdbstub: Add support for MTE in user mode Gustavo Romero
@ 2024-06-14 10:51 ` Alex Bennée
2024-06-14 16:16 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 10:51 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
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 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 | 253 +++++++++++++++++++++++++
> target/arm/internals.h | 2 +
> 5 files changed, 268 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..1cbcd6fa98 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -21,10 +21,13 @@
> #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"
> #include "cpregs.h"
> +#include "mte.h"
> +#include "tcg/mte_helper.h"
>
> typedef struct RegisterSysregFeatureParam {
> CPUState *cs;
> @@ -474,6 +477,246 @@ 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 = 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;
> +
> + 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.
> + */
> + 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 */
> +#endif /* TARGET_AARCH64 */
> +
> +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);
> +
> +#ifdef CONFIG_USER_ONLY
> + /* MTE */
> + if (cpu_isar_feature(aa64_mte3, cpu)) {
> + g_string_append(qsupported_features, ";memory-tagging+");
> +
> + g_array_append_val(query_table, cmd_handler_table[qMemTags]);
> + g_array_append_val(query_table, cmd_handler_table[qIsAddressTagged]);
> +
> + g_array_append_val(set_table, cmd_handler_table[QMemTags]);
> + }
> +#endif
This fails:
FAILED: libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o
cc -m64 -Ilibqemu-arm-linux-user.fa.p -I. -I../.. -Itarget/arm -I../../target/arm -I../../common-user/host/x86_64 -I../../linux-user/include/host/x86_64 -I../../linux-user/include -Ilinux-user -I../../linux-user -Ilinux-user/arm -I../../linux-user/arm -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/capstone -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote /home/alex/lsrc/qemu.git/host/include/x86_64 -iquote /home/alex/lsrc/qemu.git/host/include/generic -iquote /home/alex/lsrc/qemu.git/tcg/i386 -pthread -mcx16 -mpopcnt -msse4.2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -isystem../../linux-headers -isystemlinux-headers -DCOMPILING_PER_TARGET '-DCONFIG_TARGET="arm-linux-user-config-target.h"' '-DCONFIG_DEVICES="arm-linux-user-config-devices.h"' -MD -MQ libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o -MF libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o.d -o libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o -c ../../target/arm/gdbstub.c
In file included from /usr/include/glib-2.0/glib.h:33,
from /home/alex/lsrc/qemu.git/include/glib-compat.h:32,
from /home/alex/lsrc/qemu.git/include/qemu/osdep.h:161,
from ../../target/arm/gdbstub.c:20:
../../target/arm/gdbstub.c: In function ‘arm_cpu_register_gdb_commands’:
../../target/arm/gdbstub.c:693:41: error: ‘cmd_handler_table’ undeclared (first use in this function)
693 | g_array_append_val(query_table, cmd_handler_table[qMemTags]);
| ^~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
| ^
../../target/arm/gdbstub.c:693:41: note: each undeclared identifier is reported only once for each function it appears in
693 | g_array_append_val(query_table, cmd_handler_table[qMemTags]);
| ^~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
| ^
../../target/arm/gdbstub.c:693:59: error: ‘qMemTags’ undeclared (first use in this function)
693 | g_array_append_val(query_table, cmd_handler_table[qMemTags]);
| ^~~~~~~~
/usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
| ^
../../target/arm/gdbstub.c:694:59: error: ‘qIsAddressTagged’ undeclared (first use in this function)
694 | g_array_append_val(query_table, cmd_handler_table[qIsAddressTagged]);
| ^~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
| ^
../../target/arm/gdbstub.c:696:57: error: ‘QMemTags’ undeclared (first use in this function)
696 | g_array_append_val(set_table, cmd_handler_table[QMemTags]);
| ^~~~~~~~
/usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
| ^
In file included from ../../target/arm/gdbstub.c:29:
../../target/arm/mte.h: At top level:
../../target/arm/mte.h:27:13: error: ‘set_mte_tcf0’ defined but not used [-Werror=unused-function]
27 | static void set_mte_tcf0(CPUArchState *env, abi_long value)
| ^~~~~~~~~~~~
cc1: all warnings being treated as errors
As the command handler table has a different set of ifdef protections to
the fill code. However I think it might be easier to move all these bits
over to gdbstub64.c which is implicitly TARGET_AARCH64.
I would suggest keeping arm_cpu_register_gdb_commands but add something
like:
if (arm_feature(env, ARM_FEATURE_AARCH64)) {
#ifdef TARGET_AARCH64
aarch64_cpu_register_gdb_commands(...)
#endif
}
to setup the Aarch64 MTE specific bits.
> +
> + /* 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 +750,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/internals.h b/target/arm/internals.h
> index 11b5da2562..e27a9fa1e0 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);
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-13 17:21 ` [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
2024-06-13 17:35 ` Philippe Mathieu-Daudé
@ 2024-06-14 11:21 ` Alex Bennée
1 sibling, 0 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:21 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> 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/mte.h | 53 +++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 20 deletions(-)
> create mode 100644 target/arm/mte.h
>
> diff --git a/linux-user/aarch64/target_prctl.h b/linux-user/aarch64/target_prctl.h
> index aa8e203c15..8a11404222 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/mte.h"
We are moving a helper that is TCG only but using the common code path.
I suspect the prototype better belongs in target/arm/tcg/mte_helper.h or
possibly target/arm/tcg/mte_user_helper.h if we are just using it for
user-mode.
>
> 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);
> + set_mte_tcf0(env, arg2);
>
> /*
> * Write PR_MTE_TAG to GCR_EL1[Exclude].
> diff --git a/target/arm/mte.h b/target/arm/mte.h
> new file mode 100644
> index 0000000000..89712aad70
> --- /dev/null
> +++ b/target/arm/mte.h
> @@ -0,0 +1,53 @@
> +/*
> + * ARM MemTag convenience functions.
> + *
> + * Copyright (c) 2024 Linaro, Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef MTE_H
> +#define MTE_H
> +
> +#ifdef CONFIG_TCG
> +#ifdef CONFIG_USER_ONLY
> +#include "sys/prctl.h"
> +
> +static void set_mte_tcf0(CPUArchState *env, abi_long value)
Same comments as Phil regarding should it be inline, add a prefix etc...
> +{
> + /*
> + * 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 /* CONFIG_TCG */
> +
> +#endif /* MTE_H */
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/9] gdbstub: Clean up process_string_cmd
2024-06-13 17:20 ` [PATCH v2 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
@ 2024-06-14 11:24 ` Alex Bennée
0 siblings, 0 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:24 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> 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>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/9] gdbstub: Move GdbCmdParseEntry into a new header file
2024-06-13 17:20 ` [PATCH v2 2/9] gdbstub: Move GdbCmdParseEntry into a new header file Gustavo Romero
@ 2024-06-14 11:25 ` Alex Bennée
0 siblings, 0 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:25 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> 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>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/9] gdbstub: Add support for target-specific stubs
2024-06-13 17:20 ` [PATCH v2 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
@ 2024-06-14 11:27 ` Alex Bennée
2024-06-17 6:33 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:27 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)
> +{
Maybe g_assert(!extended_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)
> +{
g_assert(!extended_query_table)
> + 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);
> +
We should make it clear these functions should only be called once
(although I guess in a heterogeneous future we might have to do something
more clever).
> #endif /* GDBSTUB_COMMANDS_H */
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/9] target/arm: Fix exception case in allocation_tag_mem_probe
2024-06-13 17:20 ` [PATCH v2 4/9] target/arm: Fix exception case in allocation_tag_mem_probe Gustavo Romero
@ 2024-06-14 11:29 ` Alex Bennée
0 siblings, 0 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:29 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> 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>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 7/9] gdbstub: Make get cpu and hex conversion functions non-internal
2024-06-13 17:21 ` [PATCH v2 7/9] gdbstub: Make get cpu and hex conversion functions non-internal Gustavo Romero
@ 2024-06-14 11:34 ` Alex Bennée
0 siblings, 0 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:34 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> Make the gdb_first_attached_cpu and gdb_hextomem non-internal so they
> are not confined to use only in gdbstub.c.
so they can be used by architecture extensions to the gdbstub?
we don't want everyone using these functions, or if we do maybe we
should consider moving them to cutils?
Otherwise:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> 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 */
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests
2024-06-13 17:21 ` [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests Gustavo Romero
@ 2024-06-14 11:42 ` Alex Bennée
2024-06-14 15:58 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 11:42 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> 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..ec49eb8d2b
> --- /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 99", 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..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;
> + }
Whats this trying to do? I would expect the test case to be able to run
normally without being debugged by gdb, so why do we need a particular
command line to shortcut it here?
> +
> + /* 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;
> +}
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/9] target/arm: Make some MTE helpers widely available
2024-06-13 18:13 ` Gustavo Romero
@ 2024-06-14 12:34 ` Philippe Mathieu-Daudé
2024-06-17 6:37 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-14 12:34 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, peter.maydell, alex.bennee,
richard.henderson
On 13/6/24 20:13, Gustavo Romero wrote:
> Hi Phil!
>
> On 6/13/24 2:32 PM, Philippe Mathieu-Daudé wrote:
>> Hi Gustavo,
>>
>> On 13/6/24 19:20, Gustavo Romero wrote:
>>> Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
>>> available to other subsystems by moving them from mte_helper.c to a new
>>> header file, mte_helper.h.
>>>
>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>> ---
>>> target/arm/tcg/mte_helper.c | 184 +------------------------------
>>> target/arm/tcg/mte_helper.h | 211 ++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 212 insertions(+), 183 deletions(-)
>>> create mode 100644 target/arm/tcg/mte_helper.h
>>> + */
>>> +
>>> +#ifndef TARGET_ARM_MTE_H
>>> +#define TARGET_ARM_MTE_H
>>> +
>>> +#include "exec/exec-all.h"
>>
>> Why do you need "exec/exec-all.h"?
>
> Otherwise one gets:
>
> In file included from ../target/arm/gdbstub.c:30:
> ../target/arm/tcg/mte_helper.h: In function ‘allocation_tag_mem_probe’:
> ../target/arm/tcg/mte_helper.h:77:9: error: implicit declaration of
> function ‘cpu_loop_exit_sigsegv’; did you mean ‘cpu_loop_exit_noexc’?
> [-Werror=implicit-function-declaration]
> 77 | cpu_loop_exit_sigsegv(env_cpu(env), ptr, ptr_access,
> | ^~~~~~~~~~~~~~~~~~~~~
> | cpu_loop_exit_noexc
> ../target/arm/tcg/mte_helper.h:77:9: error: nested extern declaration of
> ‘cpu_loop_exit_sigsegv’ [-Werror=nested-externs]
>
> Any other idea on how to satisfy it?
OK, I'll fix once I get my include/exec/ rework merged.
>>> +#include "exec/ram_addr.h"
>>> +#include "hw/core/tcg-cpu-ops.h"
>>> +#include "qemu/log.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.)
>>> + */
>>> +static inline 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)
>>
>> Do we really need an inlined function? Since it calls non-inlined
>> methods, I don't really see the point.
>
> inline is just a hint and I think that in general at least the overhead
> for calling this function is reduced, but it's hard to say what the
> compile heuristics will do exactly without looking at the compiled code.
My question is about having the function definition in an header,
instead of its prototype (and the definition in a .c source file).
> But I can remove it for this function and leave it just for
> {load,store}_tag1.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/9] Add MTE stubs for aarch64 user mode
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
` (8 preceding siblings ...)
2024-06-13 17:21 ` [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests Gustavo Romero
@ 2024-06-14 15:49 ` Alex Bennée
2024-06-14 16:01 ` Gustavo Romero
2024-06-17 7:02 ` Gustavo Romero
9 siblings, 2 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-14 15:49 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> 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: gdb_extend_qsupported_features,
> gdb_extend_query_table, and gdb_extend_set_table. 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
It looks like there might be some BSD build failures as well:
https://gitlab.com/stsquad/qemu/-/pipelines/1332635371/failures
>
> v2:
> - Addressed comments from Richard, Phil, and Alex
> - Made the series more granular by splitting it into more patches
> - Moved gdbstub command-specific structs and functions into a new header, gdbstub/commands.h
> - Fixed exception in allocation_tag_mem_probe()
> - Used MTE helpers ({store,load}_tag1 and allocation_tag_mem_probe) in the MTE stubs
> - Factored out MTE code to set TCF0, avoiding duplication (both prctl and gdbstub code use it)
> - Hoisted sscanf() out of loop in handle_Q_memtag stub and use gdb_hextomem instead
> - Rebased this series on Alex's gdb/next branch
>
>
> Cheers,
> Gustavo
>
> Gustavo Romero (9):
> gdbstub: Clean up process_string_cmd
> gdbstub: Move GdbCmdParseEntry into a new header file
> gdbstub: Add support for target-specific stubs
> target/arm: Fix exception case in allocation_tag_mem_probe
> target/arm: Make some MTE helpers widely available
> target/arm: Factor out code for setting MTE TCF0 field
> gdbstub: Make get cpu and hex conversion functions non-internal
> gdbstub: Add support for MTE in user mode
> tests/tcg/aarch64: Add MTE gdbstub tests
>
> configs/targets/aarch64-linux-user.mak | 2 +-
> gdb-xml/aarch64-mte.xml | 11 ++
> gdbstub/gdbstub.c | 211 +++++++++++----------
> gdbstub/internals.h | 24 ---
> gdbstub/syscalls.c | 7 +-
> gdbstub/system.c | 7 +-
> gdbstub/user-target.c | 25 +--
> gdbstub/user.c | 7 +-
> include/exec/gdbstub.h | 5 +
> include/gdbstub/commands.h | 102 ++++++++++
> linux-user/aarch64/target_prctl.h | 22 +--
> target/arm/cpu.c | 1 +
> target/arm/gdbstub.c | 253 +++++++++++++++++++++++++
> target/arm/internals.h | 2 +
> target/arm/mte.h | 53 ++++++
> target/arm/tcg/mte_helper.c | 181 +-----------------
> target/arm/tcg/mte_helper.h | 211 +++++++++++++++++++++
> tests/tcg/aarch64/Makefile.target | 11 +-
> tests/tcg/aarch64/gdbstub/test-mte.py | 86 +++++++++
> tests/tcg/aarch64/mte-8.c | 102 ++++++++++
> 20 files changed, 975 insertions(+), 348 deletions(-)
> create mode 100644 gdb-xml/aarch64-mte.xml
> create mode 100644 include/gdbstub/commands.h
> create mode 100644 target/arm/mte.h
> create mode 100644 target/arm/tcg/mte_helper.h
> create mode 100644 tests/tcg/aarch64/gdbstub/test-mte.py
> create mode 100644 tests/tcg/aarch64/mte-8.c
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests
2024-06-14 11:42 ` Alex Bennée
@ 2024-06-14 15:58 ` Gustavo Romero
0 siblings, 0 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-14 15:58 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Hi Alex!
On 6/14/24 8:42 AM, Alex Bennée wrote:
> Gustavo Romero <gustavo.romero@linaro.org> writes:
>
>> 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..ec49eb8d2b
>> --- /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 99", 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..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;
>> + }
>
> Whats this trying to do? I would expect the test case to be able to run
> normally without being debugged by gdb, so why do we need a particular
> command line to shortcut it here?
Good catch. This is a leftover. The first versions of the test
would cause a sigsegv, but I simplified it on this final version,
so it runs normally now.
Removed in v3. Thanks
>> +
>> + /* 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;
>> +}
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/9] Add MTE stubs for aarch64 user mode
2024-06-14 15:49 ` [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Alex Bennée
@ 2024-06-14 16:01 ` Gustavo Romero
2024-06-17 7:02 ` Gustavo Romero
1 sibling, 0 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-14 16:01 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
On 6/14/24 12:49 PM, Alex Bennée wrote:
> Gustavo Romero <gustavo.romero@linaro.org> writes:
>
>> 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: gdb_extend_qsupported_features,
>> gdb_extend_query_table, and gdb_extend_set_table. 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
>
> It looks like there might be some BSD build failures as well:
>
> https://gitlab.com/stsquad/qemu/-/pipelines/1332635371/failures
Thanks, I'm looking at them.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 8/9] gdbstub: Add support for MTE in user mode
2024-06-14 10:51 ` Alex Bennée
@ 2024-06-14 16:16 ` Gustavo Romero
0 siblings, 0 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-14 16:16 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Hi Alex,
On 6/14/24 7:51 AM, Alex Bennée wrote:
> 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 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 | 253 +++++++++++++++++++++++++
>> target/arm/internals.h | 2 +
>> 5 files changed, 268 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..1cbcd6fa98 100644
>> --- a/target/arm/gdbstub.c
>> +++ b/target/arm/gdbstub.c
>> @@ -21,10 +21,13 @@
>> #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"
>> #include "cpregs.h"
>> +#include "mte.h"
>> +#include "tcg/mte_helper.h"
>>
>> typedef struct RegisterSysregFeatureParam {
>> CPUState *cs;
>> @@ -474,6 +477,246 @@ 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 = 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;
>> +
>> + 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.
>> + */
>> + 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 */
>> +#endif /* TARGET_AARCH64 */
>> +
>> +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);
>> +
>> +#ifdef CONFIG_USER_ONLY
>> + /* MTE */
>> + if (cpu_isar_feature(aa64_mte3, cpu)) {
>> + g_string_append(qsupported_features, ";memory-tagging+");
>> +
>> + g_array_append_val(query_table, cmd_handler_table[qMemTags]);
>> + g_array_append_val(query_table, cmd_handler_table[qIsAddressTagged]);
>> +
>> + g_array_append_val(set_table, cmd_handler_table[QMemTags]);
>> + }
>> +#endif
>
> This fails:
>
> FAILED: libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o
> cc -m64 -Ilibqemu-arm-linux-user.fa.p -I. -I../.. -Itarget/arm -I../../target/arm -I../../common-user/host/x86_64 -I../../linux-user/include/host/x86_64 -I../../linux-user/include -Ilinux-user -I../../linux-user -Ilinux-user/arm -I../../linux-user/arm -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/capstone -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote /home/alex/lsrc/qemu.git/host/include/x86_64 -iquote /home/alex/lsrc/qemu.git/host/include/generic -iquote /home/alex/lsrc/qemu.git/tcg/i386 -pthread -mcx16 -mpopcnt -msse4.2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -isystem../../linux-headers -isystemlinux-headers -DCOMPILING_PER_TARGET '-DCONFIG_TARGET="arm-linux-user-config-target.h"' '-DCONFIG_DEVICES="arm-linux-user-config-devices.h"' -MD -MQ libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o -MF libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o.d -o libqemu-arm-linux-user.fa.p/target_arm_gdbstub.c.o -c ../../target/arm/gdbstub.c
> In file included from /usr/include/glib-2.0/glib.h:33,
> from /home/alex/lsrc/qemu.git/include/glib-compat.h:32,
> from /home/alex/lsrc/qemu.git/include/qemu/osdep.h:161,
> from ../../target/arm/gdbstub.c:20:
> ../../target/arm/gdbstub.c: In function ‘arm_cpu_register_gdb_commands’:
> ../../target/arm/gdbstub.c:693:41: error: ‘cmd_handler_table’ undeclared (first use in this function)
> 693 | g_array_append_val(query_table, cmd_handler_table[qMemTags]);
> | ^~~~~~~~~~~~~~~~~
> /usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
> 66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
> | ^
> ../../target/arm/gdbstub.c:693:41: note: each undeclared identifier is reported only once for each function it appears in
> 693 | g_array_append_val(query_table, cmd_handler_table[qMemTags]);
> | ^~~~~~~~~~~~~~~~~
> /usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
> 66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
> | ^
> ../../target/arm/gdbstub.c:693:59: error: ‘qMemTags’ undeclared (first use in this function)
> 693 | g_array_append_val(query_table, cmd_handler_table[qMemTags]);
> | ^~~~~~~~
> /usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
> 66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
> | ^
> ../../target/arm/gdbstub.c:694:59: error: ‘qIsAddressTagged’ undeclared (first use in this function)
> 694 | g_array_append_val(query_table, cmd_handler_table[qIsAddressTagged]);
> | ^~~~~~~~~~~~~~~~
> /usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
> 66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
> | ^
> ../../target/arm/gdbstub.c:696:57: error: ‘QMemTags’ undeclared (first use in this function)
> 696 | g_array_append_val(set_table, cmd_handler_table[QMemTags]);
> | ^~~~~~~~
> /usr/include/glib-2.0/glib/garray.h:66:61: note: in definition of macro ‘g_array_append_val’
> 66 | #define g_array_append_val(a,v) g_array_append_vals (a, &(v), 1)
> | ^
> In file included from ../../target/arm/gdbstub.c:29:
> ../../target/arm/mte.h: At top level:
> ../../target/arm/mte.h:27:13: error: ‘set_mte_tcf0’ defined but not used [-Werror=unused-function]
> 27 | static void set_mte_tcf0(CPUArchState *env, abi_long value)
> | ^~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> As the command handler table has a different set of ifdef protections to
> the fill code. However I think it might be easier to move all these bits
> over to gdbstub64.c which is implicitly TARGET_AARCH64.
>
> I would suggest keeping arm_cpu_register_gdb_commands but add something
> like:
>
> if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> #ifdef TARGET_AARCH64
> aarch64_cpu_register_gdb_commands(...)
> #endif
> }
>
> to setup the Aarch64 MTE specific bits.
You got that error building for arm-linux-user, which I really tested
many times. I don't understand how that sneaked in. Anyways, let me
move it to gdbstub64.c.
Got a question tho, the qsupported string and the table pointers need
to be passed to aarch64_cpu_register_gdb_commands, besides 'cpu', so they
can be expanded in gdbstub64.c and later, if necessary, continue to
expanded in gdbstub.c, so it would be:
aarch64_cpu_register_gdb_commands(cpu, qsupported_features, query_table, set_table)
Is that your suggestion?
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/9] gdbstub: Add support for target-specific stubs
2024-06-14 11:27 ` Alex Bennée
@ 2024-06-17 6:33 ` Gustavo Romero
2024-06-17 9:41 ` Alex Bennée
0 siblings, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:33 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Hi Alex,
On 6/14/24 8:27 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)
>> +{
>
> Maybe g_assert(!extended_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)
>> +{
>
> g_assert(!extended_query_table)
>
>> + 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);
>> +
>
> We should make it clear these functions should only be called once
> (although I guess in a heterogeneous future we might have to do something
> more clever).
There a some cases where actually the API functions are called multiple
times it looks like. For instance, the assert is hit in the following test:
$ ./build/qemu-aarch64 ./build/tests/tcg/aarch64-linux-user/munmap-pthread
Probably because pthread_create is involved, but I can't explain it yet.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/9] target/arm: Make some MTE helpers widely available
2024-06-14 12:34 ` Philippe Mathieu-Daudé
@ 2024-06-17 6:37 ` Gustavo Romero
0 siblings, 0 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, peter.maydell,
alex.bennee, richard.henderson
Hi Phil,
On 6/14/24 9:34 AM, Philippe Mathieu-Daudé wrote:
> On 13/6/24 20:13, Gustavo Romero wrote:
>> Hi Phil!
>>
>> On 6/13/24 2:32 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Gustavo,
>>>
>>> On 13/6/24 19:20, Gustavo Romero wrote:
>>>> Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
>>>> available to other subsystems by moving them from mte_helper.c to a new
>>>> header file, mte_helper.h.
>>>>
>>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>> ---
>>>> target/arm/tcg/mte_helper.c | 184 +------------------------------
>>>> target/arm/tcg/mte_helper.h | 211 ++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 212 insertions(+), 183 deletions(-)
>>>> create mode 100644 target/arm/tcg/mte_helper.h
>
>
>>>> + */
>>>> +
>>>> +#ifndef TARGET_ARM_MTE_H
>>>> +#define TARGET_ARM_MTE_H
>>>> +
>>>> +#include "exec/exec-all.h"
>>>
>>> Why do you need "exec/exec-all.h"?
>>
>> Otherwise one gets:
>>
>> In file included from ../target/arm/gdbstub.c:30:
>> ../target/arm/tcg/mte_helper.h: In function ‘allocation_tag_mem_probe’:
>> ../target/arm/tcg/mte_helper.h:77:9: error: implicit declaration of function ‘cpu_loop_exit_sigsegv’; did you mean ‘cpu_loop_exit_noexc’? [-Werror=implicit-function-declaration]
>> 77 | cpu_loop_exit_sigsegv(env_cpu(env), ptr, ptr_access,
>> | ^~~~~~~~~~~~~~~~~~~~~
>> | cpu_loop_exit_noexc
>> ../target/arm/tcg/mte_helper.h:77:9: error: nested extern declaration of ‘cpu_loop_exit_sigsegv’ [-Werror=nested-externs]
>>
>> Any other idea on how to satisfy it?
>
> OK, I'll fix once I get my include/exec/ rework merged.
>
>>>> +#include "exec/ram_addr.h"
>>>> +#include "hw/core/tcg-cpu-ops.h"
>>>> +#include "qemu/log.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.)
>>>> + */
>>>> +static inline 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)
>>>
>>> Do we really need an inlined function? Since it calls non-inlined
>>> methods, I don't really see the point.
>>
>> inline is just a hint and I think that in general at least the overhead
>> for calling this function is reduced, but it's hard to say what the
>> compile heuristics will do exactly without looking at the compiled code.
>
> My question is about having the function definition in an header,
> instead of its prototype (and the definition in a .c source file).
Got it. I've moved the definition back to .c file and used only the
protypes in .h. I removed the inline for this function but kept the
inline for {load,store}_tag1 because I understand they can be easily
inline by the compiler. Please see v3.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field
2024-06-14 9:02 ` Philippe Mathieu-Daudé
@ 2024-06-17 6:56 ` Gustavo Romero
0 siblings, 0 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-17 6:56 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, peter.maydell,
alex.bennee, richard.henderson
Hi Phil,
On 6/14/24 6:02 AM, Philippe Mathieu-Daudé wrote:
> On 13/6/24 20:15, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 6/13/24 2:35 PM, Philippe Mathieu-Daudé wrote:
>>> On 13/6/24 19:21, 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/mte.h | 53 +++++++++++++++++++++++++++++++
>>>> 2 files changed, 55 insertions(+), 20 deletions(-)
>>>> create mode 100644 target/arm/mte.h
>>>
>>>
>>>> diff --git a/target/arm/mte.h b/target/arm/mte.h
>>>> new file mode 100644
>>>> index 0000000000..89712aad70
>>>> --- /dev/null
>>>> +++ b/target/arm/mte.h
>>>> @@ -0,0 +1,53 @@
>>>> +/*
>>>> + * ARM MemTag convenience functions.
>>>> + *
>>>> + * Copyright (c) 2024 Linaro, Ltd.
>>>> + *
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#ifndef MTE_H
>>>> +#define MTE_H
>>>> +
>>>> +#ifdef CONFIG_TCG
>>>> +#ifdef CONFIG_USER_ONLY
>>>> +#include "sys/prctl.h"
>>>> +
>>>> +static void set_mte_tcf0(CPUArchState *env, abi_long value)
>>>
>>> Either declare it inlined (otherwise we'll get multiple symbols
>>> declared if this header is included multiple times), or
>>> preferably only expose the prototype.
>>>
>>> Also I'd use the 'arm_' prefix.
>>
>> Thanks, I forgot to add the inline hint and was really wondering if
>> I should add the arm_ prefix.
>
> If you expose the prototype, it can be used elsewhere. Here it
> will be used by linux-user code. Althought it will be used by ARM
> specific code, from this other subsystem PoV it will be clearer
> that this method is ARM specific if the prefix is used. But I'm
> being picky and it isn't a requirement.
That's alright :) I think that using the prefix is the right thing
to do. I added it in v3.
> However the question about why do we want this method inlined remains
> (usually all inlined functions need a justification).
I can't see how using 'static' would cause multiple symbols declared
if the header is included multiple times. If multiple .c files include
such a header each one would get its own independent copy of the function,
so the function would be local to each translation unit. Just like defining
a function of the same name in different .c but marking them as static, so
keeping them local to the translation unit.
That said, I agree it should be 'inline'. That's a tiny function and it
seems really simple to be inlined by the compiler.
This function can't be just marked as 'inline' because deposit() is 'static',
hence in v3 it's marked as 'static inline', just like many other cases in .h
we have.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/9] Add MTE stubs for aarch64 user mode
2024-06-14 15:49 ` [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Alex Bennée
2024-06-14 16:01 ` Gustavo Romero
@ 2024-06-17 7:02 ` Gustavo Romero
2024-06-17 9:50 ` Alex Bennée
1 sibling, 1 reply; 35+ messages in thread
From: Gustavo Romero @ 2024-06-17 7:02 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Hi Alex,
On 6/14/24 12:49 PM, Alex Bennée wrote:
> Gustavo Romero <gustavo.romero@linaro.org> writes:
>
>> 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: gdb_extend_qsupported_features,
>> gdb_extend_query_table, and gdb_extend_set_table. 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
>
> It looks like there might be some BSD build failures as well:
>
> https://gitlab.com/stsquad/qemu/-/pipelines/1332635371/failures
I fixed the build for the BSD target in v3, however I think that the GDB in
the test containers need to be bumped to GDB 15 so the MTE tests can pass,
because support for IsAddressTagged packet is necessary. GDB 15 branch
is created by it's not released yet, so I don't know to proceed..
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/9] gdbstub: Add support for target-specific stubs
2024-06-17 6:33 ` Gustavo Romero
@ 2024-06-17 9:41 ` Alex Bennée
0 siblings, 0 replies; 35+ messages in thread
From: Alex Bennée @ 2024-06-17 9:41 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> Hi Alex,
>
> On 6/14/24 8:27 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)
>>> +{
>> Maybe g_assert(!extended_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)
>>> +{
>> g_assert(!extended_query_table)
>>
>>> + 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);
>>> +
>> We should make it clear these functions should only be called once
>> (although I guess in a heterogeneous future we might have to do something
>> more clever).
>
> There a some cases where actually the API functions are called multiple
> times it looks like. For instance, the assert is hit in the following test:
>
> $ ./build/qemu-aarch64 ./build/tests/tcg/aarch64-linux-user/munmap-pthread
>
> Probably because pthread_create is involved, but I can't explain it
> yet.
Hmm, I guess new threads still need to register with gdb. I wonder if asserts like:
g_assert(extended_set_table || extended_set_table == table)
would be valid?
>
>
> Cheers,
> Gustavo
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/9] Add MTE stubs for aarch64 user mode
2024-06-17 7:02 ` Gustavo Romero
@ 2024-06-17 9:50 ` Alex Bennée
2024-06-24 5:40 ` Gustavo Romero
0 siblings, 1 reply; 35+ messages in thread
From: Alex Bennée @ 2024-06-17 9:50 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Gustavo Romero <gustavo.romero@linaro.org> writes:
> Hi Alex,
>
> On 6/14/24 12:49 PM, Alex Bennée wrote:
>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>
>>> 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: gdb_extend_qsupported_features,
>>> gdb_extend_query_table, and gdb_extend_set_table. 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
>> It looks like there might be some BSD build failures as well:
>> https://gitlab.com/stsquad/qemu/-/pipelines/1332635371/failures
>
> I fixed the build for the BSD target in v3, however I think that the GDB in
> the test containers need to be bumped to GDB 15 so the MTE tests can pass,
> because support for IsAddressTagged packet is necessary. GDB 15 branch
> is created by it's not released yet, so I don't know to proceed..
Two potential approaches, you could extend the configure segment:
if test -n "$gdb_bin"; then
gdb_version=$($gdb_bin --version | head -n 1)
if version_ge ${gdb_version##* } 9.1; then
gdb_arches=$($python "$source_path/scripts/probe-gdb-support.py" $gdb_bin)
else
gdb_bin=""
fi
fi
and set a variable exported to config-host.mak to then test in the tcg
test makefiles.
Or you could implement a gdb-version-test command in
tests/tcg/Makefile.target which you could use like the existing
cc-test/cc-option commands to extend config-cc.mak and use that to gate
the tests.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/9] Add MTE stubs for aarch64 user mode
2024-06-17 9:50 ` Alex Bennée
@ 2024-06-24 5:40 ` Gustavo Romero
0 siblings, 0 replies; 35+ messages in thread
From: Gustavo Romero @ 2024-06-24 5:40 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, philmd, peter.maydell, richard.henderson
Hi Alex,
On 6/17/24 6:50 AM, Alex Bennée wrote:
> Gustavo Romero <gustavo.romero@linaro.org> writes:
>
>> Hi Alex,
>>
>> On 6/14/24 12:49 PM, Alex Bennée wrote:
>>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>>
>>>> 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: gdb_extend_qsupported_features,
>>>> gdb_extend_query_table, and gdb_extend_set_table. 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
>>> It looks like there might be some BSD build failures as well:
>>> https://gitlab.com/stsquad/qemu/-/pipelines/1332635371/failures
>>
>> I fixed the build for the BSD target in v3, however I think that the GDB in
>> the test containers need to be bumped to GDB 15 so the MTE tests can pass,
>> because support for IsAddressTagged packet is necessary. GDB 15 branch
>> is created by it's not released yet, so I don't know to proceed..
>
> Two potential approaches, you could extend the configure segment:
>
> if test -n "$gdb_bin"; then
> gdb_version=$($gdb_bin --version | head -n 1)
> if version_ge ${gdb_version##* } 9.1; then
> gdb_arches=$($python "$source_path/scripts/probe-gdb-support.py" $gdb_bin)
> else
> gdb_bin=""
> fi
> fi
>
> and set a variable exported to config-host.mak to then test in the tcg
> test makefiles.
Done in v4. Thanks for the suggestions!
Cheers,
Gustavo
> Or you could implement a gdb-version-test command in
> tests/tcg/Makefile.target which you could use like the existing
> cc-test/cc-option commands to extend config-cc.mak and use that to gate
> the tests.
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2024-06-24 5:40 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-13 17:20 [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Gustavo Romero
2024-06-13 17:20 ` [PATCH v2 1/9] gdbstub: Clean up process_string_cmd Gustavo Romero
2024-06-14 11:24 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 2/9] gdbstub: Move GdbCmdParseEntry into a new header file Gustavo Romero
2024-06-14 11:25 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 3/9] gdbstub: Add support for target-specific stubs Gustavo Romero
2024-06-14 11:27 ` Alex Bennée
2024-06-17 6:33 ` Gustavo Romero
2024-06-17 9:41 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 4/9] target/arm: Fix exception case in allocation_tag_mem_probe Gustavo Romero
2024-06-14 11:29 ` Alex Bennée
2024-06-13 17:20 ` [PATCH v2 5/9] target/arm: Make some MTE helpers widely available Gustavo Romero
2024-06-13 17:32 ` Philippe Mathieu-Daudé
2024-06-13 18:13 ` Gustavo Romero
2024-06-14 12:34 ` Philippe Mathieu-Daudé
2024-06-17 6:37 ` Gustavo Romero
2024-06-13 17:21 ` [PATCH v2 6/9] target/arm: Factor out code for setting MTE TCF0 field Gustavo Romero
2024-06-13 17:35 ` Philippe Mathieu-Daudé
2024-06-13 18:15 ` Gustavo Romero
2024-06-14 9:02 ` Philippe Mathieu-Daudé
2024-06-17 6:56 ` Gustavo Romero
2024-06-14 11:21 ` Alex Bennée
2024-06-13 17:21 ` [PATCH v2 7/9] gdbstub: Make get cpu and hex conversion functions non-internal Gustavo Romero
2024-06-14 11:34 ` Alex Bennée
2024-06-13 17:21 ` [PATCH v2 8/9] gdbstub: Add support for MTE in user mode Gustavo Romero
2024-06-14 10:51 ` Alex Bennée
2024-06-14 16:16 ` Gustavo Romero
2024-06-13 17:21 ` [PATCH v2 9/9] tests/tcg/aarch64: Add MTE gdbstub tests Gustavo Romero
2024-06-14 11:42 ` Alex Bennée
2024-06-14 15:58 ` Gustavo Romero
2024-06-14 15:49 ` [PATCH v2 0/9] Add MTE stubs for aarch64 user mode Alex Bennée
2024-06-14 16:01 ` Gustavo Romero
2024-06-17 7:02 ` Gustavo Romero
2024-06-17 9:50 ` Alex Bennée
2024-06-24 5:40 ` Gustavo Romero
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).