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

[Thiago Jung Bauermann]

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);