Re: [Qemu-ppc] [PATCH v2 09/10] pci/shpc: perform unplug via the hotplug handler

[Igor Mammedov]

On Mon,  5 Nov 2018 11:20:43 +0100
David Hildenbrand <address@hidden> wrote:

> Introduce and use the "unplug" callback.
> 
> This is a preparation for multi-stage hotplug handlers, whereby the bus
> hotplug handler is overwritten by the machine hotplug handler. This handler
> will then pass control to the bus hotplug handler. So to get this running
> cleanly, we also have to make sure to go via the hotplug handler chain when
> actually unplugging a device after an unplug request. Lookup the hotplug
> handler and call "unplug".
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/pci-bridge/pci_bridge_dev.c  | 14 ++++++++++++++
>  hw/pci-bridge/pcie_pci_bridge.c | 14 ++++++++++++++
>  hw/pci/shpc.c                   | 11 ++++++++++-
>  include/hw/pci/shpc.h           |  2 ++
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index e1df9a52ac..91415f114c 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -219,6 +219,19 @@ static void pci_bridge_dev_plug_cb(HotplugHandler 
> *hotplug_dev,
>      shpc_device_plug_cb(hotplug_dev, dev, errp);
>  }
>  
> +static void pci_bridge_dev_unplug_cb(HotplugHandler *hotplug_dev,
> +                                     DeviceState *dev, Error **errp)
> +{
> +    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
> +
> +    if (!shpc_present(pci_hotplug_dev)) {
> +        error_setg(errp, "standard hotplug controller has been disabled for "
> +                   "this %s", TYPE_PCI_BRIDGE_DEV);
> +        return;
> +    }
> +    shpc_device_unplug_cb(hotplug_dev, dev, errp);
> +}
> +
>  static void pci_bridge_dev_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                               DeviceState *dev, Error **errp)
>  {
> @@ -251,6 +264,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, 
> void *data)
>      dc->vmsd = &pci_bridge_dev_vmstate;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      hc->plug = pci_bridge_dev_plug_cb;
> +    hc->unplug = pci_bridge_dev_unplug_cb;
>      hc->unplug_request = pci_bridge_dev_unplug_request_cb;
>  }
>  
> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
> index c634353b06..7c667bc97c 100644
> --- a/hw/pci-bridge/pcie_pci_bridge.c
> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> @@ -150,6 +150,19 @@ static void pcie_pci_bridge_plug_cb(HotplugHandler 
> *hotplug_dev,
>      shpc_device_plug_cb(hotplug_dev, dev, errp);
>  }
>  
> +static void pcie_pci_bridge_unplug_cb(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
> +    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
> +
> +    if (!shpc_present(pci_hotplug_dev)) {
> +        error_setg(errp, "standard hotplug controller has been disabled for "
> +                   "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
Is it possible to reach here at all?

> +        return;
> +    }
> +    shpc_device_unplug_cb(hotplug_dev, dev, errp);
> +}

you probably can share this function impl. with pci_bridge_dev_unplug_cb()
if you use object_get_typename().



> +
>  static void pcie_pci_bridge_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                                DeviceState *dev, Error **errp)
>  {
> @@ -180,6 +193,7 @@ static void pcie_pci_bridge_class_init(ObjectClass 
> *klass, void *data)
>      dc->reset = &pcie_pci_bridge_reset;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      hc->plug = pcie_pci_bridge_plug_cb;
> +    hc->unplug = pcie_pci_bridge_unplug_cb;
>      hc->unplug_request = pcie_pci_bridge_unplug_request_cb;
>  }
>  
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 098ffaef1d..5de905cb56 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -238,6 +238,7 @@ static void shpc_invalid_command(SHPCDevice *shpc)
>  
>  static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
>  {
> +    HotplugHandler *hotplug_ctrl;
>      int devfn;
>      int pci_slot = SHPC_IDX_TO_PCI(slot);
>      for (devfn = PCI_DEVFN(pci_slot, 0);
> @@ -245,7 +246,9 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, 
> int slot)
>           ++devfn) {
>          PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
>          if (affected_dev) {
> -            object_unparent(OBJECT(affected_dev));
> +            hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(affected_dev));
> +            hotplug_handler_unplug(hotplug_ctrl, DEVICE(affected_dev),
> +                                   &error_abort);
>
>          }
>      }
>  }
> @@ -540,6 +543,12 @@ void shpc_device_plug_cb(HotplugHandler *hotplug_dev, 
> DeviceState *dev,
>      shpc_interrupt_update(pci_hotplug_dev);
>  }
>  
> +void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                           Error **errp)
> +{
> +    object_unparent(OBJECT(dev));
> +}
> +
>  void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                     DeviceState *dev, Error **errp)
>  {
> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
> index 71293aca58..18f6ec1cd5 100644
> --- a/include/hw/pci/shpc.h
> +++ b/include/hw/pci/shpc.h
> @@ -47,6 +47,8 @@ void shpc_cap_write_config(PCIDevice *d, uint32_t addr, 
> uint32_t val, int len);
>  
>  void shpc_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                           Error **errp);
> +void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                           Error **errp);
>  void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                     DeviceState *dev, Error **errp);
>