On Mon, 2014-03-10 at 13:46 +0800, Gavin Shan wrote:
> The problem is specific to the case of BIST reset issued to IPR
> adapter on the guest side. The IPR driver calls pci_save_state(),
> issues BIST reset and then pci_restore_state(). The issued BIST
> cleans out MSIx table and pci_restore_state() doesn't restore
> the MSIx table as expected. Eventually, MSIx messages with all
> zeros are sent and causes EEH error on Power platform.
>
> The patch fixes it by writing MSIx message to MSIx table before
> reenabling the individual interrupts in the following path:
>
> qemu/hw/pci/msix.c::msix_table_mmio_write
> msix_handle_mask_update
> msix_fire_vector_notifier
> qemu/hw/misc/vfio.c::vfio_msix_vector_use
> vfio_msix_vector_do_use
>
> IOCTL Command VFIO_DEVICE_SET_IRQS to VFIO-PCI
> vfio/pci/vfio_pci.c::vfio_pci_ioctl
> vfio/pci/vfio_pci_intrs.c::vfio_pci_set_irqs_ioctl
> vfio_pci_set_msi_trigger
> vfio_msi_set_block
> vfio_msi_set_vector_signal
>
> Reported-by: Wen Xiong <wenxiong@xxxxxxxxxx>
> Signed-off-by: Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 2103576..83e0638 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -17,6 +17,7 @@
> #include <linux/interrupt.h>
> #include <linux/eventfd.h>
> #include <linux/pci.h>
> +#include <linux/msi.h>
> #include <linux/file.h>
> #include <linux/poll.h>
> #include <linux/vfio.h>
> @@ -517,6 +518,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> struct pci_dev *pdev = vdev->pdev;
> int irq = msix ? vdev->msix[vector].vector : pdev->irq + vector;
> char *name = msix ? "vfio-msix" : "vfio-msi";
> + struct msi_msg msg;
> struct eventfd_ctx *trigger;
> int ret;
>
> @@ -544,6 +546,15 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> return PTR_ERR(trigger);
> }
>
> + /* We possiblly lose the MSI/MSIx message in some cases, one
> + * of which is pci_save_state(), BIST reset and pci_restore_state()
> + * for IPR adapter. The BIST reset cleans out MSIx table and we
> + * don't have chance to restore it. Here, we have the trick to
> + * restore it before enabling individual interrupts. For MSI messages,
> + * it's harmless to write them back.
> + */
> + 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) {
Hi Gavin,
The commit log and patch comment are still pretty confusing. The
problem is not necessarily specific to the IPR card, it's just the only
instance we know of it, otherwise we'd make a quirk that only gets
invoked for this device. We're also not only making the change for the
call path listed, but for any callers of the function. I also got a bit
nervous that while it seems like it should be safe to re-write the
cached MSI message before enabling it, our argument for doing so is that
the MSIx vector table lives in device memory. This is not true for MSI
and modifying MSI modifies all the vectors, so why are we doing it? The
result is the patch below. Please let me know if you approve and I'll
apply it after further testing. Thanks,
Alex
Author: Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx>
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 <wenxiong@xxxxxxxxxx>
Signed-off-by: Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 2103576..3058b2e 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -17,6 +17,7 @@
#include <linux/interrupt.h>
#include <linux/eventfd.h>
#include <linux/pci.h>
+#include <linux/msi.h>
#include <linux/file.h>
#include <linux/poll.h>
#include <linux/vfio.h>
@@ -544,6 +545,19 @@ 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) {
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
