Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
> In the PCI expander bridge, we will want to disable those features of
> pci-bridge that relate to SHPC (standard hotplug controller):
> 
> - SHPC bar and underlying MemoryRegion
> - interrupt (INTx or MSI)
> - effective hotplug callbacks
> - other SHPC hooks (initialization, cleanup, migration etc)
> 
> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
> the above if the bit is explicitly cleared.
> 
> Suggested-by: Michael S. Tsirkin <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>


I think I would prefer naming the property "shpc".
In particular ACPI hotplug still works fine.


> ---
> 
> Notes:
>     v6:
>     - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>       hotplug-less bridge for PXB" from v5) [Michael]
>     - I tested migration-to-file and migration-from-file, with a single PXB
>       behind which a virtio-scsi HBA lived
>     - No idea how to test the hotplug callbacks. I tried the device_add and
>       device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
>       monitor reports no error at all; the callbacks don't seem to be
>       reached.
>     - Retested with windows 2012; it works
> 
>  hw/pci-bridge/pci_bridge_dev.c | 90 
> ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 78 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index d697c7b..a0a1ce9 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
>      MemoryRegion bar;
>      uint8_t chassis_nr;
>  #define PCI_BRIDGE_DEV_F_MSI_REQ 0
> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
>      uint32_t flags;
>  };
>  typedef struct PCIBridgeDev PCIBridgeDev;
>  
> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
> +{
> +    return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
> +}
> +
>  static int pci_bridge_dev_initfn(PCIDevice *dev)
>  {
>      PCIBridge *br = PCI_BRIDGE(dev);
> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>      if (err) {
>          goto bridge_error;
>      }
> -    dev->config[PCI_INTERRUPT_PIN] = 0x1;
> -    memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", 
> shpc_bar_size(dev));
> -    err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
> -    if (err) {
> -        goto shpc_error;
> +
> +    if (hotplug_enabled(bridge_dev)) {
> +        dev->config[PCI_INTERRUPT_PIN] = 0x1;
> +        memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
> +                           shpc_bar_size(dev));
> +        err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
> +        if (err) {
> +            goto shpc_error;
> +        }
>      }
> +
>      err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>      if (err) {
>          goto slotid_error;
>      }
> +
> +    if (!hotplug_enabled(bridge_dev)) {
> +        return 0;
> +    }
> +
>      if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>          msi_supported) {
>          err = msi_init(dev, 0, 1, true, true);
> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>  msi_error:
>      slotid_cap_cleanup(dev);
>  slotid_error:
> -    shpc_cleanup(dev, &bridge_dev->bar);
> +    if (hotplug_enabled(bridge_dev)) {
> +        shpc_cleanup(dev, &bridge_dev->bar);
> +    }
>  shpc_error:
>      pci_bridge_exitfn(dev);
>  bridge_error:
> @@ -89,53 +107,101 @@ bridge_error:
>  static void pci_bridge_dev_exitfn(PCIDevice *dev)
>  {
>      PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> +
>      if (msi_present(dev)) {
>          msi_uninit(dev);
>      }
>      slotid_cap_cleanup(dev);
> -    shpc_cleanup(dev, &bridge_dev->bar);
> +    if (hotplug_enabled(bridge_dev)) {
> +        shpc_cleanup(dev, &bridge_dev->bar);
> +    }
>      pci_bridge_exitfn(dev);
>  }
>  
>  static void pci_bridge_dev_instance_finalize(Object *obj)
>  {
> +    /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
>      shpc_free(PCI_DEVICE(obj));
>  }
>  
>  static void pci_bridge_dev_write_config(PCIDevice *d,
>                                          uint32_t address, uint32_t val, int 
> len)
>  {
> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
> +
>      pci_bridge_write_config(d, address, val, len);
>      if (msi_present(d)) {
>          msi_write_config(d, address, val, len);
>      }
> -    shpc_cap_write_config(d, address, val, len);
> +    if (hotplug_enabled(bridge_dev)) {
> +        shpc_cap_write_config(d, address, val, len);
> +    }
>  }
>  
>  static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>  {
>      PCIDevice *dev = PCI_DEVICE(qdev);
> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>  
>      pci_bridge_reset(qdev);
> -    shpc_reset(dev);
> +    if (hotplug_enabled(bridge_dev)) {
> +        shpc_reset(dev);
> +    }
>  }
>  
>  static Property pci_bridge_dev_properties[] = {
>                      /* Note: 0 is not a legal chassis number. */
>      DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
>      DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, 
> true),
> +    DEFINE_PROP_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
> +                    true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
> +{
> +    PCIDevice *dev = opaque;
> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> +
> +    return hotplug_enabled(bridge_dev);
> +}
> +
>  static const VMStateDescription pci_bridge_dev_vmstate = {
>      .name = "pci_bridge",
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> -        SHPC_VMSTATE(shpc, PCIDevice, NULL),
> +        SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>  
> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
> +
> +    if (!hotplug_enabled(bridge_dev)) {
> +        error_setg(errp, "standard hotplug controller has been disabled for "
> +                   "this %s", TYPE_PCI_BRIDGE_DEV);
> +        return;
> +    }
> +    shpc_device_hotplug_cb(hotplug_dev, dev, errp);
> +}
> +
> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                                 DeviceState *dev,
> +                                                 Error **errp)
> +{
> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
> +
> +    if (!hotplug_enabled(bridge_dev)) {
> +        error_setg(errp, "standard hotplug controller has been disabled for "
> +                   "this %s", TYPE_PCI_BRIDGE_DEV);
> +        return;
> +    }
> +    shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
> +}
> +
>  static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, 
> void *data)
>      dc->props = pci_bridge_dev_properties;
>      dc->vmsd = &pci_bridge_dev_vmstate;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    hc->plug = shpc_device_hotplug_cb;
> -    hc->unplug_request = shpc_device_hot_unplug_request_cb;
> +    hc->plug = pci_bridge_dev_hotplug_cb;
> +    hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>  }
>  
>  static const TypeInfo pci_bridge_dev_info = {
> -- 
> 1.8.3.1
>