Re: [PATCH v7 0/8] Generalize start-powered-off property from ARM
David Gibson writes: > 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. Thank you! -- Thiago Jung Bauermann IBM Linux Technology Center
Re: [PATCH v7 0/8] Generalize start-powered-off property from ARM
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 _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 _fatal instead of > _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 _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
[PATCH v7 0/8] Generalize start-powered-off property from ARM
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. 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 _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 _fatal instead of _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 _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
