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