[Qemu-devel] [RFC v3 00/10] pass aer error to guest for vfio device

[Qemu-devel] [RFC v3 00/10] 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.

for turning on SERR# patch in seabios list, pls see:
http://www.seabios.org/pipermail/seabios/2015-January/008591.html

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 (10):
  pcie_aer: fix typos in pcie_aer_inject_error comment
  aer: fix a wrong init PCI_ERR_COR_STATUS w1cmask type register
  aer: introduce pcie_aer_setup to setup aer related bits
  vfio: add pcie extanded capability support
  pcie_aer: expose pcie_aer_msg() interface
  piix: disable all vfio device aercap property
  vfio_pci: change vfio device features bit macro to enum definition
  vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature
  vfio-pci: pass the aer error to guest
  pcie_aer: fix a trivial typo in PCIEAERMsg comments

 hw/i386/pc_piix.c         |  16 ++++++
 hw/pci/pcie_aer.c         |  73 +++++++++++++++----------
 hw/vfio/pci.c             | 134 ++++++++++++++++++++++++++++++++++++++++++----
 include/hw/pci/pcie_aer.h |   4 +-
 4 files changed, 188 insertions(+), 39 deletions(-)

-- 
1.9.3

[Qemu-devel] [RFC v3 01/10] 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 v3 02/10] 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 v3 03/10] aer: introduce pcie_aer_setup to setup aer related bits

[Chen Fan]

because function pcie_aer_init() is for adding a new aer capability,
but for vfio device, we only need to capture the aer capability from
vfio device configuration space, so here we introduce pcie_aer_setup()
to init all raw devices.

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

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index ece1487..18caf43 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -94,53 +94,31 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
     aer_log->log_num = 0;
 }
 
-int pcie_aer_init(PCIDevice *dev, uint16_t offset)
+void pcie_aer_setup(PCIDevice *dev, uint16_t offset, uint16_t log_max)
 {
     PCIExpressDevice *exp;
 
-    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
-                        offset, PCI_ERR_SIZEOF);
     exp = &dev->exp;
     exp->aer_cap = offset;
 
-    /* log_max is property */
-    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
-        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
-    }
-    /* clip down the value to avoid unreasobale memory usage */
-    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
-        return -EINVAL;
-    }
-    dev->exp.aer_log.log = g_malloc0(sizeof dev->exp.aer_log.log[0] *
-                                        dev->exp.aer_log.log_max);
 
     pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
                  PCI_ERR_UNC_SUPPORTED);
 
-    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
-                 PCI_ERR_UNC_SEVERITY_DEFAULT);
     pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_SEVER,
                  PCI_ERR_UNC_SUPPORTED);
 
     pci_long_test_and_set_mask(dev->w1cmask + offset + PCI_ERR_COR_STATUS,
                                PCI_ERR_COR_SUPPORTED);
 
-    pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
-                 PCI_ERR_COR_MASK_DEFAULT);
     pci_set_long(dev->wmask + offset + PCI_ERR_COR_MASK,
                  PCI_ERR_COR_SUPPORTED);
 
-    /* capabilities and control. multiple header logging is supported */
-    if (dev->exp.aer_log.log_max > 0) {
-        pci_set_long(dev->config + offset + PCI_ERR_CAP,
-                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC |
-                     PCI_ERR_CAP_MHRC);
+    if (log_max > 0) {
         pci_set_long(dev->wmask + offset + PCI_ERR_CAP,
                      PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE |
                      PCI_ERR_CAP_MHRE);
     } else {
-        pci_set_long(dev->config + offset + PCI_ERR_CAP,
-                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC);
         pci_set_long(dev->wmask + offset + PCI_ERR_CAP,
                      PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
     }
@@ -160,6 +138,43 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
         /* nothing */
         break;
     }
+}
+
+int pcie_aer_init(PCIDevice *dev, uint16_t offset)
+{
+
+    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
+                        offset, PCI_ERR_SIZEOF);
+
+    /* log_max is property */
+    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
+        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
+    }
+    /* clip down the value to avoid unreasobale memory usage */
+    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
+        return -EINVAL;
+    }
+
+    dev->exp.aer_log.log = g_malloc0(sizeof dev->exp.aer_log.log[0] *
+                                     dev->exp.aer_log.log_max);
+
+    /* capabilities and control. multiple header logging is supported */
+    if (dev->exp.aer_log.log_max > 0) {
+        pci_set_long(dev->config + offset + PCI_ERR_CAP,
+                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC |
+                     PCI_ERR_CAP_MHRC);
+    } else {
+        pci_set_long(dev->config + offset + PCI_ERR_CAP,
+                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC);
+    }
+
+    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
+                 PCI_ERR_UNC_SEVERITY_DEFAULT);
+    pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
+                 PCI_ERR_COR_MASK_DEFAULT);
+
+    pcie_aer_setup(dev, offset, dev->exp.aer_log.log_max);
+
     return 0;
 }
 
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index bcac80a..e675c7d 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -87,6 +87,7 @@ struct PCIEAERErr {
 
 extern const VMStateDescription vmstate_pcie_aer_log;
 
+void pcie_aer_setup(PCIDevice *dev, uint16_t offset, uint16_t log_max);
 int pcie_aer_init(PCIDevice *dev, uint16_t offset);
 void pcie_aer_exit(PCIDevice *dev);
 void pcie_aer_write_config(PCIDevice *dev,
-- 
1.9.3

[Qemu-devel] [RFC v3 04/10] vfio: add pcie extanded capability support

[Chen Fan]

when we detect extanded capability in vfio device, then
we should initialize the vfio device corresponding feature
register bits.
so guest OS can find it and set those bits as needed.
and initialize aer capability.

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 014a92c..75c932b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2435,6 +2435,20 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos)
     return next - pos;
 }
 
+static uint16_t vfio_ext_cap_max_size(PCIDevice *pdev, 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(pdev->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);
@@ -2658,16 +2672,85 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
     return 0;
 }
 
+static void vfio_setup_pcie_aer(VFIOPCIDevice *vdev, uint16_t pos)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint32_t err_cap;
+
+    err_cap = pci_get_long(pdev->config + pos + PCI_ERR_CAP);
+
+    if (err_cap & PCI_ERR_CAP_MHRC) {
+        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
+    } else {
+        pdev->exp.aer_log.log_max = 1;
+    }
+
+    pdev->exp.aer_log.log = g_malloc0(sizeof pdev->exp.aer_log.log[0] *
+                                      pdev->exp.aer_log.log_max);
+
+    pcie_aer_setup(pdev, pos, pdev->exp.aer_log.log_max);
+}
+
+static int vfio_add_ext_cap(VFIOPCIDevice *vdev, 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(pdev->config + pos);
+    cap_id = PCI_EXT_CAP_ID(header);
+    cap_ver = PCI_EXT_CAP_VER(header);
+    next = PCI_EXT_CAP_NEXT(header);
+
+    size = vfio_ext_cap_max_size(pdev, pos);
+
+    if (next) {
+        ret = vfio_add_ext_cap(vdev, next);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    pci_set_long(vdev->emulated_config_bits + pos, 0xffff);
+
+    switch (cap_id) {
+    case PCI_EXT_CAP_ID_ERR:
+        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
+        vfio_setup_pcie_aer(vdev, pos);
+        break;
+    default:
+        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
+        break;
+    }
+
+    return 0;
+}
+
 static int vfio_add_capabilities(VFIOPCIDevice *vdev)
 {
     PCIDevice *pdev = &vdev->pdev;
+    int ret;
 
     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) {
+        goto out;
+    }
+
+    if (!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
+        goto out;
+    }
+
+    ret = vfio_add_ext_cap(vdev, PCI_CONFIG_SPACE_SIZE);
+
+out:
+    return ret;
 }
 
 static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
-- 
1.9.3

[Qemu-devel] [RFC v3 05/10] 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 18caf43..f97fa84 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -385,7 +385,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 e675c7d..15ede17 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -103,5 +103,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 v3 06/10] piix: disable all vfio device aercap property

[Chen Fan]

because at i440FX platform, all pcie device don't support aer capability,
so for all vfio device, we don't need to expose the aer capability.

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 38b42b0..8001313 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -71,6 +71,20 @@ static bool smbios_uuid_encoded = true;
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
 
+static void pc_props_register_global(void) {
+
+    static GlobalProperty globalPropertys[] = {
+       {
+           .driver   = "vfio-pci",
+           .property = "aercap",
+           .value    = "off",
+        },
+        { /* end of list */ }
+    };
+
+    qdev_prop_register_global_list(globalPropertys);
+}
+
 /* PC hardware initialisation */
 static void pc_init1(MachineState *machine,
                      int pci_enabled,
@@ -301,6 +315,8 @@ static void pc_init1(MachineState *machine,
     if (pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
+
+    pc_props_register_global();
 }
 
 static void pc_init_pci(MachineState *machine)
-- 
1.9.3

[Qemu-devel] [RFC v3 07/10] vfio_pci: change vfio device features bit macro to enum definition

[Chen Fan]

Introduce an independent enum structure to define the features bitmap,
it would be good for adding new features definition.

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 75c932b..bf314a1 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -134,6 +134,12 @@ typedef struct VFIOMSIXInfo {
     void *mmap;
 } VFIOMSIXInfo;
 
+/* Bits in VFIOPCIDevice features field. */
+enum {
+#define VFIO_FEATURE_ENABLE_VGA_BIT 0
+    VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
+};
+
 typedef struct VFIOPCIDevice {
     PCIDevice pdev;
     VFIODevice vbasedev;
@@ -154,8 +160,6 @@ typedef struct VFIOPCIDevice {
     PCIHostDeviceAddress host;
     EventNotifier err_notifier;
     uint32_t features;
-#define VFIO_FEATURE_ENABLE_VGA_BIT 0
-#define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
     int32_t bootindex;
     uint8_t pm_cap;
     bool has_vga;
-- 
1.9.3

[Qemu-devel] [RFC v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature

[Chen Fan]

add a new "aercap" feature in vfio device, for controlling
whether expose aer capability.

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bf314a1..c21b40b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo {
 enum {
 #define VFIO_FEATURE_ENABLE_VGA_BIT 0
     VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
+#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1
+    VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT),
 };
 
 typedef struct VFIOPCIDevice {
@@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
 
     switch (cap_id) {
     case PCI_EXT_CAP_ID_ERR:
-        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
-        vfio_setup_pcie_aer(vdev, pos);
+        if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) {
+            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
+            vfio_setup_pcie_aer(vdev, pos);
+        }
         break;
     default:
         pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
@@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_VGA_BIT, false),
     DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
+    DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features,
+                    VFIO_FEATURE_ENABLE_AER_CAP_BIT, true),
     /*
      * TODO - support passed fds... is this necessary?
      * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
-- 
1.9.3

[Qemu-devel] [RFC v3 09/10] 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 | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c21b40b..5ba4e52 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3165,18 +3165,41 @@ 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 (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP &&
+        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 v3 10/10] pcie_aer: fix a trivial typo in PCIEAERMsg comments

[Chen Fan]

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

diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index 15ede17..227427e 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 v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature

[Alex Williamson]

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> add a new "aercap" feature in vfio device, for controlling
> whether expose aer capability.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index bf314a1..c21b40b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo {
>  enum {
>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>      VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
> +#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1
> +    VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT),
>  };
>  
>  typedef struct VFIOPCIDevice {
> @@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
>  
>      switch (cap_id) {
>      case PCI_EXT_CAP_ID_ERR:
> -        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> -        vfio_setup_pcie_aer(vdev, pos);
> +        if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) {
> +            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> +            vfio_setup_pcie_aer(vdev, pos);
> +        }

Maybe the question should be why we're adding extended capabilities at
all if the chipset doesn't expose them.  If we boot on 440fx, all
extended capability parsing could be disabled.  We could then add an
x-aer=off option to vfio-pci to allow the user to disable aer
specifically if they want.

>          break;
>      default:
>          pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> @@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
>      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> +    DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features,
> +                    VFIO_FEATURE_ENABLE_AER_CAP_BIT, true),
>      /*
>       * TODO - support passed fds... is this necessary?
>       * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),

Re: [Qemu-devel] [RFC v3 06/10] piix: disable all vfio device aercap property

[Alex Williamson]

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> because at i440FX platform, all pcie device don't support aer capability,
> so for all vfio device, we don't need to expose the aer capability.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc_piix.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 38b42b0..8001313 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -71,6 +71,20 @@ static bool smbios_uuid_encoded = true;
>  static bool gigabyte_align = true;
>  static bool has_reserved_memory = true;
>  
> +static void pc_props_register_global(void) {
> +
> +    static GlobalProperty globalPropertys[] = {
> +       {
> +           .driver   = "vfio-pci",
> +           .property = "aercap",
> +           .value    = "off",
> +        },
> +        { /* end of list */ }
> +    };
> +
> +    qdev_prop_register_global_list(globalPropertys);
> +}
> +

Does this compile?  vfio-pci doesn't have that property until a later
patch.

>  /* PC hardware initialisation */
>  static void pc_init1(MachineState *machine,
>                       int pci_enabled,
> @@ -301,6 +315,8 @@ static void pc_init1(MachineState *machine,
>      if (pci_enabled) {
>          pc_pci_device_init(pci_bus);
>      }
> +
> +    pc_props_register_global();
>  }
>  
>  static void pc_init_pci(MachineState *machine)

Re: [Qemu-devel] [RFC v3 07/10] vfio_pci: change vfio device features bit macro to enum definition

[Alex Williamson]

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> Introduce an independent enum structure to define the features bitmap,
> it would be good for adding new features definition.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 75c932b..bf314a1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -134,6 +134,12 @@ typedef struct VFIOMSIXInfo {
>      void *mmap;
>  } VFIOMSIXInfo;
>  
> +/* Bits in VFIOPCIDevice features field. */
> +enum {
> +#define VFIO_FEATURE_ENABLE_VGA_BIT 0
> +    VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
> +};
> +

As commented last time, I don't see why making this an enum helps
anything.  If anything, the bits could be an enum, but it makes no sense
to me to put the feature mask into an enum.

>  typedef struct VFIOPCIDevice {
>      PCIDevice pdev;
>      VFIODevice vbasedev;
> @@ -154,8 +160,6 @@ typedef struct VFIOPCIDevice {
>      PCIHostDeviceAddress host;
>      EventNotifier err_notifier;
>      uint32_t features;
> -#define VFIO_FEATURE_ENABLE_VGA_BIT 0
> -#define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>      int32_t bootindex;
>      uint8_t pm_cap;
>      bool has_vga;

Re: [Qemu-devel] [RFC v3 04/10] vfio: add pcie extanded capability support

[Alex Williamson]

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> when we detect extanded capability in vfio device, then
> we should initialize the vfio device corresponding feature
> register bits.
> so guest OS can find it and set those bits as needed.
> and initialize aer capability.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 014a92c..75c932b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2435,6 +2435,20 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos)
>      return next - pos;
>  }
>  
> +static uint16_t vfio_ext_cap_max_size(PCIDevice *pdev, 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(pdev->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);
> @@ -2658,16 +2672,85 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>      return 0;
>  }
>  
> +static void vfio_setup_pcie_aer(VFIOPCIDevice *vdev, uint16_t pos)
> +{
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint32_t err_cap;
> +
> +    err_cap = pci_get_long(pdev->config + pos + PCI_ERR_CAP);
> +
> +    if (err_cap & PCI_ERR_CAP_MHRC) {
> +        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> +    } else {
> +        pdev->exp.aer_log.log_max = 1;
> +    }
> +
> +    pdev->exp.aer_log.log = g_malloc0(sizeof pdev->exp.aer_log.log[0] *
> +                                      pdev->exp.aer_log.log_max);
> +
> +    pcie_aer_setup(pdev, pos, pdev->exp.aer_log.log_max);
> +}
> +
> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, 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(pdev->config + pos);
> +    cap_id = PCI_EXT_CAP_ID(header);
> +    cap_ver = PCI_EXT_CAP_VER(header);
> +    next = PCI_EXT_CAP_NEXT(header);
> +
> +    size = vfio_ext_cap_max_size(pdev, pos);
> +

There's a big comment in the standard capability version of this that
indicates that pci_add_capability() always adds capabilities to the head
of the chain and therefore we use this recursion to keep the same
ordering as hardware.  pcie_add_capability() seems to add at the tail of
the list, so I think this actually reverses the list versus hardware,
which is undesirable.  Code comments would be nice too.

> +    if (next) {
> +        ret = vfio_add_ext_cap(vdev, next);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    pci_set_long(vdev->emulated_config_bits + pos, 0xffff);
> +
> +    switch (cap_id) {
> +    case PCI_EXT_CAP_ID_ERR:
> +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> +        vfio_setup_pcie_aer(vdev, pos);
> +        break;
> +    default:
> +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
>  static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> +    int ret;
>  
>      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) {
> +        goto out;
> +    }
> +
> +    if (!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
> +        goto out;
> +    }
> +
> +    ret = vfio_add_ext_cap(vdev, PCI_CONFIG_SPACE_SIZE);
> +
> +out:
> +    return ret;
>  }
>  
>  static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)

Re: [Qemu-devel] [RFC v3 03/10] aer: introduce pcie_aer_setup to setup aer related bits

[Alex Williamson]

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> because function pcie_aer_init() is for adding a new aer capability,
> but for vfio device, we only need to capture the aer capability from
> vfio device configuration space, so here we introduce pcie_aer_setup()
> to init all raw devices.


I don't see why pcie_add_capability is split out, see for instance
msix_init() where the call still includes a call to
pci_add_capability().

> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie_aer.c         | 63 +++++++++++++++++++++++++++++------------------
>  include/hw/pci/pcie_aer.h |  1 +
>  2 files changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index ece1487..18caf43 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -94,53 +94,31 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
>      aer_log->log_num = 0;
>  }
>  
> -int pcie_aer_init(PCIDevice *dev, uint16_t offset)
> +void pcie_aer_setup(PCIDevice *dev, uint16_t offset, uint16_t log_max)
>  {
>      PCIExpressDevice *exp;
>  
> -    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> -                        offset, PCI_ERR_SIZEOF);
>      exp = &dev->exp;
>      exp->aer_cap = offset;
>  
> -    /* log_max is property */
> -    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> -        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> -    }
> -    /* clip down the value to avoid unreasobale memory usage */
> -    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
> -        return -EINVAL;
> -    }
> -    dev->exp.aer_log.log = g_malloc0(sizeof dev->exp.aer_log.log[0] *
> -                                        dev->exp.aer_log.log_max);
>  
>      pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
>                   PCI_ERR_UNC_SUPPORTED);
>  
> -    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
> -                 PCI_ERR_UNC_SEVERITY_DEFAULT);
>      pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_SEVER,
>                   PCI_ERR_UNC_SUPPORTED);
>  
>      pci_long_test_and_set_mask(dev->w1cmask + offset + PCI_ERR_COR_STATUS,
>                                 PCI_ERR_COR_SUPPORTED);
>  
> -    pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
> -                 PCI_ERR_COR_MASK_DEFAULT);
>      pci_set_long(dev->wmask + offset + PCI_ERR_COR_MASK,
>                   PCI_ERR_COR_SUPPORTED);
>  
> -    /* capabilities and control. multiple header logging is supported */
> -    if (dev->exp.aer_log.log_max > 0) {
> -        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> -                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC |
> -                     PCI_ERR_CAP_MHRC);
> +    if (log_max > 0) {
>          pci_set_long(dev->wmask + offset + PCI_ERR_CAP,
>                       PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE |
>                       PCI_ERR_CAP_MHRE);
>      } else {
> -        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> -                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC);
>          pci_set_long(dev->wmask + offset + PCI_ERR_CAP,
>                       PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
>      }
> @@ -160,6 +138,43 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
>          /* nothing */
>          break;
>      }
> +}
> +
> +int pcie_aer_init(PCIDevice *dev, uint16_t offset)
> +{
> +
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> +                        offset, PCI_ERR_SIZEOF);
> +
> +    /* log_max is property */
> +    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> +        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> +    }
> +    /* clip down the value to avoid unreasobale memory usage */
> +    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
> +        return -EINVAL;
> +    }
> +
> +    dev->exp.aer_log.log = g_malloc0(sizeof dev->exp.aer_log.log[0] *
> +                                     dev->exp.aer_log.log_max);
> +
> +    /* capabilities and control. multiple header logging is supported */
> +    if (dev->exp.aer_log.log_max > 0) {
> +        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC |
> +                     PCI_ERR_CAP_MHRC);
> +    } else {
> +        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC);
> +    }
> +
> +    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
> +                 PCI_ERR_UNC_SEVERITY_DEFAULT);
> +    pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
> +                 PCI_ERR_COR_MASK_DEFAULT);
> +
> +    pcie_aer_setup(dev, offset, dev->exp.aer_log.log_max);
> +
>      return 0;
>  }
>  
> diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
> index bcac80a..e675c7d 100644
> --- a/include/hw/pci/pcie_aer.h
> +++ b/include/hw/pci/pcie_aer.h
> @@ -87,6 +87,7 @@ struct PCIEAERErr {
>  
>  extern const VMStateDescription vmstate_pcie_aer_log;
>  
> +void pcie_aer_setup(PCIDevice *dev, uint16_t offset, uint16_t log_max);
>  int pcie_aer_init(PCIDevice *dev, uint16_t offset);
>  void pcie_aer_exit(PCIDevice *dev);
>  void pcie_aer_write_config(PCIDevice *dev,

Re: [Qemu-devel] [RFC v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature

[Chen Fan]

On 02/11/2015 12:39 AM, Alex Williamson wrote:
> On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
>> add a new "aercap" feature in vfio device, for controlling
>> whether expose aer capability.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index bf314a1..c21b40b 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo {
>>   enum {
>>   #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>>       VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
>> +#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1
>> +    VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT),
>>   };
>>   
>>   typedef struct VFIOPCIDevice {
>> @@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
>>   
>>       switch (cap_id) {
>>       case PCI_EXT_CAP_ID_ERR:
>> -        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> -        vfio_setup_pcie_aer(vdev, pos);
>> +        if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) {
>> +            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> +            vfio_setup_pcie_aer(vdev, pos);
>> +        }
Hi Alex,
     sorry for replaying so late. I am just back from holiday. :)

> Maybe the question should be why we're adding extended capabilities at
> all if the chipset doesn't expose them.  If we boot on 440fx, all
> extended capability parsing could be disabled.  We could then add an
> x-aer=off option to vfio-pci to allow the user to disable aer
> specifically if they want.
your meaning is adding two option:

1) one controls exposing extended caps to device. in particular on
440fx chipset we must disable it.

2) the other one controls exposing aer capability to user specifically.

right?

Thanks,
Chen





>>           break;
>>       default:
>>           pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> @@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = {
>>       DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>>                       VFIO_FEATURE_ENABLE_VGA_BIT, false),
>>       DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>> +    DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features,
>> +                    VFIO_FEATURE_ENABLE_AER_CAP_BIT, true),
>>       /*
>>        * TODO - support passed fds... is this necessary?
>>        * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
>
>
> .
>

Re: [Qemu-devel] [RFC v3 04/10] vfio: add pcie extanded capability support

[Chen Fan]

On 02/11/2015 12:39 AM, Alex Williamson wrote:
> On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
>> when we detect extanded capability in vfio device, then
>> we should initialize the vfio device corresponding feature
>> register bits.
>> so guest OS can find it and set those bits as needed.
>> and initialize aer capability.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 014a92c..75c932b 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2435,6 +2435,20 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos)
>>       return next - pos;
>>   }
>>   
>> +static uint16_t vfio_ext_cap_max_size(PCIDevice *pdev, 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(pdev->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);
>> @@ -2658,16 +2672,85 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>       return 0;
>>   }
>>   
>> +static void vfio_setup_pcie_aer(VFIOPCIDevice *vdev, uint16_t pos)
>> +{
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    uint32_t err_cap;
>> +
>> +    err_cap = pci_get_long(pdev->config + pos + PCI_ERR_CAP);
>> +
>> +    if (err_cap & PCI_ERR_CAP_MHRC) {
>> +        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
>> +    } else {
>> +        pdev->exp.aer_log.log_max = 1;
>> +    }
>> +
>> +    pdev->exp.aer_log.log = g_malloc0(sizeof pdev->exp.aer_log.log[0] *
>> +                                      pdev->exp.aer_log.log_max);
>> +
>> +    pcie_aer_setup(pdev, pos, pdev->exp.aer_log.log_max);
>> +}
>> +
>> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, 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(pdev->config + pos);
>> +    cap_id = PCI_EXT_CAP_ID(header);
>> +    cap_ver = PCI_EXT_CAP_VER(header);
>> +    next = PCI_EXT_CAP_NEXT(header);
>> +
>> +    size = vfio_ext_cap_max_size(pdev, pos);
>> +
> There's a big comment in the standard capability version of this that
> indicates that pci_add_capability() always adds capabilities to the head
> of the chain and therefore we use this recursion to keep the same
> ordering as hardware.  pcie_add_capability() seems to add at the tail of
> the list, so I think this actually reverses the list versus hardware,
> which is undesirable.  Code comments would be nice too.

IIUC, Compare pcie_add_capability() with pci_add_capability(), 
pcie_add_capability()
not only add caps at the tail of linked list, but also always add a new cap
at the tail of linked list. but for vfio device, we don't need to add a 
new cap.
we only need to parse the existed capability in config space.
so I think we should modify  pcie_add_capability() as pci_add_capability2().

Thanks,
Chen


>
>> +    if (next) {
>> +        ret = vfio_add_ext_cap(vdev, next);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    pci_set_long(vdev->emulated_config_bits + pos, 0xffff);
>> +
>> +    switch (cap_id) {
>> +    case PCI_EXT_CAP_ID_ERR:
>> +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> +        vfio_setup_pcie_aer(vdev, pos);
>> +        break;
>> +    default:
>> +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>> +        break;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>>   {
>>       PCIDevice *pdev = &vdev->pdev;
>> +    int ret;
>>   
>>       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) {
>> +        goto out;
>> +    }
>> +
>> +    if (!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
>> +        goto out;
>> +    }
>> +
>> +    ret = vfio_add_ext_cap(vdev, PCI_CONFIG_SPACE_SIZE);
>> +
>> +out:
>> +    return ret;
>>   }
>>   
>>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>
>
> .
>

Re: [Qemu-devel] [RFC v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature

[Alex Williamson]

On Thu, 2015-02-26 at 14:46 +0800, Chen Fan wrote:
> On 02/11/2015 12:39 AM, Alex Williamson wrote:
> > On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> >> add a new "aercap" feature in vfio device, for controlling
> >> whether expose aer capability.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/vfio/pci.c | 10 ++++++++--
> >>   1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index bf314a1..c21b40b 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo {
> >>   enum {
> >>   #define VFIO_FEATURE_ENABLE_VGA_BIT 0
> >>       VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
> >> +#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1
> >> +    VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT),
> >>   };
> >>   
> >>   typedef struct VFIOPCIDevice {
> >> @@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
> >>   
> >>       switch (cap_id) {
> >>       case PCI_EXT_CAP_ID_ERR:
> >> -        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> >> -        vfio_setup_pcie_aer(vdev, pos);
> >> +        if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) {
> >> +            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> >> +            vfio_setup_pcie_aer(vdev, pos);
> >> +        }
> Hi Alex,
>      sorry for replaying so late. I am just back from holiday. :)
> 
> > Maybe the question should be why we're adding extended capabilities at
> > all if the chipset doesn't expose them.  If we boot on 440fx, all
> > extended capability parsing could be disabled.  We could then add an
> > x-aer=off option to vfio-pci to allow the user to disable aer
> > specifically if they want.
> your meaning is adding two option:
> 
> 1) one controls exposing extended caps to device. in particular on
> 440fx chipset we must disable it.
> 
> 2) the other one controls exposing aer capability to user specifically.
> 
> right?

Right, a flag toggled by the chipset type doesn't work because whether
we want to expose AER is dependent on the exposed bus type to the guest.
Even if we're using an express chipset like q35, the device could be
behind a PCIe-to-PCI bridge in the guest and the guest would not have
access to extended config space, including AER.  Therefore it seems like
we simply need to test pci_bus_is_express() for a device and skip all
extended capability parsing when false.

Then we've solved the bus topology problem and we only need to be
concerned about whether the user has a need to disable AER by choice.
We probably do need to allow that for compatibility with existing PCIe
guest configurations.  I'd probably name it something like "disable_aer"
because we don't want to give the user any ideas that it can be used to
toggle AER on, only off.  Thanks,

Alex

> >>           break;
> >>       default:
> >>           pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> >> @@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = {
> >>       DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> >>                       VFIO_FEATURE_ENABLE_VGA_BIT, false),
> >>       DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> >> +    DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features,
> >> +                    VFIO_FEATURE_ENABLE_AER_CAP_BIT, true),
> >>       /*
> >>        * TODO - support passed fds... is this necessary?
> >>        * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
> >
> >
> > .
> >
> 

Re: [Qemu-devel] [RFC v3 04/10] vfio: add pcie extanded capability support

[Alex Williamson]

On Thu, 2015-02-26 at 17:37 +0800, Chen Fan wrote:
> On 02/11/2015 12:39 AM, Alex Williamson wrote:
> > On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> >> when we detect extanded capability in vfio device, then
> >> we should initialize the vfio device corresponding feature
> >> register bits.
> >> so guest OS can find it and set those bits as needed.
> >> and initialize aer capability.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/vfio/pci.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 84 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 014a92c..75c932b 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -2435,6 +2435,20 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos)
> >>       return next - pos;
> >>   }
> >>   
> >> +static uint16_t vfio_ext_cap_max_size(PCIDevice *pdev, 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(pdev->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);
> >> @@ -2658,16 +2672,85 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> >>       return 0;
> >>   }
> >>   
> >> +static void vfio_setup_pcie_aer(VFIOPCIDevice *vdev, uint16_t pos)
> >> +{
> >> +    PCIDevice *pdev = &vdev->pdev;
> >> +    uint32_t err_cap;
> >> +
> >> +    err_cap = pci_get_long(pdev->config + pos + PCI_ERR_CAP);
> >> +
> >> +    if (err_cap & PCI_ERR_CAP_MHRC) {
> >> +        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> >> +    } else {
> >> +        pdev->exp.aer_log.log_max = 1;
> >> +    }
> >> +
> >> +    pdev->exp.aer_log.log = g_malloc0(sizeof pdev->exp.aer_log.log[0] *
> >> +                                      pdev->exp.aer_log.log_max);
> >> +
> >> +    pcie_aer_setup(pdev, pos, pdev->exp.aer_log.log_max);
> >> +}
> >> +
> >> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, 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(pdev->config + pos);
> >> +    cap_id = PCI_EXT_CAP_ID(header);
> >> +    cap_ver = PCI_EXT_CAP_VER(header);
> >> +    next = PCI_EXT_CAP_NEXT(header);
> >> +
> >> +    size = vfio_ext_cap_max_size(pdev, pos);
> >> +
> > There's a big comment in the standard capability version of this that
> > indicates that pci_add_capability() always adds capabilities to the head
> > of the chain and therefore we use this recursion to keep the same
> > ordering as hardware.  pcie_add_capability() seems to add at the tail of
> > the list, so I think this actually reverses the list versus hardware,
> > which is undesirable.  Code comments would be nice too.
> 
> IIUC, Compare pcie_add_capability() with pci_add_capability(), 
> pcie_add_capability()
> not only add caps at the tail of linked list, but also always add a new cap
> at the tail of linked list. but for vfio device, we don't need to add a 
> new cap.
> we only need to parse the existed capability in config space.
> so I think we should modify  pcie_add_capability() as pci_add_capability2().

We want vfio to use the same interfaces to the QEMU core PCI code as an
emulated device.  We add capabilities so that we can inform the core
code that they're there and potentially get benefits later as emulated
devices add capabilities.  I don't see why we'd add a new interface to
the PCI code for this, we simply need to analyze how they operate and
use them accordingly.  In this case we should not simply do a blind copy
of the standard capability code because the ordering is different.
Thanks,

Alex

> >> +    if (next) {
> >> +        ret = vfio_add_ext_cap(vdev, next);
> >> +        if (ret) {
> >> +            return ret;
> >> +        }
> >> +    }
> >> +
> >> +    pci_set_long(vdev->emulated_config_bits + pos, 0xffff);
> >> +
> >> +    switch (cap_id) {
> >> +    case PCI_EXT_CAP_ID_ERR:
> >> +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> >> +        vfio_setup_pcie_aer(vdev, pos);
> >> +        break;
> >> +    default:
> >> +        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
> >> +        break;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> >>   {
> >>       PCIDevice *pdev = &vdev->pdev;
> >> +    int ret;
> >>   
> >>       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) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    if (!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = vfio_add_ext_cap(vdev, PCI_CONFIG_SPACE_SIZE);
> >> +
> >> +out:
> >> +    return ret;
> >>   }
> >>   
> >>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
> >
> >
> > .
> >
> 

Re: [Qemu-devel] [RFC v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature

[Chen Fan]

On 02/27/2015 06:18 AM, Alex Williamson wrote:
> On Thu, 2015-02-26 at 14:46 +0800, Chen Fan wrote:
>> On 02/11/2015 12:39 AM, Alex Williamson wrote:
>>> On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
>>>> add a new "aercap" feature in vfio device, for controlling
>>>> whether expose aer capability.
>>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>> ---
>>>>    hw/vfio/pci.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index bf314a1..c21b40b 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo {
>>>>    enum {
>>>>    #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>>>>        VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
>>>> +#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1
>>>> +    VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT),
>>>>    };
>>>>    
>>>>    typedef struct VFIOPCIDevice {
>>>> @@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
>>>>    
>>>>        switch (cap_id) {
>>>>        case PCI_EXT_CAP_ID_ERR:
>>>> -        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>>>> -        vfio_setup_pcie_aer(vdev, pos);
>>>> +        if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) {
>>>> +            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>>>> +            vfio_setup_pcie_aer(vdev, pos);
>>>> +        }
>> Hi Alex,
>>       sorry for replaying so late. I am just back from holiday. :)
>>
>>> Maybe the question should be why we're adding extended capabilities at
>>> all if the chipset doesn't expose them.  If we boot on 440fx, all
>>> extended capability parsing could be disabled.  We could then add an
>>> x-aer=off option to vfio-pci to allow the user to disable aer
>>> specifically if they want.
>> your meaning is adding two option:
>>
>> 1) one controls exposing extended caps to device. in particular on
>> 440fx chipset we must disable it.
>>
>> 2) the other one controls exposing aer capability to user specifically.
>>
>> right?
> Right, a flag toggled by the chipset type doesn't work because whether
> we want to expose AER is dependent on the exposed bus type to the guest.
> Even if we're using an express chipset like q35, the device could be
> behind a PCIe-to-PCI bridge in the guest and the guest would not have
> access to extended config space, including AER.  Therefore it seems like
> we simply need to test pci_bus_is_express() for a device and skip all
> extended capability parsing when false.
>
> Then we've solved the bus topology problem and we only need to be
> concerned about whether the user has a need to disable AER by choice.
> We probably do need to allow that for compatibility with existing PCIe
> guest configurations.  I'd probably name it something like "disable_aer"
> because we don't want to give the user any ideas that it can be used to
> toggle AER on, only off.  Thanks,
Hi Alex,

I has made a v4 patch which added a 'x-aer' to enable/disable aer capability
and remake the parsing extended caps method.
pls help to review it. :)

Thanks,
Chen

>
> Alex
>
>>>>            break;
>>>>        default:
>>>>            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>>>> @@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = {
>>>>        DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>>>>                        VFIO_FEATURE_ENABLE_VGA_BIT, false),
>>>>        DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>>>> +    DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features,
>>>> +                    VFIO_FEATURE_ENABLE_AER_CAP_BIT, true),
>>>>        /*
>>>>         * TODO - support passed fds... is this necessary?
>>>>         * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
>>>
>>> .
>>>
>
>
> .
>