Re: [qemu-s390x] [PATCH RFC] qdev: Let the hotplug_unplug() caller delete the device

[David Hildenbrand]

On 03.12.18 17:01, Cornelia Huck wrote:
> On Wed, 28 Nov 2018 15:54:55 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> When unplugging a device, at one point the device will be destroyed
>> via object_unparent(). This will, one the one hand, unrealize the
>> device hierarchy to be removed, and on the other hand, destroy/free the
>> device hierarchy.
>>
>> When chaining interrupt handlers, we want to overwrite a bus hotplug
> 
> s/interrupt/hotplug/, no?

Yes indeed! (no idea what my brain was coprocessing while writing this
description)

> 
>> handler by the machine hotplug handler, to be able to perform
>> some part of the plug/unplug and to forward the calls to the bus hotplug
>> handler.
>>
>> For now, the bus hotplug handler would trigger an object_unparent(), not
>> allowing us to perform some unplug action on a device after we forwarded
>> the call to the bus hotplug handler. The device would be gone at that
>> point.
>>
>> hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
>>     machine_unplug_handler(dev) {
>>         /* eventually do unplug stuff */
>>         bus_unplug_handler(dev) -> calls object_unparent(dev)
>>         /* dev is gone, we can't do more unplug stuff */
>>     }
>>
>> So move the object_unparent() to the original caller of the unplug. For
>> now, keep the unrealize() at the original places of the
>> object_unparent().
>>
>> hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
>>     machine_unplug_handler(dev) {
>>         /* eventually do unplug stuff */
>>         bus_unplug_handler(dev) -> calls unrealize(dev)
>>         /* we can do more unplug stuff but device already unrealized */
>>     }
>> object_unparent(dev)
>>
>> In the long run, every unplug action should be factored out of the
>> unrealize() function into the unplug handler (especially for PCI). Then
>> we can get rid of the additonal unrealize() calls and object_unparent()
>> will properly unrealize the device hierarchy after the device has been
>> unplugged.
>>
>> hotplug_handler_unplug(dev) -> calls machine_unplug_handler()
>>     machine_unplug_handler(dev) {
>>         /* eventually do unplug stuff */
>>         bus_unplug_handler(dev) -> only unplugs, does not unrealize
>>         /* we can do more unplug stuff */
>>     }
>> object_unparent(dev) -> will unrealize
>>
>>
>> The original approach was suggested by Igor Mammedov for the PCI
>> part, but I extended it to all hotplug handlers. I consider this one
>> step into the right direction.
> 
> From my limited overview of the hotplug infrastructure, this looks
> reasonable.
> 
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>
>> I might still be missing some cases, but I want to get some feedback first
>> if this makes sense.
>>
>> This is based on the series:
>>     [PATCH v3 00/11] pci: hotplug handler reworks​
>>
>>  hw/acpi/cpu.c            |  1 +
>>  hw/acpi/memory_hotplug.c |  1 +
>>  hw/acpi/pcihp.c          |  3 ++-
>>  hw/core/qdev.c           |  3 +--
>>  hw/i386/pc.c             |  5 ++---
>>  hw/pci/pcie.c            |  3 ++-
>>  hw/pci/shpc.c            |  3 ++-
>>  hw/ppc/spapr.c           |  4 ++--
>>  hw/ppc/spapr_pci.c       |  3 ++-
>>  hw/s390x/css-bridge.c    |  2 +-
>>  hw/s390x/s390-pci-bus.c  | 12 ++++++++++--
>>  qdev-monitor.c           |  9 +++++++--
>>  12 files changed, 33 insertions(+), 16 deletions(-)
> 
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 9abd49a9dc..a84e80f6dd 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -988,7 +988,11 @@ static void s390_pcihost_unplug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>                                   pbdev->fh, pbdev->fid);
>>      bus = pci_get_bus(pci_dev);
>>      devfn = pci_dev->devfn;
>> -    object_unparent(OBJECT(pci_dev));
>> +    if (OBJECT(pci_dev) == OBJECT(dev)) {
>> +        object_property_set_bool(OBJECT(pci_dev), false, "realized", NULL);
>> +    } else {
>> +        object_unparent(OBJECT(pci_dev));
>> +    }
>>      s390_pci_msix_free(pbdev);
>>      s390_pci_iommu_free(s, bus, devfn);
>>      pbdev->pdev = NULL;
>> @@ -997,7 +1001,11 @@ out:
>>      pbdev->fid = 0;
>>      QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
>>      g_hash_table_remove(s->zpci_table, &pbdev->idx);
>> -    object_unparent(OBJECT(pbdev));
>> +    if (OBJECT(pbdev) == OBJECT(dev)) {
>> +        object_property_set_bool(OBJECT(pbdev), false, "realized", NULL);
>> +    } else {
>> +        object_unparent(OBJECT(pbdev));
>> +    }
> 
> That's a bit... ugly. Not really your code, but the inherent ugliness
> of the architecture it uncovers; we basically have two devices paired
> with each other and we need to unplug them both, regardless on which of
> the two unplug is called.
> 
> Maybe add a comment explaining it a bit?
> 

Yes, can do. My plan is to refactor this to have two independent
hotunplug calls (and therefore two independent hotplug handler chains) .
One step at a time :)

> It looks correct, though; but I haven't tested it :)
> 
> Nothing bad jumped out at me from the rest of your patch, either.
> 

Thanks for having a look!

-- 

Thanks,

David / dhildenb