From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34747) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aC0tM-0000ib-Ki for qemu-devel@nongnu.org; Thu, 24 Dec 2015 03:04:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aC0tJ-0003TJ-0Y for qemu-devel@nongnu.org; Thu, 24 Dec 2015 03:04:56 -0500 Received: from [59.151.112.132] (port=42111 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aC0tI-0003Rx-BK for qemu-devel@nongnu.org; Thu, 24 Dec 2015 03:04:52 -0500 Message-ID: <567B7F44.1000404@cn.fujitsu.com> Date: Thu, 24 Dec 2015 13:14:44 +0800 From: Chen Fan MIME-Version: 1.0 References: <35d75210aef922bd049d93e0c8dbe13a2b5b1dd3.1447748073.git.chen.fan.fnst@cn.fujitsu.com> <20151223135443-mutt-send-email-mst@redhat.com> In-Reply-To: <20151223135443-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="windows-1252"; 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: "Michael S. Tsirkin" , Cao jin Cc: alex.williamson@redhat.com, qemu-devel@nongnu.org Hi mst, thanks for your suggestion. I have replied it in another alex's email. so we could discuss it in there.;) Thanks, Chen On 12/23/2015 08:00 PM, Michael S. Tsirkin wrote: > On Thu, Dec 17, 2015 at 09:41:51AM +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); >> } >> } >> > So this boils down to need to detect hot reset, > you don't actually call this for all resets. > Callback name should reflect this IMO. > > Also, why do we need two callbacks? > How about we have a single hot_reset callback, > and then if device has it, hot reset does not > invoke the regular reset? > > Finally, you discussed multiple resets triggering with many vfio devices > on the same bus. To solve this, will it help to have a bus-level > callback instead? > > >> 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); >> +} >> + >> +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, >> -- >> 1.9.3 >> >> > > > . >