| Message ID | 20181128145455.11733-1-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] qdev: Let the hotplug_unplug() caller delete the device | expand |
On Wed, 28 Nov 2018 15:54:55 +0100 David Hildenbrand <david@redhat.com> 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? > 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 <david@redhat.com> > --- > > 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? It looks correct, though; but I haven't tested it :) Nothing bad jumped out at me from the rest of your patch, either.
On 03.12.18 17:01, Cornelia Huck wrote: > On Wed, 28 Nov 2018 15:54:55 +0100 > David Hildenbrand <david@redhat.com> 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 <david@redhat.com> >> --- >> >> 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!
On Wed, 28 Nov 2018 15:54:55 +0100 David Hildenbrand <david@redhat.com> 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 > 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. > > Signed-off-by: David Hildenbrand <david@redhat.com> Sorry but I've lost track of all hotplug patches that where reviewed and pending form merge. Lets revisit this patch once those are merged to have a coherent view on how things stand. > --- > > 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/acpi/cpu.c b/hw/acpi/cpu.c > index f10b190019..37703a8806 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -126,6 +126,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, > dev = DEVICE(cdev->cpu); > hotplug_ctrl = qdev_get_hotplug_handler(dev); > hotplug_handler_unplug(hotplug_ctrl, dev, NULL); > + object_unparent(OBJECT(dev)); > } > break; > case ACPI_CPU_CMD_OFFSET_WR: > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index 8c7c1013f3..9fbf032c15 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -189,6 +189,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, > error_free(local_err); > break; > } > + object_unparent(OBJECT(dev)); > trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); > } > break; > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 7bc7a72340..31b1a8fe58 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -174,6 +174,7 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo > if (!acpi_pcihp_pc_no_hotplug(s, dev)) { > hotplug_ctrl = qdev_get_hotplug_handler(qdev); > hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort); > + object_unparent(OBJECT(qdev)); > } > } > } > @@ -269,7 +270,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > DeviceState *dev, Error **errp) > { > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > } > > void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 6b3cc55b27..0d9da06c7b 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -286,8 +286,7 @@ void qbus_reset_all_fn(void *opaque) > void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > - /* just zap it */ > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > } > > /* > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index f095725dba..3880f23ff2 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1777,8 +1777,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev, > } > > pc_dimm_unplug(PC_DIMM(dev), MACHINE(pcms)); > - object_unparent(OBJECT(dev)); > - > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > out: > error_propagate(errp, local_err); > } > @@ -1891,7 +1890,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev, > > found_cpu = pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, NULL); > found_cpu->cpu = NULL; > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > > /* decrement the number of CPUs */ > pcms->boot_cpus--; > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index ba3ea925e9..d6d2722d49 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -367,7 +367,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > } > > static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > @@ -375,6 +375,7 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(dev)); > > hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort); > + object_unparent(OBJECT(dev)); > } > > void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c > index 5de905cb56..0e6e42a899 100644 > --- a/hw/pci/shpc.c > +++ b/hw/pci/shpc.c > @@ -249,6 +249,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot) > hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(affected_dev)); > hotplug_handler_unplug(hotplug_ctrl, DEVICE(affected_dev), > &error_abort); > + object_unparent(OBJECT(affected_dev)); > } > } > } > @@ -546,7 +547,7 @@ void shpc_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > } > > void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev, > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 7afd1a175b..f4ce612d6b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3334,7 +3334,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev) > sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); > > pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev)); > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > spapr_pending_dimm_unplugs_remove(spapr, ds); > } > > @@ -3441,7 +3441,7 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev) > > assert(core_slot); > core_slot->cpu = NULL; > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > } > > static > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index bfb02ee96b..c230f6a8b0 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1373,6 +1373,7 @@ void spapr_phb_remove_pci_device_cb(DeviceState *dev) > HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); > > hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > + object_unparent(OBJECT(dev)); > } > > static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb, > @@ -1495,7 +1496,7 @@ static void spapr_pci_unplug(HotplugHandler *plug_handler, > * an 'idle' state, as the device cleanup code expects. > */ > pci_device_reset(PCI_DEVICE(plugged_dev)); > - object_unparent(OBJECT(plugged_dev)); > + object_property_set_bool(OBJECT(plugged_dev), false, "realized", NULL); > } > > static void spapr_pci_unplug_request(HotplugHandler *plug_handler, > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > index 1bd6c8b458..614c2a3b8b 100644 > --- a/hw/s390x/css-bridge.c > +++ b/hw/s390x/css-bridge.c > @@ -51,7 +51,7 @@ static void ccw_device_unplug(HotplugHandler *hotplug_dev, > > css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0); > > - object_unparent(OBJECT(dev)); > + object_property_set_bool(OBJECT(dev), false, "realized", NULL); > } > > static void virtual_css_bus_reset(BusState *qbus) > 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)); > + } > } > > static void s390_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev, > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 07147c63bf..7705acd6c7 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -862,6 +862,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) > DeviceClass *dc = DEVICE_GET_CLASS(dev); > HotplugHandler *hotplug_ctrl; > HotplugHandlerClass *hdc; > + Error *local_err = NULL; > > if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { > error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); > @@ -890,10 +891,14 @@ void qdev_unplug(DeviceState *dev, Error **errp) > * otherwise just remove it synchronously */ > hdc = HOTPLUG_HANDLER_GET_CLASS(hotplug_ctrl); > if (hdc->unplug_request) { > - hotplug_handler_unplug_request(hotplug_ctrl, dev, errp); > + hotplug_handler_unplug_request(hotplug_ctrl, dev, &local_err); > } else { > - hotplug_handler_unplug(hotplug_ctrl, dev, errp); > + hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); > + if (!local_err) { > + object_unparent(OBJECT(dev)); > + } > } > + error_propagate(errp, local_err); > } > > void qmp_device_del(const char *id, Error **errp)
On 04.12.18 14:33, Igor Mammedov wrote: > On Wed, 28 Nov 2018 15:54:55 +0100 > David Hildenbrand <david@redhat.com> 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 >> 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. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > Sorry but I've lost track of all hotplug patches that where > reviewed and pending form merge. > Lets revisit this patch once those are merged to have a coherent > view on how things stand. At least from my side, there is only [PATCH v3 00/11] pci: hotplug handler reworks
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index f10b190019..37703a8806 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -126,6 +126,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, dev = DEVICE(cdev->cpu); hotplug_ctrl = qdev_get_hotplug_handler(dev); hotplug_handler_unplug(hotplug_ctrl, dev, NULL); + object_unparent(OBJECT(dev)); } break; case ACPI_CPU_CMD_OFFSET_WR: diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index 8c7c1013f3..9fbf032c15 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -189,6 +189,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, error_free(local_err); break; } + object_unparent(OBJECT(dev)); trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); } break; diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 7bc7a72340..31b1a8fe58 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -174,6 +174,7 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo if (!acpi_pcihp_pc_no_hotplug(s, dev)) { hotplug_ctrl = qdev_get_hotplug_handler(qdev); hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort); + object_unparent(OBJECT(qdev)); } } } @@ -269,7 +270,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, DeviceState *dev, Error **errp) { - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); } void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 6b3cc55b27..0d9da06c7b 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -286,8 +286,7 @@ void qbus_reset_all_fn(void *opaque) void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - /* just zap it */ - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); } /* diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f095725dba..3880f23ff2 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1777,8 +1777,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev, } pc_dimm_unplug(PC_DIMM(dev), MACHINE(pcms)); - object_unparent(OBJECT(dev)); - + object_property_set_bool(OBJECT(dev), false, "realized", NULL); out: error_propagate(errp, local_err); } @@ -1891,7 +1890,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev, found_cpu = pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, NULL); found_cpu->cpu = NULL; - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); /* decrement the number of CPUs */ pcms->boot_cpus--; diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index ba3ea925e9..d6d2722d49 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -367,7 +367,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); } static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) @@ -375,6 +375,7 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(dev)); hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort); + object_unparent(OBJECT(dev)); } void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c index 5de905cb56..0e6e42a899 100644 --- a/hw/pci/shpc.c +++ b/hw/pci/shpc.c @@ -249,6 +249,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot) hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(affected_dev)); hotplug_handler_unplug(hotplug_ctrl, DEVICE(affected_dev), &error_abort); + object_unparent(OBJECT(affected_dev)); } } } @@ -546,7 +547,7 @@ void shpc_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); } void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev, diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 7afd1a175b..f4ce612d6b 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3334,7 +3334,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev) sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev)); - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); spapr_pending_dimm_unplugs_remove(spapr, ds); } @@ -3441,7 +3441,7 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev) assert(core_slot); core_slot->cpu = NULL; - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); } static diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index bfb02ee96b..c230f6a8b0 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1373,6 +1373,7 @@ void spapr_phb_remove_pci_device_cb(DeviceState *dev) HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); + object_unparent(OBJECT(dev)); } static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb, @@ -1495,7 +1496,7 @@ static void spapr_pci_unplug(HotplugHandler *plug_handler, * an 'idle' state, as the device cleanup code expects. */ pci_device_reset(PCI_DEVICE(plugged_dev)); - object_unparent(OBJECT(plugged_dev)); + object_property_set_bool(OBJECT(plugged_dev), false, "realized", NULL); } static void spapr_pci_unplug_request(HotplugHandler *plug_handler, diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c index 1bd6c8b458..614c2a3b8b 100644 --- a/hw/s390x/css-bridge.c +++ b/hw/s390x/css-bridge.c @@ -51,7 +51,7 @@ static void ccw_device_unplug(HotplugHandler *hotplug_dev, css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0); - object_unparent(OBJECT(dev)); + object_property_set_bool(OBJECT(dev), false, "realized", NULL); } static void virtual_css_bus_reset(BusState *qbus) 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)); + } } static void s390_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev, diff --git a/qdev-monitor.c b/qdev-monitor.c index 07147c63bf..7705acd6c7 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -862,6 +862,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) DeviceClass *dc = DEVICE_GET_CLASS(dev); HotplugHandler *hotplug_ctrl; HotplugHandlerClass *hdc; + Error *local_err = NULL; if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); @@ -890,10 +891,14 @@ void qdev_unplug(DeviceState *dev, Error **errp) * otherwise just remove it synchronously */ hdc = HOTPLUG_HANDLER_GET_CLASS(hotplug_ctrl); if (hdc->unplug_request) { - hotplug_handler_unplug_request(hotplug_ctrl, dev, errp); + hotplug_handler_unplug_request(hotplug_ctrl, dev, &local_err); } else { - hotplug_handler_unplug(hotplug_ctrl, dev, errp); + hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); + if (!local_err) { + object_unparent(OBJECT(dev)); + } } + error_propagate(errp, local_err); } void qmp_device_del(const char *id, Error **errp)
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 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. Signed-off-by: David Hildenbrand <david@redhat.com> --- 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(-)