[Qemu-devel] [PATCH 0/3] vfio-pci: support recovery of AER non fatal error

[Qemu-devel] [PATCH 0/3] vfio-pci: support recovery of AER non fatal error

[Cao jin]

This is nearly new design of the feature, so re-number the verion from 0.

About The test:
Hardware problem(unsteady) still occurs like before. The test server is in
another country spot A, and my contact of the country located spot B, so
it is not quite convenient to find help(plug cable, or check the hardware).
So, my NIC(has 2 functions) still just has func1 connected to gateway.
If there is other people who has the hardware could test the patches, that
would be great help.


Basically, there are two phenomenon of unsteady hardware:
1. Start vm, the hardware emit fatal error itself before I did anything,
   cause vm stop.
2. Start vm, assign IP to func1, then ping the gateway, it will show
   "Destination Host Unreachable" after dozens of or hundreds of successful
   ping, and guest dmesg shows nothing abnormal.  I think this phenomenon is
   the *strong evidence* of saying unsteady hardware, I speculate that
   the cable has problem.

   on the opposite, I also saw perfect result 2 times in my numerous tests,
   which just assign func1 while func0 has no user. It can ping several housrs(
   more than 15000 times ping) withtout any problem, during the period, inject
   non fatal error to func0 & func1, error recovery is very good.

   So, most of time, I must do the test quickly before the hardware goes crazy,
   until get what I expected.


Test:
scenario 1: assign func1 to vm while func0 has no user.
scenario 2: assign both functions to 1 vm, with the same topology as host.
scenario 3: assign both functions to 1 vm, under different bus.
scenario 4: assign each function to a separate vm.

the steps is: assign IP to func1, ping the gateway, inject non fatal error to
both functions, see if func1 still can ping after recovery.

Although we don't have cable for func0, but in the test like scenario 4,
inject to func0, it doesn't affect func1's recovery, so I think it can prove
that one function's recovery doesn't affect another.


Extra info FYI:
1. During the test, some debug lines are added in vfio_err_notifier_handler,
   read the uncor status register in this function when fatal error occured,
   it shows all F's every time.
2. Based on the v10 patch & the corresponding kernel part, modified as
   comments: revert the eventfd handling(don't signal uncor status), and
   guest link reset will induce the host link reset. The test result shows:
   non fatal error recovery is good; fatal error recovery has same result
   with what Alex find before(guest kernel crash), because guest device
   driver's error_detected() access the MMIO registers, get all F's.


Cao jin (3):
  pcie aer: verify if AER functionality is available
  vfio pci: new function to init AER capability
  vfio-pci: process non fatal error of AER

 hw/pci/pcie_aer.c          |  28 +++++++
 hw/vfio/pci.c              | 180 +++++++++++++++++++++++++++++++++++++++++++--
 hw/vfio/pci.h              |   3 +
 linux-headers/linux/vfio.h |   1 +
 4 files changed, 207 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] pcie aer: verify if AER functionality is available

[Cao jin]

For devices which support AER, verify it can work or not in the system:
1. AER capable device is a PCIe device, it can't be plugged into PCI bus
2. If root port doesn't support AER, then there is no need to expose the
   AER capability

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pcie_aer.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index daf1f65..a2e9818 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -100,6 +100,34 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
 int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
                   uint16_t size, Error **errp)
 {
+    PCIDevice *parent_dev;
+    uint8_t type;
+    uint8_t parent_type;
+
+    /* Topology test: see if there is need to expose AER cap */
+    type = pcie_cap_get_type(dev);
+    parent_dev = pci_bridge_get_device(dev->bus);
+    while (parent_dev) {
+        parent_type = pcie_cap_get_type(parent_dev);
+
+        if (type == PCI_EXP_TYPE_ENDPOINT &&
+                (parent_type != PCI_EXP_TYPE_ROOT_PORT &&
+                 parent_type != PCI_EXP_TYPE_DOWNSTREAM)) {
+            error_setg(errp, "Parent device is not a PCIe component");
+            return -ENOTSUP;
+        }
+
+        if (parent_type == PCI_EXP_TYPE_ROOT_PORT) {
+            if (!parent_dev->exp.aer_cap)
+            {
+                error_setg(errp, "Root port does not support AER");
+                return -ENOTSUP;
+            }
+        }
+
+        parent_dev = pci_bridge_get_device(parent_dev->bus);
+    }
+
     pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, cap_ver,
                         offset, size);
     dev->exp.aer_cap = offset;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] vfio pci: new function to init AER capability

[Cao jin]

Enable AER opportunistically.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 41 ++++++++++++++++++++++++++++++++++++-----
 hw/vfio/pci.h |  1 +
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 332f41d..3d0d005 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1855,18 +1855,42 @@ out:
     return 0;
 }
 
-static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
+static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
+                          int pos, uint16_t size, Error **errp)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint32_t errcap;
+
+    errcap = vfio_pci_read_config(pdev, pos + PCI_ERR_CAP, 4);
+    /*
+     * The ability to record multiple headers is depending on
+     * the state of the Multiple Header Recording Capable bit and
+     * enabled by the Multiple Header Recording Enable bit.
+     */
+    if ((errcap & PCI_ERR_CAP_MHRC) &&
+        (errcap & PCI_ERR_CAP_MHRE)) {
+        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
+    } else {
+        pdev->exp.aer_log.log_max = 0;
+    }
+
+    pcie_cap_deverr_init(pdev);
+    return pcie_aer_init(pdev, cap_ver, pos, size, errp);
+}
+
+static int vfio_add_ext_cap(VFIOPCIDevice *vdev, Error **errp)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint32_t header;
     uint16_t cap_id, next, size;
     uint8_t cap_ver;
     uint8_t *config;
+    int ret = 0;
 
     /* Only add extended caps if we have them and the guest can see them */
     if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) ||
         !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
-        return;
+        return 0;
     }
 
     /*
@@ -1915,6 +1939,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
                                    PCI_EXT_CAP_NEXT_MASK);
 
         switch (cap_id) {
+        case PCI_EXT_CAP_ID_ERR:
+            ret = vfio_setup_aer(vdev, cap_ver, next, size, errp);
+            break;
         case PCI_EXT_CAP_ID_SRIOV: /* Read-only VF BARs confuse OVMF */
         case PCI_EXT_CAP_ID_ARI: /* XXX Needs next function virtualization */
             trace_vfio_add_ext_cap_dropped(vdev->vbasedev.name, cap_id, next);
@@ -1923,6 +1950,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
             pcie_add_capability(pdev, cap_id, cap_ver, next, size);
         }
 
+        if (ret) {
+            goto out;
+        }
     }
 
     /* Cleanup chain head ID if necessary */
@@ -1930,8 +1960,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
         pci_set_word(pdev->config + PCI_CONFIG_SPACE_SIZE, 0);
     }
 
+out:
     g_free(config);
-    return;
+    return ret;
 }
 
 static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
@@ -1949,8 +1980,8 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
         return ret;
     }
 
-    vfio_add_ext_cap(vdev);
-    return 0;
+    ret = vfio_add_ext_cap(vdev, errp);
+    return ret;
 }
 
 static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index a8366bb..34e8b04 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -15,6 +15,7 @@
 #include "qemu-common.h"
 #include "exec/memory.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
 #include "hw/vfio/vfio-common.h"
 #include "qemu/event_notifier.h"
 #include "qemu/queue.h"
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] vfio-pci: process non fatal error of AER

[Cao jin]

Make use of the non fatal error eventfd that the kernel module provide
to process the AER non fatal error. Fatal error still goes into the
legacy way which results in VM stop.

Register the handler, wait for notification. Construct aer message and
pass it to root port on notification. Root port will trigger an interrupt
to signal guest, then, the guest driver will do the recovery.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c              | 139 +++++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/pci.h              |   2 +
 linux-headers/linux/vfio.h |   1 +
 3 files changed, 142 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3d0d005..55c6e05 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2422,6 +2422,21 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
                      "Could not enable error recovery for the device",
                      vbasedev->name);
     }
+
+    irq_info.index = VFIO_PCI_NON_FATAL_ERR_IRQ_INDEX;
+    irq_info.count = 0; /* clear */
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
+    if (ret) {
+        /* This can fail for an old kernel or legacy PCI dev */
+        trace_vfio_populate_device_get_irq_info_failure();
+    } else if (irq_info.count == 1) {
+        vdev->pci_aer_non_fatal = true;
+    } else {
+        error_report(WARN_PREFIX
+                     "Couldn't enable non fatal error recovery for the device",
+                     vbasedev->name);
+    }
+
 }
 
 static void vfio_put_device(VFIOPCIDevice *vdev)
@@ -2432,6 +2447,128 @@ static void vfio_put_device(VFIOPCIDevice *vdev)
     vfio_put_base_device(&vdev->vbasedev);
 }
 
+static void vfio_non_fatal_err_notifier_handler(void *opaque)
+{
+    VFIOPCIDevice *vdev = opaque;
+    PCIDevice *dev = &vdev->pdev;
+    PCIEAERMsg msg = {
+        .severity = PCI_ERR_ROOT_CMD_NONFATAL_EN,
+        .source_id = (pci_bus_num(dev->bus) << 8) | dev->devfn,
+    };
+
+    if (!event_notifier_test_and_clear(&vdev->non_fatal_err_notifier)) {
+        return;
+    }
+
+    /* Populate the aer msg and send it to root port */
+    if (dev->exp.aer_cap) {
+        uint8_t *aer_cap = dev->config + dev->exp.aer_cap;
+        uint32_t uncor_status;
+        bool isfatal;
+
+        uncor_status = vfio_pci_read_config(dev,
+                            dev->exp.aer_cap + PCI_ERR_UNCOR_STATUS, 4);
+        if (!uncor_status) {
+            return;
+        }
+
+        isfatal = uncor_status & pci_get_long(aer_cap + PCI_ERR_UNCOR_SEVER);
+        if (isfatal) {
+            goto stop;
+        }
+
+        error_report("%s sending non fatal event to root port. uncor status = "
+                     "0x%"PRIx32, vdev->vbasedev.name, uncor_status);
+        pcie_aer_msg(dev, &msg);
+        return;
+    }
+
+stop:
+    /* Terminate the guest in case of fatal error */
+    error_report("%s(%s) fatal error detected. Please collect any data"
+            " possible and then kill the guest", __func__, vdev->vbasedev.name);
+    vm_stop(RUN_STATE_INTERNAL_ERROR);
+}
+
+/*
+ * Register non fatal error notifier for devices supporting error recovery.
+ * If we encounter a failure in this function, we report an error
+ * and continue after disabling error recovery support for the device.
+ */
+static void vfio_register_non_fatal_err_notifier(VFIOPCIDevice *vdev)
+{
+    int ret;
+    int argsz;
+    struct vfio_irq_set *irq_set;
+    int32_t *pfd;
+
+    if (!vdev->pci_aer_non_fatal) {
+        return;
+    }
+
+    if (event_notifier_init(&vdev->non_fatal_err_notifier, 0)) {
+        error_report("vfio: Unable to init event notifier for non-fatal error detection");
+        vdev->pci_aer_non_fatal = false;
+        return;
+    }
+
+    argsz = sizeof(*irq_set) + sizeof(*pfd);
+
+    irq_set = g_malloc0(argsz);
+    irq_set->argsz = argsz;
+    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+                     VFIO_IRQ_SET_ACTION_TRIGGER;
+    irq_set->index = VFIO_PCI_NON_FATAL_ERR_IRQ_INDEX;
+    irq_set->start = 0;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+
+    *pfd = event_notifier_get_fd(&vdev->non_fatal_err_notifier);
+    qemu_set_fd_handler(*pfd, vfio_non_fatal_err_notifier_handler, NULL, vdev);
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
+    if (ret) {
+        error_report("vfio: Failed to set up non-fatal error notification");
+        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
+        event_notifier_cleanup(&vdev->non_fatal_err_notifier);
+        vdev->pci_aer_non_fatal = false;
+    }
+    g_free(irq_set);
+}
+
+static void vfio_unregister_non_fatal_err_notifier(VFIOPCIDevice *vdev)
+{
+    int argsz;
+    struct vfio_irq_set *irq_set;
+    int32_t *pfd;
+    int ret;
+
+    if (!vdev->pci_aer_non_fatal) {
+        return;
+    }
+
+    argsz = sizeof(*irq_set) + sizeof(*pfd);
+
+    irq_set = g_malloc0(argsz);
+    irq_set->argsz = argsz;
+    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+                     VFIO_IRQ_SET_ACTION_TRIGGER;
+    irq_set->index = VFIO_PCI_NON_FATAL_ERR_IRQ_INDEX;
+    irq_set->start = 0;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+    *pfd = -1;
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
+    if (ret) {
+        error_report("vfio: Failed to de-assign error fd: %m");
+    }
+    g_free(irq_set);
+    qemu_set_fd_handler(event_notifier_get_fd(&vdev->non_fatal_err_notifier),
+                        NULL, NULL, vdev);
+    event_notifier_cleanup(&vdev->non_fatal_err_notifier);
+}
+
 static void vfio_err_notifier_handler(void *opaque)
 {
     VFIOPCIDevice *vdev = opaque;
@@ -2860,6 +2997,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         }
     }
 
+    vfio_register_non_fatal_err_notifier(vdev);
     vfio_register_err_notifier(vdev);
     vfio_register_req_notifier(vdev);
     vfio_setup_resetfn_quirk(vdev);
@@ -2900,6 +3038,7 @@ static void vfio_exitfn(PCIDevice *pdev)
 
     vfio_unregister_req_notifier(vdev);
     vfio_unregister_err_notifier(vdev);
+    vfio_unregister_non_fatal_err_notifier(vdev);
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
     if (vdev->intx.mmap_timer) {
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 34e8b04..9d83349 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -119,6 +119,7 @@ typedef struct VFIOPCIDevice {
     void *igd_opregion;
     PCIHostDeviceAddress host;
     EventNotifier err_notifier;
+    EventNotifier non_fatal_err_notifier;
     EventNotifier req_notifier;
     int (*resetfn)(struct VFIOPCIDevice *);
     uint32_t vendor_id;
@@ -137,6 +138,7 @@ typedef struct VFIOPCIDevice {
     uint32_t igd_gms;
     uint8_t pm_cap;
     bool pci_aer;
+    bool pci_aer_non_fatal;
     bool req_enabled;
     bool has_flr;
     bool has_pm_reset;
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 759b850..c8f699c 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -433,6 +433,7 @@ enum {
 	VFIO_PCI_MSIX_IRQ_INDEX,
 	VFIO_PCI_ERR_IRQ_INDEX,
 	VFIO_PCI_REQ_IRQ_INDEX,
+	VFIO_PCI_NON_FATAL_ERR_IRQ_INDEX,
 	VFIO_PCI_NUM_IRQS
 };
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] vfio-pci: support recovery of AER non fatal error

[Cao jin]

ping

On 02/27/2017 03:30 PM, Cao jin wrote:
> This is nearly new design of the feature, so re-number the verion from 0.
> 
> About The test:
> Hardware problem(unsteady) still occurs like before. The test server is in
> another country spot A, and my contact of the country located spot B, so
> it is not quite convenient to find help(plug cable, or check the hardware).
> So, my NIC(has 2 functions) still just has func1 connected to gateway.
> If there is other people who has the hardware could test the patches, that
> would be great help.
> 
> 
> Basically, there are two phenomenon of unsteady hardware:
> 1. Start vm, the hardware emit fatal error itself before I did anything,
>    cause vm stop.
> 2. Start vm, assign IP to func1, then ping the gateway, it will show
>    "Destination Host Unreachable" after dozens of or hundreds of successful
>    ping, and guest dmesg shows nothing abnormal.  I think this phenomenon is
>    the *strong evidence* of saying unsteady hardware, I speculate that
>    the cable has problem.
> 
>    on the opposite, I also saw perfect result 2 times in my numerous tests,
>    which just assign func1 while func0 has no user. It can ping several housrs(
>    more than 15000 times ping) withtout any problem, during the period, inject
>    non fatal error to func0 & func1, error recovery is very good.
> 
>    So, most of time, I must do the test quickly before the hardware goes crazy,
>    until get what I expected.
> 
> 
> Test:
> scenario 1: assign func1 to vm while func0 has no user.
> scenario 2: assign both functions to 1 vm, with the same topology as host.
> scenario 3: assign both functions to 1 vm, under different bus.
> scenario 4: assign each function to a separate vm.
> 
> the steps is: assign IP to func1, ping the gateway, inject non fatal error to
> both functions, see if func1 still can ping after recovery.
> 
> Although we don't have cable for func0, but in the test like scenario 4,
> inject to func0, it doesn't affect func1's recovery, so I think it can prove
> that one function's recovery doesn't affect another.
> 
> 
> Extra info FYI:
> 1. During the test, some debug lines are added in vfio_err_notifier_handler,
>    read the uncor status register in this function when fatal error occured,
>    it shows all F's every time.
> 2. Based on the v10 patch & the corresponding kernel part, modified as
>    comments: revert the eventfd handling(don't signal uncor status), and
>    guest link reset will induce the host link reset. The test result shows:
>    non fatal error recovery is good; fatal error recovery has same result
>    with what Alex find before(guest kernel crash), because guest device
>    driver's error_detected() access the MMIO registers, get all F's.
> 
> 
> Cao jin (3):
>   pcie aer: verify if AER functionality is available
>   vfio pci: new function to init AER capability
>   vfio-pci: process non fatal error of AER
> 
>  hw/pci/pcie_aer.c          |  28 +++++++
>  hw/vfio/pci.c              | 180 +++++++++++++++++++++++++++++++++++++++++++--
>  hw/vfio/pci.h              |   3 +
>  linux-headers/linux/vfio.h |   1 +
>  4 files changed, 207 insertions(+), 5 deletions(-)
> 

-- 
Sincerely,
Cao jin