[PATCH 0/2] hw/core: Don't dump incompletely reset cpu

[PATCH 0/2] hw/core: Don't dump incompletely reset cpu

[Richard Henderson]

The start of CPUState reset.hold happens before any subclasses,
which can result in attempting to dump uninitialized data.
In the case of Sparc, this will SIGSEGV.


r~


Richard Henderson (2):
  hw/core: Dump cpu_reset in the reset.exit phase
  hw/core: Use qemu_log_trylock/unlock in cpu_common_reset_exit

 hw/core/cpu-common.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH 1/2] hw/core: Dump cpu_reset in the reset.exit phase

[Richard Henderson]

During reset.hold, the cpu is in an inconsistent state,
where the leaf class has not had a chance to initialize
state at all.

This is visible as a SIGSEGV in "qemu-system-sparc64 -d cpu_reset".

Move the dump to the exit phase, where all initialization
is certain to be complete.

Reported-by: Henk van der Laak <henk@laaksoft.nl>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/core/cpu-common.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 39e674aca2..26321be785 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -119,11 +119,6 @@ static void cpu_common_reset_hold(Object *obj, ResetType type)
 {
     CPUState *cpu = CPU(obj);
 
-    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
-        qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
-        log_cpu_state(cpu, cpu->cc->reset_dump_flags);
-    }
-
     cpu->interrupt_request = 0;
     cpu->halted = cpu->start_powered_off;
     cpu->mem_io_pc = 0;
@@ -137,6 +132,16 @@ static void cpu_common_reset_hold(Object *obj, ResetType type)
     cpu_exec_reset_hold(cpu);
 }
 
+static void cpu_common_reset_exit(Object *obj, ResetType type)
+{
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        CPUState *cpu = CPU(obj);
+
+        qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
+        log_cpu_state(cpu, cpu->cc->reset_dump_flags);
+    }
+}
+
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
 {
     ObjectClass *oc;
@@ -380,6 +385,7 @@ static void cpu_common_class_init(ObjectClass *klass, const void *data)
     dc->realize = cpu_common_realizefn;
     dc->unrealize = cpu_common_unrealizefn;
     rc->phases.hold = cpu_common_reset_hold;
+    rc->phases.exit = cpu_common_reset_exit;
     cpu_class_init_props(dc);
     /*
      * Reason: CPUs still need special care by board code: wiring up
-- 
2.43.0

[PATCH 2/2] hw/core: Use qemu_log_trylock/unlock in cpu_common_reset_exit

[Richard Henderson]

Ensure that the "CPU Reset" message won't be separated
from the cpu_dump_state output.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/core/cpu-common.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 26321be785..259cf2a3c3 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -135,10 +135,15 @@ static void cpu_common_reset_hold(Object *obj, ResetType type)
 static void cpu_common_reset_exit(Object *obj, ResetType type)
 {
     if (qemu_loglevel_mask(CPU_LOG_RESET)) {
-        CPUState *cpu = CPU(obj);
+        FILE *f = qemu_log_trylock();
 
-        qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
-        log_cpu_state(cpu, cpu->cc->reset_dump_flags);
+        if (f) {
+            CPUState *cpu = CPU(obj);
+
+            fprintf(f, "CPU Reset (CPU %d)\n", cpu->cpu_index);
+            cpu_dump_state(cpu, f, cpu->cc->reset_dump_flags);
+            qemu_log_unlock(f);
+        }
     }
 }
 
-- 
2.43.0

Re: [PATCH 1/2] hw/core: Dump cpu_reset in the reset.exit phase

[Peter Maydell]

On Wed, 27 Aug 2025 at 06:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> During reset.hold, the cpu is in an inconsistent state,
> where the leaf class has not had a chance to initialize
> state at all.
>
> This is visible as a SIGSEGV in "qemu-system-sparc64 -d cpu_reset".
>
> Move the dump to the exit phase, where all initialization
> is certain to be complete.
>
> Reported-by: Henk van der Laak <henk@laaksoft.nl>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/core/cpu-common.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 39e674aca2..26321be785 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -119,11 +119,6 @@ static void cpu_common_reset_hold(Object *obj, ResetType type)
>  {
>      CPUState *cpu = CPU(obj);
>
> -    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> -        qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
> -        log_cpu_state(cpu, cpu->cc->reset_dump_flags);
> -    }
> -
>      cpu->interrupt_request = 0;
>      cpu->halted = cpu->start_powered_off;
>      cpu->mem_io_pc = 0;
> @@ -137,6 +132,16 @@ static void cpu_common_reset_hold(Object *obj, ResetType type)
>      cpu_exec_reset_hold(cpu);
>  }
>
> +static void cpu_common_reset_exit(Object *obj, ResetType type)
> +{
> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> +        CPUState *cpu = CPU(obj);
> +
> +        qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
> +        log_cpu_state(cpu, cpu->cc->reset_dump_flags);
> +    }
> +}
> +
>  ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
>  {
>      ObjectClass *oc;
> @@ -380,6 +385,7 @@ static void cpu_common_class_init(ObjectClass *klass, const void *data)
>      dc->realize = cpu_common_realizefn;
>      dc->unrealize = cpu_common_unrealizefn;
>      rc->phases.hold = cpu_common_reset_hold;
> +    rc->phases.exit = cpu_common_reset_exit;
>      cpu_class_init_props(dc);
>      /*
>       * Reason: CPUs still need special care by board code: wiring up

If we ever have CPUs that actually update their state in
the reset exit phase (e.g. if we manage to complete the refactoring
that would let us implement M-profile "load starting PC and SP
from memory" in reset-exit after rom blob loading rather than
having a hack to do it in reset-hold), this won't capture that.
But it's clearly better than trying to do it in the common
reset-hold method...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] hw/core: Use qemu_log_trylock/unlock in cpu_common_reset_exit

[Peter Maydell]

On Wed, 27 Aug 2025 at 06:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Ensure that the "CPU Reset" message won't be separated
> from the cpu_dump_state output.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/core/cpu-common.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 26321be785..259cf2a3c3 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -135,10 +135,15 @@ static void cpu_common_reset_hold(Object *obj, ResetType type)
>  static void cpu_common_reset_exit(Object *obj, ResetType type)
>  {
>      if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> -        CPUState *cpu = CPU(obj);
> +        FILE *f = qemu_log_trylock();
>
> -        qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
> -        log_cpu_state(cpu, cpu->cc->reset_dump_flags);
> +        if (f) {
> +            CPUState *cpu = CPU(obj);
> +
> +            fprintf(f, "CPU Reset (CPU %d)\n", cpu->cpu_index);
> +            cpu_dump_state(cpu, f, cpu->cc->reset_dump_flags);
> +            qemu_log_unlock(f);
> +        }
>      }
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] hw/core: Use qemu_log_trylock/unlock in cpu_common_reset_exit

[Philippe Mathieu-Daudé]

On 27/8/25 07:38, Richard Henderson wrote:
> Ensure that the "CPU Reset" message won't be separated
> from the cpu_dump_state output.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/core/cpu-common.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)

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

Re: [PATCH 1/2] hw/core: Dump cpu_reset in the reset.exit phase

[Philippe Mathieu-Daudé]

On 28/8/25 17:24, Peter Maydell wrote:
> On Wed, 27 Aug 2025 at 06:39, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> During reset.hold, the cpu is in an inconsistent state,
>> where the leaf class has not had a chance to initialize
>> state at all.
>>
>> This is visible as a SIGSEGV in "qemu-system-sparc64 -d cpu_reset".
>>
>> Move the dump to the exit phase, where all initialization
>> is certain to be complete.
>>
>> Reported-by: Henk van der Laak <henk@laaksoft.nl>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   hw/core/cpu-common.c | 16 +++++++++++-----
>>   1 file changed, 11 insertions(+), 5 deletions(-)


>> @@ -380,6 +385,7 @@ static void cpu_common_class_init(ObjectClass *klass, const void *data)
>>       dc->realize = cpu_common_realizefn;
>>       dc->unrealize = cpu_common_unrealizefn;
>>       rc->phases.hold = cpu_common_reset_hold;
>> +    rc->phases.exit = cpu_common_reset_exit;
>>       cpu_class_init_props(dc);
>>       /*
>>        * Reason: CPUs still need special care by board code: wiring up
> 
> If we ever have CPUs that actually update their state in
> the reset exit phase (e.g. if we manage to complete the refactoring
> that would let us implement M-profile "load starting PC and SP
> from memory" in reset-exit after rom blob loading rather than
> having a hack to do it in reset-hold), this won't capture that.
> But it's clearly better than trying to do it in the common
> reset-hold method...
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

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