[PATCH v4 0/8] Generalize start-powered-off property from ARM

[PATCH v4 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

This version tries to fix an issue found by David Gibson when running
the Travis CI:

Unexpected error in qdev_prop_set_after_realize() at /home/travis/build/dgibson/qemu/hw/core/qdev-properties.c:30:
qemu-system-mips64el: Attempt to set property 'start-powered-off' on anonymous device (type 'I6400-mips64-cpu') after it was realized
Broken pipe
/home/travis/build/dgibson/qemu/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
Aborted (core dumped)
ERROR qom-test - too few tests run (expected 8, got 0)
/home/travis/build/dgibson/qemu/tests/Makefile.include:650: recipe for target 'check-qtest-mips64el' failed

Philippe Mathieu-Daudé spotted the problem:

> Good catch. hw/mips/cps.c, hw/ppc/e500.c and hw/sparc/sun4m.c are
> incorrectly setting the property after the cpu is realized because
> the cpu is created with cpu_create(). We need to create them with
> object_initialize_child() and realize them manually with qdev_realize().

But I found very few examples of CPUs initialized with
object_initialize_child() (e.g., atmega.c, rx62n.c, nrf51_soc.c)
so instead of using object_initialize_child(), I replaced the call to
cpu_create() with object_new() and a call to qdev_realize() shortly after
the  start-powered-off property is set. I thought this would be the more
prudent change, keeping the code as close to the previous one as possible.

I tried reproducing the Travis CI problem with
`make docker-test-misc@debian-mips64el-cross` but I didn't succeed, so
I'm not sure if this version solves the issue.


Original cover letter bellow, followed by changelog:

The ARM code has a start-powered-off property in ARMCPU, which is a
subclass of CPUState. This property causes arm_cpu_reset() to set
CPUState::halted to 1, signalling that the CPU should start in a halted
state. Other architectures also have code which aim to achieve the same
effect, but without using a property.

The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu
before cs->halted is set to 1, causing the vcpu to run while it's still in
an unitialized state (more details in patch 3).

Peter Maydell mentioned the ARM start-powered-off property and
Eduardo Habkost suggested making it generic, so this patch series does
that, for all cases which I was able to find via grep in the code.

The only problem is that I was only able to test these changes on a ppc64le
pseries KVM guest, so except for patches 2 and 3, all others are only
build-tested. Also, my grasp of QOM lifecycle is basically non-existant so
please be aware of that when reviewing this series.

The last patch may be wrong, as pointed out by Eduardo, so I marked it as
RFC. It may make sense to drop it.

Applies cleanly on yesterday's master.

Changes since v3:

General:
- Added David's, Greg's and Cornelia's Reviewed-by and Acked-by to some
  of the patches.
- Rebased on top of dgibson/ppc-for-5.2.

Patch "ppc/e500: Use start-powered-off CPUState property"
Patch "mips/cps: Use start-powered-off CPUState property"
Patch "sparc/sun4m: Use start-powered-off CPUState property"
- Initialize CPU object with object_new() and qdev_realize() instead
  of cpu_create().
- Removed Reviewed-by's and Acked-by's from these patches because of
  these changes.

Changes since v2:

General:
- Added Philippe's Reviewed-by to some of the patches.

Patch "ppc/spapr: Use start-powered-off CPUState property"
- Set the CPUState::start_powered_off variable directly rather than using
  object_property_set_bool(). Suggested by Philippe.

Patch "sparc/sun4m: Remove main_cpu_reset()"
- New patch. Suggested by Philippe.

Patch "sparc/sun4m: Use start-powered-off CPUState property"
- Remove secondary_cpu_reset(). Suggested by Philippe.
- Remove setting of `cs->halted = 1` from cpu_devinit(). Suggested by Philippe.

Patch "Don't set CPUState::halted in cpu_devinit()"
- Squashed into previous patch. Suggested by Philippe.

Patch "sparc/sun4m: Use one cpu_reset() function for main and secondary CPUs"
- Dropped.

Patch "target/s390x: Use start-powered-off CPUState property"
- Set the CPUState::start_powered_off variable directly rather than using
  object_property_set_bool(). Suggested by Philippe.
- Mention in the commit message Eduardo's observation that before this
  patch, the code didn't set cs->halted on reset.

Thiago Jung Bauermann (8):
  target/arm: Move start-powered-off property to generic CPUState
  target/arm: Move setting of CPU halted state to generic code
  ppc/spapr: Use start-powered-off CPUState property
  ppc/e500: Use start-powered-off CPUState property
  mips/cps: Use start-powered-off CPUState property
  sparc/sun4m: Remove main_cpu_reset()
  sparc/sun4m: Use start-powered-off CPUState property
  target/s390x: Use start-powered-off CPUState property

 exec.c                  |  1 +
 hw/core/cpu.c           |  2 +-
 hw/mips/cps.c           | 16 ++++++++++++----
 hw/ppc/e500.c           | 19 +++++++++++++++----
 hw/ppc/spapr_cpu_core.c | 10 +++++-----
 hw/sparc/sun4m.c        | 37 ++++++++++---------------------------
 include/hw/core/cpu.h   |  4 ++++
 target/arm/cpu.c        |  4 +---
 target/arm/cpu.h        |  3 ---
 target/arm/kvm32.c      |  2 +-
 target/arm/kvm64.c      |  2 +-
 target/s390x/cpu.c      |  2 +-
 12 files changed, 52 insertions(+), 50 deletions(-)

[PATCH v4 1/8] target/arm: Move start-powered-off property to generic CPUState

[Thiago Jung Bauermann]

There are other platforms which also have CPUs that start powered off, so
generalize the start-powered-off property so that it can be used by them.

Note that ARMv7MState also has a property of the same name but this patch
doesn't change it because that class isn't a subclass of CPUState so it
wouldn't be a trivial change.

This change should not cause any change in behavior.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 exec.c                | 1 +
 include/hw/core/cpu.h | 4 ++++
 target/arm/cpu.c      | 5 ++---
 target/arm/cpu.h      | 3 ---
 target/arm/kvm32.c    | 2 +-
 target/arm/kvm64.c    | 2 +-
 6 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index 6f381f98e2..82e82fab09 100644
--- a/exec.c
+++ b/exec.c
@@ -899,6 +899,7 @@ Property cpu_common_props[] = {
     DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
                      MemoryRegion *),
 #endif
+    DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8f145733ce..9fc2696db5 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -374,6 +374,10 @@ struct CPUState {
     bool created;
     bool stop;
     bool stopped;
+
+    /* Should CPU start in powered-off state? */
+    bool start_powered_off;
+
     bool unplug;
     bool crash_occurred;
     bool exit_request;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 111579554f..ec65c7653f 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -174,8 +174,8 @@ static void arm_cpu_reset(DeviceState *dev)
     env->vfp.xregs[ARM_VFP_MVFR1] = cpu->isar.mvfr1;
     env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2;
 
-    cpu->power_state = cpu->start_powered_off ? PSCI_OFF : PSCI_ON;
-    s->halted = cpu->start_powered_off;
+    cpu->power_state = s->start_powered_off ? PSCI_OFF : PSCI_ON;
+    s->halted = s->start_powered_off;
 
     if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
         env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
@@ -2182,7 +2182,6 @@ static const ARMCPUInfo arm_cpus[] = {
 };
 
 static Property arm_cpu_properties[] = {
-    DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
     DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
     DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0),
     DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8ed423ea..a925d26996 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -810,9 +810,6 @@ struct ARMCPU {
      */
     uint32_t psci_version;
 
-    /* Should CPU start in PSCI powered-off state? */
-    bool start_powered_off;
-
     /* Current power state, access guarded by BQL */
     ARMPSCIState power_state;
 
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0af46b41c8..1f2b8f8b7a 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -218,7 +218,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
     /* Determine init features for this CPU */
     memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
-    if (cpu->start_powered_off) {
+    if (cs->start_powered_off) {
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
     }
     if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1169237905..f8a6d905fb 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -775,7 +775,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
     /* Determine init features for this CPU */
     memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
-    if (cpu->start_powered_off) {
+    if (cs->start_powered_off) {
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
     }
     if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {

[PATCH v4 2/8] target/arm: Move setting of CPU halted state to generic code

[Thiago Jung Bauermann]

This change is in a separate patch because it's not so obvious that it
won't cause a regression.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/core/cpu.c    | 2 +-
 target/arm/cpu.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 594441a150..71bb7859f1 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -258,7 +258,7 @@ static void cpu_common_reset(DeviceState *dev)
     }
 
     cpu->interrupt_request = 0;
-    cpu->halted = 0;
+    cpu->halted = cpu->start_powered_off;
     cpu->mem_io_pc = 0;
     cpu->icount_extra = 0;
     atomic_set(&cpu->icount_decr_ptr->u32, 0);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ec65c7653f..b6c65e4df6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -175,7 +175,6 @@ static void arm_cpu_reset(DeviceState *dev)
     env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2;
 
     cpu->power_state = s->start_powered_off ? PSCI_OFF : PSCI_ON;
-    s->halted = s->start_powered_off;
 
     if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
         env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';

[PATCH v4 3/8] ppc/spapr: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

PowerPC sPAPR CPUs start in the halted state, and spapr_reset_vcpu()
attempts to implement this by setting CPUState::halted to 1. But that's too
late for the case of hotplugged CPUs in a machine configure with 2 or more
threads per core.

By then, other parts of QEMU have already caused the vCPU to run in an
unitialized state a couple of times. For example, ppc_cpu_reset() calls
ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
a KVM_RUN ioctl on the new vCPU before the guest is able to make the
start-cpu RTAS call to initialize its register state.

This problem doesn't seem to cause visible issues for regular guests, but
on a secure guest running under the Ultravisor it does. The Ultravisor
relies on being able to snoop on the start-cpu RTAS call to map vCPUs to
guests, and this issue causes it to see a stray vCPU that doesn't belong to
any guest.

Fix by setting the start-powered-off CPUState property in
spapr_create_vcpu(), which makes cpu_common_reset() initialize
CPUState::halted to 1 at an earlier moment.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/ppc/spapr_cpu_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c4f47dcc04..2125fdac34 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
 
     cpu_reset(cs);
 
-    /* All CPUs start halted.  CPU0 is unhalted from the machine level
-     * reset code and the rest are explicitly started up by the guest
-     * using an RTAS call */
-    cs->halted = 1;
-
     env->spr[SPR_HIOR] = 0;
 
     lpcr = env->spr[SPR_LPCR];
@@ -274,6 +269,11 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
 
     cs = CPU(obj);
     cpu = POWERPC_CPU(obj);
+    /*
+     * All CPUs start halted. CPU0 is unhalted from the machine level reset code
+     * and the rest are explicitly started up by the guest using an RTAS call.
+     */
+    cs->start_powered_off = true;
     cs->cpu_index = cc->core_id + i;
     spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
     if (local_err) {

[PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use
the start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

Also change creation of CPU object from cpu_create() to object_new() and
qdev_realize() because cpu_create() realizes the CPU and it's not possible
to set a property after the object is realized.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/ppc/e500.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ab9884e315..0077aca74d 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
 
     cpu_reset(cs);
 
-    /* Secondary CPU starts in halted state for now. Needs to change when
-       implementing non-kernel boot. */
-    cs->halted = 1;
     cs->exception_index = EXCP_HLT;
 }
 
@@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
         PowerPCCPU *cpu;
         CPUState *cs;
         qemu_irq *input;
+        Error *err = NULL;
 
-        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
+        cpu = POWERPC_CPU(object_new(machine->cpu_type));
         env = &cpu->env;
         cs = CPU(cpu);
 
@@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
         } else {
             /* Secondary CPUs */
             qemu_register_reset(ppce500_cpu_reset_sec, cpu);
+
+            /*
+             * Secondary CPU starts in halted state for now. Needs to change
+             * when implementing non-kernel boot.
+             */
+            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
+                                     &error_abort);
+        }
+
+        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
+            error_report_err(err);
+            object_unref(OBJECT(cs));
+            exit(EXIT_FAILURE);
         }
     }
 

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Philippe Mathieu-Daudé]

On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use
> the start-powered-off property which makes cpu_common_reset() initialize it
> to 1 in common code.
> 
> Also change creation of CPU object from cpu_create() to object_new() and
> qdev_realize() because cpu_create() realizes the CPU and it's not possible
> to set a property after the object is realized.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  hw/ppc/e500.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index ab9884e315..0077aca74d 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
>  
>      cpu_reset(cs);
>  
> -    /* Secondary CPU starts in halted state for now. Needs to change when
> -       implementing non-kernel boot. */
> -    cs->halted = 1;
>      cs->exception_index = EXCP_HLT;
>  }
>  
> @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
>          PowerPCCPU *cpu;
>          CPUState *cs;
>          qemu_irq *input;
> +        Error *err = NULL;
>  
> -        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> +        cpu = POWERPC_CPU(object_new(machine->cpu_type));
>          env = &cpu->env;
>          cs = CPU(cpu);
>  
> @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
>          } else {
>              /* Secondary CPUs */
>              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
> +
> +            /*
> +             * Secondary CPU starts in halted state for now. Needs to change
> +             * when implementing non-kernel boot.
> +             */
> +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
> +                                     &error_abort);

[*]

> +        }
> +
> +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
> +            error_report_err(err);
> +            object_unref(OBJECT(cs));
> +            exit(EXIT_FAILURE);
>          }

The last 4 lines are equivalent to:

           qdev_realize(DEVICE(cs), NULL, &error_fatal)) {

This is also the preferred form, as we can not propagate errors
from the machine_init() handler.

Since you use &error_abort in [*], maybe you want to use it here too.

>      }
>  
> 

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Philippe Mathieu-Daudé]

On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote:
> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
>> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use
>> the start-powered-off property which makes cpu_common_reset() initialize it
>> to 1 in common code.
>>
>> Also change creation of CPU object from cpu_create() to object_new() and
>> qdev_realize() because cpu_create() realizes the CPU and it's not possible
>> to set a property after the object is realized.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>>  hw/ppc/e500.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index ab9884e315..0077aca74d 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
>>  
>>      cpu_reset(cs);
>>  
>> -    /* Secondary CPU starts in halted state for now. Needs to change when
>> -       implementing non-kernel boot. */
>> -    cs->halted = 1;
>>      cs->exception_index = EXCP_HLT;
>>  }
>>  
>> @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
>>          PowerPCCPU *cpu;
>>          CPUState *cs;
>>          qemu_irq *input;
>> +        Error *err = NULL;
>>  
>> -        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> +        cpu = POWERPC_CPU(object_new(machine->cpu_type));
>>          env = &cpu->env;
>>          cs = CPU(cpu);
>>  
>> @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
>>          } else {
>>              /* Secondary CPUs */
>>              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>> +
>> +            /*
>> +             * Secondary CPU starts in halted state for now. Needs to change
>> +             * when implementing non-kernel boot.
>> +             */
>> +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
>> +                                     &error_abort);
> 
> [*]
> 
>> +        }
>> +
>> +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
>> +            error_report_err(err);
>> +            object_unref(OBJECT(cs));
>> +            exit(EXIT_FAILURE);
>>          }
> 
> The last 4 lines are equivalent to:
> 
>            qdev_realize(DEVICE(cs), NULL, &error_fatal)) {

I meant:

             qdev_realize(DEVICE(cs), NULL, &error_fatal);

> 
> This is also the preferred form, as we can not propagate errors
> from the machine_init() handler.
> 
> Since you use &error_abort in [*], maybe you want to use it here too.
> 
>>      }
>>  
>>
> 

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Hello Philippe,

Thanks for your review.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote:
>> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
>>> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use
>>> the start-powered-off property which makes cpu_common_reset() initialize it
>>> to 1 in common code.
>>>
>>> Also change creation of CPU object from cpu_create() to object_new() and
>>> qdev_realize() because cpu_create() realizes the CPU and it's not possible
>>> to set a property after the object is realized.
>>>
>>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>>> ---
>>>  hw/ppc/e500.c | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index ab9884e315..0077aca74d 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
>>>
>>>      cpu_reset(cs);
>>>
>>> -    /* Secondary CPU starts in halted state for now. Needs to change when
>>> -       implementing non-kernel boot. */
>>> -    cs->halted = 1;
>>>      cs->exception_index = EXCP_HLT;
>>>  }
>>>
>>> @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
>>>          PowerPCCPU *cpu;
>>>          CPUState *cs;
>>>          qemu_irq *input;
>>> +        Error *err = NULL;
>>>
>>> -        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>>> +        cpu = POWERPC_CPU(object_new(machine->cpu_type));
>>>          env = &cpu->env;
>>>          cs = CPU(cpu);
>>>
>>> @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
>>>          } else {
>>>              /* Secondary CPUs */
>>>              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>>> +
>>> +            /*
>>> +             * Secondary CPU starts in halted state for now. Needs to change
>>> +             * when implementing non-kernel boot.
>>> +             */
>>> +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
>>> +                                     &error_abort);
>>
>> [*]
>>
>>> +        }
>>> +
>>> +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
>>> +            error_report_err(err);
>>> +            object_unref(OBJECT(cs));
>>> +            exit(EXIT_FAILURE);
>>>          }
>>
>> The last 4 lines are equivalent to:
>>
>>            qdev_realize(DEVICE(cs), NULL, &error_fatal)) {
>
> I meant:
>
>              qdev_realize(DEVICE(cs), NULL, &error_fatal);

Ah! Thanks for pointing it out. I'll use that (along with
qdev_realize_and_unref()).

>
>>
>> This is also the preferred form, as we can not propagate errors
>> from the machine_init() handler.
>>
>> Since you use &error_abort in [*], maybe you want to use it here too.

I think &error_fatal is better since it preserves the behavior from
cpu_create().

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:

> Hello Philippe,
>
> Thanks for your review.
>
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote:
>>>> @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
>>>>          } else {
>>>>              /* Secondary CPUs */
>>>>              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>>>> +
>>>> +            /*
>>>> +             * Secondary CPU starts in halted state for now. Needs to change
>>>> +             * when implementing non-kernel boot.
>>>> +             */
>>>> +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
>>>> +                                     &error_abort);
>>>
>>> [*]
>>>
>>>> +        }
>>>> +
>>>> +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
>>>> +            error_report_err(err);
>>>> +            object_unref(OBJECT(cs));
>>>> +            exit(EXIT_FAILURE);
>>>>          }
>>>
>>> The last 4 lines are equivalent to:
>>>
>>>            qdev_realize(DEVICE(cs), NULL, &error_fatal)) {
>>
>> I meant:
>>
>>              qdev_realize(DEVICE(cs), NULL, &error_fatal);
>
> Ah! Thanks for pointing it out. I'll use that (along with
> qdev_realize_and_unref()).
>
>>
>>>
>>> This is also the preferred form, as we can not propagate errors
>>> from the machine_init() handler.
>>>
>>> Since you use &error_abort in [*], maybe you want to use it here too.
>
> I think &error_fatal is better since it preserves the behavior from
> cpu_create().

I'll change [*] to &error_fatal as well, for consistency.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Igor Mammedov]

On Tue, 18 Aug 2020 09:22:05 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
> > Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use
> > the start-powered-off property which makes cpu_common_reset() initialize it
> > to 1 in common code.
> > 
> > Also change creation of CPU object from cpu_create() to object_new() and
> > qdev_realize() because cpu_create() realizes the CPU and it's not possible
> > to set a property after the object is realized.
> > 
> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > ---
> >  hw/ppc/e500.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index ab9884e315..0077aca74d 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
> >  
> >      cpu_reset(cs);
> >  
> > -    /* Secondary CPU starts in halted state for now. Needs to change when
> > -       implementing non-kernel boot. */
> > -    cs->halted = 1;
> >      cs->exception_index = EXCP_HLT;
> >  }
> >  
> > @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
> >          PowerPCCPU *cpu;
> >          CPUState *cs;
> >          qemu_irq *input;
> > +        Error *err = NULL;
> >  
> > -        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> > +        cpu = POWERPC_CPU(object_new(machine->cpu_type));
> >          env = &cpu->env;
> >          cs = CPU(cpu);
> >  
> > @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
> >          } else {
> >              /* Secondary CPUs */
> >              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
> > +
> > +            /*
> > +             * Secondary CPU starts in halted state for now. Needs to change
> > +             * when implementing non-kernel boot.
> > +             */
> > +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
> > +                                     &error_abort);  
> 
> [*]
> 
> > +        }
> > +
> > +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
> > +            error_report_err(err);
> > +            object_unref(OBJECT(cs));
> > +            exit(EXIT_FAILURE);
> >          }  
> 
> The last 4 lines are equivalent to:
> 
>            qdev_realize(DEVICE(cs), NULL, &error_fatal)) {
> 
> This is also the preferred form, as we can not propagate errors
> from the machine_init() handler.
not exactly, it will leak refference, but we are dying anyways so who cares.

> 
> Since you use &error_abort in [*], maybe you want to use it here too.
> 
> >      }
> >  
> >   
> 
> 

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Igor Mammedov]

On Tue, 18 Aug 2020 00:33:19 -0300
Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:

[...]

> Also change creation of CPU object from cpu_create() to object_new() and
> qdev_realize() because cpu_create() realizes the CPU and it's not possible
> to set a property after the object is realized.

cpu_create was introduced to remove code duplication in simple cases where
we do not need to set properties on created cpu.

returning back to manual object_new + realize() is fine if it 's only
small set of of boards. If it's tree-wide change then that would bring
back all code duplication that cpu_create() got rid of.

An alternative way is to use 'hotplug' callbacks to let board set
additional properties before cpu's realize is called.

example:
  hw/ppc/spapr.c:
   spapr_machine_class_init()
     mc->get_hotplug_handler = spapr_get_hotplug_handler;                         
     hc->pre_plug = spapr_machine_device_pre_plug; 
   ...
   static const TypeInfo spapr_machine_info = {
   ...
        { TYPE_HOTPLUG_HANDLER },

that might work in generic case if it is put into generic machine code,
and existing users of mc->get_hotplug_handler/hc->pre_plug were taken care of.
In which case board would only need to set MachineClass:cpu-start-powered-of
to gate property setting.

 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  hw/ppc/e500.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index ab9884e315..0077aca74d 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
>  
>      cpu_reset(cs);
>  
> -    /* Secondary CPU starts in halted state for now. Needs to change when
> -       implementing non-kernel boot. */
> -    cs->halted = 1;
>      cs->exception_index = EXCP_HLT;
>  }
>  
> @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
>          PowerPCCPU *cpu;
>          CPUState *cs;
>          qemu_irq *input;
> +        Error *err = NULL;
>  
> -        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> +        cpu = POWERPC_CPU(object_new(machine->cpu_type));
>          env = &cpu->env;
>          cs = CPU(cpu);
>  
> @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
>          } else {
>              /* Secondary CPUs */
>              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
> +
> +            /*
> +             * Secondary CPU starts in halted state for now. Needs to change
> +             * when implementing non-kernel boot.
> +             */
> +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
> +                                     &error_abort);
> +        }
> +
> +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
> +            error_report_err(err);
> +            object_unref(OBJECT(cs));
> +            exit(EXIT_FAILURE);
>          }

btw:
board leaks cpu reference (from cpu_create()/object_new()) since qdev_realize()
adds it's own and the caller of object_new() is suposed to free the original one.

in this case qdev_realize_and_unref() fits nicely.


>      }
>  
> 

Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Hi Igor,

Thank you for reviewing these patches, and the tips you provided here
and on other messages on how to fix the refcount issues.

Igor Mammedov <imammedo@redhat.com> writes:

> On Tue, 18 Aug 2020 00:33:19 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
> [...]
>
>> Also change creation of CPU object from cpu_create() to object_new() and
>> qdev_realize() because cpu_create() realizes the CPU and it's not possible
>> to set a property after the object is realized.
>
> cpu_create was introduced to remove code duplication in simple cases where
> we do not need to set properties on created cpu.
>
> returning back to manual object_new + realize() is fine if it 's only
> small set of of boards. If it's tree-wide change then that would bring
> back all code duplication that cpu_create() got rid of.

This is only necessary for boards where the secondary CPUs start powered
off, so it's not a tree-wide change.

> An alternative way is to use 'hotplug' callbacks to let board set
> additional properties before cpu's realize is called.
>
> example:
>   hw/ppc/spapr.c:
>    spapr_machine_class_init()
>      mc->get_hotplug_handler = spapr_get_hotplug_handler;
>      hc->pre_plug = spapr_machine_device_pre_plug;
>    ...
>    static const TypeInfo spapr_machine_info = {
>    ...
>         { TYPE_HOTPLUG_HANDLER },
>
> that might work in generic case if it is put into generic machine code,
> and existing users of mc->get_hotplug_handler/hc->pre_plug were taken care of.
> In which case board would only need to set MachineClass:cpu-start-powered-of
> to gate property setting.

Thank you for this idea. Though if possible I'd like to keep the new
code as similar as possible to the original code to minimize unwanted
"perturbations" in how and when objects are created and initialized.

>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>>  hw/ppc/e500.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index ab9884e315..0077aca74d 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
>>
>>      cpu_reset(cs);
>>
>> -    /* Secondary CPU starts in halted state for now. Needs to change when
>> -       implementing non-kernel boot. */
>> -    cs->halted = 1;
>>      cs->exception_index = EXCP_HLT;
>>  }
>>
>> @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine)
>>          PowerPCCPU *cpu;
>>          CPUState *cs;
>>          qemu_irq *input;
>> +        Error *err = NULL;
>>
>> -        cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> +        cpu = POWERPC_CPU(object_new(machine->cpu_type));
>>          env = &cpu->env;
>>          cs = CPU(cpu);
>>
>> @@ -897,6 +895,19 @@ void ppce500_init(MachineState *machine)
>>          } else {
>>              /* Secondary CPUs */
>>              qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>> +
>> +            /*
>> +             * Secondary CPU starts in halted state for now. Needs to change
>> +             * when implementing non-kernel boot.
>> +             */
>> +            object_property_set_bool(OBJECT(cs), "start-powered-off", true,
>> +                                     &error_abort);
>> +        }
>> +
>> +        if (!qdev_realize(DEVICE(cs), NULL, &err)) {
>> +            error_report_err(err);
>> +            object_unref(OBJECT(cs));
>> +            exit(EXIT_FAILURE);
>>          }
>
> btw:
> board leaks cpu reference (from cpu_create()/object_new()) since qdev_realize()
> adds it's own and the caller of object_new() is suposed to free the original one.
>
> in this case qdev_realize_and_unref() fits nicely.

I will make this change.
--
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Instead of setting CPUState::halted to 1 in main_cpu_reset(), use the
start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

Also change creation of CPU object from cpu_create() to object_new() and
qdev_realize() because cpu_create() realizes the CPU and it's not possible to
set a property after the object is realized.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/mips/cps.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 615e1a1ad2..be85357dc0 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque)
     CPUState *cs = CPU(cpu);
 
     cpu_reset(cs);
-
-    /* All VPs are halted on reset. Leave powering up to CPC. */
-    cs->halted = 1;
 }
 
 static bool cpu_mips_itu_supported(CPUMIPSState *env)
@@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     bool saar_present = false;
 
     for (i = 0; i < s->num_vp; i++) {
-        cpu = MIPS_CPU(cpu_create(s->cpu_type));
+        Error *err = NULL;
+
+        cpu = MIPS_CPU(object_new(s->cpu_type));
 
         /* Init internal devices */
         cpu_mips_irq_init_cpu(cpu);
@@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
             env->itc_tag = mips_itu_get_tag_region(&s->itu);
             env->itu = &s->itu;
         }
+        /* All VPs are halted on reset. Leave powering up to CPC. */
+        object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
+                                 &error_abort);
         qemu_register_reset(main_cpu_reset, cpu);
+
+        if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
+            error_report_err(err);
+            object_unref(OBJECT(cpu));
+            exit(EXIT_FAILURE);
+        }
     }
 
     cpu = MIPS_CPU(first_cpu);

Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property

[Philippe Mathieu-Daudé]

On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
> Instead of setting CPUState::halted to 1 in main_cpu_reset(), use the
> start-powered-off property which makes cpu_common_reset() initialize it
> to 1 in common code.
> 
> Also change creation of CPU object from cpu_create() to object_new() and
> qdev_realize() because cpu_create() realizes the CPU and it's not possible to
> set a property after the object is realized.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  hw/mips/cps.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 615e1a1ad2..be85357dc0 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque)
>      CPUState *cs = CPU(cpu);
>  
>      cpu_reset(cs);
> -
> -    /* All VPs are halted on reset. Leave powering up to CPC. */
> -    cs->halted = 1;
>  }
>  
>  static bool cpu_mips_itu_supported(CPUMIPSState *env)
> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>      bool saar_present = false;
>  
>      for (i = 0; i < s->num_vp; i++) {
> -        cpu = MIPS_CPU(cpu_create(s->cpu_type));
> +        Error *err = NULL;
> +
> +        cpu = MIPS_CPU(object_new(s->cpu_type));
>  
>          /* Init internal devices */
>          cpu_mips_irq_init_cpu(cpu);
> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>              env->itc_tag = mips_itu_get_tag_region(&s->itu);
>              env->itu = &s->itu;
>          }
> +        /* All VPs are halted on reset. Leave powering up to CPC. */
> +        object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
> +                                 &error_abort);
>          qemu_register_reset(main_cpu_reset, cpu);
> +
> +        if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
> +            error_report_err(err);
> +            object_unref(OBJECT(cpu));
> +            exit(EXIT_FAILURE);
> +        }

Here errp is available, so we can propagate the error to the caller:

           if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
               return;
           }

For example in hw/mips/boston.c:

    object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS);
    object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
                            &error_fatal);
    object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
                            &error_fatal);
    sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);

This will be propagated here ---------------^^^^^^^^^^^^^

>      }
>  
>      cpu = MIPS_CPU(first_cpu);
> 

Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property

[Philippe Mathieu-Daudé]

On 8/18/20 9:26 AM, Philippe Mathieu-Daudé wrote:
> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
>> Instead of setting CPUState::halted to 1 in main_cpu_reset(), use the
>> start-powered-off property which makes cpu_common_reset() initialize it
>> to 1 in common code.
>>
>> Also change creation of CPU object from cpu_create() to object_new() and
>> qdev_realize() because cpu_create() realizes the CPU and it's not possible to
>> set a property after the object is realized.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>>  hw/mips/cps.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
>> index 615e1a1ad2..be85357dc0 100644
>> --- a/hw/mips/cps.c
>> +++ b/hw/mips/cps.c
>> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque)
>>      CPUState *cs = CPU(cpu);
>>  
>>      cpu_reset(cs);
>> -
>> -    /* All VPs are halted on reset. Leave powering up to CPC. */
>> -    cs->halted = 1;
>>  }
>>  
>>  static bool cpu_mips_itu_supported(CPUMIPSState *env)
>> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>>      bool saar_present = false;
>>  
>>      for (i = 0; i < s->num_vp; i++) {
>> -        cpu = MIPS_CPU(cpu_create(s->cpu_type));
>> +        Error *err = NULL;
>> +
>> +        cpu = MIPS_CPU(object_new(s->cpu_type));
>>  
>>          /* Init internal devices */
>>          cpu_mips_irq_init_cpu(cpu);
>> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>>              env->itc_tag = mips_itu_get_tag_region(&s->itu);
>>              env->itu = &s->itu;
>>          }
>> +        /* All VPs are halted on reset. Leave powering up to CPC. */
>> +        object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
>> +                                 &error_abort);
>>          qemu_register_reset(main_cpu_reset, cpu);
>> +
>> +        if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
>> +            error_report_err(err);
>> +            object_unref(OBJECT(cpu));
>> +            exit(EXIT_FAILURE);
>> +        }
> 
> Here errp is available, so we can propagate the error to the caller:
> 
>            if (!qdev_realize(DEVICE(cpu), NULL, errp)) {

Igor corrected me in the previous patch, to avoid leaking the
reference this snippet misses:

                 object_unref(OBJECT(cpu));

>                return;
>            }
> 
> For example in hw/mips/boston.c:
> 
>     object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS);
>     object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
>                             &error_fatal);
>     object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
>                             &error_fatal);
>     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
> 
> This will be propagated here ---------------^^^^^^^^^^^^^
> 
>>      }
>>  
>>      cpu = MIPS_CPU(first_cpu);
>>

Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property

[Philippe Mathieu-Daudé]

On 8/18/20 1:03 PM, Philippe Mathieu-Daudé wrote:
> On 8/18/20 9:26 AM, Philippe Mathieu-Daudé wrote:
>> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
>>> Instead of setting CPUState::halted to 1 in main_cpu_reset(), use the
>>> start-powered-off property which makes cpu_common_reset() initialize it
>>> to 1 in common code.
>>>
>>> Also change creation of CPU object from cpu_create() to object_new() and
>>> qdev_realize() because cpu_create() realizes the CPU and it's not possible to
>>> set a property after the object is realized.
>>>
>>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>>> ---
>>>  hw/mips/cps.c | 16 ++++++++++++----
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
>>> index 615e1a1ad2..be85357dc0 100644
>>> --- a/hw/mips/cps.c
>>> +++ b/hw/mips/cps.c
>>> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque)
>>>      CPUState *cs = CPU(cpu);
>>>  
>>>      cpu_reset(cs);
>>> -
>>> -    /* All VPs are halted on reset. Leave powering up to CPC. */
>>> -    cs->halted = 1;
>>>  }
>>>  
>>>  static bool cpu_mips_itu_supported(CPUMIPSState *env)
>>> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>>>      bool saar_present = false;
>>>  
>>>      for (i = 0; i < s->num_vp; i++) {
>>> -        cpu = MIPS_CPU(cpu_create(s->cpu_type));
>>> +        Error *err = NULL;
>>> +
>>> +        cpu = MIPS_CPU(object_new(s->cpu_type));
>>>  
>>>          /* Init internal devices */
>>>          cpu_mips_irq_init_cpu(cpu);
>>> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>>>              env->itc_tag = mips_itu_get_tag_region(&s->itu);
>>>              env->itu = &s->itu;
>>>          }
>>> +        /* All VPs are halted on reset. Leave powering up to CPC. */
>>> +        object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
>>> +                                 &error_abort);
>>>          qemu_register_reset(main_cpu_reset, cpu);
>>> +
>>> +        if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
>>> +            error_report_err(err);
>>> +            object_unref(OBJECT(cpu));
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>
>> Here errp is available, so we can propagate the error to the caller:
>>
>>            if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
> 
> Igor corrected me in the previous patch, to avoid leaking the
> reference this snippet misses:
> 
>                  object_unref(OBJECT(cpu));

Well this is simply:

              if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) {
> 
>>                return;
>>            }
>>
>> For example in hw/mips/boston.c:
>>
>>     object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS);
>>     object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
>>                             &error_fatal);
>>     object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
>>                             &error_fatal);
>>     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
>>
>> This will be propagated here ---------------^^^^^^^^^^^^^
>>
>>>      }
>>>  
>>>      cpu = MIPS_CPU(first_cpu);
>>>
> 

Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
>> Instead of setting CPUState::halted to 1 in main_cpu_reset(), use the
>> start-powered-off property which makes cpu_common_reset() initialize it
>> to 1 in common code.
>>
>> Also change creation of CPU object from cpu_create() to object_new() and
>> qdev_realize() because cpu_create() realizes the CPU and it's not possible to
>> set a property after the object is realized.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>>  hw/mips/cps.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
>> index 615e1a1ad2..be85357dc0 100644
>> --- a/hw/mips/cps.c
>> +++ b/hw/mips/cps.c
>> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque)
>>      CPUState *cs = CPU(cpu);
>>
>>      cpu_reset(cs);
>> -
>> -    /* All VPs are halted on reset. Leave powering up to CPC. */
>> -    cs->halted = 1;
>>  }
>>
>>  static bool cpu_mips_itu_supported(CPUMIPSState *env)
>> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>>      bool saar_present = false;
>>
>>      for (i = 0; i < s->num_vp; i++) {
>> -        cpu = MIPS_CPU(cpu_create(s->cpu_type));
>> +        Error *err = NULL;
>> +
>> +        cpu = MIPS_CPU(object_new(s->cpu_type));
>>
>>          /* Init internal devices */
>>          cpu_mips_irq_init_cpu(cpu);
>> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>>              env->itc_tag = mips_itu_get_tag_region(&s->itu);
>>              env->itu = &s->itu;
>>          }
>> +        /* All VPs are halted on reset. Leave powering up to CPC. */
>> +        object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
>> +                                 &error_abort);
>>          qemu_register_reset(main_cpu_reset, cpu);
>> +
>> +        if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
>> +            error_report_err(err);
>> +            object_unref(OBJECT(cpu));
>> +            exit(EXIT_FAILURE);
>> +        }
>
> Here errp is available, so we can propagate the error to the caller:
>
>            if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
>                return;
>            }

Ah, nice. I made this change (using qdev_realize_and_unref()).

I also changed object_property_set_bool() to use errp as well instead of
&error_abort (and also early return on error).

> For example in hw/mips/boston.c:
>
>     object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS);
>     object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
>                             &error_fatal);
>     object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
>                             &error_fatal);
>     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
>
> This will be propagated here ---------------^^^^^^^^^^^^^

Interesting. Thanks for the explanation.

--
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH v4 6/8] sparc/sun4m: Remove main_cpu_reset()

[Thiago Jung Bauermann]

We rely on cpu_common_reset() to set cs->halted to 0, so main_cpu_reset()
is pointless.

Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/sparc/sun4m.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 9be930415f..f1d92df781 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -218,15 +218,6 @@ static void dummy_cpu_set_irq(void *opaque, int irq, int level)
 {
 }
 
-static void main_cpu_reset(void *opaque)
-{
-    SPARCCPU *cpu = opaque;
-    CPUState *cs = CPU(cpu);
-
-    cpu_reset(cs);
-    cs->halted = 0;
-}
-
 static void secondary_cpu_reset(void *opaque)
 {
     SPARCCPU *cpu = opaque;
@@ -827,9 +818,7 @@ static void cpu_devinit(const char *cpu_type, unsigned int id,
     env = &cpu->env;
 
     cpu_sparc_set_id(env, id);
-    if (id == 0) {
-        qemu_register_reset(main_cpu_reset, cpu);
-    } else {
+    if (id != 0) {
         qemu_register_reset(secondary_cpu_reset, cpu);
         cs = CPU(cpu);
         cs->halted = 1;

[PATCH v4 7/8] sparc/sun4m: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Instead of setting CPUState::halted to 1 in secondary_cpu_reset(), use the
start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

This makes secondary_cpu_reset() unnecessary, so remove it.

Also remove setting of cs->halted from cpu_devinit(), which seems out of
place when compared to similar code in other architectures (e.g.,
ppce500_init() in hw/ppc/e500.c).

Finally, change creation of CPU object from cpu_create() to object_new()
and qdev_realize() because cpu_create() realizes the CPU and it's not
possible to set a property after the object is realized.

Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/sparc/sun4m.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index f1d92df781..41a7c5fa86 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -218,15 +218,6 @@ static void dummy_cpu_set_irq(void *opaque, int irq, int level)
 {
 }
 
-static void secondary_cpu_reset(void *opaque)
-{
-    SPARCCPU *cpu = opaque;
-    CPUState *cs = CPU(cpu);
-
-    cpu_reset(cs);
-    cs->halted = 1;
-}
-
 static void cpu_halt_signal(void *opaque, int irq, int level)
 {
     if (level && current_cpu) {
@@ -810,21 +801,24 @@ static const TypeInfo ram_info = {
 static void cpu_devinit(const char *cpu_type, unsigned int id,
                         uint64_t prom_addr, qemu_irq **cpu_irqs)
 {
-    CPUState *cs;
     SPARCCPU *cpu;
     CPUSPARCState *env;
+    Error *err = NULL;
 
-    cpu = SPARC_CPU(cpu_create(cpu_type));
+    cpu = SPARC_CPU(object_new(cpu_type));
     env = &cpu->env;
 
     cpu_sparc_set_id(env, id);
-    if (id != 0) {
-        qemu_register_reset(secondary_cpu_reset, cpu);
-        cs = CPU(cpu);
-        cs->halted = 1;
-    }
+    object_property_set_bool(OBJECT(cpu), "start-powered-off", id != 0,
+                             &error_abort);
     *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
     env->prom_addr = prom_addr;
+
+    if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        exit(EXIT_FAILURE);
+    }
 }
 
 static void dummy_fdc_tc(void *opaque, int irq, int level)

Re: [PATCH v4 7/8] sparc/sun4m: Use start-powered-off CPUState property

[Philippe Mathieu-Daudé]

On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote:
> Instead of setting CPUState::halted to 1 in secondary_cpu_reset(), use the
> start-powered-off property which makes cpu_common_reset() initialize it
> to 1 in common code.
> 
> This makes secondary_cpu_reset() unnecessary, so remove it.
> 
> Also remove setting of cs->halted from cpu_devinit(), which seems out of
> place when compared to similar code in other architectures (e.g.,
> ppce500_init() in hw/ppc/e500.c).
> 
> Finally, change creation of CPU object from cpu_create() to object_new()
> and qdev_realize() because cpu_create() realizes the CPU and it's not
> possible to set a property after the object is realized.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  hw/sparc/sun4m.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index f1d92df781..41a7c5fa86 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -218,15 +218,6 @@ static void dummy_cpu_set_irq(void *opaque, int irq, int level)
>  {
>  }
>  
> -static void secondary_cpu_reset(void *opaque)
> -{
> -    SPARCCPU *cpu = opaque;
> -    CPUState *cs = CPU(cpu);
> -
> -    cpu_reset(cs);
> -    cs->halted = 1;
> -}
> -
>  static void cpu_halt_signal(void *opaque, int irq, int level)
>  {
>      if (level && current_cpu) {
> @@ -810,21 +801,24 @@ static const TypeInfo ram_info = {
>  static void cpu_devinit(const char *cpu_type, unsigned int id,
>                          uint64_t prom_addr, qemu_irq **cpu_irqs)
>  {
> -    CPUState *cs;
>      SPARCCPU *cpu;
>      CPUSPARCState *env;
> +    Error *err = NULL;
>  
> -    cpu = SPARC_CPU(cpu_create(cpu_type));
> +    cpu = SPARC_CPU(object_new(cpu_type));
>      env = &cpu->env;
>  
>      cpu_sparc_set_id(env, id);
> -    if (id != 0) {
> -        qemu_register_reset(secondary_cpu_reset, cpu);
> -        cs = CPU(cpu);
> -        cs->halted = 1;
> -    }
> +    object_property_set_bool(OBJECT(cpu), "start-powered-off", id != 0,
> +                             &error_abort);
>      *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
>      env->prom_addr = prom_addr;
> +
> +    if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
> +        error_report_err(err);
> +        object_unref(OBJECT(cpu));
> +        exit(EXIT_FAILURE);
> +    }

Simply use:

       qdev_realize(DEVICE(cpu), NULL, &error_abort);

>  }
>  
>  static void dummy_fdc_tc(void *opaque, int irq, int level)
> 

[PATCH v4 8/8] target/s390x: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the
start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

Note that this changes behavior by setting cs->halted to 1 on reset, which
didn't happen before.

Acked-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 target/s390x/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 08eb674d22..73d7d6007e 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -291,7 +291,7 @@ static void s390_cpu_initfn(Object *obj)
     S390CPU *cpu = S390_CPU(obj);
 
     cpu_set_cpustate_pointers(cpu);
-    cs->halted = 1;
+    cs->start_powered_off = true;
     cs->exception_index = EXCP_HLT;
 #if !defined(CONFIG_USER_ONLY)
     object_property_add(obj, "crash-information", "GuestPanicInformation",