[Qemu-devel] [RFC v7 00/11] vfio-pci: pass the aer error to guest

[Qemu-devel] [RFC v7 00/11] vfio-pci: pass the aer error to guest

[Chen Fan]

For now, for vfio pci passthough devices when qemu receives
an error from host aer report, there just terminate the guest,
but usually user want to know what error occurred but stop the
guest, so this patches add aer capability support for vfio device,
and pass the error to guest, and have guest driver to recover
from the error.
and turning on SERR# for error forwording in bridge control register
patch in seabios has been merged.

v6-v7:
   1. add has_bus_reset to support reset host bus.

v5-v6:
   1. add secondary bus callbacks to reset host bus.

v3-v4:
   1. add 'x-aer' for user to off aer capability.
   2. refactor vfio device to parse extended capabilities.

v2-v3:
   1. refactor vfio device to parse extended capability.
   2. add global property for piix4 to disable vfio aer cap.

v1-v2:
   1. turn on SERR# for bridge control register in firmware.
   2. initilize aer capability for vfio device.
   3. fix some trivial bug.


Chen Fan (11):
  vfio: extract vfio_get_hot_reset_info as a single function
  vfio: squeeze out vfio_pci_do_hot_reset for support bus reset
  qdev: add bus reset_notifiers callbacks for host bus reset
  vfio: add check host bus reset is support or not
  vfio: do hot bus reset when do virtual secondary bus reset
  vfio: add pcie extanded capability support
  aer: impove pcie_aer_init to support vfio device
  vfio: add aer support for vfio device
  pcie_aer: expose pcie_aer_msg() interface
  vfio-pci: pass the aer error to guest
  vfio: add 'aer' property to expose aercap

 hw/i386/pc_q35.c                   |   4 +
 hw/pci-bridge/ioh3420.c            |   2 +-
 hw/pci-bridge/xio3130_downstream.c |   2 +-
 hw/pci-bridge/xio3130_upstream.c   |   2 +-
 hw/pci/pci.c                       |   6 +
 hw/pci/pci_bridge.c                |   3 +
 hw/pci/pcie_aer.c                  |   6 +-
 hw/vfio/pci.c                      | 481 ++++++++++++++++++++++++++++++++-----
 include/hw/compat.h                |   8 +
 include/hw/pci/pci.h               |   2 +
 include/hw/pci/pci_bus.h           |   2 +
 include/hw/pci/pcie_aer.h          |   3 +-
 12 files changed, 459 insertions(+), 62 deletions(-)

-- 
1.9.3

[Qemu-devel] [RFC v7 01/11] vfio: extract vfio_get_hot_reset_info as a single function

[Chen Fan]

the function is used to get affected devices by bus reset.
so here extract it, and can used for aer soon.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 63 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e0e339a..fe4963f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2641,6 +2641,48 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
     }
 }
 
+/*
+ * return negative with errno, return 0 on success.
+ * if success, the point of ret_info fill with the affected device reset info.
+ *
+ */
+static int vfio_get_hot_reset_info(VFIOPCIDevice *vdev,
+                                   struct vfio_pci_hot_reset_info **ret_info)
+{
+    struct vfio_pci_hot_reset_info *info;
+    int ret, count;
+
+    *ret_info = NULL;
+
+    info = g_malloc0(sizeof(*info));
+    info->argsz = sizeof(*info);
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
+    if (ret && errno != ENOSPC) {
+        ret = -errno;
+        goto error;
+    }
+
+    count = info->count;
+
+    info = g_realloc(info, sizeof(*info) +
+                     (count * sizeof(struct vfio_pci_dependent_device)));
+    info->argsz = sizeof(*info) +
+                  (count * sizeof(struct vfio_pci_dependent_device));
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
+    if (ret) {
+        ret = -errno;
+        goto error;
+    }
+
+    *ret_info = info;
+
+    ret = 0;
+error:
+    return ret;
+}
+
 static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
 {
     PCIDevice *pdev = &vdev->pdev;
@@ -2792,32 +2834,19 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
     vfio_pci_pre_reset(vdev);
     vdev->vbasedev.needs_reset = false;
 
-    info = g_malloc0(sizeof(*info));
-    info->argsz = sizeof(*info);
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
-    if (ret && errno != ENOSPC) {
-        ret = -errno;
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret) {
         if (!vdev->has_pm_reset) {
             error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
                          "no available reset mechanism.", vdev->host.domain,
                          vdev->host.bus, vdev->host.slot, vdev->host.function);
+        } else {
+            error_report("vfio: hot reset info failed: %m");
         }
         goto out_single;
     }
 
-    count = info->count;
-    info = g_realloc(info, sizeof(*info) + (count * sizeof(*devices)));
-    info->argsz = sizeof(*info) + (count * sizeof(*devices));
     devices = &info->devices[0];
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
-    if (ret) {
-        ret = -errno;
-        error_report("vfio: hot reset info failed: %m");
-        goto out_single;
-    }
-
     trace_vfio_pci_hot_reset_has_dep_devices(vdev->vbasedev.name);
 
     /* Verify that we have all the groups required */
-- 
1.9.3

[Qemu-devel] [RFC v7 02/11] vfio: squeeze out vfio_pci_do_hot_reset for support bus reset

[Chen Fan]

squeeze out vfio_pci_do_hot_reset to do host bus reset when AER recovery.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 75 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fe4963f..037a2c6 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2683,6 +2683,48 @@ error:
     return ret;
 }
 
+static int vfio_pci_do_hot_reset(VFIOPCIDevice *vdev,
+                                 struct vfio_pci_hot_reset_info *info)
+{
+    VFIOGroup *group;
+    struct vfio_pci_hot_reset *reset;
+    int32_t *fds;
+    int ret, i, count;
+    struct vfio_pci_dependent_device *devices;
+
+    /* Determine how many group fds need to be passed */
+    count = 0;
+    devices = &info->devices[0];
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+        for (i = 0; i < info->count; i++) {
+            if (group->groupid == devices[i].group_id) {
+                count++;
+                break;
+            }
+        }
+    }
+
+    reset = g_malloc0(sizeof(*reset) + (count * sizeof(*fds)));
+    reset->argsz = sizeof(*reset) + (count * sizeof(*fds));
+    fds = &reset->group_fds[0];
+
+    /* Fill in group fds */
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+        for (i = 0; i < info->count; i++) {
+            if (group->groupid == devices[i].group_id) {
+                fds[reset->count++] = group->fd;
+                break;
+            }
+        }
+    }
+
+    /* Bus reset! */
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
+    g_free(reset);
+
+    return ret;
+}
+
 static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
 {
     PCIDevice *pdev = &vdev->pdev;
@@ -2824,9 +2866,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
     VFIOGroup *group;
     struct vfio_pci_hot_reset_info *info;
     struct vfio_pci_dependent_device *devices;
-    struct vfio_pci_hot_reset *reset;
-    int32_t *fds;
-    int ret, i, count;
+    int ret, i;
     bool multi = false;
 
     trace_vfio_pci_hot_reset(vdev->vbasedev.name, single ? "one" : "multi");
@@ -2907,34 +2947,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
         goto out_single;
     }
 
-    /* Determine how many group fds need to be passed */
-    count = 0;
-    QLIST_FOREACH(group, &vfio_group_list, next) {
-        for (i = 0; i < info->count; i++) {
-            if (group->groupid == devices[i].group_id) {
-                count++;
-                break;
-            }
-        }
-    }
-
-    reset = g_malloc0(sizeof(*reset) + (count * sizeof(*fds)));
-    reset->argsz = sizeof(*reset) + (count * sizeof(*fds));
-    fds = &reset->group_fds[0];
-
-    /* Fill in group fds */
-    QLIST_FOREACH(group, &vfio_group_list, next) {
-        for (i = 0; i < info->count; i++) {
-            if (group->groupid == devices[i].group_id) {
-                fds[reset->count++] = group->fd;
-                break;
-            }
-        }
-    }
-
-    /* Bus reset! */
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
-    g_free(reset);
+    ret = vfio_pci_do_hot_reset(vdev, info);
 
     trace_vfio_pci_hot_reset_result(vdev->vbasedev.name,
                                     ret ? "%m" : "Success");
-- 
1.9.3

[Qemu-devel] [RFC v7 03/11] qdev: add bus reset_notifiers callbacks for host bus reset

[Chen Fan]

Particularly, For vfio device, 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 bus
callbacks to specify to do host bus reset.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/pci/pci.c                  |  6 ++++++
 hw/pci/pci_bridge.c           |  3 +++
 hw/vfio/pci.c                 | 12 ++++++++++++
 include/hw/pci/pci.h          |  2 ++
 include/hw/pci/pci_bus.h      |  2 ++
 include/hw/vfio/vfio-common.h |  1 +
 6 files changed, 26 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 48f19a3..cac7769 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -74,11 +74,17 @@ static const VMStateDescription vmstate_pcibus = {
     }
 };
 
+void pci_bus_add_reset_notifier(PCIBus *bus, Notifier *notify)
+{
+    notifier_list_add(&bus->reset_notifiers, notify);
+}
+
 static void pci_bus_realize(BusState *qbus, Error **errp)
 {
     PCIBus *bus = PCI_BUS(qbus);
 
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
+    notifier_list_init(&bus->reset_notifiers);
 }
 
 static void pci_bus_unrealize(BusState *qbus, Error **errp)
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 40c97b1..88c3240 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -267,6 +267,9 @@ void pci_bridge_write_config(PCIDevice *d,
 
     newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
     if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
+        /* Particularly for vfio devices to reset host bus */
+        notifier_list_notify(&s->sec_bus.reset_notifiers, NULL);
+
         /* Trigger hot reset on 0->1 transition. */
         qbus_reset_all(&s->sec_bus.qbus);
     }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 037a2c6..43869e9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -154,6 +154,7 @@ typedef struct VFIOPCIDevice {
     PCIHostDeviceAddress host;
     EventNotifier err_notifier;
     EventNotifier req_notifier;
+    Notifier sec_bus_reset_notifier;
     int (*resetfn)(struct VFIOPCIDevice *);
     uint32_t features;
 #define VFIO_FEATURE_ENABLE_VGA_BIT 0
@@ -3524,6 +3525,14 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
     }
 }
 
+static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
+{
+    VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, sec_bus_reset_notifier);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+
+    vbasedev->needs_bus_reset = true;
+}
+
 static int vfio_initfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
@@ -3674,6 +3683,9 @@ static int vfio_initfn(PCIDevice *pdev)
     vfio_register_req_notifier(vdev);
     vfio_setup_resetfn(vdev);
 
+    vdev->sec_bus_reset_notifier.notify = vfio_pci_host_needs_bus_reset;
+    pci_bus_add_reset_notifier(pdev->bus, &vdev->sec_bus_reset_notifier);
+
     return 0;
 
 out_teardown:
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 5d050c8..eae236a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -7,6 +7,7 @@
 #include "exec/memory.h"
 #include "sysemu/dma.h"
 #include "qapi/error.h"
+#include "qemu/notify.h"
 
 /* PCI includes legacy ISA access.  */
 #include "hw/isa/isa.h"
@@ -371,6 +372,7 @@ void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
 void pci_device_set_intx_routing_notifier(PCIDevice *dev,
                                           PCIINTxRoutingNotifier notifier);
 void pci_device_reset(PCIDevice *dev);
+void pci_bus_add_reset_notifier(PCIBus *bus, Notifier *notify);
 
 PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
                                const char *default_model,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index fabaeee..3b551d7 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -29,6 +29,8 @@ struct PCIBus {
        Keep a count of the number of devices with raised IRQs.  */
     int nirq;
     int *irq_count;
+
+    NotifierList reset_notifiers;
 };
 
 typedef struct PCIBridgeWindows PCIBridgeWindows;
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 0d1fb80..5f5691a 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -102,6 +102,7 @@ typedef struct VFIODevice {
     bool reset_works;
     bool needs_reset;
     bool allow_mmap;
+    bool needs_bus_reset;
     VFIODeviceOps *ops;
     unsigned int num_irqs;
     unsigned int num_regions;
-- 
1.9.3

[Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not

[Chen Fan]

when machine is done, we should check the all vfio devices
whether support host bus reset, then when need virtual secondary
bus reset, we should reset host bus.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 43869e9..ff639db 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice {
     bool req_enabled;
     bool has_flr;
     bool has_pm_reset;
+    bool has_bus_reset;
     bool rom_read_failed;
 } VFIOPCIDevice;
 
@@ -3533,6 +3534,82 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
     vbasedev->needs_bus_reset = true;
 }
 
+static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
+{
+    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    VFIOGroup *group;
+    int ret, i;
+
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret < 0) {
+        goto out;
+    }
+
+    devices = &info->devices[0];
+
+    /* Verify that we have all the groups required */
+    for (i = 0; i < info->count; i++) {
+        PCIHostDeviceAddress host;
+        VFIOPCIDevice *tmp;
+        VFIODevice *vbasedev_iter;
+
+        host.domain = devices[i].segment;
+        host.bus = devices[i].bus;
+        host.slot = PCI_SLOT(devices[i].devfn);
+        host.function = PCI_FUNC(devices[i].devfn);
+
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
+            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
+                continue;
+            }
+            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
+            if (vfio_pci_host_match(&host, &tmp->host)) {
+                if (PCI_BUS(vdev->pdev.bus) !=
+                    PCI_BUS(tmp->pdev.bus)) {
+                    goto out;
+                }
+            }
+        }
+    }
+
+    vdev->has_bus_reset = true;
+
+out:
+    g_free(info);
+}
+
+static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
+{
+    VFIOGroup *group;
+    VFIODevice *vbasedev;
+
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+         QLIST_FOREACH(vbasedev, &group->device_list, next) {
+             vfio_check_host_bus_reset(vbasedev);
+         }
+    }
+}
+
+static Notifier machine_notifier = {
+    .notify = vfio_pci_machine_done_notify,
+};
+
 static int vfio_initfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
@@ -3821,6 +3898,12 @@ static const TypeInfo vfio_pci_dev_info = {
 static void register_vfio_pci_dev_type(void)
 {
     type_register_static(&vfio_pci_dev_info);
+
+    /*
+     * Register notifier when machine init is done, since we need
+     * check the configration manner after all vfio device are inited.
+     */
+    qemu_add_machine_init_done_notifier(&machine_notifier);
 }
 
 type_init(register_vfio_pci_dev_type)
-- 
1.9.3

[Qemu-devel] [RFC v7 05/11] vfio: do hot bus reset when do virtual secondary bus reset

[Chen Fan]

when do virtual secondary bus reset, the vfio device under
this bus need to do host bus reset to reset the device.
so add this case.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c                 | 12 +++++++++++-
 include/hw/vfio/vfio-common.h |  1 -
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ff639db..cc169ea 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
     bool has_flr;
     bool has_pm_reset;
     bool has_bus_reset;
+    bool needs_bus_reset;
     bool rom_read_failed;
 } VFIOPCIDevice;
 
@@ -3531,7 +3532,8 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
     VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, sec_bus_reset_notifier);
     VFIODevice *vbasedev = &vdev->vbasedev;
 
-    vbasedev->needs_bus_reset = true;
+    vdev->needs_bus_reset = true;
+    vbasedev->needs_reset = true;
 }
 
 static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
@@ -3807,6 +3809,14 @@ static void vfio_pci_reset(DeviceState *dev)
 
     trace_vfio_pci_reset(vdev->vbasedev.name);
 
+    if (vdev->has_bus_reset && vdev->needs_bus_reset) {
+        vdev->needs_bus_reset = false;
+        if (vdev->vbasedev.needs_reset) {
+            vfio_pci_hot_reset(vdev, false);
+        }
+        return;
+    }
+
     vfio_pci_pre_reset(vdev);
 
     if (vdev->resetfn && !vdev->resetfn(vdev)) {
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 5f5691a..0d1fb80 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -102,7 +102,6 @@ typedef struct VFIODevice {
     bool reset_works;
     bool needs_reset;
     bool allow_mmap;
-    bool needs_bus_reset;
     VFIODeviceOps *ops;
     unsigned int num_irqs;
     unsigned int num_regions;
-- 
1.9.3

[Qemu-devel] [RFC v7 06/11] vfio: add pcie extanded capability support

[Chen Fan]

For vfio pcie device, we could expose the extended capability on
PCIE bus. in order to avoid config space broken, we introduce
a copy config for parsing extended caps. and rebuild the pcie
extended config space.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index cc169ea..7a446b0 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2496,6 +2496,21 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos)
     return next - pos;
 }
 
+
+static uint16_t vfio_ext_cap_max_size(const uint8_t *config, uint16_t pos)
+{
+    uint16_t tmp, next = PCIE_CONFIG_SPACE_SIZE - 1;
+
+    for (tmp = PCI_CONFIG_SPACE_SIZE; tmp;
+        tmp = PCI_EXT_CAP_NEXT(pci_get_long(config + tmp))) {
+        if (tmp > pos && tmp < next) {
+            next = tmp;
+        }
+    }
+
+    return next - pos;
+}
+
 static void vfio_set_word_bits(uint8_t *buf, uint16_t val, uint16_t mask)
 {
     pci_set_word(buf, (pci_get_word(buf) & ~mask) | val);
@@ -2803,16 +2818,72 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
     return 0;
 }
 
+static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint32_t header;
+    uint16_t cap_id, next, size;
+    uint8_t cap_ver;
+
+    for (next = PCI_CONFIG_SPACE_SIZE; next;
+         next = PCI_EXT_CAP_NEXT(pci_get_long(config + next))) {
+        header = pci_get_long(config + next);
+        cap_id = PCI_EXT_CAP_ID(header);
+        cap_ver = PCI_EXT_CAP_VER(header);
+
+        /*
+         * If it becomes important to configure extended capabilities to their
+         * actual size, use this as the default when it's something we don't
+         * recognize. Since QEMU doesn't actually handle many of the config
+         * accesses, exact size doesn't seem worthwhile.
+         */
+        size = vfio_ext_cap_max_size(config, next);
+
+        pcie_add_capability(pdev, cap_id, cap_ver, next, size);
+        if (next == PCI_CONFIG_SPACE_SIZE) {
+            /* Begin the rebuild, we should set the next offset zero. */
+            pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
+        }
+
+        /* Use emulated header pointer to allow dropping extended caps */
+        pci_set_long(vdev->emulated_config_bits + next, 0xffffffff);
+    }
+
+    return 0;
+}
+
 static int vfio_add_capabilities(VFIOPCIDevice *vdev)
 {
     PCIDevice *pdev = &vdev->pdev;
+    int ret;
+    uint8_t *config;
 
     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
         !pdev->config[PCI_CAPABILITY_LIST]) {
         return 0; /* Nothing to add */
     }
 
-    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
+    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
+    if (ret) {
+        return ret;
+    }
+
+    /* on PCI bus, it doesn't make sense to expose extended capabilities. */
+    if (!pci_is_express(pdev) ||
+        !pci_bus_is_express(pdev->bus) ||
+        !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
+        return 0;
+    }
+
+    /*
+     * In order to avoid config space broken, here using a copy config to
+     * parse extended capabilities.
+     */
+    config = g_memdup(pdev->config, vdev->config_size);
+    ret = vfio_add_ext_cap(vdev, config);
+
+    g_free(config);
+    return ret;
 }
 
 static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
-- 
1.9.3

[Qemu-devel] [RFC v7 07/11] aer: impove pcie_aer_init to support vfio device

[Chen Fan]

pcie_aer_init was used to emulate an aer capability for pcie device,
but for vfio device, the aer config space size is mutable and is not
always equal to PCI_ERR_SIZEOF(0x48). it depends on where the TLP Prefix
register required, so here we add a size argument.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/pci-bridge/ioh3420.c            | 2 +-
 hw/pci-bridge/xio3130_downstream.c | 2 +-
 hw/pci-bridge/xio3130_upstream.c   | 2 +-
 hw/pci/pcie_aer.c                  | 4 ++--
 include/hw/pci/pcie_aer.h          | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index cce2fdd..4d9cd3f 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -129,7 +129,7 @@ static int ioh3420_initfn(PCIDevice *d)
         goto err_pcie_cap;
     }
     pcie_cap_root_init(d);
-    rc = pcie_aer_init(d, IOH_EP_AER_OFFSET);
+    rc = pcie_aer_init(d, IOH_EP_AER_OFFSET, PCI_ERR_SIZEOF);
     if (rc < 0) {
         goto err;
     }
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index b3a6479..9737041 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -92,7 +92,7 @@ static int xio3130_downstream_initfn(PCIDevice *d)
         goto err_pcie_cap;
     }
     pcie_cap_arifwd_init(d);
-    rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
+    rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
     if (rc < 0) {
         goto err;
     }
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index eada582..4d7f894 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -81,7 +81,7 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     }
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
-    rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
+    rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
     if (rc < 0) {
         goto err;
     }
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index b48c09c..ccf577e 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -94,12 +94,12 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
     aer_log->log_num = 0;
 }
 
-int pcie_aer_init(PCIDevice *dev, uint16_t offset)
+int pcie_aer_init(PCIDevice *dev, uint16_t offset, uint16_t size)
 {
     PCIExpressDevice *exp;
 
     pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
-                        offset, PCI_ERR_SIZEOF);
+                        offset, size);
     exp = &dev->exp;
     exp->aer_cap = offset;
 
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index 2fb8388..156acb0 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -87,7 +87,7 @@ struct PCIEAERErr {
 
 extern const VMStateDescription vmstate_pcie_aer_log;
 
-int pcie_aer_init(PCIDevice *dev, uint16_t offset);
+int pcie_aer_init(PCIDevice *dev, uint16_t offset, uint16_t size);
 void pcie_aer_exit(PCIDevice *dev);
 void pcie_aer_write_config(PCIDevice *dev,
                            uint32_t addr, uint32_t val, int len);
-- 
1.9.3

[Qemu-devel] [RFC v7 08/11] vfio: add aer support for vfio device

[Chen Fan]

Calling pcie_aer_init to initilize aer related registers for
vfio device, then reload physical related registers to expose
device capability.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7a446b0..61a348e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2818,12 +2818,47 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
     return 0;
 }
 
+static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
+    uint32_t severity, errcap;
+    int ret;
+
+    errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + 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);
+    ret = pcie_aer_init(pdev, pos, size);
+    if (ret) {
+        return ret;
+    }
+
+    /* load physical registers */
+    severity = vfio_pci_read_config(pdev,
+                   pdev->exp.aer_cap + PCI_ERR_UNCOR_SEVER, 4);
+    pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
+
+    return 0;
+}
+
 static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint32_t header;
     uint16_t cap_id, next, size;
     uint8_t cap_ver;
+    int ret = 0;
 
     for (next = PCI_CONFIG_SPACE_SIZE; next;
          next = PCI_EXT_CAP_NEXT(pci_get_long(config + next))) {
@@ -2839,7 +2874,19 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
          */
         size = vfio_ext_cap_max_size(config, next);
 
-        pcie_add_capability(pdev, cap_id, cap_ver, next, size);
+        switch (cap_id) {
+        case PCI_EXT_CAP_ID_ERR:
+            ret = vfio_setup_aer(vdev, next, size);
+            break;
+        default:
+            pcie_add_capability(pdev, cap_id, cap_ver, next, size);
+            break;
+        }
+
+        if (ret) {
+            return ret;
+        }
+
         if (next == PCI_CONFIG_SPACE_SIZE) {
             /* Begin the rebuild, we should set the next offset zero. */
             pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
-- 
1.9.3

[Qemu-devel] [RFC v7 09/11] pcie_aer: expose pcie_aer_msg() interface

[Chen Fan]

For vfio device, we need to propagate the aer error to
Guest OS. we use the pcie_aer_msg() to send aer error
to guest.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/pci/pcie_aer.c         | 2 +-
 include/hw/pci/pcie_aer.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index ccf577e..58010c4 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -370,7 +370,7 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
  *
  * Walk up the bus tree from the device, propagate the error message.
  */
-static void pcie_aer_msg(PCIDevice *dev, const PCIEAERMsg *msg)
+void pcie_aer_msg(PCIDevice *dev, const PCIEAERMsg *msg)
 {
     uint8_t type;
 
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index 156acb0..c2ee4e2 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -102,5 +102,6 @@ void pcie_aer_root_write_config(PCIDevice *dev,
 
 /* error injection */
 int pcie_aer_inject_error(PCIDevice *dev, const PCIEAERErr *err);
+void pcie_aer_msg(PCIDevice *dev, const PCIEAERMsg *msg);
 
 #endif /* QEMU_PCIE_AER_H */
-- 
1.9.3

[Qemu-devel] [RFC v7 10/11] vfio-pci: pass the aer error to guest

[Chen Fan]

when the vfio device encounters an uncorrectable error in host,
the vfio_pci driver will signal the eventfd registered by this
vfio device, the results in the qemu eventfd handler getting
invoked.

this patch is to pass the error to guest and have the guest driver
recover from the error.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/vfio/pci.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 61a348e..f2fc5d6 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3296,18 +3296,40 @@ static void vfio_put_device(VFIOPCIDevice *vdev)
 static void vfio_err_notifier_handler(void *opaque)
 {
     VFIOPCIDevice *vdev = opaque;
+    PCIDevice *dev = &vdev->pdev;
+    PCIEAERMsg msg = {
+        .severity = 0,
+        .source_id = (pci_bus_num(dev->bus) << 8) | dev->devfn,
+    };
 
     if (!event_notifier_test_and_clear(&vdev->err_notifier)) {
         return;
     }
 
+    /* we should read the error details from the real hardware
+     * configuration spaces, here we only need to do is signaling
+     * to guest an uncorrectable error has occurred.
+     */
+     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);
+
+        isfatal = uncor_status & pci_get_long(aer_cap + PCI_ERR_UNCOR_SEVER);
+
+        msg.severity = isfatal ? PCI_ERR_ROOT_CMD_FATAL_EN :
+                                 PCI_ERR_ROOT_CMD_NONFATAL_EN;
+
+        pcie_aer_msg(dev, &msg);
+        return;
+    }
+
     /*
-     * TBD. Retrieve the error details and decide what action
-     * needs to be taken. One of the actions could be to pass
-     * the error to the guest and have the guest driver recover
-     * from the error. This requires that PCIe capabilities be
-     * exposed to the guest. For now, we just terminate the
-     * guest to contain the error.
+     * If the aer capability is not exposed to the guest. we just
+     * terminate the guest to contain the error.
      */
 
     error_report("%s(%04x:%02x:%02x.%x) Unrecoverable error detected.  "
-- 
1.9.3

[Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap

[Chen Fan]

add 'aer' property to let user able to decide whether expose
the aer capability. by default we should disable aer feature,
because it needs configuration restrictions.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc_q35.c    |  4 +++
 hw/vfio/pci.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/compat.h |  8 +++++
 3 files changed, 96 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e67f2de..94a3a1c 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -439,6 +439,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
     PC_Q35_2_3_MACHINE_OPTIONS,
     .name = "pc-q35-2.3",
     .init = pc_q35_init_2_3,
+    .compat_props = (GlobalProperty[]) {
+        HW_COMPAT_2_3,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f2fc5d6..b4b9495 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -42,6 +42,7 @@
 #include "trace.h"
 #include "hw/vfio/vfio.h"
 #include "hw/vfio/vfio-common.h"
+#include "hw/pci/pci_bridge.h"
 
 struct VFIOPCIDevice;
 
@@ -161,6 +162,8 @@ typedef struct VFIOPCIDevice {
 #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
 #define VFIO_FEATURE_ENABLE_REQ_BIT 1
 #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
+#define VFIO_FEATURE_ENABLE_AER_BIT 2
+#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
     int32_t bootindex;
     uint8_t pm_cap;
     bool has_vga;
@@ -2821,10 +2824,31 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
 static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
 {
     PCIDevice *pdev = &vdev->pdev;
+    PCIDevice *dev_iter;
     uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
     uint32_t severity, errcap;
+    uint8_t type;
     int ret;
 
+    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+        return 0;
+    }
+
+    dev_iter = pci_bridge_get_device(pdev->bus);
+    if (!dev_iter) {
+        goto error;
+    }
+
+    while (dev_iter) {
+        type = pcie_cap_get_type(dev_iter);
+        if ((type != PCI_EXP_TYPE_ROOT_PORT &&
+             type != PCI_EXP_TYPE_UPSTREAM &&
+             type != PCI_EXP_TYPE_DOWNSTREAM)) {
+            goto error;
+        }
+        dev_iter = pci_bridge_get_device(dev_iter->bus);
+    }
+
     errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
     /*
      * The ability to record multiple headers is depending on
@@ -2850,6 +2874,11 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
     pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
 
     return 0;
+
+error:
+    error_report("vfio: Unable to enable AER for device %s, parent bus "
+                 "do not support signal AER", vdev->vbasedev.name);
+    return -1;
 }
 
 static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
@@ -3733,9 +3762,52 @@ static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
     vdev->has_bus_reset = true;
 
 out:
+    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
+        !vdev->has_bus_reset) {
+        error_report("vfio: Cannot enable AER for device %s,"
+                     "which is not support host bus reset.",
+                     vdev->vbasedev.name);
+        exit(1);
+    }
     g_free(info);
 }
 
+static int vfio_pci_validate_aer_devices(DeviceState *dev,
+                                         void *opaque)
+{
+    VFIOPCIDevice *vdev;
+
+    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
+        return 0;
+    }
+
+    vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(dev));
+    if (!vdev->has_bus_reset) {
+        error_report("vfio: device %s is not support host bus reset,"
+                     "but on the same bus has vfio device enable AER.",
+                     vdev->vbasedev.name);
+        exit(1);
+    }
+    return 0;
+}
+
+static void vfio_check_virtual_bus_reset(VFIODevice *vbasedev)
+{
+    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+
+    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+        return;
+    }
+
+    /*
+     * Verify that we have all the vfio devices support host bus reset
+     * on the bus
+     */
+    qbus_walk_children(BUS(vdev->pdev.bus), NULL, NULL,
+                       vfio_pci_validate_aer_devices,
+                       NULL, NULL);
+}
+
 static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
 {
     VFIOGroup *group;
@@ -3746,6 +3818,16 @@ static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
              vfio_check_host_bus_reset(vbasedev);
          }
     }
+
+    /*
+     * All devices need support host bus reset on each virtual bus
+     * if one device enable AER.
+     */
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+         QLIST_FOREACH(vbasedev, &group->device_list, next) {
+             vfio_check_virtual_bus_reset(vbasedev);
+         }
+    }
 }
 
 static Notifier machine_notifier = {
@@ -4004,6 +4086,8 @@ static Property vfio_pci_dev_properties[] = {
                     VFIO_FEATURE_ENABLE_VGA_BIT, false),
     DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_REQ_BIT, true),
+    DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
+                    VFIO_FEATURE_ENABLE_AER_BIT, false),
     DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
     DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
     /*
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 313682a..f722919 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,7 +1,15 @@
 #ifndef HW_COMPAT_H
 #define HW_COMPAT_H
 
+#define HW_COMPAT_2_3 \
+        {\
+            .driver   = "vfio-pci",\
+            .property = "x-aer",\
+            .value    = "off",\
+        }
+
 #define HW_COMPAT_2_1 \
+        HW_COMPAT_2_3, \
         {\
             .driver   = "intel-hda",\
             .property = "old_msi_addr",\
-- 
1.9.3

Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap

[Alex Williamson]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> add 'aer' property to let user able to decide whether expose
> the aer capability. by default we should disable aer feature,
> because it needs configuration restrictions.

But the previous patch already enabled it, so a bisect might get it
enabled then disabled.

> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc_q35.c    |  4 +++
>  hw/vfio/pci.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/compat.h |  8 +++++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index e67f2de..94a3a1c 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -439,6 +439,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,
>      .name = "pc-q35-2.3",
>      .init = pc_q35_init_2_3,
> +    .compat_props = (GlobalProperty[]) {
> +        HW_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index f2fc5d6..b4b9495 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -42,6 +42,7 @@
>  #include "trace.h"
>  #include "hw/vfio/vfio.h"
>  #include "hw/vfio/vfio-common.h"
> +#include "hw/pci/pci_bridge.h"
>  
>  struct VFIOPCIDevice;
>  
> @@ -161,6 +162,8 @@ typedef struct VFIOPCIDevice {
>  #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>  #define VFIO_FEATURE_ENABLE_REQ_BIT 1
>  #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
> +#define VFIO_FEATURE_ENABLE_AER_BIT 2
> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
>      int32_t bootindex;
>      uint8_t pm_cap;
>      bool has_vga;
> @@ -2821,10 +2824,31 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>  static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> +    PCIDevice *dev_iter;
>      uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
>      uint32_t severity, errcap;
> +    uint8_t type;
>      int ret;
>  
> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> +        return 0;
> +    }
> +
> +    dev_iter = pci_bridge_get_device(pdev->bus);
> +    if (!dev_iter) {

So this is testing that it can't be on the root complex endpoint.

> +        goto error;
> +    }
> +
> +    while (dev_iter) {
> +        type = pcie_cap_get_type(dev_iter);
> +        if ((type != PCI_EXP_TYPE_ROOT_PORT &&
> +             type != PCI_EXP_TYPE_UPSTREAM &&
> +             type != PCI_EXP_TYPE_DOWNSTREAM)) {
> +            goto error;
> +        }

And this tests that we have PCIe devices up to the root complex, but do
do we need to check whether they support aer?

> +        dev_iter = pci_bridge_get_device(dev_iter->bus);
> +    }
> +
>      errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
>      /*
>       * The ability to record multiple headers is depending on
> @@ -2850,6 +2874,11 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>      pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
>  
>      return 0;
> +
> +error:
> +    error_report("vfio: Unable to enable AER for device %s, parent bus "
> +                 "do not support signal AER", vdev->vbasedev.name);
> +    return -1;
>  }
>  
>  static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
> @@ -3733,9 +3762,52 @@ static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
>      vdev->has_bus_reset = true;
>  
>  out:
> +    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> +        !vdev->has_bus_reset) {
> +        error_report("vfio: Cannot enable AER for device %s,"
> +                     "which is not support host bus reset.",
> +                     vdev->vbasedev.name);
> +        exit(1);
> +    }

And this is done via the machine init callback, so again we're not doing
anything to support hotplug.

>      g_free(info);
>  }
>  
> +static int vfio_pci_validate_aer_devices(DeviceState *dev,
> +                                         void *opaque)
> +{
> +    VFIOPCIDevice *vdev;
> +
> +    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> +        return 0;
> +    }
> +
> +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(dev));
> +    if (!vdev->has_bus_reset) {
> +        error_report("vfio: device %s is not support host bus reset,"
> +                     "but on the same bus has vfio device enable AER.",
> +                     vdev->vbasedev.name);
> +        exit(1);
> +    }
> +    return 0;
> +}
> +
> +static void vfio_check_virtual_bus_reset(VFIODevice *vbasedev)
> +{
> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +
> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> +        return;
> +    }
> +
> +    /*
> +     * Verify that we have all the vfio devices support host bus reset
> +     * on the bus
> +     */
> +    qbus_walk_children(BUS(vdev->pdev.bus), NULL, NULL,
> +                       vfio_pci_validate_aer_devices,
> +                       NULL, NULL);
> +}
> +
>  static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>  {
>      VFIOGroup *group;
> @@ -3746,6 +3818,16 @@ static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>               vfio_check_host_bus_reset(vbasedev);
>           }
>      }
> +
> +    /*
> +     * All devices need support host bus reset on each virtual bus
> +     * if one device enable AER.
> +     */
> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +             vfio_check_virtual_bus_reset(vbasedev);
> +         }
> +    }

I'm not sure I see how this call path can catch anything missed by
vfio_check_host_bus_reset().

>  }
>  
>  static Notifier machine_notifier = {
> @@ -4004,6 +4086,8 @@ static Property vfio_pci_dev_properties[] = {
>                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
>      DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_REQ_BIT, true),
> +    DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
> +                    VFIO_FEATURE_ENABLE_AER_BIT, false),
>      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>      DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
>      /*
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 313682a..f722919 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -1,7 +1,15 @@
>  #ifndef HW_COMPAT_H
>  #define HW_COMPAT_H
>  
> +#define HW_COMPAT_2_3 \
> +        {\
> +            .driver   = "vfio-pci",\
> +            .property = "x-aer",\

It's defined as "aer" above, not "x-aer".  I'm not sure why we need this
though if the default value is off anyway.

> +            .value    = "off",\
> +        }
> +
>  #define HW_COMPAT_2_1 \
> +        HW_COMPAT_2_3, \
>          {\
>              .driver   = "intel-hda",\
>              .property = "old_msi_addr",\

Re: [Qemu-devel] [RFC v7 05/11] vfio: do hot bus reset when do virtual secondary bus reset

[Alex Williamson]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> when do virtual secondary bus reset, the vfio device under
> this bus need to do host bus reset to reset the device.
> so add this case.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c                 | 12 +++++++++++-
>  include/hw/vfio/vfio-common.h |  1 -
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ff639db..cc169ea 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
>      bool has_flr;
>      bool has_pm_reset;
>      bool has_bus_reset;
> +    bool needs_bus_reset;
>      bool rom_read_failed;
>  } VFIOPCIDevice;
>  
> @@ -3531,7 +3532,8 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
>      VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, sec_bus_reset_notifier);
>      VFIODevice *vbasedev = &vdev->vbasedev;
>  
> -    vbasedev->needs_bus_reset = true;
> +    vdev->needs_bus_reset = true;
> +    vbasedev->needs_reset = true;

The above should have happened in patch 03.

>  }
>  
>  static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> @@ -3807,6 +3809,14 @@ static void vfio_pci_reset(DeviceState *dev)
>  
>      trace_vfio_pci_reset(vdev->vbasedev.name);
>  
> +    if (vdev->has_bus_reset && vdev->needs_bus_reset) {
> +        vdev->needs_bus_reset = false;
> +        if (vdev->vbasedev.needs_reset) {
> +            vfio_pci_hot_reset(vdev, false);

So we're not even going to check to see whether that worked?  How can we
know that the configuration is still the same as after machine init?  An
affected device could have been hot-unplugged an now in use by the host
(in which case this function would fail).  An affected device could have
been hot unplugged and added back to the VM under a different bus, now
we have an arbitrary second device affected by the bus reset from the VM
perspective.

> +        }
> +        return;
> +    }
> +
>      vfio_pci_pre_reset(vdev);
>  
>      if (vdev->resetfn && !vdev->resetfn(vdev)) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 5f5691a..0d1fb80 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -102,7 +102,6 @@ typedef struct VFIODevice {
>      bool reset_works;
>      bool needs_reset;
>      bool allow_mmap;
> -    bool needs_bus_reset;
>      VFIODeviceOps *ops;
>      unsigned int num_irqs;
>      unsigned int num_regions;

Re: [Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not

[Alex Williamson]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> when machine is done, we should check the all vfio devices
> whether support host bus reset, then when need virtual secondary
> bus reset, we should reset host bus.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 43869e9..ff639db 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice {
>      bool req_enabled;
>      bool has_flr;
>      bool has_pm_reset;
> +    bool has_bus_reset;
>      bool rom_read_failed;
>  } VFIOPCIDevice;
>  
> @@ -3533,6 +3534,82 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
>      vbasedev->needs_bus_reset = true;
>  }
>  

This function really needs some comments

> +static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> +{
> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +    struct vfio_pci_hot_reset_info *info = NULL;
> +    struct vfio_pci_dependent_device *devices;
> +    VFIOGroup *group;
> +    int ret, i;
> +
> +    ret = vfio_get_hot_reset_info(vdev, &info);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    devices = &info->devices[0];

So we have the list of host devices affected by a bus reset...

> +
> +    /* Verify that we have all the groups required */
> +    for (i = 0; i < info->count; i++) {
> +        PCIHostDeviceAddress host;
> +        VFIOPCIDevice *tmp;
> +        VFIODevice *vbasedev_iter;
> +
> +        host.domain = devices[i].segment;
> +        host.bus = devices[i].bus;
> +        host.slot = PCI_SLOT(devices[i].devfn);
> +        host.function = PCI_FUNC(devices[i].devfn);
> +

Skip the current device

> +        if (vfio_pci_host_match(&host, &vdev->host)) {
> +            continue;
> +        }
> +

Make sure we own the group for the device

> +        QLIST_FOREACH(group, &vfio_group_list, next) {
> +            if (group->groupid == devices[i].group_id) {
> +                break;
> +            }
> +        }
> +
> +        if (!group) {
> +            goto out;
> +        }
> +

Search the group device list for the device, if found make sure it's on
the same virtual bus as the target device...  This shouldn't be limited
to the group device list, it should be done for all vfio-pci devices.
For instance, a dual-port card could have 2 separate physical functions
that are each isolated and appear in different groups.  Obviously a bus
reset will affect both.  Anything affected by the reset needs to be on
(or below) the bus of the target device.  That also exposes another
issue that I don't see addressed here; a physical bus reset will cascade
to all subordinate buses, but your notifier only works via access to the
bus reset bit in the control register.  Do we also need to make sure the
reset function for PCI bridges triggers the notify?

Also, this assumes that the host and guest PCI configuration is static,
which is not necessarily true for either.  So I think that caching the
bus reset capability is wrong.

> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> +                continue;
> +            }
> +            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> +            if (vfio_pci_host_match(&host, &tmp->host)) {
> +                if (PCI_BUS(vdev->pdev.bus) !=
> +                    PCI_BUS(tmp->pdev.bus)) {
> +                    goto out;
> +                }
> +            }
> +        }
> +    }
> +
> +    vdev->has_bus_reset = true;
> +
> +out:
> +    g_free(info);
> +}
> +
> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +
> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +             vfio_check_host_bus_reset(vbasedev);
> +         }
> +    }
> +}
> +
> +static Notifier machine_notifier = {
> +    .notify = vfio_pci_machine_done_notify,
> +};
> +
>  static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> @@ -3821,6 +3898,12 @@ static const TypeInfo vfio_pci_dev_info = {
>  static void register_vfio_pci_dev_type(void)
>  {
>      type_register_static(&vfio_pci_dev_info);
> +
> +    /*
> +     * Register notifier when machine init is done, since we need
> +     * check the configration manner after all vfio device are inited.
> +     */
> +    qemu_add_machine_init_done_notifier(&machine_notifier);
>  }
>  
>  type_init(register_vfio_pci_dev_type)

Re: [Qemu-devel] [RFC v7 03/11] qdev: add bus reset_notifiers callbacks for host bus reset

[Alex Williamson]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> Particularly, For vfio device, 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 bus
> callbacks to specify to do host bus reset.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci.c                  |  6 ++++++
>  hw/pci/pci_bridge.c           |  3 +++
>  hw/vfio/pci.c                 | 12 ++++++++++++
>  include/hw/pci/pci.h          |  2 ++
>  include/hw/pci/pci_bus.h      |  2 ++
>  include/hw/vfio/vfio-common.h |  1 +
>  6 files changed, 26 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 48f19a3..cac7769 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -74,11 +74,17 @@ static const VMStateDescription vmstate_pcibus = {
>      }
>  };
>  
> +void pci_bus_add_reset_notifier(PCIBus *bus, Notifier *notify)
> +{
> +    notifier_list_add(&bus->reset_notifiers, notify);
> +}
> +

How do we support hot-unplug without a matching remove function?

>  static void pci_bus_realize(BusState *qbus, Error **errp)
>  {
>      PCIBus *bus = PCI_BUS(qbus);
>  
>      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> +    notifier_list_init(&bus->reset_notifiers);
>  }
>  
>  static void pci_bus_unrealize(BusState *qbus, Error **errp)
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 40c97b1..88c3240 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -267,6 +267,9 @@ void pci_bridge_write_config(PCIDevice *d,
>  
>      newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
>      if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> +        /* Particularly for vfio devices to reset host bus */
> +        notifier_list_notify(&s->sec_bus.reset_notifiers, NULL);
> +
>          /* Trigger hot reset on 0->1 transition. */
>          qbus_reset_all(&s->sec_bus.qbus);
>      }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c

vfio changes should be done in a separate patch, the QEMU PCI-core
changes would need to be ack'd by mst.

> index 037a2c6..43869e9 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -154,6 +154,7 @@ typedef struct VFIOPCIDevice {
>      PCIHostDeviceAddress host;
>      EventNotifier err_notifier;
>      EventNotifier req_notifier;
> +    Notifier sec_bus_reset_notifier;
>      int (*resetfn)(struct VFIOPCIDevice *);
>      uint32_t features;
>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
> @@ -3524,6 +3525,14 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
>      }
>  }
>  
> +static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
> +{
> +    VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, sec_bus_reset_notifier);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +
> +    vbasedev->needs_bus_reset = true;
> +}
> +
>  static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> @@ -3674,6 +3683,9 @@ static int vfio_initfn(PCIDevice *pdev)
>      vfio_register_req_notifier(vdev);
>      vfio_setup_resetfn(vdev);
>  
> +    vdev->sec_bus_reset_notifier.notify = vfio_pci_host_needs_bus_reset;
> +    pci_bus_add_reset_notifier(pdev->bus, &vdev->sec_bus_reset_notifier);
> +

The notifier obviously needs to be unregistered in the exitfn.

>      return 0;
>  
>  out_teardown:
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 5d050c8..eae236a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -7,6 +7,7 @@
>  #include "exec/memory.h"
>  #include "sysemu/dma.h"
>  #include "qapi/error.h"
> +#include "qemu/notify.h"
>  
>  /* PCI includes legacy ISA access.  */
>  #include "hw/isa/isa.h"
> @@ -371,6 +372,7 @@ void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
>  void pci_device_set_intx_routing_notifier(PCIDevice *dev,
>                                            PCIINTxRoutingNotifier notifier);
>  void pci_device_reset(PCIDevice *dev);
> +void pci_bus_add_reset_notifier(PCIBus *bus, Notifier *notify);
>  
>  PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
>                                 const char *default_model,
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index fabaeee..3b551d7 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -29,6 +29,8 @@ struct PCIBus {
>         Keep a count of the number of devices with raised IRQs.  */
>      int nirq;
>      int *irq_count;
> +
> +    NotifierList reset_notifiers;
>  };
>  
>  typedef struct PCIBridgeWindows PCIBridgeWindows;
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 0d1fb80..5f5691a 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -102,6 +102,7 @@ typedef struct VFIODevice {
>      bool reset_works;
>      bool needs_reset;
>      bool allow_mmap;
> +    bool needs_bus_reset;

This is PCI specific, it should be added to VFIOPCIDevice only.

>      VFIODeviceOps *ops;
>      unsigned int num_irqs;
>      unsigned int num_regions;

Re: [Qemu-devel] [RFC v7 01/11] vfio: extract vfio_get_hot_reset_info as a single function

[Alex Williamson]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> the function is used to get affected devices by bus reset.
> so here extract it, and can used for aer soon.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 63 +++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e0e339a..fe4963f 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2641,6 +2641,48 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
>      }
>  }
>  
> +/*
> + * return negative with errno, return 0 on success.
> + * if success, the point of ret_info fill with the affected device reset info.
> + *
> + */
> +static int vfio_get_hot_reset_info(VFIOPCIDevice *vdev,
> +                                   struct vfio_pci_hot_reset_info **ret_info)
> +{
> +    struct vfio_pci_hot_reset_info *info;
> +    int ret, count;
> +
> +    *ret_info = NULL;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->argsz = sizeof(*info);
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> +    if (ret && errno != ENOSPC) {
> +        ret = -errno;
> +        goto error;

info is leaked

> +    }
> +
> +    count = info->count;
> +
> +    info = g_realloc(info, sizeof(*info) +
> +                     (count * sizeof(struct vfio_pci_dependent_device)));
> +    info->argsz = sizeof(*info) +
> +                  (count * sizeof(struct vfio_pci_dependent_device));
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> +    if (ret) {
> +        ret = -errno;
> +        goto error;

info is leaked

> +    }
> +
> +    *ret_info = info;
> +
> +    ret = 0;
> +error:
> +    return ret;
> +}
> +
>  static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> @@ -2792,32 +2834,19 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>      vfio_pci_pre_reset(vdev);
>      vdev->vbasedev.needs_reset = false;
>  
> -    info = g_malloc0(sizeof(*info));
> -    info->argsz = sizeof(*info);
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> -    if (ret && errno != ENOSPC) {
> -        ret = -errno;
> +    ret = vfio_get_hot_reset_info(vdev, &info);
> +    if (ret) {
>          if (!vdev->has_pm_reset) {
>              error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
>                           "no available reset mechanism.", vdev->host.domain,
>                           vdev->host.bus, vdev->host.slot, vdev->host.function);
> +        } else {
> +            error_report("vfio: hot reset info failed: %m");
>          }
>          goto out_single;
>      }
>  
> -    count = info->count;
> -    info = g_realloc(info, sizeof(*info) + (count * sizeof(*devices)));
> -    info->argsz = sizeof(*info) + (count * sizeof(*devices));
>      devices = &info->devices[0];
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> -    if (ret) {
> -        ret = -errno;
> -        error_report("vfio: hot reset info failed: %m");
> -        goto out_single;
> -    }
> -
>      trace_vfio_pci_hot_reset_has_dep_devices(vdev->vbasedev.name);
>  
>      /* Verify that we have all the groups required */

Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap

[Chen Fan]

On 05/20/2015 03:34 AM, Alex Williamson wrote:
> On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
>> add 'aer' property to let user able to decide whether expose
>> the aer capability. by default we should disable aer feature,
>> because it needs configuration restrictions.
> But the previous patch already enabled it, so a bisect might get it
> enabled then disabled.
>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/pc_q35.c    |  4 +++
>>   hw/vfio/pci.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/compat.h |  8 +++++
>>   3 files changed, 96 insertions(+)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index e67f2de..94a3a1c 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -439,6 +439,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>       PC_Q35_2_3_MACHINE_OPTIONS,
>>       .name = "pc-q35-2.3",
>>       .init = pc_q35_init_2_3,
>> +    .compat_props = (GlobalProperty[]) {
>> +        HW_COMPAT_2_3,
>> +        { /* end of list */ }
>> +    },
>>   };
>>   
>>   #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index f2fc5d6..b4b9495 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -42,6 +42,7 @@
>>   #include "trace.h"
>>   #include "hw/vfio/vfio.h"
>>   #include "hw/vfio/vfio-common.h"
>> +#include "hw/pci/pci_bridge.h"
>>   
>>   struct VFIOPCIDevice;
>>   
>> @@ -161,6 +162,8 @@ typedef struct VFIOPCIDevice {
>>   #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>>   #define VFIO_FEATURE_ENABLE_REQ_BIT 1
>>   #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
>> +#define VFIO_FEATURE_ENABLE_AER_BIT 2
>> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
>>       int32_t bootindex;
>>       uint8_t pm_cap;
>>       bool has_vga;
>> @@ -2821,10 +2824,31 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>   static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>>   {
>>       PCIDevice *pdev = &vdev->pdev;
>> +    PCIDevice *dev_iter;
>>       uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
>>       uint32_t severity, errcap;
>> +    uint8_t type;
>>       int ret;
>>   
>> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>> +        return 0;
>> +    }
>> +
>> +    dev_iter = pci_bridge_get_device(pdev->bus);
>> +    if (!dev_iter) {
> So this is testing that it can't be on the root complex endpoint.
>
>> +        goto error;
>> +    }
>> +
>> +    while (dev_iter) {
>> +        type = pcie_cap_get_type(dev_iter);
>> +        if ((type != PCI_EXP_TYPE_ROOT_PORT &&
>> +             type != PCI_EXP_TYPE_UPSTREAM &&
>> +             type != PCI_EXP_TYPE_DOWNSTREAM)) {
>> +            goto error;
>> +        }
> And this tests that we have PCIe devices up to the root complex, but do
> do we need to check whether they support aer?
Exactly, although the ioh3420 port and  the xio3130 switch have
been support aer, but in case new devices are unsupported in the future.

>
>> +        dev_iter = pci_bridge_get_device(dev_iter->bus);
>> +    }
>> +
>>       errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
>>       /*
>>        * The ability to record multiple headers is depending on
>> @@ -2850,6 +2874,11 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>>       pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
>>   
>>       return 0;
>> +
>> +error:
>> +    error_report("vfio: Unable to enable AER for device %s, parent bus "
>> +                 "do not support signal AER", vdev->vbasedev.name);
>> +    return -1;
>>   }
>>   
>>   static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
>> @@ -3733,9 +3762,52 @@ static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
>>       vdev->has_bus_reset = true;
>>   
>>   out:
>> +    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
>> +        !vdev->has_bus_reset) {
>> +        error_report("vfio: Cannot enable AER for device %s,"
>> +                     "which is not support host bus reset.",
>> +                     vdev->vbasedev.name);
>> +        exit(1);
>> +    }
> And this is done via the machine init callback, so again we're not doing
> anything to support hotplug.
Exactly, I missed the case of hotplug. I think I should implement
the bus reset capability at each initialize time and meanwhile update
the other devices bus reset capability on the same bus.

>
>>       g_free(info);
>>   }
>>   
>> +static int vfio_pci_validate_aer_devices(DeviceState *dev,
>> +                                         void *opaque)
>> +{
>> +    VFIOPCIDevice *vdev;
>> +
>> +    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>> +        return 0;
>> +    }
>> +
>> +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(dev));
>> +    if (!vdev->has_bus_reset) {
>> +        error_report("vfio: device %s is not support host bus reset,"
>> +                     "but on the same bus has vfio device enable AER.",
>> +                     vdev->vbasedev.name);
>> +        exit(1);
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void vfio_check_virtual_bus_reset(VFIODevice *vbasedev)
>> +{
>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> +
>> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Verify that we have all the vfio devices support host bus reset
>> +     * on the bus
>> +     */
>> +    qbus_walk_children(BUS(vdev->pdev.bus), NULL, NULL,
>> +                       vfio_pci_validate_aer_devices,
>> +                       NULL, NULL);
>> +}
>> +
>>   static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>>   {
>>       VFIOGroup *group;
>> @@ -3746,6 +3818,16 @@ static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>>                vfio_check_host_bus_reset(vbasedev);
>>            }
>>       }
>> +
>> +    /*
>> +     * All devices need support host bus reset on each virtual bus
>> +     * if one device enable AER.
>> +     */
>> +    QLIST_FOREACH(group, &vfio_group_list, next) {
>> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +             vfio_check_virtual_bus_reset(vbasedev);
>> +         }
>> +    }
> I'm not sure I see how this call path can catch anything missed by
> vfio_check_host_bus_reset().
I think we should support the case that on the same virtual bus
if one device enable AER, probably the other devices is on the
separate bus. at least the other devices should support host
bus reset.

1. on the same virtual bus and on the same host bus

virtual bus    -------------------------
                          |                 |
                      deviceA     deviceB
                          |                 |
host bus      -------------------------


2. on the same virtual bus but on the respective host bus

virtual bus    -------------------------
                          |                 |
                      deviceA     deviceB
                          |                 |
host bus      ----------    ------------

Thanks,
Chen


>
>>   }
>>   
>>   static Notifier machine_notifier = {
>> @@ -4004,6 +4086,8 @@ static Property vfio_pci_dev_properties[] = {
>>                       VFIO_FEATURE_ENABLE_VGA_BIT, false),
>>       DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
>>                       VFIO_FEATURE_ENABLE_REQ_BIT, true),
>> +    DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
>> +                    VFIO_FEATURE_ENABLE_AER_BIT, false),
>>       DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>>       DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
>>       /*
>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>> index 313682a..f722919 100644
>> --- a/include/hw/compat.h
>> +++ b/include/hw/compat.h
>> @@ -1,7 +1,15 @@
>>   #ifndef HW_COMPAT_H
>>   #define HW_COMPAT_H
>>   
>> +#define HW_COMPAT_2_3 \
>> +        {\
>> +            .driver   = "vfio-pci",\
>> +            .property = "x-aer",\
> It's defined as "aer" above, not "x-aer".  I'm not sure why we need this
> though if the default value is off anyway.
>
>> +            .value    = "off",\
>> +        }
>> +
>>   #define HW_COMPAT_2_1 \
>> +        HW_COMPAT_2_3, \
>>           {\
>>               .driver   = "intel-hda",\
>>               .property = "old_msi_addr",\
>
>
> .
>

Re: [Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not

[Chen Fan]

On 05/20/2015 03:34 AM, Alex Williamson wrote:
> On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
>> when machine is done, we should check the all vfio devices
>> whether support host bus reset, then when need virtual secondary
>> bus reset, we should reset host bus.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 83 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 43869e9..ff639db 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice {
>>       bool req_enabled;
>>       bool has_flr;
>>       bool has_pm_reset;
>> +    bool has_bus_reset;
>>       bool rom_read_failed;
>>   } VFIOPCIDevice;
>>   
>> @@ -3533,6 +3534,82 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
>>       vbasedev->needs_bus_reset = true;
>>   }
>>   
> This function really needs some comments
>
>> +static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
>> +{
>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> +    struct vfio_pci_hot_reset_info *info = NULL;
>> +    struct vfio_pci_dependent_device *devices;
>> +    VFIOGroup *group;
>> +    int ret, i;
>> +
>> +    ret = vfio_get_hot_reset_info(vdev, &info);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>> +
>> +    devices = &info->devices[0];
> So we have the list of host devices affected by a bus reset...
>
>> +
>> +    /* Verify that we have all the groups required */
>> +    for (i = 0; i < info->count; i++) {
>> +        PCIHostDeviceAddress host;
>> +        VFIOPCIDevice *tmp;
>> +        VFIODevice *vbasedev_iter;
>> +
>> +        host.domain = devices[i].segment;
>> +        host.bus = devices[i].bus;
>> +        host.slot = PCI_SLOT(devices[i].devfn);
>> +        host.function = PCI_FUNC(devices[i].devfn);
>> +
> Skip the current device
>
>> +        if (vfio_pci_host_match(&host, &vdev->host)) {
>> +            continue;
>> +        }
>> +
> Make sure we own the group for the device
>
>> +        QLIST_FOREACH(group, &vfio_group_list, next) {
>> +            if (group->groupid == devices[i].group_id) {
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (!group) {
>> +            goto out;
>> +        }
>> +
> Search the group device list for the device, if found make sure it's on
> the same virtual bus as the target device...  This shouldn't be limited
> to the group device list, it should be done for all vfio-pci devices.
why? if a bus reset affect both groups, the upper step ought to
find the exact group, we only need to check the affected devices
is below or on the bus.


> For instance, a dual-port card could have 2 separate physical functions
> that are each isolated and appear in different groups.  Obviously a bus
> reset will affect both.
yes.

>    Anything affected by the reset needs to be on
> (or below) the bus of the target device.  That also exposes another
> issue that I don't see addressed here; a physical bus reset will cascade
> to all subordinate buses, but your notifier only works via access to the
> bus reset bit in the control register.  Do we also need to make sure the
> reset function for PCI bridges triggers the notify?
yes, the notify only was registered on parent bus, when exist multiple
subordinate buses, we also need to notify the all vfio-pci devices under
the bus.

Thanks,
Chen

>
> Also, this assumes that the host and guest PCI configuration is static,
> which is not necessarily true for either.  So I think that caching the
> bus reset capability is wrong.
>
>> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
>> +                continue;
>> +            }
>> +            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
>> +            if (vfio_pci_host_match(&host, &tmp->host)) {
>> +                if (PCI_BUS(vdev->pdev.bus) !=
>> +                    PCI_BUS(tmp->pdev.bus)) {
>> +                    goto out;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    vdev->has_bus_reset = true;
>> +
>> +out:
>> +    g_free(info);
>> +}
>> +
>> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>> +{
>> +    VFIOGroup *group;
>> +    VFIODevice *vbasedev;
>> +
>> +    QLIST_FOREACH(group, &vfio_group_list, next) {
>> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +             vfio_check_host_bus_reset(vbasedev);
>> +         }
>> +    }
>> +}
>> +
>> +static Notifier machine_notifier = {
>> +    .notify = vfio_pci_machine_done_notify,
>> +};
>> +
>>   static int vfio_initfn(PCIDevice *pdev)
>>   {
>>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> @@ -3821,6 +3898,12 @@ static const TypeInfo vfio_pci_dev_info = {
>>   static void register_vfio_pci_dev_type(void)
>>   {
>>       type_register_static(&vfio_pci_dev_info);
>> +
>> +    /*
>> +     * Register notifier when machine init is done, since we need
>> +     * check the configration manner after all vfio device are inited.
>> +     */
>> +    qemu_add_machine_init_done_notifier(&machine_notifier);
>>   }
>>   
>>   type_init(register_vfio_pci_dev_type)
>
>
> .
>

Re: [Qemu-devel] [RFC v7 05/11] vfio: do hot bus reset when do virtual secondary bus reset

[Chen Fan]

On 05/20/2015 03:34 AM, Alex Williamson wrote:
> On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
>> when do virtual secondary bus reset, the vfio device under
>> this bus need to do host bus reset to reset the device.
>> so add this case.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c                 | 12 +++++++++++-
>>   include/hw/vfio/vfio-common.h |  1 -
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ff639db..cc169ea 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
>>       bool has_flr;
>>       bool has_pm_reset;
>>       bool has_bus_reset;
>> +    bool needs_bus_reset;
>>       bool rom_read_failed;
>>   } VFIOPCIDevice;
>>   
>> @@ -3531,7 +3532,8 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
>>       VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, sec_bus_reset_notifier);
>>       VFIODevice *vbasedev = &vdev->vbasedev;
>>   
>> -    vbasedev->needs_bus_reset = true;
>> +    vdev->needs_bus_reset = true;
>> +    vbasedev->needs_reset = true;
> The above should have happened in patch 03.
>
>>   }
>>   
>>   static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
>> @@ -3807,6 +3809,14 @@ static void vfio_pci_reset(DeviceState *dev)
>>   
>>       trace_vfio_pci_reset(vdev->vbasedev.name);
>>   
>> +    if (vdev->has_bus_reset && vdev->needs_bus_reset) {
>> +        vdev->needs_bus_reset = false;
>> +        if (vdev->vbasedev.needs_reset) {
>> +            vfio_pci_hot_reset(vdev, false);
> So we're not even going to check to see whether that worked?  How can we
> know that the configuration is still the same as after machine init?  An
> affected device could have been hot-unplugged an now in use by the host
> (in which case this function would fail).  An affected device could have
> been hot unplugged and added back to the VM under a different bus, now
> we have an arbitrary second device affected by the bus reset from the VM
> perspective.
Due to we may have the arbitrary configuration manner after hotplug
/unplug vfio-pci devices, the enabled aer feature probably became to
disabled. so shall we consider not to expose the aer feature to user,
and enable/disable it automatically?

Thanks,
Chen



>
>> +        }
>> +        return;
>> +    }
>> +
>>       vfio_pci_pre_reset(vdev);
>>   
>>       if (vdev->resetfn && !vdev->resetfn(vdev)) {
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 5f5691a..0d1fb80 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -102,7 +102,6 @@ typedef struct VFIODevice {
>>       bool reset_works;
>>       bool needs_reset;
>>       bool allow_mmap;
>> -    bool needs_bus_reset;
>>       VFIODeviceOps *ops;
>>       unsigned int num_irqs;
>>       unsigned int num_regions;
>
>
> .
>

Re: [Qemu-devel] [RFC v7 05/11] vfio: do hot bus reset when do virtual secondary bus reset

[Alex Williamson]

On Wed, 2015-05-20 at 17:59 +0800, Chen Fan wrote:
> On 05/20/2015 03:34 AM, Alex Williamson wrote:
> > On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> >> when do virtual secondary bus reset, the vfio device under
> >> this bus need to do host bus reset to reset the device.
> >> so add this case.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/vfio/pci.c                 | 12 +++++++++++-
> >>   include/hw/vfio/vfio-common.h |  1 -
> >>   2 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index ff639db..cc169ea 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
> >>       bool has_flr;
> >>       bool has_pm_reset;
> >>       bool has_bus_reset;
> >> +    bool needs_bus_reset;
> >>       bool rom_read_failed;
> >>   } VFIOPCIDevice;
> >>   
> >> @@ -3531,7 +3532,8 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
> >>       VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, sec_bus_reset_notifier);
> >>       VFIODevice *vbasedev = &vdev->vbasedev;
> >>   
> >> -    vbasedev->needs_bus_reset = true;
> >> +    vdev->needs_bus_reset = true;
> >> +    vbasedev->needs_reset = true;
> > The above should have happened in patch 03.
> >
> >>   }
> >>   
> >>   static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> >> @@ -3807,6 +3809,14 @@ static void vfio_pci_reset(DeviceState *dev)
> >>   
> >>       trace_vfio_pci_reset(vdev->vbasedev.name);
> >>   
> >> +    if (vdev->has_bus_reset && vdev->needs_bus_reset) {
> >> +        vdev->needs_bus_reset = false;
> >> +        if (vdev->vbasedev.needs_reset) {
> >> +            vfio_pci_hot_reset(vdev, false);
> > So we're not even going to check to see whether that worked?  How can we
> > know that the configuration is still the same as after machine init?  An
> > affected device could have been hot-unplugged an now in use by the host
> > (in which case this function would fail).  An affected device could have
> > been hot unplugged and added back to the VM under a different bus, now
> > we have an arbitrary second device affected by the bus reset from the VM
> > perspective.
> Due to we may have the arbitrary configuration manner after hotplug
> /unplug vfio-pci devices, the enabled aer feature probably became to
> disabled. so shall we consider not to expose the aer feature to user,
> and enable/disable it automatically?

I think that opportunistically enabling it would result in something so
complicated and unpredictable to the user that it would be
indistinguishable from a bug.  We also want to enforce that we can
support it if the user specifies they want it for a particular device.
We can manage that for guest configurations, verifying that each device
that requires it supports hotplug or if it doesn't require it, doesn't
interfere with devices that do.  We can reject hot-adds of devices that
compromise our bus reset capabilities.  The part that's out of our
control is changes on the host, but I think those are sufficiently rare
that we can verify before aer message injection that the configuration
is still viable and either inject or perform the safe halt that we do
now.  Thanks,

Alex

> >> +        }
> >> +        return;
> >> +    }
> >> +
> >>       vfio_pci_pre_reset(vdev);
> >>   
> >>       if (vdev->resetfn && !vdev->resetfn(vdev)) {
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index 5f5691a..0d1fb80 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -102,7 +102,6 @@ typedef struct VFIODevice {
> >>       bool reset_works;
> >>       bool needs_reset;
> >>       bool allow_mmap;
> >> -    bool needs_bus_reset;
> >>       VFIODeviceOps *ops;
> >>       unsigned int num_irqs;
> >>       unsigned int num_regions;
> >
> >
> > .
> >
> 

Re: [Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not

[Alex Williamson]

On Wed, 2015-05-20 at 15:24 +0800, Chen Fan wrote:
> On 05/20/2015 03:34 AM, Alex Williamson wrote:
> > On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> >> when machine is done, we should check the all vfio devices
> >> whether support host bus reset, then when need virtual secondary
> >> bus reset, we should reset host bus.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/vfio/pci.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 83 insertions(+)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 43869e9..ff639db 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice {
> >>       bool req_enabled;
> >>       bool has_flr;
> >>       bool has_pm_reset;
> >> +    bool has_bus_reset;
> >>       bool rom_read_failed;
> >>   } VFIOPCIDevice;
> >>   
> >> @@ -3533,6 +3534,82 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque)
> >>       vbasedev->needs_bus_reset = true;
> >>   }
> >>   
> > This function really needs some comments
> >
> >> +static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> >> +{
> >> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> >> +    struct vfio_pci_hot_reset_info *info = NULL;
> >> +    struct vfio_pci_dependent_device *devices;
> >> +    VFIOGroup *group;
> >> +    int ret, i;
> >> +
> >> +    ret = vfio_get_hot_reset_info(vdev, &info);
> >> +    if (ret < 0) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    devices = &info->devices[0];
> > So we have the list of host devices affected by a bus reset...
> >
> >> +
> >> +    /* Verify that we have all the groups required */
> >> +    for (i = 0; i < info->count; i++) {
> >> +        PCIHostDeviceAddress host;
> >> +        VFIOPCIDevice *tmp;
> >> +        VFIODevice *vbasedev_iter;
> >> +
> >> +        host.domain = devices[i].segment;
> >> +        host.bus = devices[i].bus;
> >> +        host.slot = PCI_SLOT(devices[i].devfn);
> >> +        host.function = PCI_FUNC(devices[i].devfn);
> >> +
> > Skip the current device
> >
> >> +        if (vfio_pci_host_match(&host, &vdev->host)) {
> >> +            continue;
> >> +        }
> >> +
> > Make sure we own the group for the device
> >
> >> +        QLIST_FOREACH(group, &vfio_group_list, next) {
> >> +            if (group->groupid == devices[i].group_id) {
> >> +                break;
> >> +            }
> >> +        }
> >> +
> >> +        if (!group) {
> >> +            goto out;
> >> +        }
> >> +
> > Search the group device list for the device, if found make sure it's on
> > the same virtual bus as the target device...  This shouldn't be limited
> > to the group device list, it should be done for all vfio-pci devices.
> why? if a bus reset affect both groups, the upper step ought to
> find the exact group, we only need to check the affected devices
> is below or on the bus.

So if we have the scenario I describe below and we assume those
functions are the only things in our affected device list, we'd skip the
one that matches our target device.  For the other one, we'd find the
group, traverse the group device list to find the vdev and test whether
it's on the same bus (or a subordinate of the same bus).  Ok, that does
seem to work if we add the subordinate support.  Thanks,

Alex

> > For instance, a dual-port card could have 2 separate physical functions
> > that are each isolated and appear in different groups.  Obviously a bus
> > reset will affect both.
> yes.
> 
> >    Anything affected by the reset needs to be on
> > (or below) the bus of the target device.  That also exposes another
> > issue that I don't see addressed here; a physical bus reset will cascade
> > to all subordinate buses, but your notifier only works via access to the
> > bus reset bit in the control register.  Do we also need to make sure the
> > reset function for PCI bridges triggers the notify?
> yes, the notify only was registered on parent bus, when exist multiple
> subordinate buses, we also need to notify the all vfio-pci devices under
> the bus.
> 
> Thanks,
> Chen
> 
> >
> > Also, this assumes that the host and guest PCI configuration is static,
> > which is not necessarily true for either.  So I think that caching the
> > bus reset capability is wrong.
> >
> >> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> >> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> >> +                continue;
> >> +            }
> >> +            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> >> +            if (vfio_pci_host_match(&host, &tmp->host)) {
> >> +                if (PCI_BUS(vdev->pdev.bus) !=
> >> +                    PCI_BUS(tmp->pdev.bus)) {
> >> +                    goto out;
> >> +                }
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    vdev->has_bus_reset = true;
> >> +
> >> +out:
> >> +    g_free(info);
> >> +}
> >> +
> >> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> >> +{
> >> +    VFIOGroup *group;
> >> +    VFIODevice *vbasedev;
> >> +
> >> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> >> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
> >> +             vfio_check_host_bus_reset(vbasedev);
> >> +         }
> >> +    }
> >> +}
> >> +
> >> +static Notifier machine_notifier = {
> >> +    .notify = vfio_pci_machine_done_notify,
> >> +};
> >> +
> >>   static int vfio_initfn(PCIDevice *pdev)
> >>   {
> >>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> >> @@ -3821,6 +3898,12 @@ static const TypeInfo vfio_pci_dev_info = {
> >>   static void register_vfio_pci_dev_type(void)
> >>   {
> >>       type_register_static(&vfio_pci_dev_info);
> >> +
> >> +    /*
> >> +     * Register notifier when machine init is done, since we need
> >> +     * check the configration manner after all vfio device are inited.
> >> +     */
> >> +    qemu_add_machine_init_done_notifier(&machine_notifier);
> >>   }
> >>   
> >>   type_init(register_vfio_pci_dev_type)
> >
> >
> > .
> >
> 

Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap

[Alex Williamson]

On Wed, 2015-05-20 at 11:43 +0800, Chen Fan wrote:
> On 05/20/2015 03:34 AM, Alex Williamson wrote:
> > On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> >> add 'aer' property to let user able to decide whether expose
> >> the aer capability. by default we should disable aer feature,
> >> because it needs configuration restrictions.
> > But the previous patch already enabled it, so a bisect might get it
> > enabled then disabled.
> >
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/i386/pc_q35.c    |  4 +++
> >>   hw/vfio/pci.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   include/hw/compat.h |  8 +++++
> >>   3 files changed, 96 insertions(+)
> >>
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index e67f2de..94a3a1c 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -439,6 +439,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
> >>       PC_Q35_2_3_MACHINE_OPTIONS,
> >>       .name = "pc-q35-2.3",
> >>       .init = pc_q35_init_2_3,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    },
> >>   };
> >>   
> >>   #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index f2fc5d6..b4b9495 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -42,6 +42,7 @@
> >>   #include "trace.h"
> >>   #include "hw/vfio/vfio.h"
> >>   #include "hw/vfio/vfio-common.h"
> >> +#include "hw/pci/pci_bridge.h"
> >>   
> >>   struct VFIOPCIDevice;
> >>   
> >> @@ -161,6 +162,8 @@ typedef struct VFIOPCIDevice {
> >>   #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> >>   #define VFIO_FEATURE_ENABLE_REQ_BIT 1
> >>   #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
> >> +#define VFIO_FEATURE_ENABLE_AER_BIT 2
> >> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
> >>       int32_t bootindex;
> >>       uint8_t pm_cap;
> >>       bool has_vga;
> >> @@ -2821,10 +2824,31 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> >>   static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
> >>   {
> >>       PCIDevice *pdev = &vdev->pdev;
> >> +    PCIDevice *dev_iter;
> >>       uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
> >>       uint32_t severity, errcap;
> >> +    uint8_t type;
> >>       int ret;
> >>   
> >> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    dev_iter = pci_bridge_get_device(pdev->bus);
> >> +    if (!dev_iter) {
> > So this is testing that it can't be on the root complex endpoint.
> >
> >> +        goto error;
> >> +    }
> >> +
> >> +    while (dev_iter) {
> >> +        type = pcie_cap_get_type(dev_iter);
> >> +        if ((type != PCI_EXP_TYPE_ROOT_PORT &&
> >> +             type != PCI_EXP_TYPE_UPSTREAM &&
> >> +             type != PCI_EXP_TYPE_DOWNSTREAM)) {
> >> +            goto error;
> >> +        }
> > And this tests that we have PCIe devices up to the root complex, but do
> > do we need to check whether they support aer?
> Exactly, although the ioh3420 port and  the xio3130 switch have
> been support aer, but in case new devices are unsupported in the future.

Yes, that's my concern, we don't want to make assumptions based on the
devices we have today that need to be revisited later.

> >> +        dev_iter = pci_bridge_get_device(dev_iter->bus);
> >> +    }
> >> +
> >>       errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
> >>       /*
> >>        * The ability to record multiple headers is depending on
> >> @@ -2850,6 +2874,11 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
> >>       pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
> >>   
> >>       return 0;
> >> +
> >> +error:
> >> +    error_report("vfio: Unable to enable AER for device %s, parent bus "
> >> +                 "do not support signal AER", vdev->vbasedev.name);
> >> +    return -1;
> >>   }
> >>   
> >>   static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
> >> @@ -3733,9 +3762,52 @@ static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> >>       vdev->has_bus_reset = true;
> >>   
> >>   out:
> >> +    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> >> +        !vdev->has_bus_reset) {
> >> +        error_report("vfio: Cannot enable AER for device %s,"
> >> +                     "which is not support host bus reset.",
> >> +                     vdev->vbasedev.name);
> >> +        exit(1);
> >> +    }
> > And this is done via the machine init callback, so again we're not doing
> > anything to support hotplug.
> Exactly, I missed the case of hotplug. I think I should implement
> the bus reset capability at each initialize time and meanwhile update
> the other devices bus reset capability on the same bus.

Right, so it's probably better to do the validation from the initfn
rather than via a separate machine init callback so that we can fail a
device_add if it would compromise our ability to do an aer bus reset.

The host device hotplug scenario is harder, maybe before injecting the
aer message to the guest, we need to verify whether a bus reset is still
available.  Once injected, we wouldn't know whether the inability to do
a bus reset should be fatal or not.

> >>       g_free(info);
> >>   }
> >>   
> >> +static int vfio_pci_validate_aer_devices(DeviceState *dev,
> >> +                                         void *opaque)
> >> +{
> >> +    VFIOPCIDevice *vdev;
> >> +
> >> +    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(dev));
> >> +    if (!vdev->has_bus_reset) {
> >> +        error_report("vfio: device %s is not support host bus reset,"
> >> +                     "but on the same bus has vfio device enable AER.",
> >> +                     vdev->vbasedev.name);
> >> +        exit(1);
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +static void vfio_check_virtual_bus_reset(VFIODevice *vbasedev)
> >> +{
> >> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> >> +
> >> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * Verify that we have all the vfio devices support host bus reset
> >> +     * on the bus
> >> +     */
> >> +    qbus_walk_children(BUS(vdev->pdev.bus), NULL, NULL,
> >> +                       vfio_pci_validate_aer_devices,
> >> +                       NULL, NULL);
> >> +}
> >> +
> >>   static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> >>   {
> >>       VFIOGroup *group;
> >> @@ -3746,6 +3818,16 @@ static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> >>                vfio_check_host_bus_reset(vbasedev);
> >>            }
> >>       }
> >> +
> >> +    /*
> >> +     * All devices need support host bus reset on each virtual bus
> >> +     * if one device enable AER.
> >> +     */
> >> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> >> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
> >> +             vfio_check_virtual_bus_reset(vbasedev);
> >> +         }
> >> +    }
> > I'm not sure I see how this call path can catch anything missed by
> > vfio_check_host_bus_reset().
> I think we should support the case that on the same virtual bus
> if one device enable AER, probably the other devices is on the
> separate bus. at least the other devices should support host
> bus reset.
> 
> 1. on the same virtual bus and on the same host bus
> 
> virtual bus    -------------------------
>                           |                 |
>                       deviceA     deviceB
>                           |                 |
> host bus      -------------------------
> 
> 
> 2. on the same virtual bus but on the respective host bus
> 
> virtual bus    -------------------------
>                           |                 |
>                       deviceA     deviceB
>                           |                 |
> host bus      ----------    ------------
> 

That's an interesting question, but I'm not sure I agree that collateral
devices need to support bus reset, or if it's sufficient that they just
need to support any reset mechanism.  We don't really let the guest see
the post-reset state of the device anyway, the host will always have
done a save/restore of state already.  So if aer is not supported on the
device and we therefore know that the guest isn't attempting to do link
renegotiation or fundamental reset specific to that device, isn't any
reset that quiesces the device sufficient?  Thanks,

Alex

> >>   }
> >>   
> >>   static Notifier machine_notifier = {
> >> @@ -4004,6 +4086,8 @@ static Property vfio_pci_dev_properties[] = {
> >>                       VFIO_FEATURE_ENABLE_VGA_BIT, false),
> >>       DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
> >>                       VFIO_FEATURE_ENABLE_REQ_BIT, true),
> >> +    DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
> >> +                    VFIO_FEATURE_ENABLE_AER_BIT, false),
> >>       DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> >>       DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
> >>       /*
> >> diff --git a/include/hw/compat.h b/include/hw/compat.h
> >> index 313682a..f722919 100644
> >> --- a/include/hw/compat.h
> >> +++ b/include/hw/compat.h
> >> @@ -1,7 +1,15 @@
> >>   #ifndef HW_COMPAT_H
> >>   #define HW_COMPAT_H
> >>   
> >> +#define HW_COMPAT_2_3 \
> >> +        {\
> >> +            .driver   = "vfio-pci",\
> >> +            .property = "x-aer",\
> > It's defined as "aer" above, not "x-aer".  I'm not sure why we need this
> > though if the default value is off anyway.
> >
> >> +            .value    = "off",\
> >> +        }
> >> +
> >>   #define HW_COMPAT_2_1 \
> >> +        HW_COMPAT_2_3, \
> >>           {\
> >>               .driver   = "intel-hda",\
> >>               .property = "old_msi_addr",\
> >
> >
> > .
> >
> 

Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap

[Chen Fan]

On 05/21/2015 02:47 AM, Alex Williamson wrote:
> On Wed, 2015-05-20 at 11:43 +0800, Chen Fan wrote:
>> On 05/20/2015 03:34 AM, Alex Williamson wrote:
>>> On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
>>>> add 'aer' property to let user able to decide whether expose
>>>> the aer capability. by default we should disable aer feature,
>>>> because it needs configuration restrictions.
>>> But the previous patch already enabled it, so a bisect might get it
>>> enabled then disabled.
>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>> ---
>>>>    hw/i386/pc_q35.c    |  4 +++
>>>>    hw/vfio/pci.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/hw/compat.h |  8 +++++
>>>>    3 files changed, 96 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>>> index e67f2de..94a3a1c 100644
>>>> --- a/hw/i386/pc_q35.c
>>>> +++ b/hw/i386/pc_q35.c
>>>> @@ -439,6 +439,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>>>        PC_Q35_2_3_MACHINE_OPTIONS,
>>>>        .name = "pc-q35-2.3",
>>>>        .init = pc_q35_init_2_3,
>>>> +    .compat_props = (GlobalProperty[]) {
>>>> +        HW_COMPAT_2_3,
>>>> +        { /* end of list */ }
>>>> +    },
>>>>    };
>>>>    
>>>>    #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index f2fc5d6..b4b9495 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -42,6 +42,7 @@
>>>>    #include "trace.h"
>>>>    #include "hw/vfio/vfio.h"
>>>>    #include "hw/vfio/vfio-common.h"
>>>> +#include "hw/pci/pci_bridge.h"
>>>>    
>>>>    struct VFIOPCIDevice;
>>>>    
>>>> @@ -161,6 +162,8 @@ typedef struct VFIOPCIDevice {
>>>>    #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>>>>    #define VFIO_FEATURE_ENABLE_REQ_BIT 1
>>>>    #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
>>>> +#define VFIO_FEATURE_ENABLE_AER_BIT 2
>>>> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
>>>>        int32_t bootindex;
>>>>        uint8_t pm_cap;
>>>>        bool has_vga;
>>>> @@ -2821,10 +2824,31 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>>>    static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>>>>    {
>>>>        PCIDevice *pdev = &vdev->pdev;
>>>> +    PCIDevice *dev_iter;
>>>>        uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
>>>>        uint32_t severity, errcap;
>>>> +    uint8_t type;
>>>>        int ret;
>>>>    
>>>> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    dev_iter = pci_bridge_get_device(pdev->bus);
>>>> +    if (!dev_iter) {
>>> So this is testing that it can't be on the root complex endpoint.
>>>
>>>> +        goto error;
>>>> +    }
>>>> +
>>>> +    while (dev_iter) {
>>>> +        type = pcie_cap_get_type(dev_iter);
>>>> +        if ((type != PCI_EXP_TYPE_ROOT_PORT &&
>>>> +             type != PCI_EXP_TYPE_UPSTREAM &&
>>>> +             type != PCI_EXP_TYPE_DOWNSTREAM)) {
>>>> +            goto error;
>>>> +        }
>>> And this tests that we have PCIe devices up to the root complex, but do
>>> do we need to check whether they support aer?
>> Exactly, although the ioh3420 port and  the xio3130 switch have
>> been support aer, but in case new devices are unsupported in the future.
> Yes, that's my concern, we don't want to make assumptions based on the
> devices we have today that need to be revisited later.
>
>>>> +        dev_iter = pci_bridge_get_device(dev_iter->bus);
>>>> +    }
>>>> +
>>>>        errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
>>>>        /*
>>>>         * The ability to record multiple headers is depending on
>>>> @@ -2850,6 +2874,11 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>>>>        pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
>>>>    
>>>>        return 0;
>>>> +
>>>> +error:
>>>> +    error_report("vfio: Unable to enable AER for device %s, parent bus "
>>>> +                 "do not support signal AER", vdev->vbasedev.name);
>>>> +    return -1;
>>>>    }
>>>>    
>>>>    static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
>>>> @@ -3733,9 +3762,52 @@ static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
>>>>        vdev->has_bus_reset = true;
>>>>    
>>>>    out:
>>>> +    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
>>>> +        !vdev->has_bus_reset) {
>>>> +        error_report("vfio: Cannot enable AER for device %s,"
>>>> +                     "which is not support host bus reset.",
>>>> +                     vdev->vbasedev.name);
>>>> +        exit(1);
>>>> +    }
>>> And this is done via the machine init callback, so again we're not doing
>>> anything to support hotplug.
>> Exactly, I missed the case of hotplug. I think I should implement
>> the bus reset capability at each initialize time and meanwhile update
>> the other devices bus reset capability on the same bus.
> Right, so it's probably better to do the validation from the initfn
> rather than via a separate machine init callback so that we can fail a
> device_add if it would compromise our ability to do an aer bus reset.
>
> The host device hotplug scenario is harder, maybe before injecting the
> aer message to the guest, we need to verify whether a bus reset is still
> available.  Once injected, we wouldn't know whether the inability to do
> a bus reset should be fatal or not.
>
>>>>        g_free(info);
>>>>    }
>>>>    
>>>> +static int vfio_pci_validate_aer_devices(DeviceState *dev,
>>>> +                                         void *opaque)
>>>> +{
>>>> +    VFIOPCIDevice *vdev;
>>>> +
>>>> +    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(dev));
>>>> +    if (!vdev->has_bus_reset) {
>>>> +        error_report("vfio: device %s is not support host bus reset,"
>>>> +                     "but on the same bus has vfio device enable AER.",
>>>> +                     vdev->vbasedev.name);
>>>> +        exit(1);
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void vfio_check_virtual_bus_reset(VFIODevice *vbasedev)
>>>> +{
>>>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>>>> +
>>>> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Verify that we have all the vfio devices support host bus reset
>>>> +     * on the bus
>>>> +     */
>>>> +    qbus_walk_children(BUS(vdev->pdev.bus), NULL, NULL,
>>>> +                       vfio_pci_validate_aer_devices,
>>>> +                       NULL, NULL);
>>>> +}
>>>> +
>>>>    static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>>>>    {
>>>>        VFIOGroup *group;
>>>> @@ -3746,6 +3818,16 @@ static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>>>>                 vfio_check_host_bus_reset(vbasedev);
>>>>             }
>>>>        }
>>>> +
>>>> +    /*
>>>> +     * All devices need support host bus reset on each virtual bus
>>>> +     * if one device enable AER.
>>>> +     */
>>>> +    QLIST_FOREACH(group, &vfio_group_list, next) {
>>>> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>>> +             vfio_check_virtual_bus_reset(vbasedev);
>>>> +         }
>>>> +    }
>>> I'm not sure I see how this call path can catch anything missed by
>>> vfio_check_host_bus_reset().
>> I think we should support the case that on the same virtual bus
>> if one device enable AER, probably the other devices is on the
>> separate bus. at least the other devices should support host
>> bus reset.
>>
>> 1. on the same virtual bus and on the same host bus
>>
>> virtual bus    -------------------------
>>                            |                 |
>>                        deviceA     deviceB
>>                            |                 |
>> host bus      -------------------------
>>
>>
>> 2. on the same virtual bus but on the respective host bus
>>
>> virtual bus    -------------------------
>>                            |                 |
>>                        deviceA     deviceB
>>                            |                 |
>> host bus      ----------    ------------
>>
> That's an interesting question, but I'm not sure I agree that collateral
> devices need to support bus reset, or if it's sufficient that they just
> need to support any reset mechanism.  We don't really let the guest see
> the post-reset state of the device anyway, the host will always have
> done a save/restore of state already.  So if aer is not supported on the
> device and we therefore know that the guest isn't attempting to do link
> renegotiation or fundamental reset specific to that device, isn't any
> reset that quiesces the device sufficient?  Thanks,
I agree.

Thanks,
Chen

>
> Alex
>
>>>>    }
>>>>    
>>>>    static Notifier machine_notifier = {
>>>> @@ -4004,6 +4086,8 @@ static Property vfio_pci_dev_properties[] = {
>>>>                        VFIO_FEATURE_ENABLE_VGA_BIT, false),
>>>>        DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
>>>>                        VFIO_FEATURE_ENABLE_REQ_BIT, true),
>>>> +    DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
>>>> +                    VFIO_FEATURE_ENABLE_AER_BIT, false),
>>>>        DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>>>>        DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
>>>>        /*
>>>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>>> index 313682a..f722919 100644
>>>> --- a/include/hw/compat.h
>>>> +++ b/include/hw/compat.h
>>>> @@ -1,7 +1,15 @@
>>>>    #ifndef HW_COMPAT_H
>>>>    #define HW_COMPAT_H
>>>>    
>>>> +#define HW_COMPAT_2_3 \
>>>> +        {\
>>>> +            .driver   = "vfio-pci",\
>>>> +            .property = "x-aer",\
>>> It's defined as "aer" above, not "x-aer".  I'm not sure why we need this
>>> though if the default value is off anyway.
>>>
>>>> +            .value    = "off",\
>>>> +        }
>>>> +
>>>>    #define HW_COMPAT_2_1 \
>>>> +        HW_COMPAT_2_3, \
>>>>            {\
>>>>                .driver   = "intel-hda",\
>>>>                .property = "old_msi_addr",\
>>>
>>> .
>>>
>
>
>
> .
>