[PATCH] arm/monitor: add register name resolution

[PATCH] arm/monitor: add register name resolution

[Nikola Brkovic]

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)},
+
+    { NULL }
+};
+
+const MonitorDef *target_monitor_defs(void)
+{
+    return monitor_defs;
+}
-- 
2.37.3

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

[Peter Maydell]

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

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

[Nikola Brkovic]

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