From b8f02af096b1fc9fd46680cbe55214e477eb76d3 Mon Sep 17 00:00:00 2001 From: Gavin Shan Date: Mon, 29 Sep 2014 10:16:24 -0600 Subject: vfio/pci: Restore MSIx message prior to enabling The MSIx vector table lives in device memory, which may be cleared as part of a backdoor device reset. This is the case on the IBM IPR HBA when the BIST is run on the device. When assigned to a QEMU guest, the guest driver does a pci_save_state(), issues a BIST, then does a pci_restore_state(). The BIST clears the MSIx vector table, but due to the way interrupts are configured the pci_restore_state() does not restore the vector table as expected. Eventually this results in an EEH error on Power platforms when the device attempts to signal an interrupt with the zero'd table entry. Fix the problem by restoring the host cached MSI message prior to enabling each vector. Reported-by: Wen Xiong Signed-off-by: Gavin Shan Signed-off-by: Alex Williamson --- drivers/vfio/pci/vfio_pci_intrs.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'drivers/vfio/pci') diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 9dd49c9839ac..553212f037c3 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -548,6 +549,20 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, return PTR_ERR(trigger); } + /* + * The MSIx vector table resides in device memory which may be cleared + * via backdoor resets. We don't allow direct access to the vector + * table so even if a userspace driver attempts to save/restore around + * such a reset it would be unsuccessful. To avoid this, restore the + * cached value of the message prior to enabling. + */ + if (msix) { + struct msi_msg msg; + + get_cached_msi_msg(irq, &msg); + write_msi_msg(irq, &msg); + } + ret = request_irq(irq, vfio_msihandler, 0, vdev->ctx[vector].name, trigger); if (ret) { -- cgit v1.2.3 From 93899a679fd6b2534b5c297d9316bae039ebcbe1 Mon Sep 17 00:00:00 2001 From: Alex Williamson Date: Mon, 29 Sep 2014 17:18:39 -0600 Subject: vfio-pci: Fix remove path locking Locking both the remove() and release() path results in a deadlock that should have been obvious. To fix this we can get and hold the vfio_device reference as we evaluate whether to do a bus/slot reset. This will automatically block any remove() calls, allowing us to remove the explict lock. Fixes 61d792562b53. Signed-off-by: Alex Williamson Cc: stable@vger.kernel.org [3.17] --- drivers/vfio/pci/vfio_pci.c | 136 +++++++++++++++++++------------------------- 1 file changed, 57 insertions(+), 79 deletions(-) (limited to 'drivers/vfio/pci') diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index f7825332a325..9558da3f06a0 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -876,15 +876,11 @@ static void vfio_pci_remove(struct pci_dev *pdev) { struct vfio_pci_device *vdev; - mutex_lock(&driver_lock); - vdev = vfio_del_group_dev(&pdev->dev); if (vdev) { iommu_group_put(pdev->dev.iommu_group); kfree(vdev); } - - mutex_unlock(&driver_lock); } static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, @@ -927,108 +923,90 @@ static struct pci_driver vfio_pci_driver = { .err_handler = &vfio_err_handlers, }; -/* - * Test whether a reset is necessary and possible. We mark devices as - * needs_reset when they are released, but don't have a function-local reset - * available. If any of these exist in the affected devices, we want to do - * a bus/slot reset. We also need all of the affected devices to be unused, - * so we abort if any device has a non-zero refcnt. driver_lock prevents a - * device from being opened during the scan or unbound from vfio-pci. - */ -static int vfio_pci_test_bus_reset(struct pci_dev *pdev, void *data) -{ - bool *needs_reset = data; - struct pci_driver *pci_drv = ACCESS_ONCE(pdev->driver); - int ret = -EBUSY; - - if (pci_drv == &vfio_pci_driver) { - struct vfio_device *device; - struct vfio_pci_device *vdev; - - device = vfio_device_get_from_dev(&pdev->dev); - if (!device) - return ret; - - vdev = vfio_device_data(device); - if (vdev) { - if (vdev->needs_reset) - *needs_reset = true; - - if (!vdev->refcnt) - ret = 0; - } - - vfio_device_put(device); - } - - /* - * TODO: vfio-core considers groups to be viable even if some devices - * are attached to known drivers, like pci-stub or pcieport. We can't - * freeze devices from being unbound to those drivers like we can - * here though, so it would be racy to test for them. We also can't - * use device_lock() to prevent changes as that would interfere with - * PCI-core taking device_lock during bus reset. For now, we require - * devices to be bound to vfio-pci to get a bus/slot reset on release. - */ - - return ret; -} +struct vfio_devices { + struct vfio_device **devices; + int cur_index; + int max_index; +}; -/* Clear needs_reset on all affected devices after successful bus/slot reset */ -static int vfio_pci_clear_needs_reset(struct pci_dev *pdev, void *data) +static int vfio_pci_get_devs(struct pci_dev *pdev, void *data) { + struct vfio_devices *devs = data; struct pci_driver *pci_drv = ACCESS_ONCE(pdev->driver); - if (pci_drv == &vfio_pci_driver) { - struct vfio_device *device; - struct vfio_pci_device *vdev; + if (pci_drv != &vfio_pci_driver) + return -EBUSY; - device = vfio_device_get_from_dev(&pdev->dev); - if (!device) - return 0; + if (devs->cur_index == devs->max_index) + return -ENOSPC; - vdev = vfio_device_data(device); - if (vdev) - vdev->needs_reset = false; - - vfio_device_put(device); - } + devs->devices[devs->cur_index] = vfio_device_get_from_dev(&pdev->dev); + if (!devs->devices[devs->cur_index]) + return -EINVAL; + devs->cur_index++; return 0; } /* * Attempt to do a bus/slot reset if there are devices affected by a reset for * this device that are needs_reset and all of the affected devices are unused - * (!refcnt). Callers of this function are required to hold driver_lock such - * that devices can not be unbound from vfio-pci or opened by a user while we - * test for and perform a bus/slot reset. + * (!refcnt). Callers are required to hold driver_lock when calling this to + * prevent device opens and concurrent bus reset attempts. We prevent device + * unbinds by acquiring and holding a reference to the vfio_device. + * + * NB: vfio-core considers a group to be viable even if some devices are + * bound to drivers like pci-stub or pcieport. Here we require all devices + * to be bound to vfio_pci since that's the only way we can be sure they + * stay put. */ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) { + struct vfio_devices devs = { .cur_index = 0 }; + int i = 0, ret = -EINVAL; bool needs_reset = false, slot = false; - int ret; + struct vfio_pci_device *tmp; if (!pci_probe_reset_slot(vdev->pdev->slot)) slot = true; else if (pci_probe_reset_bus(vdev->pdev->bus)) return; - if (vfio_pci_for_each_slot_or_bus(vdev->pdev, - vfio_pci_test_bus_reset, - &needs_reset, slot) || !needs_reset) + if (vfio_pci_for_each_slot_or_bus(vdev->pdev, vfio_pci_count_devs, + &i, slot) || !i) return; - if (slot) - ret = pci_try_reset_slot(vdev->pdev->slot); - else - ret = pci_try_reset_bus(vdev->pdev->bus); - - if (ret) + devs.max_index = i; + devs.devices = kcalloc(i, sizeof(struct vfio_device *), GFP_KERNEL); + if (!devs.devices) return; - vfio_pci_for_each_slot_or_bus(vdev->pdev, - vfio_pci_clear_needs_reset, NULL, slot); + if (vfio_pci_for_each_slot_or_bus(vdev->pdev, + vfio_pci_get_devs, &devs, slot)) + goto put_devs; + + for (i = 0; i < devs.cur_index; i++) { + tmp = vfio_device_data(devs.devices[i]); + if (tmp->needs_reset) + needs_reset = true; + if (tmp->refcnt) + goto put_devs; + } + + if (needs_reset) + ret = slot ? pci_try_reset_slot(vdev->pdev->slot) : + pci_try_reset_bus(vdev->pdev->bus); + +put_devs: + for (i = 0; i < devs.cur_index; i++) { + if (!ret) { + tmp = vfio_device_data(devs.devices[i]); + tmp->needs_reset = false; + } + vfio_device_put(devs.devices[i]); + } + + kfree(devs.devices); } static void __exit vfio_pci_cleanup(void) -- cgit v1.2.3