From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37300) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5JQH-0002Ux-Ir for qemu-devel@nongnu.org; Wed, 17 Jun 2015 15:54:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5JQE-00033Z-89 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 15:54:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41708) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5JQD-00033L-UL for qemu-devel@nongnu.org; Wed, 17 Jun 2015 15:54:54 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 5755E2EBD16 for ; Wed, 17 Jun 2015 19:54:53 +0000 (UTC) Message-ID: <5581D08A.90701@redhat.com> Date: Wed, 17 Jun 2015 22:54:50 +0300 From: Marcel Apfelbaum 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> <558180F0.7040704@redhat.com> In-Reply-To: <558180F0.7040704@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Laszlo Ersek , "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org 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 >>>>> 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. 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 >>>>> >