| Message ID | 20200723025657.644724-1-bauerman@linux.ibm.com |
|---|---|
| Headers | show |
| Series | Generalize start-powered-off property from ARM | expand |
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?
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.
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 >
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.
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
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.
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
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.
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.
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
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(-) > >
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().
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 <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);