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
signature.asc
Description: PGP signature
