From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5E7f-0004n4-FR for qemu-devel@nongnu.org; Wed, 17 Jun 2015 10:15:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5E7Y-0006Xn-W5 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 10:15:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48681) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5E7Y-0006Wr-Nr for qemu-devel@nongnu.org; Wed, 17 Jun 2015 10:15:16 -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 6736F2D1566 for ; Wed, 17 Jun 2015 14:15:16 +0000 (UTC) Message-ID: <558180F0.7040704@redhat.com> Date: Wed, 17 Jun 2015 16:15:12 +0200 From: Laszlo Ersek MIME-Version: 1.0 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> <20150617155751-mutt-send-email-mst@redhat.com> In-Reply-To: <20150617155751-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" , Marcel Apfelbaum Cc: qemu-devel@nongnu.org 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 >>>> 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. 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. 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 >>>>