Re: [Qemu-devel] [PATCH 9/9] hotplug: refactor hotplug to leverage new QOM functions

[liu ping fan]

On Sun, Aug 26, 2012 at 11:51 PM, Anthony Liguori <address@hidden> wrote:
> 1) DeviceState::unplug requests for an eject to happen
>    - the default implementation is a forced eject
>
> 2) A bus can eject a device by setting the parent_bus to NULL
>    - this detaches the device from the bus
>    - this does *not* cause the device to disappear
>
> 3) The current implementation on unplug also registers an eject notifier
>    - the eject notifier will detach the device the parent.  This will cause 
> the
>      device to disappear
>
> 4) A purely guest initiated unplug will not delete a device but will cause the
>    device to appear detached from the guests PoV.
>
> Signed-off-by: Anthony Liguori <address@hidden>
> ---
>  hw/acpi_piix4.c   |    3 ++-
>  hw/pci.c          |   10 +++++++++-
>  hw/pcie.c         |    2 +-
>  hw/qdev.c         |   22 ++++++++++++++++++++++
>  hw/qdev.h         |    2 ++
>  hw/shpc.c         |    2 +-
>  hw/xen_platform.c |    2 +-
>  7 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 72d6e5c..eac53b3 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -305,7 +305,8 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, 
> unsigned slots)
>              if (pc->no_hotplug) {
>                  slot_free = false;
>              } else {
> -                qdev_free(qdev);
> +                /* Force eject of device */
> +                qdev_set_parent_bus(qdev, NULL);

Do we need to wait for guest's ACKs for all of this device's children.
Then we can change this node's topology in the device tree.
I think, we can color the current device as unplug_state=ACK, and then
decide whether to detached it or not.
Each unplug ack from guest, will first check 1st.whether current node
can be release or not.  2nd. if can released, then go bottom-up
through the device tree to check whether the upper device can be
released or not.
If the down device(devB) removal cause the up device(devA) becomes a
leaf, then we can remove devA.
A leaf device is defined as : has no BusState kids OR all of its
BusState kids are empty.

This method can avoid sudden remove.
>              }
>          }
>      }
> diff --git a/hw/pci.c b/hw/pci.c
> index 437af70..cc555c2 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -46,6 +46,14 @@ static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static int pcibus_reset(BusState *qbus);
>
> +static void pcibus_remove_child(BusState *bus, DeviceState *dev)
> +{
> +    PCIDevice *pci_dev = PCI_DEVICE(dev);
> +    PCIBus *pci_bus = PCI_BUS(bus);
> +
> +    pci_bus->devices[pci_dev->devfn] = NULL;
> +}
> +
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> @@ -65,6 +73,7 @@ static void pci_bus_class_init(ObjectClass *klass, void 
> *data)
>      k->get_dev_path = pcibus_get_dev_path;
>      k->get_fw_dev_path = pcibus_get_fw_dev_path;
>      k->reset = pcibus_reset;
> +    k->remove_child = pcibus_remove_child;
>  }
>
>  static const TypeInfo pci_bus_info = {
> @@ -833,7 +842,6 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev, PCIBus *bus,
>  static void do_pci_unregister_device(PCIDevice *pci_dev)
>  {
>      qemu_free_irqs(pci_dev->irq);
> -    pci_dev->bus->devices[pci_dev->devfn] = NULL;
>      pci_config_free(pci_dev);
>  }
>
> diff --git a/hw/pcie.c b/hw/pcie.c
> index 7c92f19..d10ffea 100644
> --- a/hw/pcie.c
> +++ b/hw/pcie.c
> @@ -235,7 +235,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
>                                     PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
>      } else {
> -        qdev_free(&pci_dev->qdev);
> +        qdev_set_parent_bus(DEVICE(pci_dev), NULL);
>          pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
>                                       PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 525a0cb..be41f00 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -62,6 +62,7 @@ static void qdev_property_add_legacy(DeviceState *dev, 
> Property *prop,
>
>  static void bus_remove_child(BusState *bus, DeviceState *child)
>  {
> +    BusClass *bc = BUS_GET_CLASS(bus);
>      BusChild *kid;
>
>      QTAILQ_FOREACH(kid, &bus->children, sibling) {
> @@ -71,6 +72,11 @@ static void bus_remove_child(BusState *bus, DeviceState 
> *child)
>              snprintf(name, sizeof(name), "child[%d]", kid->index);
>              QTAILQ_REMOVE(&bus->children, kid, sibling);
>              object_property_del(OBJECT(bus), name, NULL);
> +
> +            if (bc->remove_child) {
> +                bc->remove_child(bus, kid->child);
> +            }
> +
>              g_free(kid);
>              return;
>          }
> @@ -192,9 +198,20 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int 
> alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>
> +static void qdev_finish_unplug(Notifier *notifier, void *data)
> +{
> +    DeviceState *dev = DEVICE(data);
> +
> +    /* unparent the object -- this should release the last reference to the
> +       child*/
> +    object_unparent(OBJECT(dev));
> +    g_free(notifier);
> +}
> +
>  void qdev_unplug(DeviceState *dev, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    Notifier *notifier;
>
>      if (!dev->parent_bus->allow_hotplug) {
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> @@ -204,6 +221,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>
>      qdev_hot_removed = true;
>
> +    notifier = g_malloc0(sizeof(*notifier));
> +    notifier->notify = qdev_finish_unplug;
> +
> +    notifier_list_add(&dev->eject_notifier, notifier);
> +
>      if (dc->unplug(dev) < 0) {
>          error_set(errp, QERR_UNDEFINED_ERROR);
>          return;
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 5009072..7ae8d5d 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -99,6 +99,8 @@ struct BusClass {
>       */
>      char *(*get_fw_dev_path)(DeviceState *dev);
>      int (*reset)(BusState *bus);
> +
> +    void (*remove_child)(BusState *bus, DeviceState *dev);
>  };
>
>  typedef struct BusChild {
> diff --git a/hw/shpc.c b/hw/shpc.c
> index a5baf24..ce507e7 100644
> --- a/hw/shpc.c
> +++ b/hw/shpc.c
> @@ -253,7 +253,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, 
> int slot)
>           ++devfn) {
>          PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
>          if (affected_dev) {
> -            qdev_free(&affected_dev->qdev);
> +            qdev_set_parent_bus(DEVICE(affected_dev), NULL);
>          }
>      }
>  }
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index 0d6c2ff..5c5ecf8 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -87,7 +87,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
>  {
>      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
>              PCI_CLASS_NETWORK_ETHERNET) {
> -        qdev_free(&d->qdev);
> +        qdev_set_parent_bus(dev, NULL);
>      }
>  }
>
> --
> 1.7.5.4
>
>