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

[David Gibson]

On Wed, Aug 26, 2020 at 02:55:27AM -0300, Thiago Jung Bauermann wrote:
> This version fixes `make check` failures in ppc/e500.c, mips/cps.c and
> sparc/sun4m.c. This was done by moving the qdev_realize_and_unref() call as
> close as possible to the object_new() call, in order to keep the CPU object
> construction as similar as possible to the earlier version which used
> cpu_create().
> 
> I also had to change the patch which removed the main_cpu_reset() function
> from sparc/sun4m.c. It was causing a `make check` failure but I can't
> really explain why. See this message for a few more details:
> 
> https://lists.nongnu.org/archive/html/qemu-ppc/2020-08/msg00419.html
> 
> I dropped the Reviewed-by's on the changed patches because of these
> changes.
> 
> 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, thanks.

> Changes since v6:
> 
> Patch "ppc/e500: Use start-powered-off CPUState property"
> Patch "mips/cps: Use start-powered-off CPUState property"
> - Moved setting of start-powered-off property and qdev_realize_and_unref()
>   to right after object_new(machine->cpu_type).
> - Dropped Philippe's Reviewed-by due to this change.
> 
> Patch "sparc/sun4m: Remove main_cpu_reset()"
> - Dropped.
> 
> Patch "sparc/sun4m: Don't set cs->halted = 0 in main_cpu_reset()"
> - New patch.
> 
> Patch "sparc/sun4m: Use start-powered-off CPUState property"
> - Merged secondary_cpu_reset() with main_cpu_reset().
> - Dropped Philippe's Reviewed-by due to this change.
> 
> 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: Don't set cs->halted = 0 in 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           | 15 +++++++++++----
>  hw/ppc/e500.c           | 13 +++++++++----
>  hw/ppc/spapr_cpu_core.c | 10 +++++-----
>  hw/sparc/sun4m.c        | 26 ++++++--------------------
>  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, 41 insertions(+), 43 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