From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56865) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5DvV-0007BC-P5 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 10:02:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5DvQ-0008Qh-Gw for qemu-devel@nongnu.org; Wed, 17 Jun 2015 10:02:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5DvQ-0008Qd-9o for qemu-devel@nongnu.org; Wed, 17 Jun 2015 10:02:44 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id E9784C0158 for ; Wed, 17 Jun 2015 14:02:43 +0000 (UTC) Date: Wed, 17 Jun 2015 16:02:41 +0200 From: "Michael S. Tsirkin" Message-ID: <20150617155751-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> <20150617154135-mutt-send-email-mst@redhat.com> <55817C54.1000201@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55817C54.1000201@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 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 > >> 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. > > 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. > > 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 > >>