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 | 36 +++++++++++++++++++++++++++++++++++-
 target/rx/cpu.c   |  9 ++-------
 target/rx/cpu.h   |  3 +++
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 4afd77efd5..9e395ae345 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 "exec/cpu_ldst.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
 #include "system/qtest.h"
@@ -56,6 +57,34 @@ 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);
+
+    cpu_reset(cs);
+
+    if (env->use_reset_pc) {
+        /*
+         * Load the PC with the starting address for the kernel
+         */
+        env->pc = env->reset_pc;
+    } else {
+        /*
+         * 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 void rx_load_image(RXCPU *cpu, const char *filename,
                           uint32_t start, uint32_t size)
 {
@@ -68,7 +97,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;
+    cpu->env.reset_pc = start;
+    cpu->env.use_reset_pc = true;
 
     /* setup exception trap trampoline */
     /* linux kernel only works little-endian mode */
@@ -87,6 +117,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 +184,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, void *data)
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 37a6fdd569..528cda486c 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -76,7 +76,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);
@@ -84,11 +83,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;
@@ -142,7 +136,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     rcc->parent_realize(dev, errp);
 }
@@ -189,6 +182,8 @@ static void rx_cpu_init(Object *obj)
 {
     RXCPU *cpu = RX_CPU(obj);
 
+    cpu->env.reset_pc = 0;
+    cpu->env.use_reset_pc = false;
     qdev_init_gpio_in(DEVICE(cpu), rx_cpu_set_irq, 2);
 }
 
diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index 5ba1874bd7..c42a03efb3 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -98,6 +98,9 @@ typedef struct CPUArchState {
     uint32_t ack_ipl;           /* execute ipl */
     float_status fp_status;
     qemu_irq ack;               /* Interrupt acknowledge */
+
+    bool use_reset_pc;          /* Use reset_pc instead of reset vector */
+    uint32_t reset_pc;          /* PC reset value when use_reset_pc */
 } CPURXState;
 
 /*
-- 
2.47.2

On Tue, 18 Feb 2025 at 21:22, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> 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 | 36 +++++++++++++++++++++++++++++++++++-
>  target/rx/cpu.c   |  9 ++-------
>  target/rx/cpu.h   |  3 +++
>  3 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
> index 4afd77efd5..9e395ae345 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 "exec/cpu_ldst.h"
>  #include "hw/loader.h"
>  #include "hw/rx/rx62n.h"
>  #include "system/qtest.h"
> @@ -56,6 +57,34 @@ 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);
> +
> +    cpu_reset(cs);
> +
> +    if (env->use_reset_pc) {
> +        /*
> +         * Load the PC with the starting address for the kernel
> +         */
> +        env->pc = env->reset_pc;
> +    } else {
> +        /*
> +         * 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);
> +        }
> +    }
> +}

Unless there's a strong reason for doing something different,
I would favour following the same pattern arm does for this.
(Or were you following existing code in some other target?
I certainly wouldn't be surprised if we already did this in
multiple different ways...)

Anyway, Arm splits up the work like this:
 * the CPU reset function does the "load initial PC from
   reset vector table" part (including using rom_ptr_for_as()
   to decide whether to do cpu_ldl_data() or not)
 * the board boot code's reset function does:
    cpu_reset();
    if (need to override the start PC because of the image loaded) {
        cpu_set_pc(cs, image_pc);
    }
    /* and any other CPU setup that's specific to kernel load etc */

That way if the user chooses to use the 'generic loader'
(-device loader) to load their guest image rather than
-kernel, we will correctly load the reset PC out
of their image.

You might then prefer to put the initial image_pc into
the RxGdbSimMachineState instead of the CPURXState,
since the code that cares about it directly is all
in hw/rx/ rather than target/rx/.

thanks
-- PMM

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.


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