Re: [PATCH 3/4] hw/rx: Reset the CPU at qemu reset time

[Keith Packard via <qemu-devel@nongnu.org>]

[-- Attachment #1.1: Type: text/plain, Size: 277 bytes --]

From: Peter Maydell <peter.maydell@linaro.org>
Date: Fri, 07 Mar 2025 13:26:14 +0000

> Unless there's a strong reason for doing something different,
> I would favour following the same pattern arm does for this.

Thanks for the suggested cleanup; this looks a lot nicer now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-hw-rx-Reset-the-CPU-at-qemu-reset-time.patch --]
[-- Type: text/x-diff, Size: 5188 bytes --]

From 02d0f2b006500dec62e91bd571a8722c354133e5 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Wed, 12 Feb 2025 16:42:38 -0800
Subject: [PATCH] hw/rx: Reset the CPU at qemu reset time

This ensure that the CPU gets reset every time QEMU resets. Use either
the kernel entry point or the reset vector if no kernel was loaded.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/rx/rx-gdbsim.c | 31 ++++++++++++++++++++++++++++++-
 target/rx/cpu.c   | 20 +++++++++++++-------
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 5b9004e9e1..5765cb91c6 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -22,6 +22,7 @@
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "accel/tcg/cpu-ldst.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
 #include "system/qtest.h"
@@ -47,6 +48,9 @@ struct RxGdbSimMachineState {
     MachineState parent_obj;
     /*< public >*/
     RX62NState mcu;
+
+    bool use_reset_pc;          /* Use reset_pc instead of reset vector */
+    uint32_t reset_pc;          /* PC reset value when use_reset_pc */
 };
 typedef struct RxGdbSimMachineState RxGdbSimMachineState;
 
@@ -56,9 +60,29 @@ DECLARE_OBJ_CHECKERS(RxGdbSimMachineState, RxGdbSimMachineClass,
                      RX_GDBSIM_MACHINE, TYPE_RX_GDBSIM_MACHINE)
 
 
+static void rx_cpu_reset(void *opaque)
+{
+    RXCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+    CPURXState *env = cpu_env(cs);
+    MachineState *machine = MACHINE(qdev_get_machine());
+    RxGdbSimMachineState *s = RX_GDBSIM_MACHINE(machine);
+
+    cpu_reset(cs);
+
+    if (s->use_reset_pc) {
+        /*
+         * Load the PC with the starting address for the kernel
+         */
+        env->pc = s->reset_pc;
+    }
+}
+
 static void rx_load_image(RXCPU *cpu, const char *filename,
                           uint32_t start, uint32_t size)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
+    RxGdbSimMachineState *s = RX_GDBSIM_MACHINE(machine);
     static uint32_t extable[32];
     long kernel_size;
     int i;
@@ -68,7 +92,8 @@ static void rx_load_image(RXCPU *cpu, const char *filename,
         fprintf(stderr, "qemu: could not load kernel '%s'\n", filename);
         exit(1);
     }
-    cpu->env.pc = start;
+    s->reset_pc = start;
+    s->use_reset_pc = true;
 
     /* setup exception trap trampoline */
     /* linux kernel only works little-endian mode */
@@ -87,6 +112,7 @@ static void rx_gdbsim_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *dtb_filename = machine->dtb;
     uint8_t rng_seed[32];
+    CPUState *cs;
 
     if (machine->ram_size < mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
@@ -153,6 +179,9 @@ static void rx_gdbsim_init(MachineState *machine)
             s->mcu.cpu.env.regs[1] = SDRAM_BASE + dtb_offset;
         }
     }
+    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
+        qemu_register_reset(rx_cpu_reset, RX_CPU(cs));
+    }
 }
 
 static void rx_gdbsim_class_init(ObjectClass *oc, const void *data)
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index c6dd5d6f83..4d568bbd92 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -29,6 +29,7 @@
 #include "fpu/softfloat.h"
 #include "tcg/debug-assert.h"
 #include "accel/tcg/cpu-ops.h"
+#include "accel/tcg/cpu-ldst.h"
 
 static void rx_cpu_set_pc(CPUState *cs, vaddr value)
 {
@@ -89,7 +90,6 @@ static void rx_cpu_reset_hold(Object *obj, ResetType type)
     CPUState *cs = CPU(obj);
     RXCPUClass *rcc = RX_CPU_GET_CLASS(obj);
     CPURXState *env = cpu_env(cs);
-    uint32_t *resetvec;
 
     if (rcc->parent_phases.hold) {
         rcc->parent_phases.hold(obj, type);
@@ -97,11 +97,6 @@ static void rx_cpu_reset_hold(Object *obj, ResetType type)
 
     memset(env, 0, offsetof(CPURXState, end_reset_fields));
 
-    resetvec = rom_ptr(0xfffffffc, 4);
-    if (resetvec) {
-        /* In the case of kernel, it is ignored because it is not set. */
-        env->pc = ldl_p(resetvec);
-    }
     rx_cpu_unpack_psw(env, 0, 1);
     env->regs[0] = env->isp = env->usp = 0;
     env->fpsw = 0;
@@ -124,6 +119,18 @@ static void rx_cpu_reset_hold(Object *obj, ResetType type)
      * be the correct setting.
      */
     set_float_ftz_detection(float_ftz_before_rounding, &env->fp_status);
+
+    /*
+     * Load the initial PC from the reset vector. If there is
+     * a ROM containing that vector use that, otherwise read
+     * it from target memory.
+     */
+    uint32_t *resetvec_p = rom_ptr_for_as(cs->as, 0xfffffffc, 4);
+    if (resetvec_p) {
+        env->pc = ldl_p(resetvec_p);
+    } else {
+        env->pc = cpu_ldl_data(env, 0xfffffffc);
+    }
 }
 
 static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
@@ -155,7 +162,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     rcc->parent_realize(dev, errp);
 }
-- 
2.49.0


[-- Attachment #1.3: Type: text/plain, Size: 243 bytes --]


Btw, I went and looked at other qemu target support for -kernel; a whole
bunch support loading of arbitrary .elf files using that parameter. I
had to temporarily add that patch back in to test the enclosed reset
change...

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]