qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
Date: Wed, 17 Jun 2015 22:54:50 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 06/17/2015 05:15 PM, Laszlo Ersek wrote:
On 06/17/15 16:02, Michael S. Tsirkin wrote:
On Wed, Jun 17, 2015 at 03:55:32PM +0200, Laszlo Ersek wrote:
On 06/17/15 15:42, Michael S. Tsirkin wrote:
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.

Will do.

Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
what would be your preference?

PCI_BRIDGE_DEV_F_SHPC_REQ

Also add macro for "shpc" property string to avoid duplication.
Also clear msi explicitly - no need for that to depend on shpc.

Should I also rename the helper function hotplug_enabled() to
shpc_enabled()?

It should be tested only once - before shpc is initialized.
All other users should test QEMU_PCI_CAP_SHPC - if you like,
using a helper shpc_present.

I'll make one attempt (in v7) to get this right. If I miss it, I'll ask
Marcel to please take over the series.
No problem here,
I'll take it from V8 if necessary, but I think it will be just fine. :)

Thanks,
Marcel


Thanks
Laszlo



Thanks
Laszlo



---

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






reply via email to

[Prev in Thread] Current Thread [Next in Thread]