init_event_facility() creates the SCLP events bus with two SCLP event
devices (sclpquiesce and sclp-cpu-hotplug).  It leaves the devices
unrealized.  A comment explains they will be realized "via the bus".

The bus's realize method sclp_events_bus_realize() indeed realizes all
unrealized devices on this bus.  It carries a TODO comment claiming
this "has to be done in common code".  No other bus realize method
realizes its devices.

The common code in question is bus_set_realized(), which has a TODO
comment asking for recursive realization.  It's been asking for years.

The only devices sclp_events_bus_realize() will ever realize are the
two init_event_facility() puts there.

Simplify as follows:

* Make the devices members of the event facility instance struct, just
  like the bus.  object_initialize_child() is simpler than
  object_property_add_child() and object_unref().

* Realize them in the event facility realize method.

This is in line with how such things are done elsewhere.

Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: David Hildenbrand <david@redhat.com>
Cc: qemu-s390x@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/s390x/event-facility.c | 59 ++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 35 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 97a4f0b1f5..1ecaa20556 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -39,6 +39,7 @@ typedef struct SCLPEventsBus {
 struct SCLPEventFacility {
     SysBusDevice parent_obj;
     SCLPEventsBus sbus;
+    SCLPEvent quiesce, cpu_hotplug;
     /* guest's receive mask */
     union {
         uint32_t receive_mask_pieces[2];
@@ -328,34 +329,9 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
 
 #define TYPE_SCLP_EVENTS_BUS "s390-sclp-events-bus"
 
-static void sclp_events_bus_realize(BusState *bus, Error **errp)
-{
-    Error *err = NULL;
-    BusChild *kid;
-
-    /* TODO: recursive realization has to be done in common code */
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        DeviceState *dev = kid->child;
-
-        object_property_set_bool(OBJECT(dev), true, "realized", &err);
-        if (err) {
-            error_propagate(errp, err);
-            return;
-        }
-    }
-}
-
-static void sclp_events_bus_class_init(ObjectClass *klass, void *data)
-{
-    BusClass *bc = BUS_CLASS(klass);
-
-    bc->realize = sclp_events_bus_realize;
-}
-
 static const TypeInfo sclp_events_bus_info = {
     .name = TYPE_SCLP_EVENTS_BUS,
     .parent = TYPE_BUS,
-    .class_init = sclp_events_bus_class_init,
 };
 
 static void command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
@@ -443,27 +419,39 @@ static void init_event_facility(Object *obj)
 {
     SCLPEventFacility *event_facility = EVENT_FACILITY(obj);
     DeviceState *sdev = DEVICE(obj);
-    Object *new;
 
     event_facility->mask_length = 4;
     event_facility->allow_all_mask_sizes = true;
     object_property_add_bool(obj, "allow_all_mask_sizes",
                              sclp_event_get_allow_all_mask_sizes,
                              sclp_event_set_allow_all_mask_sizes);
+
     /* Spawn a new bus for SCLP events */
     qbus_create_inplace(&event_facility->sbus, sizeof(event_facility->sbus),
                         TYPE_SCLP_EVENTS_BUS, sdev, NULL);
 
-    new = object_new(TYPE_SCLP_QUIESCE);
-    object_property_add_child(obj, TYPE_SCLP_QUIESCE, new);
-    object_unref(new);
-    qdev_set_parent_bus(DEVICE(new), BUS(&event_facility->sbus));
+    object_initialize_child(obj, TYPE_SCLP_QUIESCE,
+                            &event_facility->quiesce,
+                            TYPE_SCLP_QUIESCE);
 
-    new = object_new(TYPE_SCLP_CPU_HOTPLUG);
-    object_property_add_child(obj, TYPE_SCLP_CPU_HOTPLUG, new);
-    object_unref(new);
-    qdev_set_parent_bus(DEVICE(new), BUS(&event_facility->sbus));
-    /* the facility will automatically realize the devices via the bus */
+    object_initialize_child(obj, TYPE_SCLP_CPU_HOTPLUG,
+                            &event_facility->cpu_hotplug,
+                            TYPE_SCLP_CPU_HOTPLUG);
+}
+
+static void realize_event_facility(DeviceState *dev, Error **errp)
+{
+    SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
+    Error *local_err = NULL;
+
+    qdev_realize(DEVICE(&event_facility->quiesce),
+                 BUS(&event_facility->sbus), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
+                 BUS(&event_facility->sbus), errp);
 }
 
 static void reset_event_facility(DeviceState *dev)
@@ -479,6 +467,7 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(sbdc);
     SCLPEventFacilityClass *k = EVENT_FACILITY_CLASS(dc);
 
+    dc->realize = realize_event_facility;
     dc->reset = reset_event_facility;
     dc->vmsd = &vmstate_event_facility;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-- 
2.21.1

[...]
> The common code in question is bus_set_realized(), which has a TODO
> comment asking for recursive realization.  It's been asking for years.
> 
> The only devices sclp_events_bus_realize() will ever realize are the
> two init_event_facility() puts there.
> 
> Simplify as follows:
> 
> * Make the devices members of the event facility instance struct, just
>   like the bus.  object_initialize_child() is simpler than
>   object_property_add_child() and object_unref().
> 
> * Realize them in the event facility realize method.
> 
> This is in line with how such things are done elsewhere.

Makes sense, now that we've been waiting for recursive bus realization
for years.

> 
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390x/event-facility.c | 59 ++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 97a4f0b1f5..1ecaa20556 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -39,6 +39,7 @@ typedef struct SCLPEventsBus {
>  struct SCLPEventFacility {
>      SysBusDevice parent_obj;
>      SCLPEventsBus sbus;
> +    SCLPEvent quiesce, cpu_hotplug;
>      /* guest's receive mask */
>      union {
>          uint32_t receive_mask_pieces[2];
> @@ -328,34 +329,9 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
>  
>  #define TYPE_SCLP_EVENTS_BUS "s390-sclp-events-bus"
>  
> -static void sclp_events_bus_realize(BusState *bus, Error **errp)
> -{
> -    Error *err = NULL;
> -    BusChild *kid;
> -
> -    /* TODO: recursive realization has to be done in common code */
> -    QTAILQ_FOREACH(kid, &bus->children, sibling) {
> -        DeviceState *dev = kid->child;
> -
> -        object_property_set_bool(OBJECT(dev), true, "realized", &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -            return;
> -        }
> -    }
> -}
> -
> -static void sclp_events_bus_class_init(ObjectClass *klass, void *data)
> -{
> -    BusClass *bc = BUS_CLASS(klass);
> -
> -    bc->realize = sclp_events_bus_realize;
> -}
> -
>  static const TypeInfo sclp_events_bus_info = {
>      .name = TYPE_SCLP_EVENTS_BUS,
>      .parent = TYPE_BUS,
> -    .class_init = sclp_events_bus_class_init,
>  };
>  
>  static void command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
> @@ -443,27 +419,39 @@ static void init_event_facility(Object *obj)
>  {
>      SCLPEventFacility *event_facility = EVENT_FACILITY(obj);
>      DeviceState *sdev = DEVICE(obj);
> -    Object *new;
>  
>      event_facility->mask_length = 4;
>      event_facility->allow_all_mask_sizes = true;
>      object_property_add_bool(obj, "allow_all_mask_sizes",
>                               sclp_event_get_allow_all_mask_sizes,
>                               sclp_event_set_allow_all_mask_sizes);
> +
>      /* Spawn a new bus for SCLP events */
>      qbus_create_inplace(&event_facility->sbus, sizeof(event_facility->sbus),
>                          TYPE_SCLP_EVENTS_BUS, sdev, NULL);
>  
> -    new = object_new(TYPE_SCLP_QUIESCE);
> -    object_property_add_child(obj, TYPE_SCLP_QUIESCE, new);
> -    object_unref(new);
> -    qdev_set_parent_bus(DEVICE(new), BUS(&event_facility->sbus));
> +    object_initialize_child(obj, TYPE_SCLP_QUIESCE,
> +                            &event_facility->quiesce,
> +                            TYPE_SCLP_QUIESCE);
>  
> -    new = object_new(TYPE_SCLP_CPU_HOTPLUG);
> -    object_property_add_child(obj, TYPE_SCLP_CPU_HOTPLUG, new);
> -    object_unref(new);
> -    qdev_set_parent_bus(DEVICE(new), BUS(&event_facility->sbus));
> -    /* the facility will automatically realize the devices via the bus */
> +    object_initialize_child(obj, TYPE_SCLP_CPU_HOTPLUG,
> +                            &event_facility->cpu_hotplug,
> +                            TYPE_SCLP_CPU_HOTPLUG);
> +}
> +
> +static void realize_event_facility(DeviceState *dev, Error **errp)
> +{
> +    SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
> +    Error *local_err = NULL;
> +
> +    qdev_realize(DEVICE(&event_facility->quiesce),
> +                 BUS(&event_facility->sbus), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
> +                 BUS(&event_facility->sbus), errp);

Just wondering, do we have to care about un-realizing quiesce in case
this fails?

>  }
>  
>  static void reset_event_facility(DeviceState *dev)
> @@ -479,6 +467,7 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(sbdc);
>      SCLPEventFacilityClass *k = EVENT_FACILITY_CLASS(dc);
>  
> +    dc->realize = realize_event_facility;
>      dc->reset = reset_event_facility;
>      dc->vmsd = &vmstate_event_facility;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> 

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

>> +static void realize_event_facility(DeviceState *dev, Error **errp)
>> +{
>> +    SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
>> +    Error *local_err = NULL;
>> +
>> +    qdev_realize(DEVICE(&event_facility->quiesce),
>> +                 BUS(&event_facility->sbus), &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
>> +                 BUS(&event_facility->sbus), errp);
> 
> Just wondering, do we have to care about un-realizing quiesce in case
> this fails?

Just remembered that we fail creating the machine and therefore abort. So not necessary :)

> 
>> }
>> 
>> static void reset_event_facility(DeviceState *dev)
>> @@ -479,6 +467,7 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
>>     DeviceClass *dc = DEVICE_CLASS(sbdc);
>>     SCLPEventFacilityClass *k = EVENT_FACILITY_CLASS(dc);
>> 
>> +    dc->realize = realize_event_facility;
>>     dc->reset = reset_event_facility;
>>     dc->vmsd = &vmstate_event_facility;
>>     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> 
> 
> LGTM
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> -- 
> Thanks,
> 
> David / dhildenb

David Hildenbrand <david@redhat.com> writes:

>>> +static void realize_event_facility(DeviceState *dev, Error **errp)
>>> +{
>>> +    SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
>>> +    Error *local_err = NULL;
>>> +
>>> +    qdev_realize(DEVICE(&event_facility->quiesce),
>>> +                 BUS(&event_facility->sbus), &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
>>> +                 BUS(&event_facility->sbus), errp);
>> 
>> Just wondering, do we have to care about un-realizing quiesce in case
>> this fails?
>
> Just remembered that we fail creating the machine and therefore abort. So not necessary :)

True.

But let's review briefly what happens when a realize method fails.

In theory, realize fails cleanly, i.e. doing nothing.  Another attempt
could be made then.

In practice, realize failure is always followed by destruction, unless
preempted by outright exit(1).

Destroying a device must also destroy its components.

Paolo, is destroying a realized device okay, or does it have to be
unrealized first?  I can't see automatic unrealize on destruction...

>>> }
>>> 
>>> static void reset_event_facility(DeviceState *dev)
>>> @@ -479,6 +467,7 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
>>>     DeviceClass *dc = DEVICE_CLASS(sbdc);
>>>     SCLPEventFacilityClass *k = EVENT_FACILITY_CLASS(dc);
>>> 
>>> +    dc->realize = realize_event_facility;
>>>     dc->reset = reset_event_facility;
>>>     dc->vmsd = &vmstate_event_facility;
>>>     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>> 
>> 
>> LGTM
>> 
>> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

On 25/05/20 09:01, Markus Armbruster wrote:
>> Just remembered that we fail creating the machine and therefore abort. So not necessary :)
> True.
> 
> But let's review briefly what happens when a realize method fails.
> 
> In theory, realize fails cleanly, i.e. doing nothing.  Another attempt
> could be made then.
> 
> In practice, realize failure is always followed by destruction, unless
> preempted by outright exit(1).
> 
> Destroying a device must also destroy its components.
> 
> Paolo, is destroying a realized device okay, or does it have to be
> unrealized first?  I can't see automatic unrealize on destruction...

It cannot happen, because a device must be unparented before it's
destroyed and unparenting takes care of unrealizing the device.  So the
stageobject lifetime should always proceed in this order:

   created
   created, with parent
   created, with parent, with bus (if applicable)
   realizing
   realized
   unrealizing
   unrealized
   unrealized, without parent, with bus (if applicable)
   unrealized, without parent, without bus
   finalizing (without references)
   finalized
   freed

Where the second and third would be fixed by moving /machine/unattached
from device_set_realized to qdev_realize.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/05/20 09:01, Markus Armbruster wrote:
>>> Just remembered that we fail creating the machine and therefore abort. So not necessary :)
>> True.
>> 
>> But let's review briefly what happens when a realize method fails.
>> 
>> In theory, realize fails cleanly, i.e. doing nothing.  Another attempt
>> could be made then.
>> 
>> In practice, realize failure is always followed by destruction, unless
>> preempted by outright exit(1).
>> 
>> Destroying a device must also destroy its components.
>> 
>> Paolo, is destroying a realized device okay, or does it have to be
>> unrealized first?  I can't see automatic unrealize on destruction...
>
> It cannot happen, because a device must be unparented before it's
> destroyed and unparenting takes care of unrealizing the device.

I can't see where unparenting takes care of unrealizing.  Can you help
me?

>                                                                  So the
> stageobject lifetime should always proceed in this order:
>
>    created
>    created, with parent
>    created, with parent, with bus (if applicable)
>    realizing
>    realized
>    unrealizing
>    unrealized
>    unrealized, without parent, with bus (if applicable)
>    unrealized, without parent, without bus
>    finalizing (without references)
>    finalized
>    freed
>
> Where the second and third would be fixed by moving /machine/unattached
> from device_set_realized to qdev_realize.
>
> Paolo

On 26/05/20 08:27, Markus Armbruster wrote:
>> It cannot happen, because a device must be unparented before it's
>> destroyed and unparenting takes care of unrealizing the device.
> 
> I can't see where unparenting takes care of unrealizing.  Can you help
> me?

Hidden in plain sight:

static void device_unparent(Object *obj)
{
    DeviceState *dev = DEVICE(obj);
    BusState *bus;

    if (dev->realized) {
        object_property_set_bool(obj, false, "realized", NULL);
    }
    ...
}

and the call stack is object_unparent -> object_property_del_child ->
object_finalize_child_property (via prop->release) -> class->unparent.

Thanks,

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 26/05/20 08:27, Markus Armbruster wrote:
>>> It cannot happen, because a device must be unparented before it's
>>> destroyed and unparenting takes care of unrealizing the device.
>> 
>> I can't see where unparenting takes care of unrealizing.  Can you help
>> me?
>
> Hidden in plain sight:
>
> static void device_unparent(Object *obj)
> {
>     DeviceState *dev = DEVICE(obj);
>     BusState *bus;
>
>     if (dev->realized) {
>         object_property_set_bool(obj, false, "realized", NULL);
>     }
>     ...
> }
>
> and the call stack is object_unparent -> object_property_del_child ->
> object_finalize_child_property (via prop->release) -> class->unparent.

Aha.

My attempt to trigger automatic unrealize of a child device after the
parent device's realize failed was unsuccessful.  I tried with
pci-serial-2x, hacked up to make it fail as if the second child's
realize failed, and hacked up some more to make it rely on automatic
unrealize.  No dice:

    $ qemu-system-x86_64 -S -nodefaults -display none -monitor stdio
    QEMU 5.0.50 monitor - type 'help' for more information
    (qemu) device_add pci-serial-2x
    ### serial_realize
    Error: mock error
    (qemu) q

Even though it doesn't really matter here, as David pointed out, it's
something I'd like to understand.


diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 5f9ccfcc93..433b5caefc 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -56,7 +56,7 @@ static void multi_serial_pci_exit(PCIDevice *dev)
 
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
-        object_property_set_bool(OBJECT(s), false, "realized", &error_abort);
+//        object_property_set_bool(OBJECT(s), false, "realized", &error_abort);
         memory_region_del_subregion(&pci->iobar, &s->io);
         g_free(pci->name[i]);
     }
@@ -106,6 +106,9 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
 
     for (i = 0; i < nports; i++) {
         s = pci->state + i;
+        if (i)
+            error_setg(errp, "mock error");
+        else
         object_property_set_bool(OBJECT(s), true, "realized", &err);
         if (err != NULL) {
             error_propagate(errp, err);
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 7d74694587..55b0bbd8b0 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -938,6 +938,8 @@ static void serial_realize(DeviceState *dev, Error **errp)
 {
     SerialState *s = SERIAL(dev);
 
+    printf("### %s\n", __func__);
+
     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
 
     s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
@@ -954,6 +956,8 @@ static void serial_unrealize(DeviceState *dev)
 {
     SerialState *s = SERIAL(dev);
 
+    printf("### %s\n", __func__);
+
     qemu_chr_fe_deinit(&s->chr, false);
 
     timer_del(s->modem_status_poll);

Markus Armbruster <armbru@redhat.com> writes:

> David Hildenbrand <david@redhat.com> writes:
>
>>>> +static void realize_event_facility(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    qdev_realize(DEVICE(&event_facility->quiesce),
>>>> +                 BUS(&event_facility->sbus), &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
>>>> +                 BUS(&event_facility->sbus), errp);
>>> 
>>> Just wondering, do we have to care about un-realizing quiesce in case
>>> this fails?
>>
>> Just remembered that we fail creating the machine and therefore abort. So not necessary :)
>
> True.

I chose to clean up on error anyway in v2.  Thanks!

On Tue, 19 May 2020 16:55:46 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> init_event_facility() creates the SCLP events bus with two SCLP event
> devices (sclpquiesce and sclp-cpu-hotplug).  It leaves the devices
> unrealized.  A comment explains they will be realized "via the bus".
> 
> The bus's realize method sclp_events_bus_realize() indeed realizes all
> unrealized devices on this bus.  It carries a TODO comment claiming
> this "has to be done in common code".  No other bus realize method
> realizes its devices.
> 
> The common code in question is bus_set_realized(), which has a TODO
> comment asking for recursive realization.  It's been asking for years.
> 
> The only devices sclp_events_bus_realize() will ever realize are the
> two init_event_facility() puts there.
> 
> Simplify as follows:
> 
> * Make the devices members of the event facility instance struct, just
>   like the bus.  object_initialize_child() is simpler than
>   object_property_add_child() and object_unref().
> 
> * Realize them in the event facility realize method.
> 
> This is in line with how such things are done elsewhere.
> 
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390x/event-facility.c | 59 ++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 35 deletions(-)

So, what should happen with this patch? Should it go with the rest of
the series, or should it go through the s390 tree?

On 26/05/20 11:45, Cornelia Huck wrote:
> On Tue, 19 May 2020 16:55:46 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> init_event_facility() creates the SCLP events bus with two SCLP event
>> devices (sclpquiesce and sclp-cpu-hotplug).  It leaves the devices
>> unrealized.  A comment explains they will be realized "via the bus".
>>
>> The bus's realize method sclp_events_bus_realize() indeed realizes all
>> unrealized devices on this bus.  It carries a TODO comment claiming
>> this "has to be done in common code".  No other bus realize method
>> realizes its devices.
>>
>> The common code in question is bus_set_realized(), which has a TODO
>> comment asking for recursive realization.  It's been asking for years.
>>
>> The only devices sclp_events_bus_realize() will ever realize are the
>> two init_event_facility() puts there.
>>
>> Simplify as follows:
>>
>> * Make the devices members of the event facility instance struct, just
>>   like the bus.  object_initialize_child() is simpler than
>>   object_property_add_child() and object_unref().
>>
>> * Realize them in the event facility realize method.
>>
>> This is in line with how such things are done elsewhere.
>>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: Halil Pasic <pasic@linux.ibm.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/s390x/event-facility.c | 59 ++++++++++++++++-----------------------
>>  1 file changed, 24 insertions(+), 35 deletions(-)
> 
> So, what should happen with this patch? Should it go with the rest of
> the series, or should it go through the s390 tree?

I think an Acked-by is the simplest way to handle it, since qdev_realize
doesn't exist upstream.

Paolo

On Tue, 26 May 2020 13:23:43 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 26/05/20 11:45, Cornelia Huck wrote:
> > On Tue, 19 May 2020 16:55:46 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >   
> >> init_event_facility() creates the SCLP events bus with two SCLP event
> >> devices (sclpquiesce and sclp-cpu-hotplug).  It leaves the devices
> >> unrealized.  A comment explains they will be realized "via the bus".
> >>
> >> The bus's realize method sclp_events_bus_realize() indeed realizes all
> >> unrealized devices on this bus.  It carries a TODO comment claiming
> >> this "has to be done in common code".  No other bus realize method
> >> realizes its devices.
> >>
> >> The common code in question is bus_set_realized(), which has a TODO
> >> comment asking for recursive realization.  It's been asking for years.
> >>
> >> The only devices sclp_events_bus_realize() will ever realize are the
> >> two init_event_facility() puts there.
> >>
> >> Simplify as follows:
> >>
> >> * Make the devices members of the event facility instance struct, just
> >>   like the bus.  object_initialize_child() is simpler than
> >>   object_property_add_child() and object_unref().
> >>
> >> * Realize them in the event facility realize method.
> >>
> >> This is in line with how such things are done elsewhere.
> >>
> >> Cc: Cornelia Huck <cohuck@redhat.com>
> >> Cc: Halil Pasic <pasic@linux.ibm.com>
> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Cc: Richard Henderson <rth@twiddle.net>
> >> Cc: David Hildenbrand <david@redhat.com>
> >> Cc: qemu-s390x@nongnu.org
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  hw/s390x/event-facility.c | 59 ++++++++++++++++-----------------------
> >>  1 file changed, 24 insertions(+), 35 deletions(-)  
> > 
> > So, what should happen with this patch? Should it go with the rest of
> > the series, or should it go through the s390 tree?  
> 
> I think an Acked-by is the simplest way to handle it, since qdev_realize
> doesn't exist upstream.

Ok, let's keep them together.

Acked-by: Cornelia Huck <cohuck@redhat.com>

On 19.05.20 16:55, Markus Armbruster wrote:
> init_event_facility() creates the SCLP events bus with two SCLP event
> devices (sclpquiesce and sclp-cpu-hotplug).  It leaves the devices
> unrealized.  A comment explains they will be realized "via the bus".
> 
> The bus's realize method sclp_events_bus_realize() indeed realizes all
> unrealized devices on this bus.  It carries a TODO comment claiming
> this "has to be done in common code".  No other bus realize method
> realizes its devices.
> 
> The common code in question is bus_set_realized(), which has a TODO
> comment asking for recursive realization.  It's been asking for years.
> 
> The only devices sclp_events_bus_realize() will ever realize are the
> two init_event_facility() puts there.
> 
> Simplify as follows:
> 
> * Make the devices members of the event facility instance struct, just
>   like the bus.  object_initialize_child() is simpler than
>   object_property_add_child() and object_unref().
> 
> * Realize them in the event facility realize method.
> 
> This is in line with how such things are done elsewhere.
> 
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390x/event-facility.c | 59 ++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 97a4f0b1f5..1ecaa20556 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -39,6 +39,7 @@ typedef struct SCLPEventsBus {
>  struct SCLPEventFacility {
>      SysBusDevice parent_obj;
>      SCLPEventsBus sbus;
> +    SCLPEvent quiesce, cpu_hotplug;
>      /* guest's receive mask */
>      union {
>          uint32_t receive_mask_pieces[2];
> @@ -328,34 +329,9 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
>  
>  #define TYPE_SCLP_EVENTS_BUS "s390-sclp-events-bus"
>  
> -static void sclp_events_bus_realize(BusState *bus, Error **errp)
> -{
> -    Error *err = NULL;
> -    BusChild *kid;
> -
> -    /* TODO: recursive realization has to be done in common code */
> -    QTAILQ_FOREACH(kid, &bus->children, sibling) {
> -        DeviceState *dev = kid->child;
> -
> -        object_property_set_bool(OBJECT(dev), true, "realized", &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -            return;
> -        }
> -    }
> -}
> -
> -static void sclp_events_bus_class_init(ObjectClass *klass, void *data)
> -{
> -    BusClass *bc = BUS_CLASS(klass);
> -
> -    bc->realize = sclp_events_bus_realize;
> -}
> -
>  static const TypeInfo sclp_events_bus_info = {
>      .name = TYPE_SCLP_EVENTS_BUS,
>      .parent = TYPE_BUS,
> -    .class_init = sclp_events_bus_class_init,
>  };
>  
>  static void command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
> @@ -443,27 +419,39 @@ static void init_event_facility(Object *obj)
>  {
>      SCLPEventFacility *event_facility = EVENT_FACILITY(obj);
>      DeviceState *sdev = DEVICE(obj);
> -    Object *new;
>  
>      event_facility->mask_length = 4;
>      event_facility->allow_all_mask_sizes = true;
>      object_property_add_bool(obj, "allow_all_mask_sizes",
>                               sclp_event_get_allow_all_mask_sizes,
>                               sclp_event_set_allow_all_mask_sizes);
> +
>      /* Spawn a new bus for SCLP events */
>      qbus_create_inplace(&event_facility->sbus, sizeof(event_facility->sbus),
>                          TYPE_SCLP_EVENTS_BUS, sdev, NULL);
>  
> -    new = object_new(TYPE_SCLP_QUIESCE);
> -    object_property_add_child(obj, TYPE_SCLP_QUIESCE, new);
> -    object_unref(new);
> -    qdev_set_parent_bus(DEVICE(new), BUS(&event_facility->sbus));
> +    object_initialize_child(obj, TYPE_SCLP_QUIESCE,
> +                            &event_facility->quiesce,
> +                            TYPE_SCLP_QUIESCE);
>  
> -    new = object_new(TYPE_SCLP_CPU_HOTPLUG);
> -    object_property_add_child(obj, TYPE_SCLP_CPU_HOTPLUG, new);
> -    object_unref(new);
> -    qdev_set_parent_bus(DEVICE(new), BUS(&event_facility->sbus));
> -    /* the facility will automatically realize the devices via the bus */
> +    object_initialize_child(obj, TYPE_SCLP_CPU_HOTPLUG,
> +                            &event_facility->cpu_hotplug,
> +                            TYPE_SCLP_CPU_HOTPLUG);
> +}
> +
> +static void realize_event_facility(DeviceState *dev, Error **errp)
> +{
> +    SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
> +    Error *local_err = NULL;
> +
> +    qdev_realize(DEVICE(&event_facility->quiesce),
> +                 BUS(&event_facility->sbus), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
> +                 BUS(&event_facility->sbus), errp);
>  }
>  
>  static void reset_event_facility(DeviceState *dev)
> @@ -479,6 +467,7 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(sbdc);
>      SCLPEventFacilityClass *k = EVENT_FACILITY_CLASS(dc);
>  
> +    dc->realize = realize_event_facility;
>      dc->reset = reset_event_facility;
>      dc->vmsd = &vmstate_event_facility;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> 

Think you forgot

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb