[Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[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.

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 (9):
  pcie_aer: fix typos in pcie_aer_inject_error comment
  aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
  vfio: add pcie extanded capability support
  aer: impove pcie_aer_init to support vfio device
  vfio: add aer support for vfio device
  vfio: add 'x-aer' option to disable aer capability
  pcie_aer: expose pcie_aer_msg() interface
  vfio-pci: pass the aer error to guest
  pcie: fix several trivial typos

 hw/pci-bridge/ioh3420.c            |   3 +-
 hw/pci-bridge/xio3130_downstream.c |   3 +-
 hw/pci-bridge/xio3130_upstream.c   |   3 +-
 hw/pci/pcie_aer.c                  |  17 ++--
 hw/vfio/pci.c                      | 160 +++++++++++++++++++++++++++++++++++--
 include/hw/pci/pci.h               |   2 +-
 include/hw/pci/pcie_aer.h          |   7 +-
 7 files changed, 174 insertions(+), 21 deletions(-)

-- 
1.9.3

[Qemu-devel] [RFC v4 1/9] pcie_aer: fix typos in pcie_aer_inject_error comment

[Chen Fan]

Refer to "PCI Express Base Spec3.0", this comments can't
fit the description in spec, so we should fix them.

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

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 1f4be16..7ca077a 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -618,11 +618,11 @@ static bool pcie_aer_inject_uncor_error(PCIEAERInject *inj, bool is_fatal)
  * non-Function specific error must be recorded in all functions.
  * It is the responsibility of the caller of this function.
  * It is also caller's responsibility to determine which function should
- * report the rerror.
+ * report the error.
  *
  * 6.2.4 Error Logging
- * 6.2.5 Sqeunce of Device Error Signaling and Logging Operations
- * table 6-2: Flowchard Showing Sequence of Device Error Signaling and Logging
+ * 6.2.5 Sequence of Device Error Signaling and Logging Operations
+ * table 6-2: Flowchart Showing Sequence of Device Error Signaling and Logging
  *            Operations
  */
 int pcie_aer_inject_error(PCIDevice *dev, const PCIEAERErr *err)
-- 
1.9.3

[Qemu-devel] [RFC v4 2/9] aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register

[Chen Fan]

>From pcie spec, the bits attributes are RW1CS in Correctable
Error Status Register, so this patch fix a wrong definition
for PCI_ERR_COR_STATUS register with w1cmask type.

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

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 7ca077a..ece1487 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -123,7 +123,7 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
                  PCI_ERR_UNC_SUPPORTED);
 
     pci_long_test_and_set_mask(dev->w1cmask + offset + PCI_ERR_COR_STATUS,
-                               PCI_ERR_COR_STATUS);
+                               PCI_ERR_COR_SUPPORTED);
 
     pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
                  PCI_ERR_COR_MASK_DEFAULT);
-- 
1.9.3

[Qemu-devel] [RFC v4 3/9] vfio: add pcie extanded capability support

[Chen Fan]

For vfio pcie device, we could expose the extanded 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 84e9d99..96cb52b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2482,6 +2482,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);
@@ -2705,16 +2720,82 @@ 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,
+                            uint16_t pos)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint32_t header;
+    uint16_t cap_id, next, size;
+    uint8_t cap_ver;
+    int ret;
+
+    header = pci_get_long(config + pos);
+    cap_id = PCI_EXT_CAP_ID(header);
+    cap_ver = PCI_EXT_CAP_VER(header);
+    next = PCI_EXT_CAP_NEXT(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, pos);
+
+    pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
+    if (pos == PCI_CONFIG_SPACE_SIZE) {
+        /* Begin the rebuild, we should set the next offset zero. */
+        pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
+    }
+
+    /* Use emulated header pointer to allow dropping extended caps */
+    pci_set_long(vdev->emulated_config_bits + pos, 0xffffffff);
+
+    if (next) {
+        ret = vfio_add_ext_cap(vdev, config, next);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    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_bus_is_express(vdev->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 capabilitiess.
+     */
+    config = g_malloc0(vdev->config_size);
+    if (!config) {
+        return -ENOMEM;
+    }
+    memcpy(config, pdev->config, vdev->config_size);
+    ret = vfio_add_ext_cap(vdev, config, PCI_CONFIG_SPACE_SIZE);
+
+    g_free(config);
+    return ret;
 }
 
 static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
-- 
1.9.3

[Qemu-devel] [RFC v4 4/9] aer: impove pcie_aer_init to support vfio device

[Chen Fan]

extend pcie_aer_init arguments to adjust vfio device.

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

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index cce2fdd..354574f 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -129,7 +129,8 @@ 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, PCI_ERR_VER,
+                       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..407f75f 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -92,7 +92,8 @@ 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, PCI_ERR_VER,
+                       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..52b130f 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -81,7 +81,8 @@ 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, PCI_ERR_VER,
+                       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 ece1487..a76a943 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -94,12 +94,13 @@ 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, uint8_t cap_ver,
+                  uint16_t offset, uint16_t size)
 {
     PCIExpressDevice *exp;
 
-    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
-                        offset, PCI_ERR_SIZEOF);
+    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR,
+                        cap_ver, 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 bcac80a..afa074e 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -87,7 +87,9 @@ struct PCIEAERErr {
 
 extern const VMStateDescription vmstate_pcie_aer_log;
 
-int pcie_aer_init(PCIDevice *dev, uint16_t offset);
+int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
+                  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 v4 5/9] 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 | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 96cb52b..db4ba23 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2720,6 +2720,28 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
     return 0;
 }
 
+static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
+                          int pos, uint16_t size)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
+    uint32_t severity;
+    int ret;
+
+    pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
+    ret = pcie_aer_init(pdev, cap_ver, 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,
                             uint16_t pos)
 {
@@ -2742,7 +2764,18 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config,
      */
     size = vfio_ext_cap_max_size(config, pos);
 
-    pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
+    switch (cap_id) {
+    case PCI_EXT_CAP_ID_ERR:
+        ret = vfio_setup_aer(vdev, cap_ver, pos, size);
+        if (ret) {
+            return ret;
+        }
+        break;
+    default:
+        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
+        break;
+    }
+
     if (pos == PCI_CONFIG_SPACE_SIZE) {
         /* Begin the rebuild, we should set the next offset zero. */
         pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
-- 
1.9.3

[Qemu-devel] [RFC v4 6/9] vfio: add 'x-aer' option to disable aer capability

[Chen Fan]

add 'x-aer' option to disable aer capability if user
want.

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index db4ba23..5471437 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice {
     uint32_t features;
 #define VFIO_FEATURE_ENABLE_VGA_BIT 0
 #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
+#define VFIO_FEATURE_ENABLE_AER_BIT 1
+#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
     int32_t bootindex;
     uint8_t pm_cap;
     bool has_vga;
@@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
     uint32_t severity;
     int ret;
 
+    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+        return 0;
+    }
+
     pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
     ret = pcie_aer_init(pdev, cap_ver, pos, size);
     if (ret) {
@@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = {
                        intx.mmap_timeout, 1100),
     DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_VGA_BIT, false),
+    DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features,
+                    VFIO_FEATURE_ENABLE_AER_BIT, true),
     DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
     /*
      * TODO - support passed fds... is this necessary?
-- 
1.9.3

[Qemu-devel] [RFC v4 7/9] 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 a76a943..c44ea6b 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -371,7 +371,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 afa074e..fdc1794 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -104,5 +104,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 v4 8/9] 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 | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5471437..5669c55 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3241,18 +3241,42 @@ 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 (pci_is_express(dev) &&
+        pci_bus_is_express(dev->bus) &&
+        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 v4 9/9] pcie: fix several trivial typos

[Chen Fan]

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

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index bdee464..b82de15 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -137,7 +137,7 @@ enum {
 #define PCI_CONFIG_HEADER_SIZE 0x40
 /* Size of the standard PCI config space */
 #define PCI_CONFIG_SPACE_SIZE 0x100
-/* Size of the standart PCIe config space: 4KB */
+/* Size of the standard PCIe config space: 4KB */
 #define PCIE_CONFIG_SPACE_SIZE  0x1000
 
 #define PCI_NUM_PINS 4 /* A-D */
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index fdc1794..4901ce0 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -51,7 +51,7 @@ struct PCIEAERLog {
     PCIEAERErr *log;
 };
 
-/* aer error message: error signaling message has only error sevirity and
+/* aer error message: error signaling message has only error severity and
    source id. See 2.2.8.3 error signaling messages */
 struct PCIEAERMsg {
     /*
-- 
1.9.3

Re: [Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[Chen Fan]

ping...

On 03/02/2015 03:16 PM, Chen Fan wrote:
> 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.
>
> 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 (9):
>    pcie_aer: fix typos in pcie_aer_inject_error comment
>    aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
>    vfio: add pcie extanded capability support
>    aer: impove pcie_aer_init to support vfio device
>    vfio: add aer support for vfio device
>    vfio: add 'x-aer' option to disable aer capability
>    pcie_aer: expose pcie_aer_msg() interface
>    vfio-pci: pass the aer error to guest
>    pcie: fix several trivial typos
>
>   hw/pci-bridge/ioh3420.c            |   3 +-
>   hw/pci-bridge/xio3130_downstream.c |   3 +-
>   hw/pci-bridge/xio3130_upstream.c   |   3 +-
>   hw/pci/pcie_aer.c                  |  17 ++--
>   hw/vfio/pci.c                      | 160 +++++++++++++++++++++++++++++++++++--
>   include/hw/pci/pci.h               |   2 +-
>   include/hw/pci/pcie_aer.h          |   7 +-
>   7 files changed, 174 insertions(+), 21 deletions(-)
>

Re: [Qemu-devel] [RFC v4 3/9] vfio: add pcie extanded capability support

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> For vfio pcie device, we could expose the extanded capability on

s/extanded/extended/

> 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 84e9d99..96cb52b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2482,6 +2482,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);
> @@ -2705,16 +2720,82 @@ 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,
> +                            uint16_t pos)
> +{
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint32_t header;
> +    uint16_t cap_id, next, size;
> +    uint8_t cap_ver;
> +    int ret;
> +
> +    header = pci_get_long(config + pos);
> +    cap_id = PCI_EXT_CAP_ID(header);
> +    cap_ver = PCI_EXT_CAP_VER(header);
> +    next = PCI_EXT_CAP_NEXT(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, pos);
> +
> +    pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> +    if (pos == PCI_CONFIG_SPACE_SIZE) {
> +        /* Begin the rebuild, we should set the next offset zero. */
> +        pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
> +    }
> +
> +    /* Use emulated header pointer to allow dropping extended caps */
> +    pci_set_long(vdev->emulated_config_bits + pos, 0xffffffff);
> +
> +    if (next) {
> +        ret = vfio_add_ext_cap(vdev, config, next);
> +        if (ret) {
> +            return ret;
> +        }
> +    }

This recursion seems pointless.  We use it for the standard capabilities
to make the capability list ordering correct and to avoid the config
space copy that is necessary for this version, but I don't see the
advantage to using it here vs a for-loop in the below function.
Recursion is more complicated, so let's only use it if it provides some
benefit.

> +
> +    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_bus_is_express(vdev->pdev.bus) ||
> +        !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {


Don't we need a pci_is_express(pdev) here too?  Also, we already have
pdev->bus to use as an argument, no need to start with the vdev.

> +        return 0;
> +    }
> +
> +    /*
> +     * In order to avoid config space broken, here using a copy config to
> +     * parse extended capabilitiess.

spelling

> +     */
> +    config = g_malloc0(vdev->config_size);
> +    if (!config) {
> +        return -ENOMEM;
> +    }

g_malloc can't fail, no need for this error out condition.  It's
strange, but it's the QEMU way.

> +    memcpy(config, pdev->config, vdev->config_size);

There's a g_memdup() function

> +    ret = vfio_add_ext_cap(vdev, config, PCI_CONFIG_SPACE_SIZE);
> +
> +    g_free(config);
> +    return ret;
>  }
>  
>  static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)

Re: [Qemu-devel] [RFC v4 4/9] aer: impove pcie_aer_init to support vfio device

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> extend pcie_aer_init arguments to adjust vfio device.

Some discussion of why vfio wants this would be useful.


> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/pci-bridge/ioh3420.c            | 3 ++-
>  hw/pci-bridge/xio3130_downstream.c | 3 ++-
>  hw/pci-bridge/xio3130_upstream.c   | 3 ++-
>  hw/pci/pcie_aer.c                  | 7 ++++---
>  include/hw/pci/pcie_aer.h          | 4 +++-
>  5 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index cce2fdd..354574f 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -129,7 +129,8 @@ 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, PCI_ERR_VER,
> +                       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..407f75f 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -92,7 +92,8 @@ 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, PCI_ERR_VER,
> +                       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..52b130f 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -81,7 +81,8 @@ 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, PCI_ERR_VER,
> +                       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 ece1487..a76a943 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -94,12 +94,13 @@ 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, uint8_t cap_ver,
> +                  uint16_t offset, uint16_t size)
>  {
>      PCIExpressDevice *exp;
>  
> -    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> -                        offset, PCI_ERR_SIZEOF);
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR,
> +                        cap_ver, 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 bcac80a..afa074e 100644
> --- a/include/hw/pci/pcie_aer.h
> +++ b/include/hw/pci/pcie_aer.h
> @@ -87,7 +87,9 @@ struct PCIEAERErr {
>  
>  extern const VMStateDescription vmstate_pcie_aer_log;
>  
> -int pcie_aer_init(PCIDevice *dev, uint16_t offset);
> +int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
> +                  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);

Re: [Qemu-devel] [RFC v4 6/9] vfio: add 'x-aer' option to disable aer capability

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> add 'x-aer' option to disable aer capability if user
> want.

I'm generally one to favor using the x- flag, but we need to figure out
if we need to make this be a supported option or not.  We also need to
decide whether we should include your previous patch that toggled this
support based on machine type.  We don't care about migration
compatibility, but users might depend on specific error behavior and we
don't want to break that for existing machine types.  This would include
only the q35 machine types through QEMU 2.3, I believe.

My impression is that it should probably be supported.  There may be
cases where the guest OS does not support AER and we want to be able to
invoke the old vm_stop() behavior.


> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index db4ba23..5471437 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice {
>      uint32_t features;
>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>  #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> +#define VFIO_FEATURE_ENABLE_AER_BIT 1
> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)

Bit 1 is already taken by:

47cbe50 vfio-pci: Enable device request notification support

>      int32_t bootindex;
>      uint8_t pm_cap;
>      bool has_vga;
> @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
>      uint32_t severity;
>      int ret;
>  
> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> +        return 0;
> +    }
> +
>      pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
>      ret = pcie_aer_init(pdev, cap_ver, pos, size);
>      if (ret) {
> @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = {
>                         intx.mmap_timeout, 1100),
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
> +    DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features,
> +                    VFIO_FEATURE_ENABLE_AER_BIT, true),
>      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>      /*
>       * TODO - support passed fds... is this necessary?

Re: [Qemu-devel] [RFC v4 8/9] vfio-pci: pass the aer error to guest

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> 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 | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5471437..5669c55 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3241,18 +3241,42 @@ 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 (pci_is_express(dev) &&
> +        pci_bus_is_express(dev->bus) &&
> +        dev->exp.aer_cap) {

Isn't simply testing dev->exp.aer_cap sufficient?

> +        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.  "

Re: [Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[Alex Williamson]

On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> 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.
> 
> 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 (9):
>   pcie_aer: fix typos in pcie_aer_inject_error comment
>   aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
>   vfio: add pcie extanded capability support
>   aer: impove pcie_aer_init to support vfio device
>   vfio: add aer support for vfio device
>   vfio: add 'x-aer' option to disable aer capability
>   pcie_aer: expose pcie_aer_msg() interface
>   vfio-pci: pass the aer error to guest
>   pcie: fix several trivial typos
> 
>  hw/pci-bridge/ioh3420.c            |   3 +-
>  hw/pci-bridge/xio3130_downstream.c |   3 +-
>  hw/pci-bridge/xio3130_upstream.c   |   3 +-
>  hw/pci/pcie_aer.c                  |  17 ++--
>  hw/vfio/pci.c                      | 160 +++++++++++++++++++++++++++++++++++--
>  include/hw/pci/pci.h               |   2 +-
>  include/hw/pci/pcie_aer.h          |   7 +-
>  7 files changed, 174 insertions(+), 21 deletions(-)
> 

I would encourage you to submit any of the trivial typos and spelling
fixes separately, including them in a vfio series is only going to slow
down acceptance since it touches core-pci code, which I do not maintain.
Likewise we'll minimally need ACKs from MST for the PCI changes, but it
may be a wise move to send them separately with full justification as
well.  We're into the QEMU 2.3 freeze, so aside from trivial fixes, the
new functionality will need to wait until after 2.3 is tagged.  Thanks,

Alex

Re: [Qemu-devel] [RFC v4 0/9] pass aer error to guest for vfio device

[Chen Fan]

On 03/10/2015 04:34 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> 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.
>>
>> 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 (9):
>>    pcie_aer: fix typos in pcie_aer_inject_error comment
>>    aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
>>    vfio: add pcie extanded capability support
>>    aer: impove pcie_aer_init to support vfio device
>>    vfio: add aer support for vfio device
>>    vfio: add 'x-aer' option to disable aer capability
>>    pcie_aer: expose pcie_aer_msg() interface
>>    vfio-pci: pass the aer error to guest
>>    pcie: fix several trivial typos
>>
>>   hw/pci-bridge/ioh3420.c            |   3 +-
>>   hw/pci-bridge/xio3130_downstream.c |   3 +-
>>   hw/pci-bridge/xio3130_upstream.c   |   3 +-
>>   hw/pci/pcie_aer.c                  |  17 ++--
>>   hw/vfio/pci.c                      | 160 +++++++++++++++++++++++++++++++++++--
>>   include/hw/pci/pci.h               |   2 +-
>>   include/hw/pci/pcie_aer.h          |   7 +-
>>   7 files changed, 174 insertions(+), 21 deletions(-)
>>
> I would encourage you to submit any of the trivial typos and spelling
> fixes separately, including them in a vfio series is only going to slow
> down acceptance since it touches core-pci code, which I do not maintain.
> Likewise we'll minimally need ACKs from MST for the PCI changes, but it
> may be a wise move to send them separately with full justification as
> well.  We're into the QEMU 2.3 freeze, so aside from trivial fixes, the
> new functionality will need to wait until after 2.3 is tagged.  Thanks,
I got it, Thanks for your suggestion. I will send the trivial typos 
separately.

Thanks,
Chen


>
> Alex
>
> .
>

Re: [Qemu-devel] [RFC v4 4/9] aer: impove pcie_aer_init to support vfio device

[Chen Fan]

On 03/10/2015 04:29 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> extend pcie_aer_init arguments to adjust vfio device.
> Some discussion of why vfio wants this would be useful.
qemu treats vfio device as an emulated device.
and these attributes of aer can be emulated too.

so here I would use pcie_aer_init to add an aer capability
directly. and get rid of this patch from series.


Thanks,
Chen
>
>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci-bridge/ioh3420.c            | 3 ++-
>>   hw/pci-bridge/xio3130_downstream.c | 3 ++-
>>   hw/pci-bridge/xio3130_upstream.c   | 3 ++-
>>   hw/pci/pcie_aer.c                  | 7 ++++---
>>   include/hw/pci/pcie_aer.h          | 4 +++-
>>   5 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
>> index cce2fdd..354574f 100644
>> --- a/hw/pci-bridge/ioh3420.c
>> +++ b/hw/pci-bridge/ioh3420.c
>> @@ -129,7 +129,8 @@ 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, PCI_ERR_VER,
>> +                       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..407f75f 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -92,7 +92,8 @@ 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, PCI_ERR_VER,
>> +                       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..52b130f 100644
>> --- a/hw/pci-bridge/xio3130_upstream.c
>> +++ b/hw/pci-bridge/xio3130_upstream.c
>> @@ -81,7 +81,8 @@ 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, PCI_ERR_VER,
>> +                       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 ece1487..a76a943 100644
>> --- a/hw/pci/pcie_aer.c
>> +++ b/hw/pci/pcie_aer.c
>> @@ -94,12 +94,13 @@ 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, uint8_t cap_ver,
>> +                  uint16_t offset, uint16_t size)
>>   {
>>       PCIExpressDevice *exp;
>>   
>> -    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
>> -                        offset, PCI_ERR_SIZEOF);
>> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR,
>> +                        cap_ver, 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 bcac80a..afa074e 100644
>> --- a/include/hw/pci/pcie_aer.h
>> +++ b/include/hw/pci/pcie_aer.h
>> @@ -87,7 +87,9 @@ struct PCIEAERErr {
>>   
>>   extern const VMStateDescription vmstate_pcie_aer_log;
>>   
>> -int pcie_aer_init(PCIDevice *dev, uint16_t offset);
>> +int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
>> +                  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);
>
>
> .
>

Re: [Qemu-devel] [RFC v4 3/9] vfio: add pcie extanded capability support

[Chen Fan]

On 03/10/2015 04:28 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> For vfio pcie device, we could expose the extanded capability on
> s/extanded/extended/
>
>> 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 84e9d99..96cb52b 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2482,6 +2482,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);
>> @@ -2705,16 +2720,82 @@ 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,
>> +                            uint16_t pos)
>> +{
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    uint32_t header;
>> +    uint16_t cap_id, next, size;
>> +    uint8_t cap_ver;
>> +    int ret;
>> +
>> +    header = pci_get_long(config + pos);
>> +    cap_id = PCI_EXT_CAP_ID(header);
>> +    cap_ver = PCI_EXT_CAP_VER(header);
>> +    next = PCI_EXT_CAP_NEXT(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, pos);
>> +
>> +    pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> +    if (pos == PCI_CONFIG_SPACE_SIZE) {
>> +        /* Begin the rebuild, we should set the next offset zero. */
>> +        pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0));
>> +    }
>> +
>> +    /* Use emulated header pointer to allow dropping extended caps */
>> +    pci_set_long(vdev->emulated_config_bits + pos, 0xffffffff);
>> +
>> +    if (next) {
>> +        ret = vfio_add_ext_cap(vdev, config, next);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    }
> This recursion seems pointless.  We use it for the standard capabilities
> to make the capability list ordering correct and to avoid the config
> space copy that is necessary for this version, but I don't see the
> advantage to using it here vs a for-loop in the below function.
> Recursion is more complicated, so let's only use it if it provides some
> benefit.
indeed.

Thanks,
Chen


>
>> +
>> +    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_bus_is_express(vdev->pdev.bus) ||
>> +        !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
>
> Don't we need a pci_is_express(pdev) here too?  Also, we already have
> pdev->bus to use as an argument, no need to start with the vdev.
>
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * In order to avoid config space broken, here using a copy config to
>> +     * parse extended capabilitiess.
> spelling
>
>> +     */
>> +    config = g_malloc0(vdev->config_size);
>> +    if (!config) {
>> +        return -ENOMEM;
>> +    }
> g_malloc can't fail, no need for this error out condition.  It's
> strange, but it's the QEMU way.
>
>> +    memcpy(config, pdev->config, vdev->config_size);
> There's a g_memdup() function
>
>> +    ret = vfio_add_ext_cap(vdev, config, PCI_CONFIG_SPACE_SIZE);
>> +
>> +    g_free(config);
>> +    return ret;
>>   }
>>   
>>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>
>
> .
>

Re: [Qemu-devel] [RFC v4 8/9] vfio-pci: pass the aer error to guest

[Chen Fan]

On 03/10/2015 04:29 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> 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 | 36 ++++++++++++++++++++++++++++++------
>>   1 file changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 5471437..5669c55 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3241,18 +3241,42 @@ 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 (pci_is_express(dev) &&
>> +        pci_bus_is_express(dev->bus) &&
>> +        dev->exp.aer_cap) {
> Isn't simply testing dev->exp.aer_cap sufficient?
Yes, I think you are right. if previous initilization install the
aer cap, the check pci_is_express() and pci_bus_is_express()
always true.

Thanks,
Chen

>
>> +        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.  "
>
>
> .
>

Re: [Qemu-devel] [RFC v4 6/9] vfio: add 'x-aer' option to disable aer capability

[Chen Fan]

On 03/10/2015 04:29 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> add 'x-aer' option to disable aer capability if user
>> want.
> I'm generally one to favor using the x- flag, but we need to figure out
> if we need to make this be a supported option or not.  We also need to
> decide whether we should include your previous patch that toggled this
> support based on machine type.  We don't care about migration
> compatibility, but users might depend on specific error behavior and we
> don't want to break that for existing machine types.  This would include
> only the q35 machine types through QEMU 2.3, I believe.
>
> My impression is that it should probably be supported.  There may be
> cases where the guest OS does not support AER and we want to be able to
> invoke the old vm_stop() behavior.
In order to compatible with existing machine type behavior,  I think
we should let user decide whether need to enable aer on qemu.
by default disabled. if the guest OS support AER, user may
enable aer, otherwise, also invoke the vm_stop() behavior.

Thanks,
Chen

>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index db4ba23..5471437 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice {
>>       uint32_t features;
>>   #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>>   #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>> +#define VFIO_FEATURE_ENABLE_AER_BIT 1
>> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
> Bit 1 is already taken by:
>
> 47cbe50 vfio-pci: Enable device request notification support
>
>>       int32_t bootindex;
>>       uint8_t pm_cap;
>>       bool has_vga;
>> @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
>>       uint32_t severity;
>>       int ret;
>>   
>> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>> +        return 0;
>> +    }
>> +
>>       pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
>>       ret = pcie_aer_init(pdev, cap_ver, pos, size);
>>       if (ret) {
>> @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = {
>>                          intx.mmap_timeout, 1100),
>>       DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>>                       VFIO_FEATURE_ENABLE_VGA_BIT, false),
>> +    DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features,
>> +                    VFIO_FEATURE_ENABLE_AER_BIT, true),
>>       DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>>       /*
>>        * TODO - support passed fds... is this necessary?
>
>
> .
>

Re: [Qemu-devel] [RFC v4 6/9] vfio: add 'x-aer' option to disable aer capability

[Alex Williamson]

On Wed, 2015-03-11 at 11:42 +0800, Chen Fan wrote:
> On 03/10/2015 04:29 AM, Alex Williamson wrote:
> > On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
> >> add 'x-aer' option to disable aer capability if user
> >> want.
> > I'm generally one to favor using the x- flag, but we need to figure out
> > if we need to make this be a supported option or not.  We also need to
> > decide whether we should include your previous patch that toggled this
> > support based on machine type.  We don't care about migration
> > compatibility, but users might depend on specific error behavior and we
> > don't want to break that for existing machine types.  This would include
> > only the q35 machine types through QEMU 2.3, I believe.
> >
> > My impression is that it should probably be supported.  There may be
> > cases where the guest OS does not support AER and we want to be able to
> > invoke the old vm_stop() behavior.
> In order to compatible with existing machine type behavior,  I think
> we should let user decide whether need to enable aer on qemu.
> by default disabled. if the guest OS support AER, user may
> enable aer, otherwise, also invoke the vm_stop() behavior.

Yes, for existing machine types, the current behavior needs to be
maintained.  The user can be given the option to enable AER and override
the machine setting.  For new machine types, I expect we'd want to
enable AER automatically with the option to disable it.  This would be
for Q35 or other express chipsets only though since we can't expose AER
to the guest for devices exposed on a conventional bus.  Thanks,

Alex

> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/vfio/pci.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index db4ba23..5471437 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice {
> >>       uint32_t features;
> >>   #define VFIO_FEATURE_ENABLE_VGA_BIT 0
> >>   #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> >> +#define VFIO_FEATURE_ENABLE_AER_BIT 1
> >> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
> > Bit 1 is already taken by:
> >
> > 47cbe50 vfio-pci: Enable device request notification support
> >
> >>       int32_t bootindex;
> >>       uint8_t pm_cap;
> >>       bool has_vga;
> >> @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> >>       uint32_t severity;
> >>       int ret;
> >>   
> >> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> >> +        return 0;
> >> +    }
> >> +
> >>       pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> >>       ret = pcie_aer_init(pdev, cap_ver, pos, size);
> >>       if (ret) {
> >> @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = {
> >>                          intx.mmap_timeout, 1100),
> >>       DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> >>                       VFIO_FEATURE_ENABLE_VGA_BIT, false),
> >> +    DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features,
> >> +                    VFIO_FEATURE_ENABLE_AER_BIT, true),
> >>       DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> >>       /*
> >>        * TODO - support passed fds... is this necessary?
> >
> >
> > .
> >
> 

Re: [Qemu-devel] [RFC v4 4/9] aer: impove pcie_aer_init to support vfio device

[Chen Fan]

On 03/10/2015 04:29 AM, Alex Williamson wrote:
> On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote:
>> extend pcie_aer_init arguments to adjust vfio device.
> Some discussion of why vfio wants this would be useful.
I remain this patch at the latest v5. and add the 'size' argument,
because, the aer config space is not fixed.  the TLP Prefix Log Register
is not always implemented. for instance, my local network device:
Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI 
Express Gigabit Ethernet controller,
     Capabilities: [100] Advanced Error Reporting
     Capabilities: [140] Virtual Channel

the size is 0x40.
So I think we need a size argument indeed.

Thanks,
Chen




>
>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci-bridge/ioh3420.c            | 3 ++-
>>   hw/pci-bridge/xio3130_downstream.c | 3 ++-
>>   hw/pci-bridge/xio3130_upstream.c   | 3 ++-
>>   hw/pci/pcie_aer.c                  | 7 ++++---
>>   include/hw/pci/pcie_aer.h          | 4 +++-
>>   5 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
>> index cce2fdd..354574f 100644
>> --- a/hw/pci-bridge/ioh3420.c
>> +++ b/hw/pci-bridge/ioh3420.c
>> @@ -129,7 +129,8 @@ 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, PCI_ERR_VER,
>> +                       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..407f75f 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -92,7 +92,8 @@ 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, PCI_ERR_VER,
>> +                       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..52b130f 100644
>> --- a/hw/pci-bridge/xio3130_upstream.c
>> +++ b/hw/pci-bridge/xio3130_upstream.c
>> @@ -81,7 +81,8 @@ 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, PCI_ERR_VER,
>> +                       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 ece1487..a76a943 100644
>> --- a/hw/pci/pcie_aer.c
>> +++ b/hw/pci/pcie_aer.c
>> @@ -94,12 +94,13 @@ 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, uint8_t cap_ver,
>> +                  uint16_t offset, uint16_t size)
>>   {
>>       PCIExpressDevice *exp;
>>   
>> -    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
>> -                        offset, PCI_ERR_SIZEOF);
>> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR,
>> +                        cap_ver, 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 bcac80a..afa074e 100644
>> --- a/include/hw/pci/pcie_aer.h
>> +++ b/include/hw/pci/pcie_aer.h
>> @@ -87,7 +87,9 @@ struct PCIEAERErr {
>>   
>>   extern const VMStateDescription vmstate_pcie_aer_log;
>>   
>> -int pcie_aer_init(PCIDevice *dev, uint16_t offset);
>> +int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
>> +                  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);
>
>
> .
>