vfio-pci: pass the aer error to guest

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 <[email protected]>

---
 hw/vfio/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 18 deletions(-)

hw/vfio/pci.c +29 -0

index e0e339a..9c05304 100644
@@ -2641,6 +2641,50 @@ 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));
+

hw/vfio/pci.c +6 -5

+error:
+    g_free(info);
+    return ret;
+}
+
 static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
 {
     PCIDevice *pdev = &vdev->pdev;

@@ -2780,7 +2824,7 @@ static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
 static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
 {
     VFIOGroup *group;
-    struct vfio_pci_hot_reset_info *info;
+    struct vfio_pci_hot_reset_info *info = NULL;
     struct vfio_pci_dependent_device *devices;
     struct vfio_pci_hot_reset *reset;
     int32_t *fds;

@@ -2792,32 +2836,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);

hw/vfio/pci.c +0 -11

         }
         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 */

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 <[email protected]>

---
 hw/vfio/pci.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

hw/vfio/pci.c +4 -0

index 78f4c82..81a4a9a 100644
@@ -2493,6 +2493,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;

hw/vfio/pci.c +42 -1

+
 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);

@@ -2802,16 +2817,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;
+    }
+

hw/vfio/pci.c +6 -0

+     */
+    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)

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 <[email protected]>

---
 hw/vfio/pci.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

hw/vfio/pci.c +4 -0

index 81a4a9a..f4e7855 100644
@@ -32,6 +32,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 #include "qemu/event_notifier.h"

@@ -160,6 +161,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;

@@ -2817,12 +2820,78 @@ 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)

hw/vfio/pci.c +41 -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;
+        }
+
+        if (!dev_iter->exp.aer_cap) {
+            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
+     * 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;
+    }
+

hw/vfio/pci.c +17 -1

+    return -1;
+}
+
 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))) {

@@ -2838,7 +2907,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));

we introduce a has_bus_reset capability to sign the vfio devices if support host bus reset.

Signed-off-by Chen Fan <[email protected]>

---
 hw/vfio/pci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

hw/vfio/pci.c +1 -0

index f4e7855..5934fd7 100644
@@ -33,6 +33,7 @@
 #include "hw/pci/msix.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_bus.h"
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 #include "qemu/event_notifier.h"

hw/vfio/pci.c +1 -0

     bool rom_read_failed;
 } VFIOPCIDevice;
 

@@ -203,6 +205,7 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
 static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                   uint32_t val, int len);
 static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev);
 
 /*
  * Disabling BAR mmaping can be slow, but toggling it around INTx can

@@ -2853,6 +2856,20 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
         dev_iter = pci_bridge_get_device(dev_iter->bus);
     }

hw/vfio/pci.c +17 -0

+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        if (!bus->devices[i]) {
+            continue;
+        }
+
+        if (bus->devices[i] == find->pdev) {
+            find->pbus = bus;
+            find->found = true;
+            break;
+        }
+    }
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev)

hw/vfio/pci.c +45 -0

+        goto out;
+    }
+
+    /* List all affected devices by bus reset */
+    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);
+
+        /* Skip the current device */
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        /* Ensure affected devices for reset under the same bus */
+        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)) {
+                struct VfioDeviceFind find = { .pdev = &tmp->pdev, .found = false };
+
+                pci_for_each_bus(bus, find_devices, &find);
+                if (!find.found) {
+                    goto out;

hw/vfio/pci.c +3 -0

+    g_free(info);
+}
+
 static int vfio_initfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);

Quoted 21 lines · Chen Fan

we introduce a has_bus_reset capability to sign the vfio devices if support host bus reset.

Signed-off-by Chen Fan <[email protected]>

---
  hw/vfio/pci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 123 insertions(+)

hw/vfio/pci.c +1 -0

index f4e7855..5934fd7 100644
@@ -33,6 +33,7 @@
  #include "hw/pci/msix.h"
  #include "hw/pci/pci.h"
  #include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_bus.h"
  #include "qemu-common.h"
  #include "qemu/error-report.h"
  #include "qemu/event_notifier.h"

hw/vfio/pci.c typedef struct VFIOPCIDevice { · line 174

Quoted 4 lines

hw/vfio/pci.c +1 -0

@@ -170,6 +171,7 @@ typedef struct VFIOPCIDevice {
      bool req_enabled;
      bool has_flr;
      bool has_pm_reset;
+    bool has_bus_reset;

I still think that caching this is a bad idea, there's no point at which we can blindly assume the capability is still present.

Quoted 14 lines

hw/vfio/pci.c +1 -0

      bool rom_read_failed;
  } VFIOPCIDevice;
 

@@ -203,6 +205,7 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
  static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                    uint32_t val, int len);
  static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev);
 
  /*
   * Disabling BAR mmaping can be slow, but toggling it around INTx can

@@ -2853,6 +2856,20 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
          dev_iter = pci_bridge_get_device(dev_iter->bus);
      }

hw/vfio/pci.c static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) · line 2867

Quoted 10 lines

hw/vfio/pci.c +9 -0

 
+    /*
+     * Don't check bus reset capability when device is enabled during
+     * qemu machine creation, which is done by machine init function.
+     */
+    if (DEVICE(vdev)->hotplugged) {
+        vfio_check_host_bus_reset(vdev);
+        if (!vdev->has_bus_reset) {
+            error_report("vfio: Cannot enable AER for device %s, "
+                         "which is not support host bus reset.",

"which does not support host bus reset."

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3698

Quoted 13 lines

hw/vfio/pci.c +6 -0

+                         vdev->vbasedev.name);
+            goto error;
+        }
+    }
+
      errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
      /*
       * The ability to record multiple headers is depending on

@@ -3678,6 +3695,112 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
      }
  }
 
+struct VfioDeviceFind {

We use VFIOFooBar for all other camel case definitions, much like PCIBus and PCIDevice below.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3709

Quoted 11 lines

hw/vfio/pci.c +11 -0

+    PCIBus *pbus;
+    PCIDevice *pdev;
+    bool found;
+};
+
+static void find_devices(PCIBus *bus, void *opaque)
+{
+    struct VfioDeviceFind *find = opaque;
+    int i;
+
+    if (find->found == true) {

if (find->found) {...

Quoted 17 lines

hw/vfio/pci.c +17 -0

+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        if (!bus->devices[i]) {
+            continue;
+        }
+
+        if (bus->devices[i] == find->pdev) {
+            find->pbus = bus;
+            find->found = true;
+            break;
+        }
+    }
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev)

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3736

Quoted 10 lines

hw/vfio/pci.c +10 -0

+{
+    PCIBus *bus = vdev->pdev.bus;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    VFIOGroup *group;
+    int ret, i;
+    bool has_bus_reset = false;
+
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret < 0) {

if (ret) {...

Quoted 45 lines

hw/vfio/pci.c +45 -0

+        goto out;
+    }
+
+    /* List all affected devices by bus reset */
+    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);
+
+        /* Skip the current device */
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        /* Ensure affected devices for reset under the same bus */
+        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)) {
+                struct VfioDeviceFind find = { .pdev = &tmp->pdev, .found = false };
+
+                pci_for_each_bus(bus, find_devices, &find);
+                if (!find.found) {
+                    goto out;

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3791

Quoted 10 lines

hw/vfio/pci.c +10 -0

+                }
+                /*
+                 * When the check device is hotplugged to a higher bus again,
+                 * which would influence the affected device which enable aer
+                 * below the bus.
+                 */
+                if (tmp->features & VFIO_FEATURE_ENABLE_AER &&
+                    find.pbus != bus) {
+                    goto out;
+                }

I think what you're trying to do here is assume that if a reset of A affects B, then a reset of B affects A, and if B is on a subordinate bus from A, then our configuration is broken, right? Can we really guarantee that assumption? If we had a physical topology that mirrored this virtual topology, that wouldn't necessarily be true. For instance, if A was a function of a multi-function device where another function was a PCIe upstream switch, B could be subordinate to that switch, so a reset of A affects B, but a reset of B doesn't affect A.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3800

Quoted 9 lines

hw/vfio/pci.c +9 -0

+                break;
+            }
+        }
+    }
+
+    has_bus_reset = true;
+
+out:
+    vdev->has_bus_reset = has_bus_reset;

I don't see any value is storing this, it can't be trusted at any point in the future. I think that any time we add a device or think about forwarding an AER message to the guest, we need to do a validation pass, testing the entire topology.

To do that we'd iterate through every device in every group for PCI devices that support AER. For each one, get the hot reset info for the affected device list. For each affected device, if it's attached to the VM, it needs to be on or below the bus of the target device. Additionally, we should test all of the non-AER supporting vfio-pci devices on or below the target bus to verify they have a reset mechanism. Run that function for each vfio-pci device as it's added, regardless of whether it's hotplug. If the test fails, fail the initfn for the current device. Also run the test prior to AER injection, if it fails demote the AER injection to a machine halt as we have now.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3806

Quoted 7 lines

hw/vfio/pci.c +3 -0

+    g_free(info);
+}
+
  static int vfio_initfn(PCIDevice *pdev)
  {
      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);

.

Quoted 191 lines · Alex Williamson

Quoted 21 lines · Chen Fan

we introduce a has_bus_reset capability to sign the vfio devices if support host bus reset.

Signed-off-by Chen Fan <[email protected]>

---
  hw/vfio/pci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 123 insertions(+)

hw/vfio/pci.c +1 -0

index f4e7855..5934fd7 100644
@@ -33,6 +33,7 @@
  #include "hw/pci/msix.h"
  #include "hw/pci/pci.h"
  #include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_bus.h"
  #include "qemu-common.h"
  #include "qemu/error-report.h"
  #include "qemu/event_notifier.h"

hw/vfio/pci.c typedef struct VFIOPCIDevice { · line 174

Quoted 4 lines

hw/vfio/pci.c +1 -0

@@ -170,6 +171,7 @@ typedef struct VFIOPCIDevice {
      bool req_enabled;
      bool has_flr;
      bool has_pm_reset;
+    bool has_bus_reset;

I still think that caching this is a bad idea, there's no point at which we can blindly assume the capability is still present.

Quoted 14 lines

hw/vfio/pci.c +1 -0

      bool rom_read_failed;
  } VFIOPCIDevice;
 

@@ -203,6 +205,7 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
  static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                    uint32_t val, int len);
  static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev);
 
  /*
   * Disabling BAR mmaping can be slow, but toggling it around INTx can

@@ -2853,6 +2856,20 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
          dev_iter = pci_bridge_get_device(dev_iter->bus);
      }

hw/vfio/pci.c static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) · line 2867

Quoted 10 lines

hw/vfio/pci.c +9 -0

 
+    /*
+     * Don't check bus reset capability when device is enabled during
+     * qemu machine creation, which is done by machine init function.
+     */
+    if (DEVICE(vdev)->hotplugged) {
+        vfio_check_host_bus_reset(vdev);
+        if (!vdev->has_bus_reset) {
+            error_report("vfio: Cannot enable AER for device %s, "
+                         "which is not support host bus reset.",

"which does not support host bus reset."

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3698

Quoted 13 lines

hw/vfio/pci.c +6 -0

+                         vdev->vbasedev.name);
+            goto error;
+        }
+    }
+
      errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
      /*
       * The ability to record multiple headers is depending on

@@ -3678,6 +3695,112 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
      }
  }
 
+struct VfioDeviceFind {

We use VFIOFooBar for all other camel case definitions, much like PCIBus and PCIDevice below.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3709

Quoted 11 lines

hw/vfio/pci.c +11 -0

+    PCIBus *pbus;
+    PCIDevice *pdev;
+    bool found;
+};
+
+static void find_devices(PCIBus *bus, void *opaque)
+{
+    struct VfioDeviceFind *find = opaque;
+    int i;
+
+    if (find->found == true) {

if (find->found) {...

Quoted 17 lines

hw/vfio/pci.c +17 -0

+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        if (!bus->devices[i]) {
+            continue;
+        }
+
+        if (bus->devices[i] == find->pdev) {
+            find->pbus = bus;
+            find->found = true;
+            break;
+        }
+    }
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev)

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3736

Quoted 10 lines

hw/vfio/pci.c +10 -0

+{
+    PCIBus *bus = vdev->pdev.bus;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    VFIOGroup *group;
+    int ret, i;
+    bool has_bus_reset = false;
+
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret < 0) {

if (ret) {...

Quoted 45 lines

hw/vfio/pci.c +45 -0

+        goto out;
+    }
+
+    /* List all affected devices by bus reset */
+    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);
+
+        /* Skip the current device */
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        /* Ensure affected devices for reset under the same bus */
+        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)) {
+                struct VfioDeviceFind find = { .pdev = &tmp->pdev, .found = false };
+
+                pci_for_each_bus(bus, find_devices, &find);
+                if (!find.found) {
+                    goto out;

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3791

Quoted 10 lines

hw/vfio/pci.c +10 -0

+                }
+                /*
+                 * When the check device is hotplugged to a higher bus again,
+                 * which would influence the affected device which enable aer
+                 * below the bus.
+                 */
+                if (tmp->features & VFIO_FEATURE_ENABLE_AER &&
+                    find.pbus != bus) {
+                    goto out;
+                }

I think what you're trying to do here is assume that if a reset of A affects B, then a reset of B affects A, and if B is on a subordinate bus from A, then our configuration is broken, right? Can we really guarantee that assumption? If we had a physical topology that mirrored this virtual topology, that wouldn't necessarily be true. For instance, if A was a function of a multi-function device where another function was a PCIe upstream switch, B could be subordinate to that switch, so a reset of A affects B, but a reset of B doesn't affect A.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3800

Quoted 9 lines

hw/vfio/pci.c +9 -0

+                break;
+            }
+        }
+    }
+
+    has_bus_reset = true;
+
+out:
+    vdev->has_bus_reset = has_bus_reset;

I don't see any value is storing this, it can't be trusted at any point in the future. I think that any time we add a device or think about forwarding an AER message to the guest, we need to do a validation pass, testing the entire topology.

Quoted 10 lines

To do that we'd iterate through every device in every group for PCI devices that support AER. For each one, get the hot reset info for the affected device list. For each affected device, if it's attached to the VM, it needs to be on or below the bus of the target device. Additionally, we should test all of the non-AER supporting vfio-pci devices on or below the target bus to verify they have a reset mechanism. Run that function for each vfio-pci device as it's added, regardless of whether it's hotplug. If the test fails, fail the initfn for the current device. Also run the test prior to AER injection, if it fails demote the AER injection to a machine halt as we have now.

I'm worry about is the case that the affected devices belonged to another groups but when initialize this device the another group has not been added. it would cause fail the initfn, but the group maybe added later. so I used the machine done event notifier to check this case. if we don't do like that. how can we check the case when all vfio-pci devices initfn ?

Quoted 202 lines · Chen Fan

Quoted 191 lines · Alex Williamson

Quoted 21 lines · Chen Fan

we introduce a has_bus_reset capability to sign the vfio devices if support host bus reset.

Signed-off-by Chen Fan <[email protected]>

---
   hw/vfio/pci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
   1 file changed, 123 insertions(+)

hw/vfio/pci.c +1 -0

index f4e7855..5934fd7 100644
@@ -33,6 +33,7 @@
   #include "hw/pci/msix.h"
   #include "hw/pci/pci.h"
   #include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_bus.h"
   #include "qemu-common.h"
   #include "qemu/error-report.h"
   #include "qemu/event_notifier.h"

hw/vfio/pci.c typedef struct VFIOPCIDevice { · line 174

Quoted 4 lines

hw/vfio/pci.c +1 -0

@@ -170,6 +171,7 @@ typedef struct VFIOPCIDevice {
       bool req_enabled;
       bool has_flr;
       bool has_pm_reset;
+    bool has_bus_reset;

I still think that caching this is a bad idea, there's no point at which we can blindly assume the capability is still present.

Quoted 14 lines

hw/vfio/pci.c +1 -0

       bool rom_read_failed;
   } VFIOPCIDevice;
 

@@ -203,6 +205,7 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
   static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                     uint32_t val, int len);
   static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev);
 
   /*
    * Disabling BAR mmaping can be slow, but toggling it around INTx can

@@ -2853,6 +2856,20 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
           dev_iter = pci_bridge_get_device(dev_iter->bus);
       }

hw/vfio/pci.c static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) · line 2867

Quoted 10 lines

hw/vfio/pci.c +9 -0

 
+    /*
+     * Don't check bus reset capability when device is enabled during
+     * qemu machine creation, which is done by machine init function.
+     */
+    if (DEVICE(vdev)->hotplugged) {
+        vfio_check_host_bus_reset(vdev);
+        if (!vdev->has_bus_reset) {
+            error_report("vfio: Cannot enable AER for device %s, "
+                         "which is not support host bus reset.",

"which does not support host bus reset."

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3698

Quoted 13 lines

hw/vfio/pci.c +6 -0

+                         vdev->vbasedev.name);
+            goto error;
+        }
+    }
+
       errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
       /*
        * The ability to record multiple headers is depending on

@@ -3678,6 +3695,112 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
       }
   }
 
+struct VfioDeviceFind {

We use VFIOFooBar for all other camel case definitions, much like PCIBus and PCIDevice below.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3709

Quoted 11 lines

hw/vfio/pci.c +11 -0

+    PCIBus *pbus;
+    PCIDevice *pdev;
+    bool found;
+};
+
+static void find_devices(PCIBus *bus, void *opaque)
+{
+    struct VfioDeviceFind *find = opaque;
+    int i;
+
+    if (find->found == true) {

if (find->found) {...

Quoted 17 lines

hw/vfio/pci.c +17 -0

+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        if (!bus->devices[i]) {
+            continue;
+        }
+
+        if (bus->devices[i] == find->pdev) {
+            find->pbus = bus;
+            find->found = true;
+            break;
+        }
+    }
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev)

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3736

Quoted 10 lines

hw/vfio/pci.c +10 -0

+{
+    PCIBus *bus = vdev->pdev.bus;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    VFIOGroup *group;
+    int ret, i;
+    bool has_bus_reset = false;
+
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret < 0) {

if (ret) {...

Quoted 45 lines

hw/vfio/pci.c +45 -0

+        goto out;
+    }
+
+    /* List all affected devices by bus reset */
+    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);
+
+        /* Skip the current device */
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        /* Ensure affected devices for reset under the same bus */
+        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)) {
+                struct VfioDeviceFind find = { .pdev = &tmp->pdev, .found = false };
+
+                pci_for_each_bus(bus, find_devices, &find);
+                if (!find.found) {
+                    goto out;

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3791

Quoted 10 lines

hw/vfio/pci.c +10 -0

+                }
+                /*
+                 * When the check device is hotplugged to a higher bus again,
+                 * which would influence the affected device which enable aer
+                 * below the bus.
+                 */
+                if (tmp->features & VFIO_FEATURE_ENABLE_AER &&
+                    find.pbus != bus) {
+                    goto out;
+                }

I think what you're trying to do here is assume that if a reset of A affects B, then a reset of B affects A, and if B is on a subordinate bus from A, then our configuration is broken, right? Can we really guarantee that assumption? If we had a physical topology that mirrored this virtual topology, that wouldn't necessarily be true. For instance, if A was a function of a multi-function device where another function was a PCIe upstream switch, B could be subordinate to that switch, so a reset of A affects B, but a reset of B doesn't affect A.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3800

Quoted 9 lines

hw/vfio/pci.c +9 -0

+                break;
+            }
+        }
+    }
+
+    has_bus_reset = true;
+
+out:
+    vdev->has_bus_reset = has_bus_reset;

I don't see any value is storing this, it can't be trusted at any point in the future. I think that any time we add a device or think about forwarding an AER message to the guest, we need to do a validation pass, testing the entire topology.

Quoted 10 lines

To do that we'd iterate through every device in every group for PCI devices that support AER. For each one, get the hot reset info for the affected device list. For each affected device, if it's attached to the VM, it needs to be on or below the bus of the target device. Additionally, we should test all of the non-AER supporting vfio-pci devices on or below the target bus to verify they have a reset mechanism. Run that function for each vfio-pci device as it's added, regardless of whether it's hotplug. If the test fails, fail the initfn for the current device. Also run the test prior to AER injection, if it fails demote the AER injection to a machine halt as we have now.

Quoted 6 lines

I'm worry about is the case that the affected devices belonged to another groups but when initialize this device the another group has not been added. it would cause fail the initfn, but the group maybe added later. so I used the machine done event notifier to check this case. if we don't do like that. how can we check the case when all vfio-pci devices initfn ?

That's why the initfn test needs to test the entire topology, not just the device being added. If we have the case you describe where we have two devices in separate groups, we add the first device, test the topology, see that the second device is affected but not yet added and allow AER to be enabled. When the second device is added, we again test the topology, we see the potential conflict and fail the initfn for the second device if the bus requirements are not met.

I don't see the advantage of using a machine init done notifier. You can perform fewer topology verification passes using that notifier, but I think you can provide a more useful error message by testing on each device addition. We also use the exact same strategy for cold-plug and hot-plug, which makes maintenance easier. Thanks,

Alex

.

Quoted 209 lines · Alex Williamson

Quoted 202 lines · Chen Fan

Quoted 191 lines · Alex Williamson

Quoted 21 lines · Chen Fan

we introduce a has_bus_reset capability to sign the vfio devices if support host bus reset.

Signed-off-by Chen Fan <[email protected]>

---
   hw/vfio/pci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
   1 file changed, 123 insertions(+)

hw/vfio/pci.c +1 -0

index f4e7855..5934fd7 100644
@@ -33,6 +33,7 @@
   #include "hw/pci/msix.h"
   #include "hw/pci/pci.h"
   #include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_bus.h"
   #include "qemu-common.h"
   #include "qemu/error-report.h"
   #include "qemu/event_notifier.h"

hw/vfio/pci.c typedef struct VFIOPCIDevice { · line 174

Quoted 4 lines

hw/vfio/pci.c +1 -0

@@ -170,6 +171,7 @@ typedef struct VFIOPCIDevice {
       bool req_enabled;
       bool has_flr;
       bool has_pm_reset;
+    bool has_bus_reset;

I still think that caching this is a bad idea, there's no point at which we can blindly assume the capability is still present.

Quoted 14 lines

hw/vfio/pci.c +1 -0

       bool rom_read_failed;
   } VFIOPCIDevice;
 

@@ -203,6 +205,7 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
   static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                     uint32_t val, int len);
   static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev);
 
   /*
    * Disabling BAR mmaping can be slow, but toggling it around INTx can

@@ -2853,6 +2856,20 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
           dev_iter = pci_bridge_get_device(dev_iter->bus);
       }

hw/vfio/pci.c static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) · line 2867

Quoted 10 lines

hw/vfio/pci.c +9 -0

 
+    /*
+     * Don't check bus reset capability when device is enabled during
+     * qemu machine creation, which is done by machine init function.
+     */
+    if (DEVICE(vdev)->hotplugged) {
+        vfio_check_host_bus_reset(vdev);
+        if (!vdev->has_bus_reset) {
+            error_report("vfio: Cannot enable AER for device %s, "
+                         "which is not support host bus reset.",

"which does not support host bus reset."

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3698

Quoted 13 lines

hw/vfio/pci.c +6 -0

+                         vdev->vbasedev.name);
+            goto error;
+        }
+    }
+
       errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
       /*
        * The ability to record multiple headers is depending on

@@ -3678,6 +3695,112 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
       }
   }
 
+struct VfioDeviceFind {

We use VFIOFooBar for all other camel case definitions, much like PCIBus and PCIDevice below.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3709

Quoted 11 lines

hw/vfio/pci.c +11 -0

+    PCIBus *pbus;
+    PCIDevice *pdev;
+    bool found;
+};
+
+static void find_devices(PCIBus *bus, void *opaque)
+{
+    struct VfioDeviceFind *find = opaque;
+    int i;
+
+    if (find->found == true) {

if (find->found) {...

Quoted 17 lines

hw/vfio/pci.c +17 -0

+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        if (!bus->devices[i]) {
+            continue;
+        }
+
+        if (bus->devices[i] == find->pdev) {
+            find->pbus = bus;
+            find->found = true;
+            break;
+        }
+    }
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev)

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3736

Quoted 10 lines

hw/vfio/pci.c +10 -0

+{
+    PCIBus *bus = vdev->pdev.bus;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    VFIOGroup *group;
+    int ret, i;
+    bool has_bus_reset = false;
+
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret < 0) {

if (ret) {...

Quoted 45 lines

hw/vfio/pci.c +45 -0

+        goto out;
+    }
+
+    /* List all affected devices by bus reset */
+    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);
+
+        /* Skip the current device */
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        /* Ensure affected devices for reset under the same bus */
+        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)) {
+                struct VfioDeviceFind find = { .pdev = &tmp->pdev, .found = false };
+
+                pci_for_each_bus(bus, find_devices, &find);
+                if (!find.found) {
+                    goto out;

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3791

Quoted 10 lines

hw/vfio/pci.c +10 -0

+                }
+                /*
+                 * When the check device is hotplugged to a higher bus again,
+                 * which would influence the affected device which enable aer
+                 * below the bus.
+                 */
+                if (tmp->features & VFIO_FEATURE_ENABLE_AER &&
+                    find.pbus != bus) {
+                    goto out;
+                }

I think what you're trying to do here is assume that if a reset of A affects B, then a reset of B affects A, and if B is on a subordinate bus from A, then our configuration is broken, right? Can we really guarantee that assumption? If we had a physical topology that mirrored this virtual topology, that wouldn't necessarily be true. For instance, if A was a function of a multi-function device where another function was a PCIe upstream switch, B could be subordinate to that switch, so a reset of A affects B, but a reset of B doesn't affect A.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3800

Quoted 9 lines

hw/vfio/pci.c +9 -0

+                break;
+            }
+        }
+    }
+
+    has_bus_reset = true;
+
+out:
+    vdev->has_bus_reset = has_bus_reset;

I don't see any value is storing this, it can't be trusted at any point in the future. I think that any time we add a device or think about forwarding an AER message to the guest, we need to do a validation pass, testing the entire topology.

Quoted 10 lines

To do that we'd iterate through every device in every group for PCI devices that support AER. For each one, get the hot reset info for the affected device list. For each affected device, if it's attached to the VM, it needs to be on or below the bus of the target device. Additionally, we should test all of the non-AER supporting vfio-pci devices on or below the target bus to verify they have a reset mechanism. Run that function for each vfio-pci device as it's added, regardless of whether it's hotplug. If the test fails, fail the initfn for the current device. Also run the test prior to AER injection, if it fails demote the AER injection to a machine halt as we have now.

Quoted 6 lines

I'm worry about is the case that the affected devices belonged to another groups but when initialize this device the another group has not been added. it would cause fail the initfn, but the group maybe added later. so I used the machine done event notifier to check this case. if we don't do like that. how can we check the case when all vfio-pci devices initfn ?

Quoted 7 lines

That's why the initfn test needs to test the entire topology, not just the device being added. If we have the case you describe where we have two devices in separate groups, we add the first device, test the topology, see that the second device is affected but not yet added and allow AER to be enabled. When the second device is added, we again test the topology, we see the potential conflict and fail the initfn for the second device if the bus requirements are not met.

In case that the second device may not be added. in this case the first device enable the aer, and pass the validate. how do we know the second device if be added ?

Quoted 217 lines · Chen Fan

Quoted 209 lines · Alex Williamson

Quoted 202 lines · Chen Fan

Quoted 191 lines · Alex Williamson

Quoted 21 lines · Chen Fan

we introduce a has_bus_reset capability to sign the vfio devices if support host bus reset.

Signed-off-by Chen Fan <[email protected]>

---
    hw/vfio/pci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 123 insertions(+)

hw/vfio/pci.c +1 -0

index f4e7855..5934fd7 100644
@@ -33,6 +33,7 @@
    #include "hw/pci/msix.h"
    #include "hw/pci/pci.h"
    #include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_bus.h"
    #include "qemu-common.h"
    #include "qemu/error-report.h"
    #include "qemu/event_notifier.h"

hw/vfio/pci.c typedef struct VFIOPCIDevice { · line 174

Quoted 4 lines

hw/vfio/pci.c +1 -0

@@ -170,6 +171,7 @@ typedef struct VFIOPCIDevice {
        bool req_enabled;
        bool has_flr;
        bool has_pm_reset;
+    bool has_bus_reset;

I still think that caching this is a bad idea, there's no point at which we can blindly assume the capability is still present.

Quoted 14 lines

hw/vfio/pci.c +1 -0

        bool rom_read_failed;
    } VFIOPCIDevice;
 

@@ -203,6 +205,7 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
    static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                      uint32_t val, int len);
    static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev);
 
    /*
     * Disabling BAR mmaping can be slow, but toggling it around INTx can

@@ -2853,6 +2856,20 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
            dev_iter = pci_bridge_get_device(dev_iter->bus);
        }

hw/vfio/pci.c static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) · line 2867

Quoted 10 lines

hw/vfio/pci.c +9 -0

 
+    /*
+     * Don't check bus reset capability when device is enabled during
+     * qemu machine creation, which is done by machine init function.
+     */
+    if (DEVICE(vdev)->hotplugged) {
+        vfio_check_host_bus_reset(vdev);
+        if (!vdev->has_bus_reset) {
+            error_report("vfio: Cannot enable AER for device %s, "
+                         "which is not support host bus reset.",

"which does not support host bus reset."

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3698

Quoted 13 lines

hw/vfio/pci.c +6 -0

+                         vdev->vbasedev.name);
+            goto error;
+        }
+    }
+
        errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
        /*
         * The ability to record multiple headers is depending on

@@ -3678,6 +3695,112 @@ static void vfio_setup_resetfn(VFIOPCIDevice *vdev)
        }
    }
 
+struct VfioDeviceFind {

We use VFIOFooBar for all other camel case definitions, much like PCIBus and PCIDevice below.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3709

Quoted 11 lines

hw/vfio/pci.c +11 -0

+    PCIBus *pbus;
+    PCIDevice *pdev;
+    bool found;
+};
+
+static void find_devices(PCIBus *bus, void *opaque)
+{
+    struct VfioDeviceFind *find = opaque;
+    int i;
+
+    if (find->found == true) {

if (find->found) {...

Quoted 17 lines

hw/vfio/pci.c +17 -0

+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        if (!bus->devices[i]) {
+            continue;
+        }
+
+        if (bus->devices[i] == find->pdev) {
+            find->pbus = bus;
+            find->found = true;
+            break;
+        }
+    }
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev)

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3736

Quoted 10 lines

hw/vfio/pci.c +10 -0

+{
+    PCIBus *bus = vdev->pdev.bus;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    VFIOGroup *group;
+    int ret, i;
+    bool has_bus_reset = false;
+
+    ret = vfio_get_hot_reset_info(vdev, &info);
+    if (ret < 0) {

if (ret) {...

Quoted 45 lines

hw/vfio/pci.c +45 -0

+        goto out;
+    }
+
+    /* List all affected devices by bus reset */
+    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);
+
+        /* Skip the current device */
+        if (vfio_pci_host_match(&host, &vdev->host)) {
+            continue;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            goto out;
+        }
+
+        /* Ensure affected devices for reset under the same bus */
+        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)) {
+                struct VfioDeviceFind find = { .pdev = &tmp->pdev, .found = false };
+
+                pci_for_each_bus(bus, find_devices, &find);
+                if (!find.found) {
+                    goto out;

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3791

Quoted 10 lines

hw/vfio/pci.c +10 -0

+                }
+                /*
+                 * When the check device is hotplugged to a higher bus again,
+                 * which would influence the affected device which enable aer
+                 * below the bus.
+                 */
+                if (tmp->features & VFIO_FEATURE_ENABLE_AER &&
+                    find.pbus != bus) {
+                    goto out;
+                }

I think what you're trying to do here is assume that if a reset of A affects B, then a reset of B affects A, and if B is on a subordinate bus from A, then our configuration is broken, right? Can we really guarantee that assumption? If we had a physical topology that mirrored this virtual topology, that wouldn't necessarily be true. For instance, if A was a function of a multi-function device where another function was a PCIe upstream switch, B could be subordinate to that switch, so a reset of A affects B, but a reset of B doesn't affect A.

hw/vfio/pci.c static void vfio_setup_resetfn(VFIOPCIDevice *vdev) · line 3800

Quoted 9 lines

hw/vfio/pci.c +9 -0

+                break;
+            }
+        }
+    }
+
+    has_bus_reset = true;
+
+out:
+    vdev->has_bus_reset = has_bus_reset;

I don't see any value is storing this, it can't be trusted at any point in the future. I think that any time we add a device or think about forwarding an AER message to the guest, we need to do a validation pass, testing the entire topology.

Quoted 10 lines

To do that we'd iterate through every device in every group for PCI devices that support AER. For each one, get the hot reset info for the affected device list. For each affected device, if it's attached to the VM, it needs to be on or below the bus of the target device. Additionally, we should test all of the non-AER supporting vfio-pci devices on or below the target bus to verify they have a reset mechanism. Run that function for each vfio-pci device as it's added, regardless of whether it's hotplug. If the test fails, fail the initfn for the current device. Also run the test prior to AER injection, if it fails demote the AER injection to a machine halt as we have now.

Quoted 6 lines

I'm worry about is the case that the affected devices belonged to another groups but when initialize this device the another group has not been added. it would cause fail the initfn, but the group maybe added later. so I used the machine done event notifier to check this case. if we don't do like that. how can we check the case when all vfio-pci devices initfn ?

Quoted 7 lines

That's why the initfn test needs to test the entire topology, not just the device being added. If we have the case you describe where we have two devices in separate groups, we add the first device, test the topology, see that the second device is affected but not yet added and allow AER to be enabled. When the second device is added, we again test the topology, we see the potential conflict and fail the initfn for the second device if the bus requirements are not met.

Quoted 3 lines

In case that the second device may not be added. in this case the first device enable the aer, and pass the validate. how do we know the second device if be added ?

Yeah, I see what you're getting at here. If we have a dual port NIC with isolation between functions such that each is a separate IOMMU group, when we add the first device with AER enabled it will fail because a bus reset affects both groups and we don't yet own the second group.

My proposal wouldn't provide a way to ever enable AER for these devices. However, the proposal of using a machine-init-done hook only allows cold-plug of such devices with AER, hotplug would get the same issue. I don't think that sort of asymmetry is supportable either.

We almost need some sort of vfio-group stub driver in qemu that we can claim ownership of a group without adding any of the devices in the group to the VM. Another option might be to make AER a "soft" requirement, demote it to a VM stop unless the topology allows it. That also creates a confusing user scenario that probably looks nearly random from an outside perspective. The only other idea I can see would be to allow some manipulation of iommu groups at the kernel level, perhaps creating a meta-group composed of one or more iommu groups. Or maybe a kernel option that could ignore isolation for specified devices to broaden the group. Are we really sure exposing AER to guests is a good idea? Requiring guest bus resets to map to host bus rests is really a mess. Thanks,

Alex

.

Signed-off-by Chen Fan <[email protected]>

---
 hw/vfio/pci.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

hw/vfio/pci.c +27 -0

index 5934fd7..91ad9ad 100644
@@ -3801,6 +3801,33 @@ out:
     g_free(info);
 }
 
+static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
+{
+    VFIOGroup *group;
+    VFIODevice *vbasedev;
+    VFIOPCIDevice *vdev;
+
+    /* Check All support AER devices if has bus reset capability */
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+        QLIST_FOREACH(vbasedev, &group->device_list, next) {
+            vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+            if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
+                vfio_check_host_bus_reset(vdev);
+                if (!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);
+                }
+            }
+        }
+    }
+}
+
+static Notifier machine_notifier = {
+    .notify = vfio_pci_machine_done_notify,
+};
+
 static int vfio_initfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);

@@ -4086,6 +4113,11 @@ static const TypeInfo vfio_pci_dev_info = {
 static void register_vfio_pci_dev_type(void)

Signed-off-by Chen Fan <[email protected]>

---
 hw/vfio/pci.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

hw/vfio/pci.c +2 -0

index 91ad9ad..a8c5988 100644
@@ -156,6 +156,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

@@ -172,6 +173,7 @@ typedef struct VFIOPCIDevice {
     bool has_flr;
     bool has_pm_reset;
     bool has_bus_reset;
+    bool needs_bus_reset;
     bool rom_read_failed;
 } VFIOPCIDevice;
 

@@ -3828,6 +3830,17 @@ static Notifier machine_notifier = {
     .notify = vfio_pci_machine_done_notify,

hw/vfio/pci.c +7 -0

+    }
+}
+
 static int vfio_initfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);

@@ -3978,6 +3991,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:

@@ -4006,6 +4022,7 @@ static void vfio_exitfn(PCIDevice *pdev)
 
     vfio_unregister_req_notifier(vdev);
     vfio_unregister_err_notifier(vdev);
+    pci_bus_remove_reset_notifier(&vdev->sec_bus_reset_notifier);
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
     if (vdev->intx.mmap_timer) {

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 <[email protected]>

---
 hw/vfio/pci.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

hw/vfio/pci.c +47 -0

index a8c5988..b05ccdf 100644
@@ -3803,6 +3803,64 @@ out:
     g_free(info);
 }
 
+/* Check when device reset the affected devices is single or multi,
+ * because there was differentiate the hot reset of mulitple in-use
+ * devices and hot reset of a single in-use device. */
+static int vfio_pci_affect_devices_is_multi(VFIOPCIDevice *vdev)
+{
+    VFIOGroup *group;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    int ret, i, count = 0;
+
+    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;
+        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;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            ret = -1;
+            goto out;
+        }
+
+        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
+            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
+                continue;

hw/vfio/pci.c +15 -0

+        if (vdev->vbasedev.needs_reset) {
+            int ret;
+
+            ret = vfio_pci_affect_devices_is_multi(vdev);
+            if (ret < 0) {
+                error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
+                             "get hot reset info failed.", vdev->host.domain,
+                             vdev->host.bus, vdev->host.slot, vdev->host.function);
+            } else {
+                vfio_pci_hot_reset(vdev, ret ? true : false);
+            }
+        }
+        return;
+    }
+
     vfio_pci_pre_reset(vdev);
 
     if (vdev->resetfn && !vdev->resetfn(vdev)) {

Quoted 70 lines · Alex Williamson

Quoted 69 lines · 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 <[email protected]>

---
  hw/vfio/pci.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 75 insertions(+)

hw/vfio/pci.c +52 -0

index a8c5988..b05ccdf 100644
@@ -3803,6 +3803,64 @@ out:
      g_free(info);
  }
 
+/* Check when device reset the affected devices is single or multi,
+ * because there was differentiate the hot reset of mulitple in-use
+ * devices and hot reset of a single in-use device. */
+static int vfio_pci_affect_devices_is_multi(VFIOPCIDevice *vdev)
+{
+    VFIOGroup *group;
+    struct vfio_pci_hot_reset_info *info = NULL;
+    struct vfio_pci_dependent_device *devices;
+    int ret, i, count = 0;
+
+    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;
+        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;
+        }
+
+        /* Ensure we own the group of the affected device */
+        QLIST_FOREACH(group, &vfio_group_list, next) {
+            if (group->groupid == devices[i].group_id) {
+                break;
+            }
+        }
+
+        if (!group) {
+            ret = -1;
+            goto out;
+        }
+
+        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
+            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
+                continue;
+            }
+            count++;
+        }
+    }
+

Quoted 8 lines

hw/vfio/pci.c out: · line 3862

Quoted 5 lines

hw/vfio/pci.c +5 -0

+    ret = count;
+out:
+    g_free(info);
+    return ret;
+}

This basically does almost all the work of the hot reset function just to figure out whether it's a single or multi-reset to call the hot reset function with the right parameter. Why not just do a hot reset?

Because the vfio_pci_hot_reset function has a bit odd, if one group has two or more function, even though only one function was passthroughed to VM, when we need to do bus reset, we should call vfio_pci_hot_reset(single), but if multi devices were passed to VM, we should call with muliti. also we should update the affected devices needs_reset status when do host bus reset. we should not do bus reset multi-times, which vfio_pci_hot_reset has done for us.

Quoted 35 lines

hw/vfio/pci.c static void vfio_pci_reset(DeviceState *dev) · line 4101

Quoted 10 lines

hw/vfio/pci.c +3 -0

+
  static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
  {
      VFIOGroup *group;

@@ -4039,6 +4097,23 @@ static void vfio_pci_reset(DeviceState *dev)
 
      trace_vfio_pci_reset(vdev->vbasedev.name);
 
+    if (vdev->needs_bus_reset) {
+        vdev->needs_bus_reset = false;

A comment would be nice to signal that this secondary test avoids duplicate bus resets.

Quoted 8 lines

hw/vfio/pci.c +8 -0

+        if (vdev->vbasedev.needs_reset) {
+            int ret;
+
+            ret = vfio_pci_affect_devices_is_multi(vdev);
+            if (ret < 0) {
+                error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
+                             "get hot reset info failed.", vdev->host.domain,
+                             vdev->host.bus, vdev->host.slot, vdev->host.function);

hw/vfio/pci.c static void vfio_pci_reset(DeviceState *dev) · line 4119

Quoted 10 lines

hw/vfio/pci.c +7 -0

+            } else {
+                vfio_pci_hot_reset(vdev, ret ? true : false);
+            }
+        }
+        return;
+    }
+
      vfio_pci_pre_reset(vdev);
 
      if (vdev->resetfn && !vdev->resetfn(vdev)) {

.

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 <[email protected]>

---
 hw/vfio/pci.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

hw/vfio/pci.c +5 -0

index b05ccdf..855a0a6 100644
@@ -3348,18 +3348,48 @@ 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;
     }
 
     /*

hw/vfio/pci.c +27 -0

+
+    /*
+     * 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 &&
+        vdev->has_bus_reset) {
+        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;
+    }
+
+    /*
+     * 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.  "