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 <kei...@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
