diff mbox series

[v5,7/9] gdbstub: Make get cpu and hex conversion functions non-internal

Message ID 20240627041349.356704-8-gustavo.romero@linaro.org (mailing list archive)
State New, archived
Headers show
Series Add MTE stubs for aarch64 user mode | expand

Commit Message

Gustavo Romero June 27, 2024, 4:13 a.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé June 27, 2024, 6:10 a.m. UTC | #1 
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);

Regards,

Phil.
Philippe Mathieu-Daudé June 27, 2024, 6:19 a.m. UTC | #2 
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...
Alex Bennée June 27, 2024, 11:05 a.m. UTC | #3 
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.
Philippe Mathieu-Daudé June 27, 2024, 12:26 p.m. UTC | #4 
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;
>       }
Gustavo Romero June 28, 2024, 5:12 a.m. UTC | #5 
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
diff mbox series

Patch

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 */