[Qemu-devel] [RFC PATCH v3 0/2] Fix MSIx lost after PE reset

[Qemu-devel] [RFC PATCH v3 0/2] Fix MSIx lost after PE reset

[Gavin Shan]

The 2 patches fix MSIx lost after PE reset. Otherwise, the MSIx
entries can't be restored successfully after PE reset and the
EEH recovery fails on broadcom tg3 adapter (as tested) in guest.

The reset path (restarting machine) has the same problem. Also,
the MSI has the same problem as well.

Note: The patchset "EEH support for guest" isn't merged yet, those
      2 patches are based on Alex Graf's "ppc-next" branch + the
      patchset supporting EEH for guest, which can be checked out
      from below link:

      git@github.com:gwshan/qemu.git (branch: eeh)

*** SUBJECT HERE ***

*** BLURB HERE ***

Gavin Shan (2):
  VFIO: Drop vfio_container_do_ioctl()
  VFIO: Clear stale interrupt vectors during reset

 hw/misc/vfio.c | 99 +++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 29 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [RFC PATCH v3 1/2] VFIO: Drop vfio_container_do_ioctl()

[Gavin Shan]

The patch drops vfio_container_do_ioctl() and merges its logic to
parent function call vfio_container_ioctl() so that the subsequent
patches can reused the found VFIO group in vfio_container_ioctl().

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/misc/vfio.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 7d5f1bb..1a3e7eb 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -4419,8 +4419,8 @@ static void register_vfio_pci_dev_type(void)
 
 type_init(register_vfio_pci_dev_type)
 
-static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid,
-                                   int req, void *param)
+int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
+                         int req, void *param)
 {
     VFIOGroup *group;
     VFIOContainer *container;
@@ -4433,22 +4433,11 @@ static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid,
     }
 
     container = group->container;
-    if (group->container) {
-        ret = ioctl(container->fd, req, param);
-        if (ret < 0) {
-            error_report("vfio: failed to ioctl container: ret=%d, %s",
-                         ret, strerror(errno));
-        }
+    if (!container) {
+        error_report("vfio: no container for group %d\n", groupid);
+        goto out;
     }
 
-    vfio_put_group(group);
-
-    return ret;
-}
-
-int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
-                         int req, void *param)
-{
     /* We allow only certain ioctls to the container */
     switch (req) {
     case VFIO_CHECK_EXTENSION:
@@ -4458,8 +4447,16 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
     default:
         /* Return an error on unknown requests */
         error_report("vfio: unsupported ioctl %X", req);
-        return -1;
+        goto out;
     }
 
-    return vfio_container_do_ioctl(as, groupid, req, param);
+    ret = ioctl(container->fd, req, param);
+    if (ret < 0) {
+        error_report("vfio: failed to ioctl container: ret=%d, %s",
+                     ret, strerror(errno));
+    }
+
+out:
+    vfio_put_group(group);
+    return ret;
 }
-- 
1.8.3.2

[Qemu-devel] [RFC PATCH v3 2/2] VFIO: Clear stale interrupt vectors during reset

[Gavin Shan]

As to one particular VFIO device, there are 4 kinds of resets as
follows: FLR (Function Level Reset), secondary bus reset, machine
reset, EEH reset. For the first 2 cases, PCI core helps keeping
consistent interrupt vectors. For the later 2 cases, we have to
drop the stale interrupt (MSI or MSIx) vectors. Otherwise, we
will potentially lose chance to restore MSI or MSIx vectors when
reenabling MSI or MSIx interrupts after reset.

The patch clears stale MSI or MSIx vectors before machine/EEH
reset so that MSI or MSIx vectors could be restored properly
after EEH PE reset.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/misc/vfio.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 1a3e7eb..026da77 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -2724,6 +2724,22 @@ static void vfio_disable_interrupts(VFIODevice *vdev)
     }
 }
 
+static void vfio_disable_and_reset_interrupts(VFIODevice *vdev)
+{
+    int interrupt = vdev->interrupt;
+
+    vfio_disable_interrupts(vdev);
+
+    switch (interrupt) {
+    case VFIO_INT_MSI:
+        msi_reset(&vdev->pdev);
+        break;
+    case VFIO_INT_MSIX:
+        msix_reset(&vdev->pdev);
+        break;
+    }
+}
+
 static int vfio_setup_msi(VFIODevice *vdev, int pos)
 {
     uint16_t ctrl;
@@ -3298,12 +3314,16 @@ static int vfio_add_capabilities(VFIODevice *vdev)
     return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
 }
 
-static void vfio_pci_pre_reset(VFIODevice *vdev)
+static void vfio_pci_pre_reset(VFIODevice *vdev, bool reset_interrupt)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint16_t cmd;
 
-    vfio_disable_interrupts(vdev);
+    if (reset_interrupt) {
+        vfio_disable_and_reset_interrupts(vdev);
+    } else {
+        vfio_disable_interrupts(vdev);
+    }
 
     /* Make sure the device is in D0 */
     if (vdev->pm_cap) {
@@ -3347,7 +3367,8 @@ static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
             host1->slot == host2->slot && host1->function == host2->function);
 }
 
-static int vfio_pci_hot_reset(VFIODevice *vdev, bool single)
+static int vfio_pci_hot_reset(VFIODevice *vdev,
+                              bool single, bool reset_interrupt)
 {
     VFIOGroup *group;
     struct vfio_pci_hot_reset_info *info;
@@ -3361,7 +3382,7 @@ static int vfio_pci_hot_reset(VFIODevice *vdev, bool single)
             vdev->host.bus, vdev->host.slot, vdev->host.function,
             single ? "one" : "multi");
 
-    vfio_pci_pre_reset(vdev);
+    vfio_pci_pre_reset(vdev, reset_interrupt);
     vdev->needs_reset = false;
 
     info = g_malloc0(sizeof(*info));
@@ -3438,7 +3459,7 @@ static int vfio_pci_hot_reset(VFIODevice *vdev, bool single)
                     ret = -EINVAL;
                     goto out_single;
                 }
-                vfio_pci_pre_reset(tmp);
+                vfio_pci_pre_reset(tmp, reset_interrupt);
                 tmp->needs_reset = false;
                 multi = true;
                 break;
@@ -3541,12 +3562,12 @@ out_single:
  */
 static int vfio_pci_hot_reset_one(VFIODevice *vdev)
 {
-    return vfio_pci_hot_reset(vdev, true);
+    return vfio_pci_hot_reset(vdev, true, false);
 }
 
-static int vfio_pci_hot_reset_multi(VFIODevice *vdev)
+static int vfio_pci_hot_reset_multi_and_interrupts(VFIODevice *vdev)
 {
-    return vfio_pci_hot_reset(vdev, false);
+    return vfio_pci_hot_reset(vdev, false, true);
 }
 
 static void vfio_pci_reset_handler(void *opaque)
@@ -3565,7 +3586,7 @@ static void vfio_pci_reset_handler(void *opaque)
     QLIST_FOREACH(group, &group_list, next) {
         QLIST_FOREACH(vdev, &group->device_list, next) {
             if (vdev->needs_reset) {
-                vfio_pci_hot_reset_multi(vdev);
+                vfio_pci_hot_reset_multi_and_interrupts(vdev);
             }
         }
     }
@@ -4334,6 +4355,11 @@ static void vfio_exitfn(PCIDevice *pdev)
     vfio_put_group(group);
 }
 
+/*
+ * The function is invoked when issuing FLR or secondary bus reset.
+ * In the case, the PCI core should drop stale MSI or MSIx vectors
+ * if applicable. So we needn't do that by ourselves.
+ */
 static void vfio_pci_reset(DeviceState *dev)
 {
     PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
@@ -4342,7 +4368,7 @@ static void vfio_pci_reset(DeviceState *dev)
     DPRINTF("%s(%04x:%02x:%02x.%x)\n", __func__, vdev->host.domain,
             vdev->host.bus, vdev->host.slot, vdev->host.function);
 
-    vfio_pci_pre_reset(vdev);
+    vfio_pci_pre_reset(vdev, false);
 
     if (vdev->reset_works && (vdev->has_flr || !vdev->has_pm_reset) &&
         !ioctl(vdev->fd, VFIO_DEVICE_RESET)) {
@@ -4442,8 +4468,26 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
     switch (req) {
     case VFIO_CHECK_EXTENSION:
     case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
-    case VFIO_EEH_PE_OP:
         break;
+    case VFIO_EEH_PE_OP: {
+        VFIODevice *vdev;
+        struct vfio_eeh_pe_op *arg = (struct vfio_eeh_pe_op *)param;
+
+        switch (arg->op) {
+        case VFIO_EEH_PE_RESET_HOT:
+        case VFIO_EEH_PE_RESET_FUNDAMENTAL:
+            /*
+             * The stale MSI or MSIx information, including controlling
+             * information and vectors should be dropped so that they
+             * can be restored properly after reset.
+             */
+            QLIST_FOREACH(vdev, &group->device_list, next) {
+                vfio_disable_and_reset_interrupts(vdev);
+            }
+        }
+
+        break;
+    }
     default:
         /* Return an error on unknown requests */
         error_report("vfio: unsupported ioctl %X", req);
-- 
1.8.3.2

Re: [Qemu-devel] [RFC PATCH v3 0/2] Fix MSIx lost after PE reset

[Gavin Shan]

On Mon, Sep 08, 2014 at 02:52:38PM +1000, Gavin Shan wrote:
>The 2 patches fix MSIx lost after PE reset. Otherwise, the MSIx
>entries can't be restored successfully after PE reset and the
>EEH recovery fails on broadcom tg3 adapter (as tested) in guest.
>
>The reset path (restarting machine) has the same problem. Also,
>the MSI has the same problem as well.
>
>Note: The patchset "EEH support for guest" isn't merged yet, those
>      2 patches are based on Alex Graf's "ppc-next" branch + the
>      patchset supporting EEH for guest, which can be checked out
>      from below link:
>
>      git@github.com:gwshan/qemu.git (branch: eeh)
>
>*** SUBJECT HERE ***
>
>*** BLURB HERE ***
>

Hrm, I forgot removing above 2 confusing lines, please ignore them.

Thanks,
Gavin

>Gavin Shan (2):
>  VFIO: Drop vfio_container_do_ioctl()
>  VFIO: Clear stale interrupt vectors during reset
>
> hw/misc/vfio.c | 99 +++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 70 insertions(+), 29 deletions(-)
>
>-- 
>1.8.3.2
>