Message ID | 20200819164306.625357-1-bauerman@linux.ibm.com (mailing list archive) |
---|---|
Headers | show |
Series | Generalize start-powered-off property from ARM | expand |
On Wed, Aug 19, 2020 at 01:42:58PM -0300, Thiago Jung Bauermann wrote: > This version has one small fix in patch 7, and adds Philippe's Reviewed-bys. > > Applies cleanly on dgibson/ppc-for-5.2. > > Original cover letter below, followed by changelog: > > > The ARM code has a start-powered-off property in ARMCPU, which is a > subclass of CPUState. This property causes arm_cpu_reset() to set > CPUState::halted to 1, signalling that the CPU should start in a halted > state. Other architectures also have code which aim to achieve the same > effect, but without using a property. > > The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu > before cs->halted is set to 1, causing the vcpu to run while it's still in > an unitialized state (more details in patch 3). > > Peter Maydell mentioned the ARM start-powered-off property and > Eduardo Habkost suggested making it generic, so this patch series does > that, for all cases which I was able to find via grep in the code. > > The only problem is that I was only able to test these changes on a ppc64le > pseries KVM guest, so except for patches 2 and 3, all others are only > build-tested. Also, my grasp of QOM lifecycle is basically non-existant so > please be aware of that when reviewing this series. > > The last patch may be wrong, as pointed out by Eduardo, so I marked it as > RFC. It may make sense to drop it. Applied to ppc-for-5.2. > > Changes since v5: > > Patch "ppc/e500: Use start-powered-off CPUState property" > Patch "mips/cps: Use start-powered-off CPUState property" > Patch "sparc/sun4m: Remove main_cpu_reset()" > Patch "target/s390x: Use start-powered-off CPUState property" > - Added Philippe's Reviewed-by. > > Patch "sparc/sun4m: Use start-powered-off CPUState property" > - Move call to qdev_realize_and_unref() right after object_property_set_bool(), > as suggested by Philippe. > > Changes since v4: > > Patch "ppc/e500: Use start-powered-off CPUState property" > Patch "sparc/sun4m: Use start-powered-off CPUState property" > - Use qdev_realize_and_unref() instead of qdev_realize(), as suggested > by Igor. > - Pass &error_fatal to qdev_realize_and_unref() instead of manually > reporting the error and exiting QEMU, as suggested by Philippe. > - Changed object_property_set_bool() to use &error_fatal instead of > &error_abort. > > Patch "mips/cps: Use start-powered-off CPUState property" > - Use qdev_realize_and_unref() instead of qdev_realize(), as suggested > by Igor. > - Use existing errp argument to propagate error back to the caller, as > suggested by Philippe. > - Changed object_property_set_bool() to use existing errp argument to > propagate error back to the caller instead of using &error_abort. > > Changes since v3: > > General: > - Added David's, Greg's and Cornelia's Reviewed-by and Acked-by to some > of the patches. > - Rebased on top of dgibson/ppc-for-5.2. > > Patch "ppc/e500: Use start-powered-off CPUState property" > Patch "mips/cps: Use start-powered-off CPUState property" > Patch "sparc/sun4m: Use start-powered-off CPUState property" > - Initialize CPU object with object_new() and qdev_realize() instead > of cpu_create(). > - Removed Reviewed-by's and Acked-by's from these patches because of > these changes. > > Changes since v2: > > General: > - Added Philippe's Reviewed-by to some of the patches. > > Patch "ppc/spapr: Use start-powered-off CPUState property" > - Set the CPUState::start_powered_off variable directly rather than using > object_property_set_bool(). Suggested by Philippe. > > Patch "sparc/sun4m: Remove main_cpu_reset()" > - New patch. Suggested by Philippe. > > Patch "sparc/sun4m: Use start-powered-off CPUState property" > - Remove secondary_cpu_reset(). Suggested by Philippe. > - Remove setting of `cs->halted = 1` from cpu_devinit(). Suggested by Philippe. > > Patch "Don't set CPUState::halted in cpu_devinit()" > - Squashed into previous patch. Suggested by Philippe. > > Patch "sparc/sun4m: Use one cpu_reset() function for main and secondary CPUs" > - Dropped. > > Patch "target/s390x: Use start-powered-off CPUState property" > - Set the CPUState::start_powered_off variable directly rather than using > object_property_set_bool(). Suggested by Philippe. > - Mention in the commit message Eduardo's observation that before this > patch, the code didn't set cs->halted on reset. > > Thiago Jung Bauermann (8): > target/arm: Move start-powered-off property to generic CPUState > target/arm: Move setting of CPU halted state to generic code > ppc/spapr: Use start-powered-off CPUState property > ppc/e500: Use start-powered-off CPUState property > mips/cps: Use start-powered-off CPUState property > sparc/sun4m: Remove main_cpu_reset() > sparc/sun4m: Use start-powered-off CPUState property > target/s390x: Use start-powered-off CPUState property > > exec.c | 1 + > hw/core/cpu.c | 2 +- > hw/mips/cps.c | 14 ++++++++++---- > hw/ppc/e500.c | 14 ++++++++++---- > hw/ppc/spapr_cpu_core.c | 10 +++++----- > hw/sparc/sun4m.c | 31 ++++--------------------------- > 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, 39 insertions(+), 50 deletions(-) >
David Gibson <david@gibson.dropbear.id.au> writes: > On Wed, Aug 19, 2020 at 01:42:58PM -0300, Thiago Jung Bauermann wrote: >> This version has one small fix in patch 7, and adds Philippe's Reviewed-bys. >> >> Applies cleanly on dgibson/ppc-for-5.2. >> >> Original cover letter below, followed by changelog: >> >> >> The ARM code has a start-powered-off property in ARMCPU, which is a >> subclass of CPUState. This property causes arm_cpu_reset() to set >> CPUState::halted to 1, signalling that the CPU should start in a halted >> state. Other architectures also have code which aim to achieve the same >> effect, but without using a property. >> >> The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu >> before cs->halted is set to 1, causing the vcpu to run while it's still in >> an unitialized state (more details in patch 3). >> >> Peter Maydell mentioned the ARM start-powered-off property and >> Eduardo Habkost suggested making it generic, so this patch series does >> that, for all cases which I was able to find via grep in the code. >> >> The only problem is that I was only able to test these changes on a ppc64le >> pseries KVM guest, so except for patches 2 and 3, all others are only >> build-tested. Also, my grasp of QOM lifecycle is basically non-existant so >> please be aware of that when reviewing this series. >> >> The last patch may be wrong, as pointed out by Eduardo, so I marked it as >> RFC. It may make sense to drop it. > > Applied to ppc-for-5.2. Great news. Thanks!