qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Nikola Brkovic
Subject: Re: [PATCH] arm/monitor: add register name resolution
Date: Fri, 23 Sep 2022 20:11:01 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]