Re: [PATCH] arm/monitor: add register name resolution

[Nikola Brkovic <nb91605@student.uni-lj.si>]

Hello, thank you for your feedback.

The choice of registers was relatively arbitrary, I chose registers
that would be accessible and valid under all profiles and not part
of the CP15.

I will look into how this could be implemented through gdbstub,
as I was not aware of that possibility.

Kind regards,
Nikola

On 9/22/22 15:15, Peter Maydell wrote:
> On Sat, 10 Sept 2022 at 15:14, Nikola Brkovic <nb91605@student.uni-lj.si> wrote:
>> This patch allows the monitor to resolve the
>> stack pointer, instruction pointer,
>> system status register and FPU status register
>> on ARM targets.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1145
>>
>> Signed-off-by: Nikola Brkovic <nb91605@student.uni-lj.si>
>> ---
>>   target/arm/monitor.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
>> index 80c64fa355..143c95bca4 100644
>> --- a/target/arm/monitor.c
>> +++ b/target/arm/monitor.c
>> @@ -31,6 +31,7 @@
>>   #include "qapi/qmp/qerror.h"
>>   #include "qapi/qmp/qdict.h"
>>   #include "qom/qom-qobject.h"
>> +#include "monitor/hmp-target.h"
>>
>>   static GICCapability *gic_cap_new(int version)
>>   {
>> @@ -228,3 +229,31 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>
>>       return expansion_info;
>>   }
>> +
>> +static target_long monitor_get_cpsr(Monitor *mon, const struct MonitorDef *md,
>> +                                    int val)
>> +{
>> +    CPUArchState *env = mon_get_cpu_env(mon);
>> +    return cpsr_read(env);
>> +}
>> +
>> +const MonitorDef monitor_defs[] = {
>> +    { "sp|r13", offsetof(CPUARMState, regs[13])},
>> +    { "lr|r14", offsetof(CPUARMState, regs[14])},
>> +#ifndef TARGET_AARCH64
>> +    { "pc|r15|ip", offsetof(CPUARMState, regs[15]) },
>> +#else
>> +    { "pc|ip", offsetof(CPUARMState, pc) },
>> +#endif
>> +
>> +    /* State registers */
>> +    { "cpsr", 0, &monitor_get_cpsr},
>> +    { "fpscr", offsetof(CPUARMState, vfp.fp_status)},
> Hi; thanks for this patch. Unfortunately, it doesn't look to
> me like it handles the fact that 64-bit vs 32-bit is a runtime
> question, not a compile-time one:
>
> (1) if this is a 64-bit CPU executing AArch64 code then we
> shouldn't be resolving sp/lr as regs[13] and regs[14]
> (2) if this is a 64-bit CPU executing AArch32 code then
> we shouldn't be resolving pc/ip as env->pc.
>
> As a more minor bug:
> (3) fpscr isn't only in vfp.fp_status, it's more complicated
> than that -- you need to call vfp_get_fpscr().
>
> Also, why only these registers ?
>
> As a wider design thing, I'm not really a fan of target_monitor_defs():
> it's the kind of quick hack that got added to QEMU decades ago
> when we supported about three different target architectures but
> which doesn't scale to the set we support now. It would be nicer
> to be able to tie this into the existing gdbstub support code for
> reading and writing registers. (Though that too has issues with
> CPUs which support multiple modes like AArch32 vs AArch64).
>
> thanks
> -- PMM