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>
Reviewed-by: Richard Henderson <richard.henderson@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.1On 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.romero@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@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); Regards, Phil.
Philippe Mathieu-Daudé <philmd@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.romero@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@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
On 27/6/24 13:05, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@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.romero@linaro.org>
>>> Reviewed-by: Richard Henderson <richard.henderson@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 +++++++++++++++------------
> @@ -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;
LGTM.
> + }
> +
> gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
> - cmd->handler(params, NULL);
> + cmd->handler(params, user_ctx);
> return true;
> }
Hi Phil, Alex,
On 6/27/24 9:26 AM, Philippe Mathieu-Daudé wrote:
> On 27/6/24 13:05, Alex Bennée wrote:
>> Philippe Mathieu-Daudé <philmd@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.romero@linaro.org>
>>>> Reviewed-by: Richard Henderson <richard.henderson@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 +++++++++++++++------------
>
>
>> @@ -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;
>
> LGTM.
Thanks for the suggestion. I added it to v6.
Cheers,
Gustavo
On 27/6/24 08:10, Philippe Mathieu-Daudé wrote: > 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.romero@linaro.org> >> Reviewed-by: Richard Henderson <richard.henderson@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); (With GDBRegisterState typedef being forward-declared). That said, this can be done as another series on top...
© 2016 - 2024 Red Hat, Inc.