[RFC PATCH 00/12] cpu_reset clean-ups for arm, sh4, mips, m68k and tricore

[RFC PATCH 00/12] cpu_reset clean-ups for arm, sh4, mips, m68k and tricore

[Alex Bennée]

We tend to apply cpu_reset inconsistently throughout our various
models which leads to unintended ordering dependencies. This got in
the way in my last plugins series:

  https://patchew.org/QEMU/20251219190849.238323-1-alex.bennee@linaro.org/

where I needed to shuffle things around to ensure that gdb register
creation was done after dependant peripherals had created their cpu
interfaces.

Regardless of that we do have a proper reset interface now and most
architectures have moved to it. This series attempts to clean-up the
remaining cases with proper qemu_register_reset() calls so reset is
called when we intend to.

Alex.

Alex Bennée (12):
  target/sh4: drop cpu_reset from realizefn
  target/m68k: introduce env->reset_pc
  hw/m68k: register a nextcube_cpu_reset handler
  hw/m68k: register a mcf5208evb_cpu_reset handler
  hw/m68k: register a an5206_cpu_reset handler
  hw/m68k: just use reset_pc for virt platform
  target/m68k: drop cpu_reset on realizefn
  hw/mips: defer finalising gcr_base until reset time
  hw/mips: drop cpu_reset in mips_cpu_realizefn
  target/tricore: move cpu_reset from tricore_cpu_realizefn
  target/arm: remove extraneous cpu_reset from realizefn
  include/hw: expand cpu_reset function docs

 include/hw/core/cpu.h |  3 +++
 target/m68k/cpu.h     |  1 +
 hw/m68k/an5206.c      | 24 +++++++++++++++++-------
 hw/m68k/mcf5208.c     | 25 +++++++++++++++++++------
 hw/m68k/next-cube.c   | 23 +++++++++++++++++------
 hw/m68k/virt.c        | 24 +++++++-----------------
 hw/mips/cps.c         | 22 +++++++++++++---------
 hw/misc/mips_cmgcr.c  |  1 -
 target/arm/cpu.c      |  1 -
 target/m68k/cpu.c     |  1 -
 target/mips/cpu.c     |  1 -
 target/sh4/cpu.c      |  1 -
 target/tricore/cpu.c  |  9 ++++++++-
 13 files changed, 85 insertions(+), 51 deletions(-)

-- 
2.47.3

[RFC PATCH 01/12] target/sh4: drop cpu_reset from realizefn

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/sh4/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 21ccb86df48..1dd21ad9ed6 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -255,7 +255,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     scc->parent_realize(dev, errp);
-- 
2.47.3

Re: [RFC PATCH 01/12] target/sh4: drop cpu_reset from realizefn

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/sh4/cpu.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 21ccb86df48..1dd21ad9ed6 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -255,7 +255,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    cpu_reset(cs);
>       qemu_init_vcpu(cs);
>   
>       scc->parent_realize(dev, errp);

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 01/12] target/sh4: drop cpu_reset from realizefn

[Philippe Mathieu-Daudé]

On 8/1/26 15:34, Alex Bennée wrote:

Missing the Why?

Queued taking from cover letter:

"Shuffle things around to ensure that gdb register creation was
done after dependant peripherals had created their cpu interfaces."

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/sh4/cpu.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 21ccb86df48..1dd21ad9ed6 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -255,7 +255,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    cpu_reset(cs);
>       qemu_init_vcpu(cs);
>   
>       scc->parent_realize(dev, errp);

[RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[Alex Bennée]

To transition CPUs to use the multi-phase resettable logic we need to
stash some information for the reset handlers. Arm does this with
arm_boot_info but for m68k all we really need is the PC we should
reset to.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/m68k/cpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index d9db6a486a8..fda015c4b7b 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -155,6 +155,7 @@ typedef struct CPUArchState {
 
     /* Fields from here on are preserved across CPU reset. */
     uint64_t features;
+    uint32_t reset_pc;
 } CPUM68KState;
 
 /*
-- 
2.47.3

Re: [RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On Thu, 8 Jan 2026, Alex Bennée wrote:
> To transition CPUs to use the multi-phase resettable logic we need to
> stash some information for the reset handlers. Arm does this with
> arm_boot_info but for m68k all we really need is the PC we should
> reset to.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/m68k/cpu.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index d9db6a486a8..fda015c4b7b 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -155,6 +155,7 @@ typedef struct CPUArchState {
>
>     /* Fields from here on are preserved across CPU reset. */
>     uint64_t features;
> +    uint32_t reset_pc;

The m64k CPUs are 32 bit but should this better use some other type like 
vaddr or hwaddr here?

Regards.
BALATON Zoltan

> } CPUM68KState;
>
> /*
>

Re: [RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On Thu, 8 Jan 2026, BALATON Zoltan wrote:
> On Thu, 8 Jan 2026, Alex Bennée wrote:
>> To transition CPUs to use the multi-phase resettable logic we need to
>> stash some information for the reset handlers. Arm does this with
>> arm_boot_info but for m68k all we really need is the PC we should
>> reset to.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> target/m68k/cpu.h | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index d9db6a486a8..fda015c4b7b 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -155,6 +155,7 @@ typedef struct CPUArchState {
>>
>>     /* Fields from here on are preserved across CPU reset. */
>>     uint64_t features;
>> +    uint32_t reset_pc;
>
> The m64k CPUs are 32 bit but should this better use some other type like 
> vaddr or hwaddr here?

Nevermind. This is matching what pc is using so that makes sense.

Regards.
BALATON Zoltan

>> } CPUM68KState;
>> 
>> /*
>

Re: [RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[Alex Bennée]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 8 Jan 2026, Alex Bennée wrote:
>> To transition CPUs to use the multi-phase resettable logic we need to
>> stash some information for the reset handlers. Arm does this with
>> arm_boot_info but for m68k all we really need is the PC we should
>> reset to.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> target/m68k/cpu.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index d9db6a486a8..fda015c4b7b 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -155,6 +155,7 @@ typedef struct CPUArchState {
>>
>>     /* Fields from here on are preserved across CPU reset. */
>>     uint64_t features;
>> +    uint32_t reset_pc;
>
> The m64k CPUs are 32 bit but should this better use some other type
> like vaddr or hwaddr here?

Possibly - but the underlying env->pc is also a unint32_t so I didn't
see the point of adding more churn.

>
> Regards.
> BALATON Zoltan
>
>> } CPUM68KState;
>>
>> /*
>>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[Thomas Huth]

Am Thu,  8 Jan 2026 14:34:13 +0000
schrieb Alex Bennée <alex.bennee@linaro.org>:

> To transition CPUs to use the multi-phase resettable logic we need to
> stash some information for the reset handlers. Arm does this with
> arm_boot_info but for m68k all we really need is the PC we should
> reset to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/m68k/cpu.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index d9db6a486a8..fda015c4b7b 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -155,6 +155,7 @@ typedef struct CPUArchState {
>  
>      /* Fields from here on are preserved across CPU reset. */
>      uint64_t features;
> +    uint32_t reset_pc;
>  } CPUM68KState;

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> To transition CPUs to use the multi-phase resettable logic we need to
> stash some information for the reset handlers. Arm does this with
> arm_boot_info but for m68k all we really need is the PC we should
> reset to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/m68k/cpu.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index d9db6a486a8..fda015c4b7b 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -155,6 +155,7 @@ typedef struct CPUArchState {
>   
>       /* Fields from here on are preserved across CPU reset. */
>       uint64_t features;
> +    uint32_t reset_pc;
>   } CPUM68KState;
>   
>   /*

I think this ought to merge with something else.
Just adding the field is clearly incomplete.

r~

Re: [RFC PATCH 02/12] target/m68k: introduce env->reset_pc

[Richard Henderson]

On 1/11/26 18:52, Richard Henderson wrote:
> On 1/9/26 01:34, Alex Bennée wrote:
>> To transition CPUs to use the multi-phase resettable logic we need to
>> stash some information for the reset handlers. Arm does this with
>> arm_boot_info but for m68k all we really need is the PC we should
>> reset to.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   target/m68k/cpu.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index d9db6a486a8..fda015c4b7b 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -155,6 +155,7 @@ typedef struct CPUArchState {
>>       /* Fields from here on are preserved across CPU reset. */
>>       uint64_t features;
>> +    uint32_t reset_pc;
>>   } CPUM68KState;
>>   /*
> 
> I think this ought to merge with something else.
> Just adding the field is clearly incomplete.

Nevermind, I now see how it's used for the various m68k machines.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[RFC PATCH 03/12] hw/m68k: register a nextcube_cpu_reset handler

[Alex Bennée]

This is a fairly simple migration to the handler. Alternatively we
could eschew stashing the value in reset_pc and just re-read the ROM
on reset.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/m68k/next-cube.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 26177c7b867..3d66f5e7607 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -16,6 +16,7 @@
 #include "exec/cpu-interrupt.h"
 #include "system/system.h"
 #include "system/qtest.h"
+#include "system/reset.h"
 #include "hw/core/irq.h"
 #include "hw/m68k/next-cube.h"
 #include "hw/core/boards.h"
@@ -1249,6 +1250,19 @@ static const TypeInfo next_pc_info = {
     .class_init = next_pc_class_init,
 };
 
+static void nextcube_cpu_reset(void *opaque)
+{
+    M68kCPU *cpu = opaque;
+    CPUM68KState *env = &cpu->env;
+    CPUState *cs = CPU(cpu);
+
+    cpu_reset(cs);
+    /* Initialize CPU registers.  */
+    env->vbr = 0;
+    env->sr  = 0x2700;
+    env->pc = env->reset_pc;
+}
+
 static void next_cube_init(MachineState *machine)
 {
     NeXTState *m = NEXT_MACHINE(machine);
@@ -1264,12 +1278,9 @@ static void next_cube_init(MachineState *machine)
         error_report("Unable to find m68k CPU definition");
         exit(1);
     }
+    qemu_register_reset(nextcube_cpu_reset, cpu);
     env = &cpu->env;
 
-    /* Initialize CPU registers.  */
-    env->vbr = 0;
-    env->sr  = 0x2700;
-
     /* Peripheral Controller */
     pcdev = qdev_new(TYPE_NEXT_PC);
     object_property_set_link(OBJECT(pcdev), "cpu", OBJECT(cpu), &error_abort);
@@ -1335,8 +1346,8 @@ static void next_cube_init(MachineState *machine)
         /* Initial PC is always at offset 4 in firmware binaries */
         ptr = rom_ptr(0x01000004, 4);
         g_assert(ptr != NULL);
-        env->pc = ldl_be_p(ptr);
-        if (env->pc >= 0x01020000) {
+        env->reset_pc = ldl_be_p(ptr);
+        if (env->reset_pc >= 0x01020000) {
             error_report("'%s' does not seem to be a valid firmware image.",
                          bios_name);
             exit(1);
-- 
2.47.3

Re: [RFC PATCH 03/12] hw/m68k: register a nextcube_cpu_reset handler

[Thomas Huth]

Am Thu,  8 Jan 2026 14:34:14 +0000
schrieb Alex Bennée <alex.bennee@linaro.org>:

> This is a fairly simple migration to the handler. Alternatively we
> could eschew stashing the value in reset_pc and just re-read the ROM
> on reset.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/m68k/next-cube.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [RFC PATCH 03/12] hw/m68k: register a nextcube_cpu_reset handler

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> This is a fairly simple migration to the handler. Alternatively we
> could eschew stashing the value in reset_pc and just re-read the ROM
> on reset.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   hw/m68k/next-cube.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[RFC PATCH 04/12] hw/m68k: register a mcf5208evb_cpu_reset handler

[Alex Bennée]

It looks like allowing a -kernel to override any firmware setting is
intentional.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/m68k/mcf5208.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index c6d1c5fae9f..5680d516a23 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -27,6 +27,7 @@
 #include "qemu/timer.h"
 #include "hw/core/ptimer.h"
 #include "system/system.h"
+#include "system/reset.h"
 #include "system/qtest.h"
 #include "net/net.h"
 #include "hw/core/boards.h"
@@ -274,6 +275,20 @@ static void mcf_fec_init(MemoryRegion *sysmem, hwaddr base, qemu_irq *irqs)
     memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(s, 0));
 }
 
+static void mcf5208evb_cpu_reset(void *opaque)
+{
+    M68kCPU *cpu = opaque;
+    CPUM68KState *env = &cpu->env;
+    CPUState *cs = CPU(cpu);
+
+    cpu_reset(cs);
+    /* Initialize CPU registers.  */
+    env->vbr = 0;
+    /* TODO: Configure BARs.  */
+    env->aregs[7] = ldl_phys(cs->as, 0);
+    env->pc = env->reset_pc;
+}
+
 static void mcf5208evb_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
@@ -289,11 +304,9 @@ static void mcf5208evb_init(MachineState *machine)
     MemoryRegion *sram = g_new(MemoryRegion, 1);
 
     cpu = M68K_CPU(cpu_create(machine->cpu_type));
-    env = &cpu->env;
+    qemu_register_reset(mcf5208evb_cpu_reset, cpu);
 
-    /* Initialize CPU registers.  */
-    env->vbr = 0;
-    /* TODO: Configure BARs.  */
+    env = &cpu->env;
 
     /* ROM at 0x00000000 */
     memory_region_init_rom(rom, NULL, "mcf5208.rom", ROM_SIZE, &error_fatal);
@@ -359,7 +372,7 @@ static void mcf5208evb_init(MachineState *machine)
         /* Initial PC is always at offset 4 in firmware binaries */
         ptr = rom_ptr(0x4, 4);
         assert(ptr != NULL);
-        env->pc = ldl_be_p(ptr);
+        env->reset_pc = ldl_be_p(ptr);
     }
 
     /* Load kernel.  */
@@ -388,7 +401,7 @@ static void mcf5208evb_init(MachineState *machine)
         exit(1);
     }
 
-    env->pc = entry;
+    env->reset_pc = entry;
 }
 
 static void mcf5208evb_machine_init(MachineClass *mc)
-- 
2.47.3

Re: [RFC PATCH 04/12] hw/m68k: register a mcf5208evb_cpu_reset handler

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> It looks like allowing a -kernel to override any firmware setting is
> intentional.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   hw/m68k/mcf5208.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 04/12] hw/m68k: register a mcf5208evb_cpu_reset handler

[Peter Maydell]

On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> It looks like allowing a -kernel to override any firmware setting is
> intentional.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/m68k/mcf5208.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index c6d1c5fae9f..5680d516a23 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -27,6 +27,7 @@
>  #include "qemu/timer.h"
>  #include "hw/core/ptimer.h"
>  #include "system/system.h"
> +#include "system/reset.h"
>  #include "system/qtest.h"
>  #include "net/net.h"
>  #include "hw/core/boards.h"
> @@ -274,6 +275,20 @@ static void mcf_fec_init(MemoryRegion *sysmem, hwaddr base, qemu_irq *irqs)
>      memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(s, 0));
>  }
>
> +static void mcf5208evb_cpu_reset(void *opaque)
> +{
> +    M68kCPU *cpu = opaque;
> +    CPUM68KState *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
> +
> +    cpu_reset(cs);
> +    /* Initialize CPU registers.  */
> +    env->vbr = 0;
> +    /* TODO: Configure BARs.  */
> +    env->aregs[7] = ldl_phys(cs->as, 0);
> +    env->pc = env->reset_pc;
> +}

It looks from my quick skim of the m68k manual like
"on reset, initial PC is read from offset 4, initial
SP is read from offset 0" is part of the architected
CPU reset, not board-specific. Maybe we should model
that inside the CPU reset method? Compare M-profile
Arm, which does basically the same thing in
arm_cpu_reset_hold().

(I really need to look again at whether we can move
that to the reset_exit phase now and drop the awkward
"maybe we need to look at a rom blob" special casing.)

-- PMM

Re: [RFC PATCH 04/12] hw/m68k: register a mcf5208evb_cpu_reset handler

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

On Tue, 13 Jan 2026, Peter Maydell wrote:
> On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> It looks like allowing a -kernel to override any firmware setting is
>> intentional.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/m68k/mcf5208.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
>> index c6d1c5fae9f..5680d516a23 100644
>> --- a/hw/m68k/mcf5208.c
>> +++ b/hw/m68k/mcf5208.c
>> @@ -27,6 +27,7 @@
>>  #include "qemu/timer.h"
>>  #include "hw/core/ptimer.h"
>>  #include "system/system.h"
>> +#include "system/reset.h"
>>  #include "system/qtest.h"
>>  #include "net/net.h"
>>  #include "hw/core/boards.h"
>> @@ -274,6 +275,20 @@ static void mcf_fec_init(MemoryRegion *sysmem, hwaddr base, qemu_irq *irqs)
>>      memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(s, 0));
>>  }
>>
>> +static void mcf5208evb_cpu_reset(void *opaque)
>> +{
>> +    M68kCPU *cpu = opaque;
>> +    CPUM68KState *env = &cpu->env;
>> +    CPUState *cs = CPU(cpu);
>> +
>> +    cpu_reset(cs);
>> +    /* Initialize CPU registers.  */
>> +    env->vbr = 0;
>> +    /* TODO: Configure BARs.  */
>> +    env->aregs[7] = ldl_phys(cs->as, 0);
>> +    env->pc = env->reset_pc;
>> +}
>
> It looks from my quick skim of the m68k manual like
> "on reset, initial PC is read from offset 4, initial
> SP is read from offset 0" is part of the architected
> CPU reset, not board-specific. Maybe we should model
> that inside the CPU reset method? Compare M-profile
> Arm, which does basically the same thing in
> arm_cpu_reset_hold().
>
> (I really need to look again at whether we can move
> that to the reset_exit phase now and drop the awkward
> "maybe we need to look at a rom blob" special casing.)

Real machines usually have a way to remap ROM to address 0 on reset 
shadowing RAM and then the firmware flips the bits needed to set up the 
memory map with RAM at 0 and ROM at somewhere higher up but the way to do 
it is different for each board. Maybe in QEMU we don't bother to model 
this and have this special case instead to simplify things and be able to 
start from the final ROM address without needing to model this ROM remap 
thing. So modeling real CPU behaviour might make board models more 
complex.

Regards,
BALATON Zoltan

Re: [RFC PATCH 04/12] hw/m68k: register a mcf5208evb_cpu_reset handler

[Peter Maydell]

On Tue, 13 Jan 2026 at 18:08, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 13 Jan 2026, Peter Maydell wrote:
> > On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> It looks like allowing a -kernel to override any firmware setting is
> >> intentional.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  hw/m68k/mcf5208.c | 25 +++++++++++++++++++------
> >>  1 file changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> >> index c6d1c5fae9f..5680d516a23 100644
> >> --- a/hw/m68k/mcf5208.c
> >> +++ b/hw/m68k/mcf5208.c
> >> @@ -27,6 +27,7 @@
> >>  #include "qemu/timer.h"
> >>  #include "hw/core/ptimer.h"
> >>  #include "system/system.h"
> >> +#include "system/reset.h"
> >>  #include "system/qtest.h"
> >>  #include "net/net.h"
> >>  #include "hw/core/boards.h"
> >> @@ -274,6 +275,20 @@ static void mcf_fec_init(MemoryRegion *sysmem, hwaddr base, qemu_irq *irqs)
> >>      memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(s, 0));
> >>  }
> >>
> >> +static void mcf5208evb_cpu_reset(void *opaque)
> >> +{
> >> +    M68kCPU *cpu = opaque;
> >> +    CPUM68KState *env = &cpu->env;
> >> +    CPUState *cs = CPU(cpu);
> >> +
> >> +    cpu_reset(cs);
> >> +    /* Initialize CPU registers.  */
> >> +    env->vbr = 0;
> >> +    /* TODO: Configure BARs.  */
> >> +    env->aregs[7] = ldl_phys(cs->as, 0);
> >> +    env->pc = env->reset_pc;
> >> +}
> >
> > It looks from my quick skim of the m68k manual like
> > "on reset, initial PC is read from offset 4, initial
> > SP is read from offset 0" is part of the architected
> > CPU reset, not board-specific. Maybe we should model
> > that inside the CPU reset method? Compare M-profile
> > Arm, which does basically the same thing in
> > arm_cpu_reset_hold().
> >
> > (I really need to look again at whether we can move
> > that to the reset_exit phase now and drop the awkward
> > "maybe we need to look at a rom blob" special casing.)
>
> Real machines usually have a way to remap ROM to address 0 on reset
> shadowing RAM and then the firmware flips the bits needed to set up the
> memory map with RAM at 0 and ROM at somewhere higher up but the way to do
> it is different for each board. Maybe in QEMU we don't bother to model
> this and have this special case instead to simplify things and be able to
> start from the final ROM address without needing to model this ROM remap
> thing. So modeling real CPU behaviour might make board models more
> complex.

To me it looks like we are already modelling "load SP
and PC from memory", we're just doing it in each
board model rather than in the CPU model for some reason...

-- PMM

Re: [RFC PATCH 04/12] hw/m68k: register a mcf5208evb_cpu_reset handler

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3731 bytes --]

On Tue, 13 Jan 2026, Peter Maydell wrote:
> On Tue, 13 Jan 2026 at 18:08, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Tue, 13 Jan 2026, Peter Maydell wrote:
>>> On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>> It looks like allowing a -kernel to override any firmware setting is
>>>> intentional.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>>  hw/m68k/mcf5208.c | 25 +++++++++++++++++++------
>>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
>>>> index c6d1c5fae9f..5680d516a23 100644
>>>> --- a/hw/m68k/mcf5208.c
>>>> +++ b/hw/m68k/mcf5208.c
>>>> @@ -27,6 +27,7 @@
>>>>  #include "qemu/timer.h"
>>>>  #include "hw/core/ptimer.h"
>>>>  #include "system/system.h"
>>>> +#include "system/reset.h"
>>>>  #include "system/qtest.h"
>>>>  #include "net/net.h"
>>>>  #include "hw/core/boards.h"
>>>> @@ -274,6 +275,20 @@ static void mcf_fec_init(MemoryRegion *sysmem, hwaddr base, qemu_irq *irqs)
>>>>      memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(s, 0));
>>>>  }
>>>>
>>>> +static void mcf5208evb_cpu_reset(void *opaque)
>>>> +{
>>>> +    M68kCPU *cpu = opaque;
>>>> +    CPUM68KState *env = &cpu->env;
>>>> +    CPUState *cs = CPU(cpu);
>>>> +
>>>> +    cpu_reset(cs);
>>>> +    /* Initialize CPU registers.  */
>>>> +    env->vbr = 0;
>>>> +    /* TODO: Configure BARs.  */
>>>> +    env->aregs[7] = ldl_phys(cs->as, 0);
>>>> +    env->pc = env->reset_pc;
>>>> +}
>>>
>>> It looks from my quick skim of the m68k manual like
>>> "on reset, initial PC is read from offset 4, initial
>>> SP is read from offset 0" is part of the architected
>>> CPU reset, not board-specific. Maybe we should model
>>> that inside the CPU reset method? Compare M-profile
>>> Arm, which does basically the same thing in
>>> arm_cpu_reset_hold().
>>>
>>> (I really need to look again at whether we can move
>>> that to the reset_exit phase now and drop the awkward
>>> "maybe we need to look at a rom blob" special casing.)
>>
>> Real machines usually have a way to remap ROM to address 0 on reset
>> shadowing RAM and then the firmware flips the bits needed to set up the
>> memory map with RAM at 0 and ROM at somewhere higher up but the way to do
>> it is different for each board. Maybe in QEMU we don't bother to model
>> this and have this special case instead to simplify things and be able to
>> start from the final ROM address without needing to model this ROM remap
>> thing. So modeling real CPU behaviour might make board models more
>> complex.
>
> To me it looks like we are already modelling "load SP
> and PC from memory", we're just doing it in each
> board model rather than in the CPU model for some reason...

Only q800 seems to do that, virt passes this info in a struct not thorugh 
memory and other boards don't seem to have a cpu reset function at all and 
just poke the cpu object directly without resetting it unless I missed 
something. Looks like each board came up with its own way independently 
but could be cleaned up the way you propose implementing the proper reset 
behaviour of the CPU then changing boards to just put the addresses in 
memory or do the ROM remap if needed. But how could boards write the 
addresses on reset without using qemu_register_reset or needing a lot of 
boilerplate to add a reset method to machine? Maybe it is simple and I 
just don't remember how machine reset works for machine or it could be 
that MACHINE class needs a simple way for boards to register a reset 
method. Maybe this is the reason why several boards use a boot info struct 
to pass reset info into cpu_reset and don't model board reset?

Regards,
BALATON Zoltan

[RFC PATCH 05/12] hw/m68k: register a an5206_cpu_reset handler

[Alex Bennée]

As the mbar/rambar0 values are currently fixed we can migrate the
setting of them to the reset handler as well.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/m68k/an5206.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/an5206.c b/hw/m68k/an5206.c
index f92a5d6a339..918c376a384 100644
--- a/hw/m68k/an5206.c
+++ b/hw/m68k/an5206.c
@@ -15,11 +15,26 @@
 #include "elf.h"
 #include "qemu/error-report.h"
 #include "system/qtest.h"
+#include "system/reset.h"
 
 #define KERNEL_LOAD_ADDR 0x10000
 #define AN5206_MBAR_ADDR 0x10000000
 #define AN5206_RAMBAR_ADDR 0x20000000
 
+static void an5206_cpu_reset(void *opaque)
+{
+    M68kCPU *cpu = opaque;
+    CPUM68KState *env = &cpu->env;
+    CPUState *cs = CPU(cpu);
+
+    cpu_reset(cs);
+    cpu->env.vbr = 0;
+    cpu->env.mbar = AN5206_MBAR_ADDR | 1;
+    cpu->env.rambar0 = AN5206_RAMBAR_ADDR | 1;
+
+    cpu->env.pc = env->reset_pc;
+}
+
 static void mcf5206_init(M68kCPU *cpu, MemoryRegion *sysmem, uint32_t base)
 {
     DeviceState *dev;
@@ -47,14 +62,9 @@ static void an5206_init(MachineState *machine)
     MemoryRegion *sram = g_new(MemoryRegion, 1);
 
     cpu = M68K_CPU(cpu_create(machine->cpu_type));
+    qemu_register_reset(an5206_cpu_reset, cpu);
     env = &cpu->env;
 
-    /* Initialize CPU registers.  */
-    env->vbr = 0;
-    /* TODO: allow changing MBAR and RAMBAR.  */
-    env->mbar = AN5206_MBAR_ADDR | 1;
-    env->rambar0 = AN5206_RAMBAR_ADDR | 1;
-
     /* DRAM at address zero */
     memory_region_add_subregion(address_space_mem, 0, machine->ram);
 
@@ -90,7 +100,7 @@ static void an5206_init(MachineState *machine)
         exit(1);
     }
 
-    env->pc = entry;
+    env->reset_pc = entry;
 }
 
 static void an5206_machine_init(MachineClass *mc)
-- 
2.47.3

Re: [RFC PATCH 05/12] hw/m68k: register a an5206_cpu_reset handler

[Thomas Huth]

Am Thu,  8 Jan 2026 14:34:16 +0000
schrieb Alex Bennée <alex.bennee@linaro.org>:

> As the mbar/rambar0 values are currently fixed we can migrate the
> setting of them to the reset handler as well.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/m68k/an5206.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [RFC PATCH 05/12] hw/m68k: register a an5206_cpu_reset handler

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> As the mbar/rambar0 values are currently fixed we can migrate the
> setting of them to the reset handler as well.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/m68k/an5206.c | 24 +++++++++++++++++-------
>   1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/m68k/an5206.c b/hw/m68k/an5206.c
> index f92a5d6a339..918c376a384 100644
> --- a/hw/m68k/an5206.c
> +++ b/hw/m68k/an5206.c
> @@ -15,11 +15,26 @@
>   #include "elf.h"
>   #include "qemu/error-report.h"
>   #include "system/qtest.h"
> +#include "system/reset.h"
>   
>   #define KERNEL_LOAD_ADDR 0x10000
>   #define AN5206_MBAR_ADDR 0x10000000
>   #define AN5206_RAMBAR_ADDR 0x20000000
>   
> +static void an5206_cpu_reset(void *opaque)
> +{
> +    M68kCPU *cpu = opaque;
> +    CPUM68KState *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
> +
> +    cpu_reset(cs);
> +    cpu->env.vbr = 0;
> +    cpu->env.mbar = AN5206_MBAR_ADDR | 1;
> +    cpu->env.rambar0 = AN5206_RAMBAR_ADDR | 1;
> +
> +    cpu->env.pc = env->reset_pc;
> +}

Don't mix cpu->env with plain env.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[RFC PATCH 06/12] hw/m68k: just use reset_pc for virt platform

[Alex Bennée]

We never actually set initial_stack so revert to the previous
behaviour and stash pc in the common env->reset_pc holding place.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/m68k/virt.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
index e67900c727d..6af74b7ba1d 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -87,21 +87,14 @@
 #define VIRT_VIRTIO_MMIO_BASE 0xff010000     /* MMIO: 0xff010000 - 0xff01ffff */
 #define VIRT_VIRTIO_IRQ_BASE  PIC_IRQ(2, 1)  /* PIC: 2, 3, 4, 5, IRQ: ALL */
 
-typedef struct {
-    M68kCPU *cpu;
-    hwaddr initial_pc;
-    hwaddr initial_stack;
-} ResetInfo;
-
 static void main_cpu_reset(void *opaque)
 {
-    ResetInfo *reset_info = opaque;
-    M68kCPU *cpu = reset_info->cpu;
+    M68kCPU *cpu = opaque;
     CPUState *cs = CPU(cpu);
 
     cpu_reset(cs);
-    cpu->env.aregs[7] = reset_info->initial_stack;
-    cpu->env.pc = reset_info->initial_pc;
+    cpu->env.aregs[7] = ldl_phys(cs->as, 0);
+    cpu->env.pc = cpu->env.reset_pc;
 }
 
 static void rerandomize_rng_seed(void *opaque)
@@ -129,8 +122,8 @@ static void virt_init(MachineState *machine)
     SysBusDevice *sysbus;
     hwaddr io_base;
     int i;
-    ResetInfo *reset_info;
     uint8_t rng_seed[32];
+    CPUM68KState *env;
 
     if (ram_size > 3399672 * KiB) {
         /*
@@ -142,13 +135,10 @@ static void virt_init(MachineState *machine)
         exit(1);
     }
 
-    reset_info = g_new0(ResetInfo, 1);
-
     /* init CPUs */
     cpu = M68K_CPU(cpu_create(machine->cpu_type));
-
-    reset_info->cpu = cpu;
-    qemu_register_reset(main_cpu_reset, reset_info);
+    qemu_register_reset(main_cpu_reset, cpu);
+    env = &cpu->env;
 
     /* RAM */
     memory_region_add_subregion(get_system_memory(), 0, machine->ram);
@@ -235,7 +225,7 @@ static void virt_init(MachineState *machine)
             error_report("could not load kernel '%s'", kernel_filename);
             exit(1);
         }
-        reset_info->initial_pc = elf_entry;
+        env->reset_pc = elf_entry;
         parameters_base = (high + 1) & ~1;
         param_ptr = param_blob;
 
-- 
2.47.3

Re: [RFC PATCH 06/12] hw/m68k: just use reset_pc for virt platform

[Thomas Huth]

Am Thu,  8 Jan 2026 14:34:17 +0000
schrieb Alex Bennée <alex.bennee@linaro.org>:

> We never actually set initial_stack so revert to the previous
> behaviour and stash pc in the common env->reset_pc holding place.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/m68k/virt.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [RFC PATCH 06/12] hw/m68k: just use reset_pc for virt platform

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> @@ -129,8 +122,8 @@ static void virt_init(MachineState *machine)
>       SysBusDevice *sysbus;
>       hwaddr io_base;
>       int i;
> -    ResetInfo *reset_info;
>       uint8_t rng_seed[32];
> +    CPUM68KState *env;
>   
>       if (ram_size > 3399672 * KiB) {
>           /*
> @@ -142,13 +135,10 @@ static void virt_init(MachineState *machine)
>           exit(1);
>       }
>   
> -    reset_info = g_new0(ResetInfo, 1);
> -
>       /* init CPUs */
>       cpu = M68K_CPU(cpu_create(machine->cpu_type));
> -
> -    reset_info->cpu = cpu;
> -    qemu_register_reset(main_cpu_reset, reset_info);
> +    qemu_register_reset(main_cpu_reset, cpu);
> +    env = &cpu->env;
>   
>       /* RAM */
>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
> @@ -235,7 +225,7 @@ static void virt_init(MachineState *machine)
>               error_report("could not load kernel '%s'", kernel_filename);
>               exit(1);
>           }
> -        reset_info->initial_pc = elf_entry;
> +        env->reset_pc = elf_entry;
>           parameters_base = (high + 1) & ~1;
>           param_ptr = param_blob;
>   

Why introduce env rather than use cpu->env like elsewhere in the function?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[RFC PATCH 07/12] target/m68k: drop cpu_reset on realizefn

[Alex Bennée]

Now all the m68k machines have cpu reset handlers we can drop this
extra case here.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/m68k/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index f1b673119d6..a540a754969 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -392,7 +392,6 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
 
     m68k_cpu_init_gdb(cpu);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     mcc->parent_realize(dev, errp);
-- 
2.47.3

Re: [RFC PATCH 07/12] target/m68k: drop cpu_reset on realizefn

[Thomas Huth]

Am Thu,  8 Jan 2026 14:34:18 +0000
schrieb Alex Bennée <alex.bennee@linaro.org>:

> Now all the m68k machines have cpu reset handlers we can drop this
> extra case here.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/m68k/cpu.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index f1b673119d6..a540a754969 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -392,7 +392,6 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      m68k_cpu_init_gdb(cpu);
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      mcc->parent_realize(dev, errp);

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [RFC PATCH 07/12] target/m68k: drop cpu_reset on realizefn

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> Now all the m68k machines have cpu reset handlers we can drop this
> extra case here.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/m68k/cpu.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index f1b673119d6..a540a754969 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -392,7 +392,6 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>   
>       m68k_cpu_init_gdb(cpu);
>   
> -    cpu_reset(cs);
>       qemu_init_vcpu(cs);
>   
>       mcc->parent_realize(dev, errp);

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[RFC PATCH 08/12] hw/mips: defer finalising gcr_base until reset time

[Alex Bennée]

Currently the cpu_reset() in mips_cpu_realizefn() hides an implicit
sequencing requirement when setting gcr_base. Without it we barf
because we end up setting the region between 0x0-0x000000001fbfffff
which trips over a qtest that accesses the GCR during "memsave 0 4096
/dev/null".

By moving to the reset phase we have to drop the property lest we are
admonished for "Attempting to set...after it was realized" but there
doesn't seem to be a need to expose the property anyway.

NB: it would be safer if I could guarantee the place in the reset tree
but I haven't quite grokked how to do that yet. Currently I see this
sequence when testing:

  ➜  env MALLOC_PERTURB_=43 G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-mips64el SPEED=thorough MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 PYTHON=/home/alex/lsrc/qemu.git/builds/debug/pyvenv/bin/python3 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 G_TEST_SLOW=1 RUST_BACKTRACE=1 /home/alex/lsrc/qemu.git/builds/debug/tests/qtest/test-hmp --tap -p /mips64el/hmp/boston
  TAP version 14
  # random seed: R02S89554f0dc696ece515363e554b13b7f9
  # Start of mips64el tests
  # Start of hmp tests
  # starting QEMU: exec ./qemu-system-mips64el -qtest unix:/tmp/qtest-883372.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-883372.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -run-with exit-with-parent=on -S -M boston -accel qtest
  mips_cpu_reset_hold: dbg
  mips_gcr_init: 0x5600f2160050 - 0
  main_cpu_reset: dbg
  mips_cpu_reset_hold: dbg
  mps_reset: 000000001fbf8000
  mips_cpu_reset_hold: dbg
  ok 1 /mips64el/hmp/boston
  # End of hmp tests
  # End of mips64el tests
  1..1

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/mips/cps.c        | 22 +++++++++++++---------
 hw/misc/mips_cmgcr.c |  1 -
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 620ee972f8f..c91243599e0 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -55,6 +55,18 @@ static void main_cpu_reset(void *opaque)
     cpu_reset(cs);
 }
 
+static void mps_reset(void *opaque)
+{
+    DeviceState *dev = opaque;
+    MIPSCPSState *s = MIPS_CPS(dev);
+    hwaddr gcr_base;
+
+    /* Global Configuration Registers - only valid once the CPU has been reset */
+    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
+    memory_region_add_subregion(&s->container, gcr_base,
+                            sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), 0));
+}
+
 static bool cpu_mips_itu_supported(CPUMIPSState *env)
 {
     bool is_mt = (env->CP0_Config5 & (1 << CP0C5_VP)) || ase_mt_available(env);
@@ -65,7 +77,6 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
 static void mips_cps_realize(DeviceState *dev, Error **errp)
 {
     MIPSCPSState *s = MIPS_CPS(dev);
-    target_ulong gcr_base;
     bool itu_present = false;
 
     if (!clock_get(s->clock)) {
@@ -144,16 +155,11 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->container, 0,
                             sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
 
-    /* Global Configuration Registers */
-    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
-
     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
     object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
                             &error_abort);
     object_property_set_int(OBJECT(&s->gcr), "gcr-rev", 0x800,
                             &error_abort);
-    object_property_set_int(OBJECT(&s->gcr), "gcr-base", gcr_base,
-                            &error_abort);
     object_property_set_link(OBJECT(&s->gcr), "gic", OBJECT(&s->gic.mr),
                              &error_abort);
     object_property_set_link(OBJECT(&s->gcr), "cpc", OBJECT(&s->cpc.mr),
@@ -161,9 +167,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gcr), errp)) {
         return;
     }
-
-    memory_region_add_subregion(&s->container, gcr_base,
-                            sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), 0));
+    qemu_register_reset(mps_reset, s);
 }
 
 static const Property mips_cps_properties[] = {
diff --git a/hw/misc/mips_cmgcr.c b/hw/misc/mips_cmgcr.c
index 3e262e828bc..9e1c8d26ea5 100644
--- a/hw/misc/mips_cmgcr.c
+++ b/hw/misc/mips_cmgcr.c
@@ -214,7 +214,6 @@ static const VMStateDescription vmstate_mips_gcr = {
 static const Property mips_gcr_properties[] = {
     DEFINE_PROP_UINT32("num-vp", MIPSGCRState, num_vps, 1),
     DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
-    DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
     DEFINE_PROP_LINK("gic", MIPSGCRState, gic_mr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_LINK("cpc", MIPSGCRState, cpc_mr, TYPE_MEMORY_REGION,
-- 
2.47.3

Re: [RFC PATCH 08/12] hw/mips: defer finalising gcr_base until reset time

[Peter Maydell]

On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Currently the cpu_reset() in mips_cpu_realizefn() hides an implicit
> sequencing requirement when setting gcr_base. Without it we barf
> because we end up setting the region between 0x0-0x000000001fbfffff
> which trips over a qtest that accesses the GCR during "memsave 0 4096
> /dev/null".
>
> By moving to the reset phase we have to drop the property lest we are
> admonished for "Attempting to set...after it was realized" but there
> doesn't seem to be a need to expose the property anyway.
>
> NB: it would be safer if I could guarantee the place in the reset tree
> but I haven't quite grokked how to do that yet. Currently I see this
> sequence when testing:
>
>   ➜  env MALLOC_PERTURB_=43 G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-mips64el SPEED=thorough MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 PYTHON=/home/alex/lsrc/qemu.git/builds/debug/pyvenv/bin/python3 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 G_TEST_SLOW=1 RUST_BACKTRACE=1 /home/alex/lsrc/qemu.git/builds/debug/tests/qtest/test-hmp --tap -p /mips64el/hmp/boston
>   TAP version 14
>   # random seed: R02S89554f0dc696ece515363e554b13b7f9
>   # Start of mips64el tests
>   # Start of hmp tests
>   # starting QEMU: exec ./qemu-system-mips64el -qtest unix:/tmp/qtest-883372.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-883372.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -run-with exit-with-parent=on -S -M boston -accel qtest
>   mips_cpu_reset_hold: dbg
>   mips_gcr_init: 0x5600f2160050 - 0
>   main_cpu_reset: dbg
>   mips_cpu_reset_hold: dbg
>   mps_reset: 000000001fbf8000
>   mips_cpu_reset_hold: dbg
>   ok 1 /mips64el/hmp/boston
>   # End of hmp tests
>   # End of mips64el tests
>   1..1
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/mips/cps.c        | 22 +++++++++++++---------
>  hw/misc/mips_cmgcr.c |  1 -
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 620ee972f8f..c91243599e0 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -55,6 +55,18 @@ static void main_cpu_reset(void *opaque)
>      cpu_reset(cs);
>  }
>
> +static void mps_reset(void *opaque)
> +{
> +    DeviceState *dev = opaque;
> +    MIPSCPSState *s = MIPS_CPS(dev);
> +    hwaddr gcr_base;
> +
> +    /* Global Configuration Registers - only valid once the CPU has been reset */
> +    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
> +    memory_region_add_subregion(&s->container, gcr_base,
> +                            sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), 0));
> +}
> +
>  static bool cpu_mips_itu_supported(CPUMIPSState *env)
>  {
>      bool is_mt = (env->CP0_Config5 & (1 << CP0C5_VP)) || ase_mt_available(env);
> @@ -65,7 +77,6 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
>  static void mips_cps_realize(DeviceState *dev, Error **errp)
>  {
>      MIPSCPSState *s = MIPS_CPS(dev);
> -    target_ulong gcr_base;
>      bool itu_present = false;
>
>      if (!clock_get(s->clock)) {
> @@ -144,16 +155,11 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(&s->container, 0,
>                              sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
>
> -    /* Global Configuration Registers */
> -    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
> -
>      object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
>      object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->gcr), "gcr-rev", 0x800,
>                              &error_abort);
> -    object_property_set_int(OBJECT(&s->gcr), "gcr-base", gcr_base,
> -                            &error_abort);
>      object_property_set_link(OBJECT(&s->gcr), "gic", OBJECT(&s->gic.mr),
>                               &error_abort);
>      object_property_set_link(OBJECT(&s->gcr), "cpc", OBJECT(&s->cpc.mr),
> @@ -161,9 +167,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->gcr), errp)) {
>          return;
>      }
> -
> -    memory_region_add_subregion(&s->container, gcr_base,
> -                            sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), 0));
> +    qemu_register_reset(mps_reset, s);

Adding calls to qemu_register_reset() is adding more uses of
an API we'd ideally like to get rid of. It's particularly
non-ideal here where we're in an implementation of a sysbus
device, which has a perfectly good reset method we could
implement.

>  }
>
>  static const Property mips_cps_properties[] = {
> diff --git a/hw/misc/mips_cmgcr.c b/hw/misc/mips_cmgcr.c
> index 3e262e828bc..9e1c8d26ea5 100644
> --- a/hw/misc/mips_cmgcr.c
> +++ b/hw/misc/mips_cmgcr.c
> @@ -214,7 +214,6 @@ static const VMStateDescription vmstate_mips_gcr = {
>  static const Property mips_gcr_properties[] = {
>      DEFINE_PROP_UINT32("num-vp", MIPSGCRState, num_vps, 1),
>      DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
> -    DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
>      DEFINE_PROP_LINK("gic", MIPSGCRState, gic_mr, TYPE_MEMORY_REGION,
>                       MemoryRegion *),
>      DEFINE_PROP_LINK("cpc", MIPSGCRState, cpc_mr, TYPE_MEMORY_REGION,

Something in these devices seems to be very weirdly modelled
and could probably do with being straightened out. Notably,
the GCR device itself has functionality for moving the
address of its memory region around when the guest writes
to the GCR_BASE register, so perhaps it should itself have
the job of making sure the MR is in the right place on reset?

If you have the GCR look at the CMGCRBase value of the CPU
and set the MR position in its reset exit phase method, then
you will probably find that sorts out your ordering issues,
because the CPU will set/reset its state in the 'hold' phase,
and then the GCR can look at it in the 'exit' phase.

thanks
-- PMM

[RFC PATCH 09/12] hw/mips: drop cpu_reset in mips_cpu_realizefn

[Alex Bennée]

We will always do a cpu_reset during mips_cpu_reset_hold().

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/mips/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index f74a9d5f615..e61497c0f6b 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -485,7 +485,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     fpu_init(env, env->cpu_model);
     mvp_init(env);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     mcc->parent_realize(dev, errp);
-- 
2.47.3

[RFC PATCH 10/12] target/tricore: move cpu_reset from tricore_cpu_realizefn

[Alex Bennée]

Implement a proper cpu reset handler for tricore cpus.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/tricore/cpu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 04319e107ba..c3dda9f6a54 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -24,6 +24,7 @@
 #include "qemu/error-report.h"
 #include "tcg/debug-assert.h"
 #include "accel/tcg/cpu-ops.h"
+#include "system/reset.h"
 
 static inline void set_feature(CPUTriCoreState *env, int feature)
 {
@@ -81,6 +82,12 @@ static void tricore_cpu_reset_hold(Object *obj, ResetType type)
     cpu_state_reset(cpu_env(cs));
 }
 
+static void tricore_cpu_reset(void *opaque)
+{
+    CPUState *cs = opaque;
+    cpu_reset(cs);
+}
+
 static bool tricore_cpu_has_work(CPUState *cs)
 {
     return true;
@@ -120,8 +127,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
     if (tricore_has_feature(env, TRICORE_FEATURE_131)) {
         set_feature(env, TRICORE_FEATURE_13);
     }
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    qemu_register_reset(tricore_cpu_reset, cs);
 
     tcc->parent_realize(dev, errp);
 }
-- 
2.47.3

Re: [RFC PATCH 10/12] target/tricore: move cpu_reset from tricore_cpu_realizefn

[Peter Maydell]

On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Implement a proper cpu reset handler for tricore cpus.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/tricore/cpu.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index 04319e107ba..c3dda9f6a54 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -24,6 +24,7 @@
>  #include "qemu/error-report.h"
>  #include "tcg/debug-assert.h"
>  #include "accel/tcg/cpu-ops.h"
> +#include "system/reset.h"
>
>  static inline void set_feature(CPUTriCoreState *env, int feature)
>  {
> @@ -81,6 +82,12 @@ static void tricore_cpu_reset_hold(Object *obj, ResetType type)
>      cpu_state_reset(cpu_env(cs));
>  }
>
> +static void tricore_cpu_reset(void *opaque)
> +{
> +    CPUState *cs = opaque;
> +    cpu_reset(cs);
> +}
> +
>  static bool tricore_cpu_has_work(CPUState *cs)
>  {
>      return true;
> @@ -120,8 +127,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (tricore_has_feature(env, TRICORE_FEATURE_131)) {
>          set_feature(env, TRICORE_FEATURE_13);
>      }
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    qemu_register_reset(tricore_cpu_reset, cs);

We currently call qemu_register_reset() inside the
CPU's own realize function only for i386 and s390;
for all other architectures we require the board code
somehow to arrange to reset the CPU objects.

We should figure out what the "right" way is we want to
handle this and be consistent...

Calling qemu_register_reset() with a function that calls
cpu_reset() is not ideal, because it means that inside
of a three-phase reset process we end up running all 3
phases of the CPU object's reset inside the 'hold' phase
of the full process. What we want is for the CPU object's
reset phases to be called as part of each phase of the
full reset process. qemu_register_resettable() takes an
object that implements Resettable, and is probably a better
idea.

-- PMM

[RFC PATCH 11/12] target/arm: remove extraneous cpu_reset from realizefn

[Alex Bennée]

We already have do_cpu_reset called when the system is reset so there
is no need to do it here.

By removing this we now only do (smp*2)-1 calls to cpu_reset. Once per
core as part of qemu_system_reset and then once per secondary core as
PSCI calls are made to reset them.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index caf7980b1fc..015131aea08 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2208,7 +2208,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     acc->parent_realize(dev, errp);
 }
-- 
2.47.3

[RFC PATCH 12/12] include/hw: expand cpu_reset function docs

[Alex Bennée]

Add a hint to the developer that this should only be called from a
reset chain.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/cpu.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index f6f17df9e64..9f938e00492 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -811,6 +811,9 @@ void cpu_list_remove(CPUState *cpu);
 /**
  * cpu_reset:
  * @cpu: The CPU whose state is to be reset.
+ *
+ * You should refrain from calling this during CPU realization and
+ * make sure this is called from the reset logic instead.
  */
 void cpu_reset(CPUState *cpu);
 
-- 
2.47.3

Re: [RFC PATCH 12/12] include/hw: expand cpu_reset function docs

[Richard Henderson]

On 1/9/26 01:34, Alex Bennée wrote:
> Add a hint to the developer that this should only be called from a
> reset chain.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/hw/core/cpu.h | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 00/12] cpu_reset clean-ups for arm, sh4, mips, m68k and tricore

[Philippe Mathieu-Daudé]

On 8/1/26 15:34, Alex Bennée wrote:
> We tend to apply cpu_reset inconsistently throughout our various
> models which leads to unintended ordering dependencies. This got in
> the way in my last plugins series:
> 
>    https://patchew.org/QEMU/20251219190849.238323-1-alex.bennee@linaro.org/
> 
> where I needed to shuffle things around to ensure that gdb register
> creation was done after dependant peripherals had created their cpu
> interfaces.
> 
> Regardless of that we do have a proper reset interface now and most
> architectures have moved to it. This series attempts to clean-up the
> remaining cases with proper qemu_register_reset() calls so reset is
> called when we intend to.

Last time I was blocked at this comment:
https://lore.kernel.org/qemu-devel/20231128170008.57ddb03e@imammedo.users.ipa.redhat.com/

> Alex Bennée (12):
>    target/sh4: drop cpu_reset from realizefn
>    target/m68k: introduce env->reset_pc
>    hw/m68k: register a nextcube_cpu_reset handler
>    hw/m68k: register a mcf5208evb_cpu_reset handler
>    hw/m68k: register a an5206_cpu_reset handler
>    hw/m68k: just use reset_pc for virt platform
>    target/m68k: drop cpu_reset on realizefn
>    hw/mips: defer finalising gcr_base until reset time
>    hw/mips: drop cpu_reset in mips_cpu_realizefn
>    target/tricore: move cpu_reset from tricore_cpu_realizefn
>    target/arm: remove extraneous cpu_reset from realizefn
>    include/hw: expand cpu_reset function docs

Re: [RFC PATCH 00/12] cpu_reset clean-ups for arm, sh4, mips, m68k and tricore

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 8/1/26 15:34, Alex Bennée wrote:
>> We tend to apply cpu_reset inconsistently throughout our various
>> models which leads to unintended ordering dependencies. This got in
>> the way in my last plugins series:
>>    https://patchew.org/QEMU/20251219190849.238323-1-alex.bennee@linaro.org/
>> where I needed to shuffle things around to ensure that gdb register
>> creation was done after dependant peripherals had created their cpu
>> interfaces.
>> Regardless of that we do have a proper reset interface now and most
>> architectures have moved to it. This series attempts to clean-up the
>> remaining cases with proper qemu_register_reset() calls so reset is
>> called when we intend to.
>
> Last time I was blocked at this comment:
> https://lore.kernel.org/qemu-devel/20231128170008.57ddb03e@imammedo.users.ipa.redhat.com/

From that:

 --cpu_reset()  <- brings cpu to known (after reset|poweron) state
   cpu_common_realizefn()
       if (dev->hotplugged) {                                                       
           cpu_synchronize_post_init(cpu);                                          
           cpu_resume(cpu);                                                         
       }
 ++cpu_reset()  <-- looks to be too late, we already told hypervisor to run undefined CPU, didn't we?

I would posit that the hotplug path is different as we online the CPU as
soon as its ready. Maybe that is just special cased as:

       if (dev->hotplugged) {
           cpu_reset(cpu);
           cpu_synchronize_post_init(cpu);                                          
           cpu_resume(cpu);                                                         
       }

Unless hotplug should also honour the reset tree in which case that
logic could be moved:

  void cpu_reset(CPUState *cpu)
  {
      DeviceState *dev = DEVICE(cpu);

      if (!dev->hotplugged) {
          device_cold_reset(DEVICE(cpu));
      } else {
          /* hotplugging implies we should know how to setup */
          cpu_synchronize_post_init(cpu);    
      }
      trace_cpu_reset(cpu->cpu_index);

  #ifdef CONFIG_TCG
      /*
       * Because vCPUs get "started" before the machine is ready we often
       * can't provide all the information plugins need during
       * cpu_common_initfn. However the vCPU will be reset a few times
       * before we eventually set things going giving plugins an
       * opportunity to update things.
       */
      qemu_plugin_vcpu_reset_hook(cpu);
  #endif
  }

Do we have test cases for hotplugging CPUs?

>
>> Alex Bennée (12):
>>    target/sh4: drop cpu_reset from realizefn
>>    target/m68k: introduce env->reset_pc
>>    hw/m68k: register a nextcube_cpu_reset handler
>>    hw/m68k: register a mcf5208evb_cpu_reset handler
>>    hw/m68k: register a an5206_cpu_reset handler
>>    hw/m68k: just use reset_pc for virt platform
>>    target/m68k: drop cpu_reset on realizefn
>>    hw/mips: defer finalising gcr_base until reset time
>>    hw/mips: drop cpu_reset in mips_cpu_realizefn
>>    target/tricore: move cpu_reset from tricore_cpu_realizefn
>>    target/arm: remove extraneous cpu_reset from realizefn
>>    include/hw: expand cpu_reset function docs

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH 00/12] cpu_reset clean-ups for arm, sh4, mips, m68k and tricore

[Peter Maydell]

On Thu, 8 Jan 2026 at 16:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > On 8/1/26 15:34, Alex Bennée wrote:
> >> We tend to apply cpu_reset inconsistently throughout our various
> >> models which leads to unintended ordering dependencies. This got in
> >> the way in my last plugins series:
> >>    https://patchew.org/QEMU/20251219190849.238323-1-alex.bennee@linaro.org/
> >> where I needed to shuffle things around to ensure that gdb register
> >> creation was done after dependant peripherals had created their cpu
> >> interfaces.
> >> Regardless of that we do have a proper reset interface now and most
> >> architectures have moved to it. This series attempts to clean-up the
> >> remaining cases with proper qemu_register_reset() calls so reset is
> >> called when we intend to.
> >
> > Last time I was blocked at this comment:
> > https://lore.kernel.org/qemu-devel/20231128170008.57ddb03e@imammedo.users.ipa.redhat.com/
>
> From that:
>
>  --cpu_reset()  <- brings cpu to known (after reset|poweron) state
>    cpu_common_realizefn()
>        if (dev->hotplugged) {
>            cpu_synchronize_post_init(cpu);
>            cpu_resume(cpu);
>        }
>  ++cpu_reset()  <-- looks to be too late, we already told hypervisor to run undefined CPU, didn't we?
>
> I would posit that the hotplug path is different as we online the CPU as
> soon as its ready. Maybe that is just special cased as:
>
>        if (dev->hotplugged) {
>            cpu_reset(cpu);
>            cpu_synchronize_post_init(cpu);
>            cpu_resume(cpu);
>        }
>
> Unless hotplug should also honour the reset tree in which case that
> logic could be moved:
>
>   void cpu_reset(CPUState *cpu)
>   {
>       DeviceState *dev = DEVICE(cpu);
>
>       if (!dev->hotplugged) {
>           device_cold_reset(DEVICE(cpu));
>       } else {
>           /* hotplugging implies we should know how to setup */
>           cpu_synchronize_post_init(cpu);
>       }
>       trace_cpu_reset(cpu->cpu_index);
>
>   #ifdef CONFIG_TCG
>       /*
>        * Because vCPUs get "started" before the machine is ready we often
>        * can't provide all the information plugins need during
>        * cpu_common_initfn. However the vCPU will be reset a few times
>        * before we eventually set things going giving plugins an
>        * opportunity to update things.
>        */
>       qemu_plugin_vcpu_reset_hook(cpu);
>   #endif
>   }

You don't want to special case anything in the cpu_reset()
function, because it is valid (indeed, encouraged :-)) to
reset CPU objects in ways that don't pass through it.
For instance, qemu_register_resettable(OBJECT(cpu)) is one way.

The problem with cpu_reset() is that it is not 3-phase aware,
so (like everything that calls device_cold_reset()) it will
call all 3 reset phases for the CPU at once, even in the middle
of doing a full-system 3-phase reset. It's fine for the
special case of "we are resetting nothing else except the CPU",
but there's a lot of places where we aren't using it like that.

thanks
-- PMM