From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45833) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9loZ-0000Nt-8Q for qemu-devel@nongnu.org; Thu, 17 Dec 2015 22:34:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9loW-00020e-0F for qemu-devel@nongnu.org; Thu, 17 Dec 2015 22:34:43 -0500 Received: from [59.151.112.132] (port=29013 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9loV-0001wL-4R for qemu-devel@nongnu.org; Thu, 17 Dec 2015 22:34:39 -0500 Message-ID: <56737D85.3030305@cn.fujitsu.com> Date: Fri, 18 Dec 2015 11:29:09 +0800 From: Chen Fan MIME-Version: 1.0 References: <35d75210aef922bd049d93e0c8dbe13a2b5b1dd3.1447748073.git.chen.fan.fnst@cn.fujitsu.com> <1450384318.2674.110.camel@redhat.com> In-Reply-To: <1450384318.2674.110.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v14 Resend 10/13] pci: add pci device pre-post reset callbacks for host bus reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson , Cao jin , qemu-devel@nongnu.org Cc: mst@redhat.com On 12/18/2015 04:31 AM, Alex Williamson wrote: > On Thu, 2015-12-17 at 09:41 +0800, Cao jin wrote: >> From: Chen Fan >> >> Particularly, For vfio devices, Once need to recovery devices >> by bus reset such as AER, we always need to reset the host bus >> to recovery the devices under the bus, so we need to add pci device >> callbacks to specify to do host bus reset. >> >> Signed-off-by: Chen Fan >> Reviewed-by: Michael S. Tsirkin >> --- >> hw/pci/pci.c | 18 ++++++++++++++++++ >> hw/pci/pci_bridge.c | 9 +++++++++ >> hw/vfio/pci.c | 26 ++++++++++++++++++++++++++ >> hw/vfio/pci.h | 2 ++ >> include/hw/pci/pci.h | 7 +++++++ >> 5 files changed, 62 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index f6ca6ef..64fa2cc 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -247,6 +247,24 @@ static void pci_do_device_reset(PCIDevice *dev) >> msix_reset(dev); >> } >> >> +void pci_device_pre_reset(PCIBus *bus, PCIDevice *dev, void *unused) >> +{ >> + PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(dev); >> + >> + if (dc->pre_reset) { >> + dc->pre_reset(dev); >> + } >> +} >> + >> +void pci_device_post_reset(PCIBus *bus, PCIDevice *dev, void >> *unused) >> +{ >> + PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(dev); >> + >> + if (dc->post_reset) { >> + dc->post_reset(dev); >> + } >> +} >> + >> /* >> * This function is called on #RST and FLR. >> * FLR if PCI_EXP_DEVCTL_BCR_FLR is set >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c >> index 40c97b1..ddb76ab 100644 >> --- a/hw/pci/pci_bridge.c >> +++ b/hw/pci/pci_bridge.c >> @@ -267,8 +267,17 @@ void pci_bridge_write_config(PCIDevice *d, >> >> newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); >> if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) { >> + /* >> + * Notify all vfio-pci devices under the bus >> + * should do physical bus reset. >> + */ >> + PCIBus *sec_bus = pci_bridge_get_sec_bus(s); >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >> + pci_device_pre_reset, NULL); >> /* Trigger hot reset on 0->1 transition. */ >> qbus_reset_all(&s->sec_bus.qbus); >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >> + pci_device_post_reset, NULL); >> } >> } >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index e17dc89..df32618 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -39,6 +39,7 @@ >> >> static void vfio_disable_interrupts(VFIOPCIDevice *vdev); >> static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool >> enabled); >> +static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single); >> >> /* >> * Disabling BAR mmaping can be slow, but toggling it around INTx >> can >> @@ -1888,6 +1889,8 @@ static int >> vfio_check_host_bus_reset(VFIOPCIDevice *vdev) >> /* List all affected devices by bus reset */ >> devices = &info->devices[0]; >> >> + vdev->single_depend_dev = (info->count == 1); >> + >> /* Verify that we have all the groups required */ >> for (i = 0; i < info->count; i++) { >> PCIHostDeviceAddress host; >> @@ -2029,10 +2032,26 @@ static int >> vfio_check_bus_reset(NotifierWithReturn *n, void *opaque) >> return vfio_check_host_bus_reset(vdev); >> } >> >> +static void vfio_aer_pre_reset(PCIDevice *pdev) >> +{ >> + VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); >> + >> + vdev->aer_reset = true; >> + vfio_pci_hot_reset(vdev, vdev->single_depend_dev); >> +} > Doesn't this lead to multiple host bus resets per guest bus reset in > many cases? It looks like we'll do it once per vfio-pci device, even > if those devices are on the same host bus. That's a 1 second operation > per device. Can we avoid that? Maybe some sort of sequence ID could > help a device figure out whether it's already been reset as part of a > dependent device for this particular guest bus reset. Thanks, That's right, I missed this case, but I don't understand the scenario how to use a sequence ID to mark the device if been reset. can you detail it ? additional, there was a mechanism to compute device whether need to be reset by hot reset. so I simply modify the code as the following: diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index a9bc67e..42774ca 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2063,13 +2063,19 @@ static void vfio_aer_pre_reset(PCIDevice *pdev) VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); vdev->aer_reset = true; - vfio_pci_hot_reset(vdev, vdev->single_depend_dev); + vdev->vbasedev.needs_reset = true; } static void vfio_aer_post_reset(PCIDevice *pdev) { VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); + if (!vdev->single_depend_dev && vdev->vbasedev.needs_reset) { + vfio_pci_hot_reset(vdev, false); + } else { + vfio_pci_hot_reset(vdev, true); + } + vdev->aer_reset = false; } what do you think of this ? Thanks, Chen > > Alex > >> + >> +static void vfio_aer_post_reset(PCIDevice *pdev) >> +{ >> + VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); >> + >> + vdev->aer_reset = false; >> +} >> + >> static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, >> int pos, uint16_t size) >> { >> PCIDevice *pdev = &vdev->pdev; >> + PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(pdev); >> PCIDevice *dev_iter; >> uint8_t type; >> uint32_t errcap; >> @@ -2079,6 +2098,9 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, >> uint8_t cap_ver, >> vdev->hotplug_notifier.notify = vfio_check_bus_reset; >> pci_bus_add_hotplug_notifier(pdev->bus, &vdev- >>> hotplug_notifier); >> >> + dc->pre_reset = vfio_aer_pre_reset; >> + dc->post_reset = vfio_aer_post_reset; >> + >> pcie_cap_deverr_init(pdev); >> return pcie_aer_init(pdev, pos, size); >> >> @@ -2978,6 +3000,10 @@ static void vfio_pci_reset(DeviceState *dev) >> >> trace_vfio_pci_reset(vdev->vbasedev.name); >> >> + if (vdev->aer_reset) { >> + return; >> + } >> + >> vfio_pci_pre_reset(vdev); >> >> if (vdev->resetfn && !vdev->resetfn(vdev)) { >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h >> index b385f07..5470b97 100644 >> --- a/hw/vfio/pci.h >> +++ b/hw/vfio/pci.h >> @@ -142,6 +142,8 @@ typedef struct VFIOPCIDevice { >> bool no_kvm_intx; >> bool no_kvm_msi; >> bool no_kvm_msix; >> + bool aer_reset; >> + bool single_depend_dev; >> >> NotifierWithReturn hotplug_notifier; >> } VFIOPCIDevice; >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index 379b6e1..6b1f2d4 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -105,6 +105,9 @@ typedef void PCIMapIORegionFunc(PCIDevice >> *pci_dev, int region_num, >> pcibus_t addr, pcibus_t size, int >> type); >> typedef void PCIUnregisterFunc(PCIDevice *pci_dev); >> >> +typedef void PCIPreResetFunc(PCIDevice *pci_dev); >> +typedef void PCIPostResetFunc(PCIDevice *pci_dev); >> + >> typedef struct PCIIORegion { >> pcibus_t addr; /* current PCI mapping address. -1 means not >> mapped */ >> #define PCI_BAR_UNMAPPED (~(pcibus_t)0) >> @@ -193,6 +196,8 @@ typedef struct PCIDeviceClass { >> PCIUnregisterFunc *exit; >> PCIConfigReadFunc *config_read; >> PCIConfigWriteFunc *config_write; >> + PCIPreResetFunc *pre_reset; >> + PCIPostResetFunc *post_reset; >> >> uint16_t vendor_id; >> uint16_t device_id; >> @@ -380,6 +385,8 @@ bool pci_intx_route_changed(PCIINTxRoute *old, >> PCIINTxRoute *new); >> void pci_bus_fire_intx_routing_notifier(PCIBus *bus); >> void pci_device_set_intx_routing_notifier(PCIDevice *dev, >> PCIINTxRoutingNotifier >> notifier); >> +void pci_device_pre_reset(PCIBus *bus, PCIDevice *d, void *opaque); >> +void pci_device_post_reset(PCIBus *bus, PCIDevice *d, void *opaque); >> void pci_device_reset(PCIDevice *dev); >> >> PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, > > > . >