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

[Igor Mammedov]

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
> 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 <address@hidden>
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)