Philippe Mathieu-Daudé <phi...@linaro.org> writes:
> On 27/6/24 06:13, Gustavo Romero wrote:
>> 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.rom...@linaro.org>
>> Reviewed-by: Richard Henderson <richard.hender...@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/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);
>
> Alex, it seems dubious to expose the API like that.
>
> IMHO GdbCmdHandler should take a GDBRegisterState argument,
> then this would become:
>
> CPUState *gdb_first_attached_cpu(GDBRegisterState *s);
Maybe instead of exposing this we can use user_ctx for something? If we
look at handle_set_reg/handle_get_reg we can see that passes down
gdbserver_state.g_cpu down to the eventual helpers. We could define
something like:
--8<---------------cut here---------------start------------->8---
fixups to avoid get_first_cpu()
5 files changed, 25 insertions(+), 18 deletions(-)
gdbstub/internals.h | 1 +
include/exec/gdbstub.h | 5 -----
include/gdbstub/commands.h | 3 +++
gdbstub/gdbstub.c | 7 ++++++-
target/arm/gdbstub64.c | 27 +++++++++++++++------------
modified gdbstub/internals.h
@@ -129,6 +129,7 @@ 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 */
modified include/exec/gdbstub.h
@@ -135,9 +135,4 @@ 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 */
modified include/gdbstub/commands.h
@@ -54,6 +54,8 @@ typedef union GdbCmdVariant {
* "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.
+ *
+ * @need_cpu_context: pass current CPU to command via user_ctx.
*/
typedef struct GdbCmdParseEntry {
GdbCmdHandler handler;
@@ -61,6 +63,7 @@ typedef struct GdbCmdParseEntry {
bool cmd_startswith;
const char *schema;
bool allow_stop_reply;
+ bool need_cpu_context;
} GdbCmdParseEntry;
#define get_cmd_parsers(p) (&g_array_index(p, GdbCmdParseEntry, 0))
modified gdbstub/gdbstub.c
@@ -938,6 +938,7 @@ static bool process_string_cmd(const char *data,
for (i = 0; i < num_cmds; i++) {
const GdbCmdParseEntry *cmd = &cmds[i];
+ void *user_ctx = NULL;
g_assert(cmd->handler && cmd->cmd);
if ((cmd->cmd_startswith && !startswith(data, cmd->cmd)) ||
@@ -952,8 +953,12 @@ static bool process_string_cmd(const char *data,
}
}
+ if (cmd->need_cpu_context) {
+ user_ctx = (void *) gdbserver_state.g_cpu;
+ }
+
gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
- cmd->handler(params, NULL);
+ cmd->handler(params, user_ctx);
return true;
}
modified target/arm/gdbstub64.c
@@ -427,9 +427,9 @@ int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf,
int reg)
return 1;
}
-static void handle_q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
+static void handle_q_memtag(GArray *params, void *user_ctx)
{
- ARMCPU *cpu = ARM_CPU(gdb_first_attached_cpu());
+ ARMCPU *cpu = ARM_CPU(user_ctx);
CPUARMState *env = &cpu->env;
uint64_t addr = gdb_get_cmd_param(params, 0)->val_ull;
@@ -470,9 +470,9 @@ static void handle_q_memtag(GArray *params, G_GNUC_UNUSED
void *user_ctx)
gdb_put_packet(str_buf->str);
}
-static void handle_q_isaddresstagged(GArray *params, G_GNUC_UNUSED void
*user_ctx)
+static void handle_q_isaddresstagged(GArray *params, void *user_ctx)
{
- ARMCPU *cpu = ARM_CPU(gdb_first_attached_cpu());
+ ARMCPU *cpu = ARM_CPU(user_ctx);
CPUARMState *env = &cpu->env;
uint64_t addr = gdb_get_cmd_param(params, 0)->val_ull;
@@ -487,9 +487,9 @@ static void handle_q_isaddresstagged(GArray *params,
G_GNUC_UNUSED void *user_ct
gdb_put_packet(reply);
}
-static void handle_Q_memtag(GArray *params, G_GNUC_UNUSED void *user_ctx)
+static void handle_Q_memtag(GArray *params, void *user_ctx)
{
- ARMCPU *cpu = ARM_CPU(gdb_first_attached_cpu());
+ ARMCPU *cpu = ARM_CPU(user_ctx);
CPUARMState *env = &cpu->env;
uint64_t start_addr = gdb_get_cmd_param(params, 0)->val_ull;
@@ -567,21 +567,24 @@ enum Command {
static GdbCmdParseEntry cmd_handler_table[NUM_CMDS] = {
[qMemTags] = {
.handler = handle_q_memtag,
- .cmd_startswith = 1,
+ .cmd_startswith = true,
.cmd = "MemTags:",
- .schema = "L,l:l0"
+ .schema = "L,l:l0",
+ .need_cpu_context = true,
},
[qIsAddressTagged] = {
.handler = handle_q_isaddresstagged,
- .cmd_startswith = 1,
+ .cmd_startswith = true,
.cmd = "IsAddressTagged:",
- .schema = "L0"
+ .schema = "L0",
+ .need_cpu_context = true,
},
[QMemTags] = {
.handler = handle_Q_memtag,
- .cmd_startswith = 1,
+ .cmd_startswith = true,
.cmd = "MemTags:",
- .schema = "L,l:l:s0"
+ .schema = "L,l:l:s0",
+ .need_cpu_context = true,
},
};
#endif /* CONFIG_USER_ONLY */
--8<---------------cut here---------------end--------------->8---
>
> Regards,
>
> Phil.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
