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

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

[Thiago Jung Bauermann]

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 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           |  6 +++---
 hw/ppc/e500.c           | 10 +++++++---
 hw/ppc/spapr_cpu_core.c | 10 +++++-----
 hw/sparc/sun4m.c        | 28 ++--------------------------
 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, 27 insertions(+), 47 deletions(-)

[PATCH v3 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>
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(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

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 v3 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>
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(-)

NB: I wasn't able to run this patch on an ARM machine. I did run it on
a ppc64le pseries KVM guest.

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 v3 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>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/ppc/spapr_cpu_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

NB: Tested on ppc64le pseries KVM guest with two threads per core. 
Hot-plugging additional cores doesn't cause the bug described above
anymore.

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 v3 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/ppc/e500.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ab9884e315..dda71bc05d 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;
 }
 
@@ -897,6 +894,13 @@ 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);
         }
     }
 

[PATCH v3 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/mips/cps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 615e1a1ad2..d5b6c78019 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)
@@ -89,6 +86,9 @@ 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);
     }
 

[PATCH v3 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>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/sparc/sun4m.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

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 v3 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).

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

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index f1d92df781..fd74e516bb 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,7 +801,6 @@ 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;
 
@@ -818,11 +808,8 @@ 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(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;
 }

[RFC PATCH v3 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.

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

NB: I was only able to test that this patch builds. I wasn't able to
run it.

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",

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:50PM -0300, Thiago Jung Bauermann wrote:
> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.
> 
> 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)) {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:51PM -0300, Thiago Jung Bauermann wrote:
> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/core/cpu.c    | 2 +-
>  target/arm/cpu.c | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> NB: I wasn't able to run this patch on an ARM machine. I did run it on
> a ppc64le pseries KVM guest.
> 
> 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';
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:52PM -0300, Thiago Jung Bauermann wrote:
65;6003;1c> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_cpu_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> NB: Tested on ppc64le pseries KVM guest with two threads per core. 
> Hot-plugging additional cores doesn't cause the bug described above
> anymore.
> 
> 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) {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:53PM -0300, 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.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/e500.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index ab9884e315..dda71bc05d 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;
>  }
>  
> @@ -897,6 +894,13 @@ 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);
>          }
>      }
>  
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:54PM -0300, 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.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mips/cps.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.
> 
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 615e1a1ad2..d5b6c78019 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)
> @@ -89,6 +86,9 @@ 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);
>      }
>  
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:55PM -0300, Thiago Jung Bauermann wrote:
> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Revieed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/sparc/sun4m.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.
> 
> 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;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:56PM -0300, 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).
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/sparc/sun4m.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index f1d92df781..fd74e516bb 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,7 +801,6 @@ 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;
>  
> @@ -818,11 +808,8 @@ 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(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;
>  }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thiago Jung Bauermann]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Jul 22, 2020 at 11:56:50PM -0300, Thiago Jung Bauermann wrote:
>> 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>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Thanks! Sory about the extra work.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

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

[Greg Kurz]

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

On Thu, 23 Jul 2020 13:06:09 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 22, 2020 at 11:56:50PM -0300, Thiago Jung Bauermann wrote:
> > 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>
> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 

Reviewed-by: Greg Kurz <groug@kaod.org>

> > ---
> >  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(-)
> > 
> > NB: I was only able to test that this patch builds. I wasn't able to
> > run it.
> > 
> > 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)) {
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Greg Kurz]

On Wed, 22 Jul 2020 23:56:51 -0300
Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:

> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/core/cpu.c    | 2 +-
>  target/arm/cpu.c | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> NB: I wasn't able to run this patch on an ARM machine. I did run it on
> a ppc64le pseries KVM guest.
> 
> 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';

Re: [RFC PATCH v3 8/8] target/s390x: Use start-powered-off CPUState property

[Cornelia Huck]

On Wed, 22 Jul 2020 23:56:57 -0300
Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:

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

I think that should be fine, as we change the cpu state to STOPPED in
the reset function, which sets halted to 1.

> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  target/s390x/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.

No noticeable difference under kvm, but running under tcg seems a bit
more sluggish than usual, and I saw some pausing on reboot (after the
bios handover to the kernel). Not sure if it were just flukes on my
laptop, would appreciate if someone else could give it a go.

> 
> 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",
> 

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

[Greg Kurz]

On Wed, 22 Jul 2020 23:56:52 -0300
Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:

> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---

Thanks for doing this ! I remember seeing partly initialized CPUs being
kicked in the past, which is clearly wrong but I never got time to find
a proper fix (especially since it didn't seem to break anything).

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> NB: Tested on ppc64le pseries KVM guest with two threads per core. 
> Hot-plugging additional cores doesn't cause the bug described above
> anymore.
> 
> 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) {

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

[Philippe Mathieu-Daudé]

On 7/23/20 4:56 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).
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  hw/sparc/sun4m.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> NB: I was only able to test that this patch builds. I wasn't able to
> run it.
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index f1d92df781..fd74e516bb 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,7 +801,6 @@ 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;
>  
> @@ -818,11 +808,8 @@ 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(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;
>  }
> 

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

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

[Philippe Mathieu-Daudé]

On 7/23/20 4:56 AM, Thiago Jung Bauermann wrote:
> 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>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  hw/ppc/spapr_cpu_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> NB: Tested on ppc64le pseries KVM guest with two threads per core. 
> Hot-plugging additional cores doesn't cause the bug described above
> anymore.
> 
> 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) {
> 

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

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

[Thiago Jung Bauermann]

Greg Kurz <groug@kaod.org> writes:

> On Thu, 23 Jul 2020 13:06:09 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
>> On Wed, Jul 22, 2020 at 11:56:50PM -0300, Thiago Jung Bauermann wrote:
>> > 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>
>> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> 
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> 
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

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

[Thiago Jung Bauermann]

Greg Kurz <groug@kaod.org> writes:

> On Wed, 22 Jul 2020 23:56:52 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
>> 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>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>
> Thanks for doing this ! I remember seeing partly initialized CPUs being
> kicked in the past, which is clearly wrong but I never got time to find
> a proper fix (especially since it didn't seem to break anything).
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks for confirming the issue, and for reviewing the code.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC PATCH v3 8/8] target/s390x: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Hi,

Cornelia Huck <cohuck@redhat.com> writes:

> On Wed, 22 Jul 2020 23:56:57 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
>> 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.
>
> I think that should be fine, as we change the cpu state to STOPPED in
> the reset function, which sets halted to 1.

Nice, thanks for checking.

>> 
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>>  target/s390x/cpu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> NB: I was only able to test that this patch builds. I wasn't able to
>> run it.
>
> No noticeable difference under kvm, but running under tcg seems a bit
> more sluggish than usual, and I saw some pausing on reboot (after the
> bios handover to the kernel). Not sure if it were just flukes on my
> laptop, would appreciate if someone else could give it a go.

I tried today setting up a TCG guest, but didn't have success yet.
Will try some more tomorrow.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

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

> 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).

Since this series fixes a bug is it eligible for 5.1, at least the
patches that were already approved by the appropriate maintainers?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[David Gibson]

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

On Tue, Jul 28, 2020 at 09:56:36PM -0300, Thiago Jung Bauermann wrote:
> 
> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> 
> > 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).
> 
> Since this series fixes a bug is it eligible for 5.1, at least the
> patches that were already approved by the appropriate maintainers?

Ok by me.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v3 8/8] target/s390x: Use start-powered-off CPUState property

[Cornelia Huck]

On Tue, 28 Jul 2020 21:51:33 -0300
Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:

> Hi,
> 
> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Wed, 22 Jul 2020 23:56:57 -0300
> > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
> >  
> >> 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.  
> >
> > I think that should be fine, as we change the cpu state to STOPPED in
> > the reset function, which sets halted to 1.  
> 
> Nice, thanks for checking.
> 
> >> 
> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> ---
> >>  target/s390x/cpu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> NB: I was only able to test that this patch builds. I wasn't able to
> >> run it.  
> >
> > No noticeable difference under kvm, but running under tcg seems a bit
> > more sluggish than usual, and I saw some pausing on reboot (after the
> > bios handover to the kernel). Not sure if it were just flukes on my
> > laptop, would appreciate if someone else could give it a go.  

Experimented a bit with it again. There's a pause when switching from
the bios to the kernel (after the load reset normal has been done, I
guess), which is always there, but seems to get more noticeable with
this patch (varying wildly, but seems longer on average.) Hard to pin
down, and I don't really see a reason why that should happen, as we
should end up with halted == 1 in any case. Might still be a fluke,
even though I see it both on my laptop and on an LPAR (when running
under tcg; not seen under kvm, which is much faster anyway.)

> 
> I tried today setting up a TCG guest, but didn't have success yet.
> Will try some more tomorrow.
> 

I'm also looking a bit at the other s390 folks :)

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Philippe Mathieu-Daudé]

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

Le jeu. 30 juil. 2020 03:00, David Gibson <david@gibson.dropbear.id.au> a
écrit :

> On Tue, Jul 28, 2020 at 09:56:36PM -0300, Thiago Jung Bauermann wrote:
> >
> > Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> >
> > > 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).
> >
> > Since this series fixes a bug is it eligible for 5.1, at least the
> > patches that were already approved by the appropriate maintainers?
>
> Ok by me.
>

Maybe just the arm generalization and ppc fix for 5.1, delaying all not
bugfix to 5.2?


> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_
> _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

[-- Attachment #2: Type: text/html, Size: 2248 bytes --]

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Le jeu. 30 juil. 2020 03:00, David Gibson <david@gibson.dropbear.id.au> a
> écrit :
>
>> On Tue, Jul 28, 2020 at 09:56:36PM -0300, Thiago Jung Bauermann wrote:
>> >
>> > Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>> >
>> > > 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).
>> >
>> > Since this series fixes a bug is it eligible for 5.1, at least the
>> > patches that were already approved by the appropriate maintainers?
>>
>> Ok by me.
>>
>
> Maybe just the arm generalization and ppc fix for 5.1, delaying all not
> bugfix to 5.2?

That would be great.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Peter Maydell]

On Thu, 23 Jul 2020 at 03:57, Thiago Jung Bauermann
<bauerman@linux.ibm.com> wrote:
>
> 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.

Acked-by: Peter Maydell <peter.maydell@linaro.org>
for the Arm bits if you want to take the bug-fixing parts of
this series in via some other tree. (I think they've all been
reviewed.)

thanks
-- PMM

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

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

> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> Le jeu. 30 juil. 2020 03:00, David Gibson <david@gibson.dropbear.id.au> a
>> écrit :
>>
>>> On Tue, Jul 28, 2020 at 09:56:36PM -0300, Thiago Jung Bauermann wrote:
>>> >
>>> > Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>> >
>>> > > 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).
>>> >
>>> > Since this series fixes a bug is it eligible for 5.1, at least the
>>> > patches that were already approved by the appropriate maintainers?
>>>
>>> Ok by me.
>>>
>>
>> Maybe just the arm generalization and ppc fix for 5.1, delaying all not
>> bugfix to 5.2?
>
> That would be great.

Any news on this? Is there something I should be doing? I saw -rc3 today
but not these patches.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Peter Maydell]

On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
<bauerman@linux.ibm.com> wrote:
> Any news on this? Is there something I should be doing? I saw -rc3 today
> but not these patches.

Sorry, you've missed the bus for 5.1 at this point. I'd assumed
that the relevant bits of the patchset would go into a PPC pullreq
if it was important for 5.1.

As I understand it, this isn't a regression from 5.0, right?

thanks
-- PMM

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
> <bauerman@linux.ibm.com> wrote:
>> Any news on this? Is there something I should be doing? I saw -rc3 today
>> but not these patches.
>
> Sorry, you've missed the bus for 5.1 at this point. I'd assumed
> that the relevant bits of the patchset would go into a PPC pullreq
> if it was important for 5.1.
>
> As I understand it, this isn't a regression from 5.0, right?

Right, it isn't.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[David Gibson]

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

On Wed, Aug 05, 2020 at 08:04:11PM +0100, Peter Maydell wrote:
> On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
> <bauerman@linux.ibm.com> wrote:
> > Any news on this? Is there something I should be doing? I saw -rc3 today
> > but not these patches.
> 
> Sorry, you've missed the bus for 5.1 at this point. I'd assumed
> that the relevant bits of the patchset would go into a PPC pullreq
> if it was important for 5.1.

Ah, bother.  Meanwhile I assumed that since it was cross-target it was
going in separately, so I didn't take it through my tree.

Oh well, I guess we apply ASAP in 5.2 and we'll do a backport
downstream.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Peter Maydell]

On Thu, 6 Aug 2020 at 06:53, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Wed, Aug 05, 2020 at 08:04:11PM +0100, Peter Maydell wrote:
> > On Wed, 5 Aug 2020 at 18:01, Thiago Jung Bauermann
> > <bauerman@linux.ibm.com> wrote:
> > > Any news on this? Is there something I should be doing? I saw -rc3 today
> > > but not these patches.
> >
> > Sorry, you've missed the bus for 5.1 at this point. I'd assumed
> > that the relevant bits of the patchset would go into a PPC pullreq
> > if it was important for 5.1.
>
> Ah, bother.  Meanwhile I assumed that since it was cross-target it was
> going in separately, so I didn't take it through my tree.

This kind of confusion is why it's a good idea to list "this ought
to be fixed for the release" issues on the Planning wiki page. That
way I will see them when I'm looking for patches that might have
been forgotten or that I need to wait for a pullreq for before tagging...

-- PMM

Re: [RFC PATCH v3 8/8] target/s390x: Use start-powered-off CPUState property

[Cornelia Huck]

On Thu, 30 Jul 2020 11:45:41 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 28 Jul 2020 21:51:33 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
> 
> > Hi,
> > 
> > Cornelia Huck <cohuck@redhat.com> writes:
> >   
> > > On Wed, 22 Jul 2020 23:56:57 -0300
> > > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
> > >    
> > >> 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.    
> > >
> > > I think that should be fine, as we change the cpu state to STOPPED in
> > > the reset function, which sets halted to 1.    
> > 
> > Nice, thanks for checking.
> >   
> > >> 
> > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > >> ---
> > >>  target/s390x/cpu.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> 
> > >> NB: I was only able to test that this patch builds. I wasn't able to
> > >> run it.    
> > >
> > > No noticeable difference under kvm, but running under tcg seems a bit
> > > more sluggish than usual, and I saw some pausing on reboot (after the
> > > bios handover to the kernel). Not sure if it were just flukes on my
> > > laptop, would appreciate if someone else could give it a go.    
> 
> Experimented a bit with it again. There's a pause when switching from
> the bios to the kernel (after the load reset normal has been done, I
> guess), which is always there, but seems to get more noticeable with
> this patch (varying wildly, but seems longer on average.) Hard to pin
> down, and I don't really see a reason why that should happen, as we
> should end up with halted == 1 in any case. Might still be a fluke,
> even though I see it both on my laptop and on an LPAR (when running
> under tcg; not seen under kvm, which is much faster anyway.)

Tried again, the pause now seems comparable to the pause prior to this
series. Might depend on the phase of the moon.

I ran kvm unit tests on it, and it looks good. So, I'm reasonable
confident that this is fine, really just seems to be a fluke.

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [RFC PATCH v3 8/8] target/s390x: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Cornelia Huck <cohuck@redhat.com> writes:

> On Thu, 30 Jul 2020 11:45:41 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Tue, 28 Jul 2020 21:51:33 -0300
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>> 
>> > Hi,
>> > 
>> > Cornelia Huck <cohuck@redhat.com> writes:
>> >   
>> > > On Wed, 22 Jul 2020 23:56:57 -0300
>> > > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>> > >    
>> > >> 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.    
>> > >
>> > > I think that should be fine, as we change the cpu state to STOPPED in
>> > > the reset function, which sets halted to 1.    
>> > 
>> > Nice, thanks for checking.
>> >   
>> > >> 
>> > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> > >> ---
>> > >>  target/s390x/cpu.c | 2 +-
>> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >> 
>> > >> NB: I was only able to test that this patch builds. I wasn't able to
>> > >> run it.    
>> > >
>> > > No noticeable difference under kvm, but running under tcg seems a bit
>> > > more sluggish than usual, and I saw some pausing on reboot (after the
>> > > bios handover to the kernel). Not sure if it were just flukes on my
>> > > laptop, would appreciate if someone else could give it a go.    
>> 
>> Experimented a bit with it again. There's a pause when switching from
>> the bios to the kernel (after the load reset normal has been done, I
>> guess), which is always there, but seems to get more noticeable with
>> this patch (varying wildly, but seems longer on average.) Hard to pin
>> down, and I don't really see a reason why that should happen, as we
>> should end up with halted == 1 in any case. Might still be a fluke,
>> even though I see it both on my laptop and on an LPAR (when running
>> under tcg; not seen under kvm, which is much faster anyway.)
>
> Tried again, the pause now seems comparable to the pause prior to this
> series. Might depend on the phase of the moon.
>
> I ran kvm unit tests on it, and it looks good. So, I'm reasonable
> confident that this is fine, really just seems to be a fluke.
>
> Acked-by: Cornelia Huck <cohuck@redhat.com>

Ah, that's a relief. Thank you very much for looking into this!

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[David Gibson]

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

On Wed, Jul 22, 2020 at 11:56:49PM -0300, Thiago Jung Bauermann wrote:
> 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.

This series appears to break the Travis build for a MIPS target:

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

> 
> 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           |  6 +++---
>  hw/ppc/e500.c           | 10 +++++++---
>  hw/ppc/spapr_cpu_core.c | 10 +++++-----
>  hw/sparc/sun4m.c        | 28 ++--------------------------
>  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, 27 insertions(+), 47 deletions(-)
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Philippe Mathieu-Daudé]

On 8/17/20 6:47 AM, David Gibson wrote:
> On Wed, Jul 22, 2020 at 11:56:49PM -0300, Thiago Jung Bauermann wrote:
>> 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.
> 
> This series appears to break the Travis build for a MIPS target:
> 
> 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

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().

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[David Gibson]

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

On Mon, Aug 17, 2020 at 07:24:43AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/17/20 6:47 AM, David Gibson wrote:
> > On Wed, Jul 22, 2020 at 11:56:49PM -0300, Thiago Jung Bauermann wrote:
> >> 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.
> > 
> > This series appears to break the Travis build for a MIPS target:
> > 
> > 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
> 
> 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().

Thiago, can you fix that up and repost please.


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Thiago Jung Bauermann]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Mon, Aug 17, 2020 at 07:24:43AM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/17/20 6:47 AM, David Gibson wrote:
>> > On Wed, Jul 22, 2020 at 11:56:49PM -0300, Thiago Jung Bauermann wrote:
>> >> 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.
>> >
>> > This series appears to break the Travis build for a MIPS target:
>> >
>> > 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
>>
>> 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().

Does it have to be with object_initialize_child()? I found very few
examples of CPUs initialized with that function (e.g., atmega.c,
rx62n.c, nrf51_soc.c).

A more direct substitute for cpu_create() would be object_new(). I
provide an example at the end of this email. What do you think?

I'm trying to test it with `make docker-test-misc@debian-mips64el-cross`,
but still having some trouble with that. Is there another way to
reproduce this issue?

> Thiago, can you fix that up and repost please.

Ok, I'm working on it.

--
Thiago Jung Bauermann
IBM Linux Technology Center


diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index d5b6c78019..be85357dc0 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -73,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);
@@ -90,6 +92,12 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
         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);