Pierrick Bouvier <pierrick.bouv...@linaro.org> writes:
> On 3/20/25 12:30, Pierrick Bouvier wrote:
>> On 3/19/25 11:22, Alex Bennée wrote:
>>> The current helper.h functions rely on hard coded assumptions about
>>> target endianess to use the tswap macros. We also end up double
>>> swapping a bunch of values if the target can run in multiple endianess
>>> modes. Avoid this by getting the target to pass the endianess and size
>>> via a MemOp and fixing up appropriately.
>>>
>>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>>> ---
>>> include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>>> gdbstub/gdbstub.c | 22 ++++++++++++++++++++++
>>> 2 files changed, 52 insertions(+)
>>> create mode 100644 include/gdbstub/registers.h
>>>
>>> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
>>> new file mode 100644
>>> index 0000000000..4abc7a6ae7
>>> --- /dev/null
>>> +++ b/include/gdbstub/registers.h
>>> @@ -0,0 +1,30 @@
>>> +/*
>>> + * GDB Common Register Helpers
>>> + *
>>> + * Copyright (c) 2025 Linaro Ltd
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef GDB_REGISTERS_H
>>> +#define GDB_REGISTERS_H
>>> +
>>> +#include "exec/memop.h"
>>> +
>>> +/**
>>> + * gdb_get_register_value() - get register value for gdb
>>> + * mo: size and endian MemOp
>>> + * buf: GByteArray to store in target order
>>> + * val: pointer to value in host order
>>> + *
>>> + * This replaces the previous legacy read functions with a single
>>> + * function to handle all sizes. Passing @mo allows the target mode to
>>> + * be taken into account and avoids using hard coded tswap() macros.
>>> + *
>>> + * Returns the number of bytes written to the array.
>>> + */
>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
>>> +
>>> +#endif /* GDB_REGISTERS_H */
>>> +
>>> +
>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>> index 282e13e163..3d7b1028e4 100644
>>> --- a/gdbstub/gdbstub.c
>>> +++ b/gdbstub/gdbstub.c
>>> @@ -32,6 +32,7 @@
>>> #include "exec/gdbstub.h"
>>> #include "gdbstub/commands.h"
>>> #include "gdbstub/syscalls.h"
>>> +#include "gdbstub/registers.h"
>>> #ifdef CONFIG_USER_ONLY
>>> #include "accel/tcg/vcpu-state.h"
>>> #include "gdbstub/user.h"
>>> @@ -45,6 +46,7 @@
>>> #include "system/runstate.h"
>>> #include "exec/replay-core.h"
>>> #include "exec/hwaddr.h"
>>> +#include "exec/memop.h"
>>> #include "internals.h"
>>> @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState
>>> *cpu, uint8_t *mem_buf, int reg)
>>> return 0;
>>> }
>>> +/*
>>> + * Target helper function to read value into GByteArray, target
>>> + * supplies the size and target endianess via the MemOp.
>>> + */
>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
>>> +{
>>> + size_t bytes = memop_size(op);
>>> +
>>> + if (op & MO_BSWAP) {
>>> + for ( int i = bytes ; i > 0; i--) {
>>> + g_byte_array_append(buf, &val[i - 1], 1);
>>> + };
>>> + } else {
>>> + g_byte_array_append(buf, val, bytes);
>>> + }
>>> +
>>> + return bytes;
>>> +}
>>> +
>>> +
>>> static void gdb_register_feature(CPUState *cpu, int base_reg,
>>> gdb_get_reg_cb get_reg, gdb_set_reg_cb
>>> set_reg,
>>> const GDBFeature *feature)
>> It could be preferable to set buf with the value, instead of simply
>> appending the value. This way, there is no need to return the size, as
>> it's contained in buffer size itself.
>> If we insist on returning the size, it's better to make it a
>> parameter
>> (and use a void parameter type), because at the moment, it gives the
>> impression the function itself returns the value, which may be confusing.
>
> Seems like it's the existing convention through
> gdb_set_reg_cb/gdb_get_reg_cb, so we have to follow this.
For the "g" packet we append multiple registers so the buffer size grows
as we append each one.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
