From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48875) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5Dbl-0005m9-JH for qemu-devel@nongnu.org; Wed, 17 Jun 2015 09:42:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5Dbg-0007Sh-LW for qemu-devel@nongnu.org; Wed, 17 Jun 2015 09:42:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46447) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5Dbg-0007Sc-Co for qemu-devel@nongnu.org; Wed, 17 Jun 2015 09:42:20 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 0AC35373F4D for ; Wed, 17 Jun 2015 13:42:20 +0000 (UTC) Date: Wed, 17 Jun 2015 15:42:17 +0200 From: "Michael S. Tsirkin" Message-ID: <20150617154135-mutt-send-email-mst@redhat.com> References: <1434545105-5811-1-git-send-email-lersek@redhat.com> <1434545105-5811-4-git-send-email-lersek@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1434545105-5811-4-git-send-email-lersek@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: Marcel Apfelbaum , qemu-devel@nongnu.org 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 > Cc: Michael S. Tsirkin > Cc: Marcel Apfelbaum > Signed-off-by: Laszlo Ersek 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 >