* [Qemu-devel] [PATCH v2 01/11] vfio: extract vfio_get_hot_reset_info as a single function
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
@ 2016-03-07 3:22 ` Cao jin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 02/11] vfio: squeeze out vfio_pci_do_hot_reset for support bus reset Cao jin
` (9 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:22 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
the function is used to get affected devices by bus reset.
so here extract it, and can used for aer soon.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 66 +++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 48 insertions(+), 18 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e671506..9ddaa99 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1691,6 +1691,51 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
}
}
+/*
+ * return negative with errno, return 0 on success.
+ * if success, the point of ret_info fill with the affected device reset info.
+ *
+ */
+static int vfio_get_hot_reset_info(VFIOPCIDevice *vdev,
+ struct vfio_pci_hot_reset_info **ret_info)
+{
+ struct vfio_pci_hot_reset_info *info;
+ int ret, count;
+
+ *ret_info = NULL;
+
+ info = g_malloc0(sizeof(*info));
+ info->argsz = sizeof(*info);
+
+ ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
+ if (ret && errno != ENOSPC) {
+ ret = -errno;
+ goto error;
+ }
+
+ count = info->count;
+
+ info = g_realloc(info, sizeof(*info) +
+ (count * sizeof(struct vfio_pci_dependent_device)));
+ info->argsz = sizeof(*info) +
+ (count * sizeof(struct vfio_pci_dependent_device));
+
+ ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
+ if (ret) {
+ ret = -errno;
+ error_report("vfio: hot reset info failed: %m");
+ goto error;
+ }
+
+ *ret_info = info;
+ info = NULL;
+
+ return 0;
+error:
+ g_free(info);
+ return ret;
+}
+
static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
{
PCIDevice *pdev = &vdev->pdev;
@@ -1830,7 +1875,7 @@ static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
{
VFIOGroup *group;
- struct vfio_pci_hot_reset_info *info;
+ struct vfio_pci_hot_reset_info *info = NULL;
struct vfio_pci_dependent_device *devices;
struct vfio_pci_hot_reset *reset;
int32_t *fds;
@@ -1842,12 +1887,8 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
vfio_pci_pre_reset(vdev);
vdev->vbasedev.needs_reset = false;
- info = g_malloc0(sizeof(*info));
- info->argsz = sizeof(*info);
-
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
- if (ret && errno != ENOSPC) {
- ret = -errno;
+ ret = vfio_get_hot_reset_info(vdev, &info);
+ if (ret) {
if (!vdev->has_pm_reset) {
error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
"no available reset mechanism.", vdev->host.domain,
@@ -1856,18 +1897,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
goto out_single;
}
- count = info->count;
- info = g_realloc(info, sizeof(*info) + (count * sizeof(*devices)));
- info->argsz = sizeof(*info) + (count * sizeof(*devices));
devices = &info->devices[0];
-
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
- if (ret) {
- ret = -errno;
- error_report("vfio: hot reset info failed: %m");
- goto out_single;
- }
-
trace_vfio_pci_hot_reset_has_dep_devices(vdev->vbasedev.name);
/* Verify that we have all the groups required */
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 02/11] vfio: squeeze out vfio_pci_do_hot_reset for support bus reset
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 01/11] vfio: extract vfio_get_hot_reset_info as a single function Cao jin
@ 2016-03-07 3:22 ` Cao jin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 03/11] vfio: add pcie extended capability support Cao jin
` (8 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:22 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
squeeze out vfio_pci_do_hot_reset to do host bus reset when AER recovery.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 75 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 44 insertions(+), 31 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9ddaa99..b03c394 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1736,6 +1736,48 @@ error:
return ret;
}
+static int vfio_pci_do_hot_reset(VFIOPCIDevice *vdev,
+ struct vfio_pci_hot_reset_info *info)
+{
+ VFIOGroup *group;
+ struct vfio_pci_hot_reset *reset;
+ int32_t *fds;
+ int ret, i, count;
+ struct vfio_pci_dependent_device *devices;
+
+ /* Determine how many group fds need to be passed */
+ count = 0;
+ devices = &info->devices[0];
+ QLIST_FOREACH(group, &vfio_group_list, next) {
+ for (i = 0; i < info->count; i++) {
+ if (group->groupid == devices[i].group_id) {
+ count++;
+ break;
+ }
+ }
+ }
+
+ reset = g_malloc0(sizeof(*reset) + (count * sizeof(*fds)));
+ reset->argsz = sizeof(*reset) + (count * sizeof(*fds));
+ fds = &reset->group_fds[0];
+
+ /* Fill in group fds */
+ QLIST_FOREACH(group, &vfio_group_list, next) {
+ for (i = 0; i < info->count; i++) {
+ if (group->groupid == devices[i].group_id) {
+ fds[reset->count++] = group->fd;
+ break;
+ }
+ }
+ }
+
+ /* Bus reset! */
+ ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
+ g_free(reset);
+
+ return ret;
+}
+
static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
{
PCIDevice *pdev = &vdev->pdev;
@@ -1877,9 +1919,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
VFIOGroup *group;
struct vfio_pci_hot_reset_info *info = NULL;
struct vfio_pci_dependent_device *devices;
- struct vfio_pci_hot_reset *reset;
- int32_t *fds;
- int ret, i, count;
+ int ret, i;
bool multi = false;
trace_vfio_pci_hot_reset(vdev->vbasedev.name, single ? "one" : "multi");
@@ -1958,34 +1998,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
goto out_single;
}
- /* Determine how many group fds need to be passed */
- count = 0;
- QLIST_FOREACH(group, &vfio_group_list, next) {
- for (i = 0; i < info->count; i++) {
- if (group->groupid == devices[i].group_id) {
- count++;
- break;
- }
- }
- }
-
- reset = g_malloc0(sizeof(*reset) + (count * sizeof(*fds)));
- reset->argsz = sizeof(*reset) + (count * sizeof(*fds));
- fds = &reset->group_fds[0];
-
- /* Fill in group fds */
- QLIST_FOREACH(group, &vfio_group_list, next) {
- for (i = 0; i < info->count; i++) {
- if (group->groupid == devices[i].group_id) {
- fds[reset->count++] = group->fd;
- break;
- }
- }
- }
-
- /* Bus reset! */
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
- g_free(reset);
+ ret = vfio_pci_do_hot_reset(vdev, info);
trace_vfio_pci_hot_reset_result(vdev->vbasedev.name,
ret ? "%m" : "Success");
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 03/11] vfio: add pcie extended capability support
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 01/11] vfio: extract vfio_get_hot_reset_info as a single function Cao jin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 02/11] vfio: squeeze out vfio_pci_do_hot_reset for support bus reset Cao jin
@ 2016-03-07 3:22 ` Cao jin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device Cao jin
` (7 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:22 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
For vfio pcie device, we could expose the extended capability on
PCIE bus. due to add a new pcie capability at the tail of the chain,
in order to avoid config space overwritten, 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 | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index b03c394..e64cce3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1518,6 +1518,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;
+
+ 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);
@@ -1853,16 +1868,71 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
return 0;
}
+static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
+{
+ PCIDevice *pdev = &vdev->pdev;
+ uint32_t header;
+ uint16_t cap_id, next, size;
+ uint8_t cap_ver;
+ uint8_t *config;
+
+ /*
+ * pcie_add_capability always inserts the new capability at the tail
+ * of the chain. Therefore to end up with a chain that matches the
+ * physical device, we cache the config space to avoid overwriting
+ * the original config space when we parse the extended capabilities.
+ */
+ config = g_memdup(pdev->config, vdev->config_size);
+
+ for (next = PCI_CONFIG_SPACE_SIZE; next;
+ next = PCI_EXT_CAP_NEXT(pci_get_long(config + next))) {
+ header = pci_get_long(config + next);
+ cap_id = PCI_EXT_CAP_ID(header);
+ cap_ver = PCI_EXT_CAP_VER(header);
+
+ /*
+ * If it becomes important to configure extended capabilities to their
+ * actual size, use this as the default when it's something we don't
+ * recognize. Since QEMU doesn't actually handle many of the config
+ * accesses, exact size doesn't seem worthwhile.
+ */
+ size = vfio_ext_cap_max_size(config, next);
+
+ pcie_add_capability(pdev, cap_id, cap_ver, next, size);
+ pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
+
+ /* Use emulated next pointer to allow dropping extended caps */
+ pci_long_test_and_set_mask(vdev->emulated_config_bits + next,
+ PCI_EXT_CAP_NEXT_MASK);
+ }
+
+ g_free(config);
+ 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) {
+ return ret;
+ }
+
+ /* on PCI bus, it doesn't make sense to expose extended capabilities. */
+ if (!pci_is_express(pdev) ||
+ !pci_bus_is_express(pdev->bus) ||
+ !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
+ return 0;
+ }
+
+ return vfio_add_ext_cap(vdev);
}
static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (2 preceding siblings ...)
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 03/11] vfio: add pcie extended capability support Cao jin
@ 2016-03-07 3:22 ` Cao jin
2016-03-08 22:55 ` Alex Williamson
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not Cao jin
` (6 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:22 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
hw/vfio/pci.h | 3 +++
2 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e64cce3..8ec9b25 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1868,6 +1868,62 @@ 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;
+ PCIDevice *dev_iter;
+ uint8_t type;
+ uint32_t errcap;
+
+ if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+ pcie_add_capability(pdev, PCI_EXT_CAP_ID_ERR,
+ cap_ver, pos, size);
+ return 0;
+ }
+
+ dev_iter = pci_bridge_get_device(pdev->bus);
+ if (!dev_iter) {
+ goto error;
+ }
+
+ while (dev_iter) {
+ type = pcie_cap_get_type(dev_iter);
+ if ((type != PCI_EXP_TYPE_ROOT_PORT &&
+ type != PCI_EXP_TYPE_UPSTREAM &&
+ type != PCI_EXP_TYPE_DOWNSTREAM)) {
+ goto error;
+ }
+
+ if (!dev_iter->exp.aer_cap) {
+ goto error;
+ }
+
+ dev_iter = pci_bridge_get_device(dev_iter->bus);
+ }
+
+ errcap = vfio_pci_read_config(pdev, pos + PCI_ERR_CAP, 4);
+ /*
+ * The ability to record multiple headers is depending on
+ * the state of the Multiple Header Recording Capable bit and
+ * enabled by the Multiple Header Recording Enable bit.
+ */
+ if ((errcap & PCI_ERR_CAP_MHRC) &&
+ (errcap & PCI_ERR_CAP_MHRE)) {
+ pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
+ } else {
+ pdev->exp.aer_log.log_max = 0;
+ }
+
+ pcie_cap_deverr_init(pdev);
+ return pcie_aer_init(pdev, pos, size);
+
+error:
+ error_report("vfio: Unable to enable AER for device %s, parent bus "
+ "does not support AER signaling", vdev->vbasedev.name);
+ return -1;
+}
+
static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
{
PCIDevice *pdev = &vdev->pdev;
@@ -1875,6 +1931,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
uint16_t cap_id, next, size;
uint8_t cap_ver;
uint8_t *config;
+ int ret = 0;
/*
* pcie_add_capability always inserts the new capability at the tail
@@ -1898,16 +1955,29 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
*/
size = vfio_ext_cap_max_size(config, next);
- pcie_add_capability(pdev, cap_id, cap_ver, next, size);
- pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
+ switch (cap_id) {
+ case PCI_EXT_CAP_ID_ERR:
+ ret = vfio_setup_aer(vdev, cap_ver, next, size);
+ break;
+ default:
+ pcie_add_capability(pdev, cap_id, cap_ver, next, size);
+ break;
+ }
+
+ if (ret) {
+ goto out;
+ }
+
+ pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
/* Use emulated next pointer to allow dropping extended caps */
pci_long_test_and_set_mask(vdev->emulated_config_bits + next,
PCI_EXT_CAP_NEXT_MASK);
}
+out:
g_free(config);
- return 0;
+ return ret;
}
static int vfio_add_capabilities(VFIOPCIDevice *vdev)
@@ -2662,6 +2732,11 @@ static int vfio_initfn(PCIDevice *pdev)
goto out_teardown;
}
+ if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
+ !pdev->exp.aer_cap) {
+ goto out_teardown;
+ }
+
/* QEMU emulates all of MSI & MSIX */
if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 6256587..e0c53f2 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -15,6 +15,7 @@
#include "qemu-common.h"
#include "exec/memory.h"
#include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
#include "hw/vfio/vfio-common.h"
#include "qemu/event_notifier.h"
#include "qemu/queue.h"
@@ -128,6 +129,8 @@ typedef struct VFIOPCIDevice {
#define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
#define VFIO_FEATURE_ENABLE_REQ_BIT 1
#define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
+#define VFIO_FEATURE_ENABLE_AER_BIT 2
+#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
int32_t bootindex;
uint8_t pm_cap;
bool has_vga;
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device Cao jin
@ 2016-03-08 22:55 ` Alex Williamson
2016-03-09 1:21 ` Chen Fan
0 siblings, 1 reply; 31+ messages in thread
From: Alex Williamson @ 2016-03-08 22:55 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, qemu-devel, mst
On Mon, 7 Mar 2016 11:22:57 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> 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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> hw/vfio/pci.h | 3 +++
> 2 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e64cce3..8ec9b25 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1868,6 +1868,62 @@ 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;
> + PCIDevice *dev_iter;
> + uint8_t type;
> + uint32_t errcap;
> +
> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> + pcie_add_capability(pdev, PCI_EXT_CAP_ID_ERR,
> + cap_ver, pos, size);
> + return 0;
> + }
> +
> + dev_iter = pci_bridge_get_device(pdev->bus);
> + if (!dev_iter) {
> + goto error;
> + }
> +
> + while (dev_iter) {
> + type = pcie_cap_get_type(dev_iter);
This asserts if dev_iter doesn't have an express capability so do we
really ever get to the error goto in practice? I think an average user
is going to get more information from your error_report than from an
assert. Thanks,
Alex
> + if ((type != PCI_EXP_TYPE_ROOT_PORT &&
> + type != PCI_EXP_TYPE_UPSTREAM &&
> + type != PCI_EXP_TYPE_DOWNSTREAM)) {
> + goto error;
> + }
> +
> + if (!dev_iter->exp.aer_cap) {
> + goto error;
> + }
> +
> + dev_iter = pci_bridge_get_device(dev_iter->bus);
> + }
> +
> + errcap = vfio_pci_read_config(pdev, pos + PCI_ERR_CAP, 4);
> + /*
> + * The ability to record multiple headers is depending on
> + * the state of the Multiple Header Recording Capable bit and
> + * enabled by the Multiple Header Recording Enable bit.
> + */
> + if ((errcap & PCI_ERR_CAP_MHRC) &&
> + (errcap & PCI_ERR_CAP_MHRE)) {
> + pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> + } else {
> + pdev->exp.aer_log.log_max = 0;
> + }
> +
> + pcie_cap_deverr_init(pdev);
> + return pcie_aer_init(pdev, pos, size);
> +
> +error:
> + error_report("vfio: Unable to enable AER for device %s, parent bus "
> + "does not support AER signaling", vdev->vbasedev.name);
> + return -1;
> +}
> +
> static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> {
> PCIDevice *pdev = &vdev->pdev;
> @@ -1875,6 +1931,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> uint16_t cap_id, next, size;
> uint8_t cap_ver;
> uint8_t *config;
> + int ret = 0;
>
> /*
> * pcie_add_capability always inserts the new capability at the tail
> @@ -1898,16 +1955,29 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> */
> size = vfio_ext_cap_max_size(config, next);
>
> - pcie_add_capability(pdev, cap_id, cap_ver, next, size);
> - pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
> + switch (cap_id) {
> + case PCI_EXT_CAP_ID_ERR:
> + ret = vfio_setup_aer(vdev, cap_ver, next, size);
> + break;
> + default:
> + pcie_add_capability(pdev, cap_id, cap_ver, next, size);
> + break;
> + }
> +
> + if (ret) {
> + goto out;
> + }
> +
> + pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
>
> /* Use emulated next pointer to allow dropping extended caps */
> pci_long_test_and_set_mask(vdev->emulated_config_bits + next,
> PCI_EXT_CAP_NEXT_MASK);
> }
>
> +out:
> g_free(config);
> - return 0;
> + return ret;
> }
>
> static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> @@ -2662,6 +2732,11 @@ static int vfio_initfn(PCIDevice *pdev)
> goto out_teardown;
> }
>
> + if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> + !pdev->exp.aer_cap) {
> + goto out_teardown;
> + }
> +
> /* QEMU emulates all of MSI & MSIX */
> if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
> memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 6256587..e0c53f2 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -15,6 +15,7 @@
> #include "qemu-common.h"
> #include "exec/memory.h"
> #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bridge.h"
> #include "hw/vfio/vfio-common.h"
> #include "qemu/event_notifier.h"
> #include "qemu/queue.h"
> @@ -128,6 +129,8 @@ typedef struct VFIOPCIDevice {
> #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> #define VFIO_FEATURE_ENABLE_REQ_BIT 1
> #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
> +#define VFIO_FEATURE_ENABLE_AER_BIT 2
> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
> int32_t bootindex;
> uint8_t pm_cap;
> bool has_vga;
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device
2016-03-08 22:55 ` Alex Williamson
@ 2016-03-09 1:21 ` Chen Fan
0 siblings, 0 replies; 31+ messages in thread
From: Chen Fan @ 2016-03-09 1:21 UTC (permalink / raw)
To: Alex Williamson, Cao jin; +Cc: izumi.taku, qemu-devel, mst
On 03/09/2016 06:55 AM, Alex Williamson wrote:
> On Mon, 7 Mar 2016 11:22:57 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> 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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> hw/vfio/pci.h | 3 +++
>> 2 files changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index e64cce3..8ec9b25 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1868,6 +1868,62 @@ 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;
>> + PCIDevice *dev_iter;
>> + uint8_t type;
>> + uint32_t errcap;
>> +
>> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>> + pcie_add_capability(pdev, PCI_EXT_CAP_ID_ERR,
>> + cap_ver, pos, size);
>> + return 0;
>> + }
>> +
>> + dev_iter = pci_bridge_get_device(pdev->bus);
>> + if (!dev_iter) {
>> + goto error;
>> + }
>> +
>> + while (dev_iter) {
>> + type = pcie_cap_get_type(dev_iter);
> This asserts if dev_iter doesn't have an express capability so do we
> really ever get to the error goto in practice? I think an average user
> is going to get more information from your error_report than from an
> assert. Thanks,
you are right, I will fix it in the next version.
Thanks,
Chen
>
> Alex
>
>> + if ((type != PCI_EXP_TYPE_ROOT_PORT &&
>> + type != PCI_EXP_TYPE_UPSTREAM &&
>> + type != PCI_EXP_TYPE_DOWNSTREAM)) {
>> + goto error;
>> + }
>> +
>> + if (!dev_iter->exp.aer_cap) {
>> + goto error;
>> + }
>> +
>> + dev_iter = pci_bridge_get_device(dev_iter->bus);
>> + }
>> +
>> + errcap = vfio_pci_read_config(pdev, pos + PCI_ERR_CAP, 4);
>> + /*
>> + * The ability to record multiple headers is depending on
>> + * the state of the Multiple Header Recording Capable bit and
>> + * enabled by the Multiple Header Recording Enable bit.
>> + */
>> + if ((errcap & PCI_ERR_CAP_MHRC) &&
>> + (errcap & PCI_ERR_CAP_MHRE)) {
>> + pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
>> + } else {
>> + pdev->exp.aer_log.log_max = 0;
>> + }
>> +
>> + pcie_cap_deverr_init(pdev);
>> + return pcie_aer_init(pdev, pos, size);
>> +
>> +error:
>> + error_report("vfio: Unable to enable AER for device %s, parent bus "
>> + "does not support AER signaling", vdev->vbasedev.name);
>> + return -1;
>> +}
>> +
>> static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>> {
>> PCIDevice *pdev = &vdev->pdev;
>> @@ -1875,6 +1931,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>> uint16_t cap_id, next, size;
>> uint8_t cap_ver;
>> uint8_t *config;
>> + int ret = 0;
>>
>> /*
>> * pcie_add_capability always inserts the new capability at the tail
>> @@ -1898,16 +1955,29 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>> */
>> size = vfio_ext_cap_max_size(config, next);
>>
>> - pcie_add_capability(pdev, cap_id, cap_ver, next, size);
>> - pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
>> + switch (cap_id) {
>> + case PCI_EXT_CAP_ID_ERR:
>> + ret = vfio_setup_aer(vdev, cap_ver, next, size);
>> + break;
>> + default:
>> + pcie_add_capability(pdev, cap_id, cap_ver, next, size);
>> + break;
>> + }
>> +
>> + if (ret) {
>> + goto out;
>> + }
>> +
>> + pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
>>
>> /* Use emulated next pointer to allow dropping extended caps */
>> pci_long_test_and_set_mask(vdev->emulated_config_bits + next,
>> PCI_EXT_CAP_NEXT_MASK);
>> }
>>
>> +out:
>> g_free(config);
>> - return 0;
>> + return ret;
>> }
>>
>> static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>> @@ -2662,6 +2732,11 @@ static int vfio_initfn(PCIDevice *pdev)
>> goto out_teardown;
>> }
>>
>> + if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
>> + !pdev->exp.aer_cap) {
>> + goto out_teardown;
>> + }
>> +
>> /* QEMU emulates all of MSI & MSIX */
>> if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
>> memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 6256587..e0c53f2 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -15,6 +15,7 @@
>> #include "qemu-common.h"
>> #include "exec/memory.h"
>> #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bridge.h"
>> #include "hw/vfio/vfio-common.h"
>> #include "qemu/event_notifier.h"
>> #include "qemu/queue.h"
>> @@ -128,6 +129,8 @@ typedef struct VFIOPCIDevice {
>> #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>> #define VFIO_FEATURE_ENABLE_REQ_BIT 1
>> #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
>> +#define VFIO_FEATURE_ENABLE_AER_BIT 2
>> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
>> int32_t bootindex;
>> uint8_t pm_cap;
>> bool has_vga;
>
>
> .
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (3 preceding siblings ...)
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device Cao jin
@ 2016-03-07 3:22 ` Cao jin
2016-03-08 22:55 ` Alex Williamson
2016-03-09 16:47 ` Michael S. Tsirkin
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete Cao jin
` (5 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:22 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
when boot up a VM that assigning vfio devices with aer enabled, we
must check the vfio device whether support host bus reset. because
when one error occur. OS driver always recover the device by do a
bus reset, in order to recover the vfio device, qemu must to do a
host bus reset to reset the device to default status. and for all
affected devices by the bus reset. we must check them whether all
are assigned to the VM.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
hw/vfio/pci.h | 1 +
2 files changed, 212 insertions(+), 7 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8ec9b25..0898e34 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
return 0;
}
+static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1,
+ PCIHostDeviceAddress *host2)
+{
+ return (host1->domain == host2->domain && host1->bus == host2->bus &&
+ host1->slot == host2->slot);
+}
+
+static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
+ PCIHostDeviceAddress *host2)
+{
+ return (vfio_pci_host_slot_match(host1, host2) &&
+ host1->function == host2->function);
+}
+
+struct VFIODeviceFind {
+ PCIDevice *pdev;
+ bool found;
+};
+
+static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev,
+ void *opaque)
+{
+ DeviceState *dev = DEVICE(pdev);
+ DeviceClass *dc = DEVICE_GET_CLASS(dev);
+ VFIOPCIDevice *vdev;
+ struct VFIODeviceFind *find = opaque;
+
+ if (find->found) {
+ return;
+ }
+
+ if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
+ if (!dc->reset) {
+ goto found;
+ }
+ return;
+ }
+ vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+ if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
+ !vdev->vbasedev.reset_works) {
+ goto found;
+ }
+
+ return;
+found:
+ find->pdev = pdev;
+ find->found = true;
+}
+
+static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
+{
+ PCIBus *bus = vdev->pdev.bus;
+ struct vfio_pci_hot_reset_info *info = NULL;
+ struct vfio_pci_dependent_device *devices;
+ VFIOGroup *group;
+ struct VFIODeviceFind find;
+ int ret, i;
+
+ ret = vfio_get_hot_reset_info(vdev, &info);
+ if (ret) {
+ error_setg(errp, "vfio: Cannot enable AER for device %s,"
+ " device does not support hot reset.",
+ vdev->vbasedev.name);
+ return;
+ }
+
+ /* List all affected devices by bus reset */
+ devices = &info->devices[0];
+
+ /* Verify that we have all the groups required */
+ for (i = 0; i < info->count; i++) {
+ PCIHostDeviceAddress host;
+ VFIOPCIDevice *tmp;
+ VFIODevice *vbasedev_iter;
+ bool found = false;
+
+ host.domain = devices[i].segment;
+ host.bus = devices[i].bus;
+ host.slot = PCI_SLOT(devices[i].devfn);
+ host.function = PCI_FUNC(devices[i].devfn);
+
+ /* Skip the current device */
+ if (vfio_pci_host_match(&host, &vdev->host)) {
+ continue;
+ }
+
+ /* Ensure we own the group of the affected device */
+ QLIST_FOREACH(group, &vfio_group_list, next) {
+ if (group->groupid == devices[i].group_id) {
+ break;
+ }
+ }
+
+ if (!group) {
+ error_setg(errp, "vfio: Cannot enable AER for device %s, "
+ "depends on group %d which is not owned.",
+ vdev->vbasedev.name, devices[i].group_id);
+ goto out;
+ }
+
+ /* Ensure affected devices for reset on the same slot */
+ QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
+ if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
+ continue;
+ }
+ tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
+ if (vfio_pci_host_match(&host, &tmp->host)) {
+ /*
+ * AER errors may be broadcast to all functions of a multi-
+ * function endpoint. If any of those sibling functions are
+ * also assigned, they need to have AER enabled or else an
+ * error may continue to cause a vm_stop condition. IOW,
+ * AER setup of this function would be pointless.
+ */
+ if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) &&
+ !(tmp->features & VFIO_FEATURE_ENABLE_AER)) {
+ error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot"
+ " the dependent device %s which does not enable AER.",
+ vdev->vbasedev.name, tmp->vbasedev.name);
+ goto out;
+ }
+
+ if (tmp->pdev.bus != bus) {
+ error_setg(errp, "vfio: Cannot enable AER for device %s, "
+ "the dependent device %s is not on the same bus",
+ vdev->vbasedev.name, tmp->vbasedev.name);
+ goto out;
+ }
+ found = true;
+ break;
+ }
+ }
+
+ /* Ensure all affected devices assigned to VM */
+ if (!found) {
+ error_setg(errp, "vfio: Cannot enable AER for device %s, "
+ "the dependent device %04x:%02x:%02x.%x "
+ "is not assigned to VM.",
+ vdev->vbasedev.name, host.domain, host.bus,
+ host.slot, host.function);
+ goto out;
+ }
+ }
+
+ /*
+ * Check the all pci devices on or below the target bus
+ * have a reset mechanism at least.
+ */
+ find.pdev = NULL;
+ find.found = false;
+ pci_for_each_device(bus, pci_bus_num(bus),
+ vfio_check_device_noreset, &find);
+ if (find.found) {
+ error_setg(errp, "vfio: Cannot enable AER for device %s, "
+ "the affected device %s does not have a reset mechanism.",
+ vdev->vbasedev.name, find.pdev->name);
+ goto out;
+ }
+
+out:
+ g_free(info);
+ return;
+}
+
+static void vfio_check_devices_host_bus_reset(Error **errp)
+{
+ VFIOGroup *group;
+ VFIODevice *vbasedev;
+ VFIOPCIDevice *vdev;
+ Error *local_err = NULL;
+
+ /* Check All vfio-pci devices if have bus reset capability */
+ QLIST_FOREACH(group, &vfio_group_list, next) {
+ QLIST_FOREACH(vbasedev, &group->device_list, next) {
+ if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
+ continue;
+ }
+ vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+ if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
+ vfio_check_host_bus_reset(vdev, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ }
+ }
+ }
+
+ return;
+}
+
static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
int pos, uint16_t size)
{
@@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
vfio_intx_enable(vdev);
}
-static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
- PCIHostDeviceAddress *host2)
-{
- return (host1->domain == host2->domain && host1->bus == host2->bus &&
- host1->slot == host2->slot && host1->function == host2->function);
-}
-
static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
{
VFIOGroup *group;
@@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
vdev->req_enabled = false;
}
+static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
+{
+ Error *local_err = NULL;
+
+ vfio_check_devices_host_bus_reset(&local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ exit(1);
+ }
+}
+
+static Notifier machine_notifier = {
+ .notify = vfio_pci_machine_done_notify,
+};
+
static int vfio_initfn(PCIDevice *pdev)
{
VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
@@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = {
static void register_vfio_pci_dev_type(void)
{
type_register_static(&vfio_pci_dev_info);
+ /*
+ * Register notifier when machine init is done, since we need
+ * check the configration manner after all vfio device are inited.
+ */
+ qemu_add_machine_init_done_notifier(&machine_notifier);
}
type_init(register_vfio_pci_dev_type)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index e0c53f2..aff46c2 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -15,6 +15,7 @@
#include "qemu-common.h"
#include "exec/memory.h"
#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "hw/vfio/vfio-common.h"
#include "qemu/event_notifier.h"
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not Cao jin
@ 2016-03-08 22:55 ` Alex Williamson
2016-03-09 1:26 ` Chen Fan
2016-03-09 16:47 ` Michael S. Tsirkin
1 sibling, 1 reply; 31+ messages in thread
From: Alex Williamson @ 2016-03-08 22:55 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, qemu-devel, mst
On Mon, 7 Mar 2016 11:22:58 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> when boot up a VM that assigning vfio devices with aer enabled, we
> must check the vfio device whether support host bus reset. because
> when one error occur. OS driver always recover the device by do a
> bus reset, in order to recover the vfio device, qemu must to do a
> host bus reset to reset the device to default status. and for all
> affected devices by the bus reset. we must check them whether all
> are assigned to the VM.
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
> hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> hw/vfio/pci.h | 1 +
> 2 files changed, 212 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8ec9b25..0898e34 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> return 0;
> }
>
> +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1,
> + PCIHostDeviceAddress *host2)
> +{
> + return (host1->domain == host2->domain && host1->bus == host2->bus &&
> + host1->slot == host2->slot);
> +}
> +
> +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> + PCIHostDeviceAddress *host2)
> +{
> + return (vfio_pci_host_slot_match(host1, host2) &&
> + host1->function == host2->function);
> +}
> +
> +struct VFIODeviceFind {
> + PCIDevice *pdev;
> + bool found;
> +};
> +
> +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev,
> + void *opaque)
> +{
> + DeviceState *dev = DEVICE(pdev);
> + DeviceClass *dc = DEVICE_GET_CLASS(dev);
> + VFIOPCIDevice *vdev;
> + struct VFIODeviceFind *find = opaque;
> +
> + if (find->found) {
> + return;
> + }
> +
> + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> + if (!dc->reset) {
> + goto found;
> + }
> + return;
> + }
> + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> + !vdev->vbasedev.reset_works) {
> + goto found;
> + }
> +
> + return;
> +found:
> + find->pdev = pdev;
> + find->found = true;
> +}
> +
> +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> +{
> + PCIBus *bus = vdev->pdev.bus;
> + struct vfio_pci_hot_reset_info *info = NULL;
> + struct vfio_pci_dependent_device *devices;
> + VFIOGroup *group;
> + struct VFIODeviceFind find;
> + int ret, i;
> +
> + ret = vfio_get_hot_reset_info(vdev, &info);
> + if (ret) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s,"
> + " device does not support hot reset.",
> + vdev->vbasedev.name);
> + return;
> + }
> +
> + /* List all affected devices by bus reset */
> + devices = &info->devices[0];
> +
> + /* Verify that we have all the groups required */
> + for (i = 0; i < info->count; i++) {
> + PCIHostDeviceAddress host;
> + VFIOPCIDevice *tmp;
> + VFIODevice *vbasedev_iter;
> + bool found = false;
> +
> + host.domain = devices[i].segment;
> + host.bus = devices[i].bus;
> + host.slot = PCI_SLOT(devices[i].devfn);
> + host.function = PCI_FUNC(devices[i].devfn);
> +
> + /* Skip the current device */
> + if (vfio_pci_host_match(&host, &vdev->host)) {
> + continue;
> + }
> +
> + /* Ensure we own the group of the affected device */
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + if (group->groupid == devices[i].group_id) {
> + break;
> + }
> + }
> +
> + if (!group) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "depends on group %d which is not owned.",
> + vdev->vbasedev.name, devices[i].group_id);
> + goto out;
> + }
> +
> + /* Ensure affected devices for reset on the same slot */
> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> + continue;
> + }
> + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> + if (vfio_pci_host_match(&host, &tmp->host)) {
> + /*
> + * AER errors may be broadcast to all functions of a multi-
> + * function endpoint. If any of those sibling functions are
> + * also assigned, they need to have AER enabled or else an
> + * error may continue to cause a vm_stop condition. IOW,
> + * AER setup of this function would be pointless.
> + */
> + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) &&
> + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot"
> + " the dependent device %s which does not enable AER.",
> + vdev->vbasedev.name, tmp->vbasedev.name);
> + goto out;
> + }
> +
> + if (tmp->pdev.bus != bus) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "the dependent device %s is not on the same bus",
> + vdev->vbasedev.name, tmp->vbasedev.name);
> + goto out;
> + }
> + found = true;
> + break;
> + }
> + }
> +
> + /* Ensure all affected devices assigned to VM */
> + if (!found) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "the dependent device %04x:%02x:%02x.%x "
> + "is not assigned to VM.",
> + vdev->vbasedev.name, host.domain, host.bus,
> + host.slot, host.function);
> + goto out;
> + }
> + }
> +
> + /*
> + * Check the all pci devices on or below the target bus
> + * have a reset mechanism at least.
> + */
> + find.pdev = NULL;
> + find.found = false;
> + pci_for_each_device(bus, pci_bus_num(bus),
I'm not fully convinced this does what it says that it does. Bus
numbers are under guest control, but using pci_bus_num(bus) here will
cause us to always take the path in pci_find_bus_nr() where it simply
returns bus. Thus we call pci_for_each_device_under_bus() with the bus
originally provided in pci_for_each_device(). But
pci_for_each_device_under_bus() only iterates the devfns immediately on
that bus, despite the name that would imply otherwise. So it seems
like the pci_for_each_device() callback would need to itself call
pci_for_each_device() if it finds a bridge. Am I missing something or
is that correct?
We could also just go on to require that there are no subordinate buses
to this bus, which seems like a reasonable thing to do. Thanks,
Alex
> + vfio_check_device_noreset, &find);
> + if (find.found) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "the affected device %s does not have a reset mechanism.",
> + vdev->vbasedev.name, find.pdev->name);
> + goto out;
> + }
> +
> +out:
> + g_free(info);
> + return;
> +}
> +
> +static void vfio_check_devices_host_bus_reset(Error **errp)
> +{
> + VFIOGroup *group;
> + VFIODevice *vbasedev;
> + VFIOPCIDevice *vdev;
> + Error *local_err = NULL;
> +
> + /* Check All vfio-pci devices if have bus reset capability */
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + QLIST_FOREACH(vbasedev, &group->device_list, next) {
> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> + continue;
> + }
> + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> + if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
> + vfio_check_host_bus_reset(vdev, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + }
> + }
> + }
> +
> + return;
> +}
> +
> static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> int pos, uint16_t size)
> {
> @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
> vfio_intx_enable(vdev);
> }
>
> -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> - PCIHostDeviceAddress *host2)
> -{
> - return (host1->domain == host2->domain && host1->bus == host2->bus &&
> - host1->slot == host2->slot && host1->function == host2->function);
> -}
> -
> static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> {
> VFIOGroup *group;
> @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> vdev->req_enabled = false;
> }
>
> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> +{
> + Error *local_err = NULL;
> +
> + vfio_check_devices_host_bus_reset(&local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + exit(1);
> + }
> +}
> +
> +static Notifier machine_notifier = {
> + .notify = vfio_pci_machine_done_notify,
> +};
> +
> static int vfio_initfn(PCIDevice *pdev)
> {
> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = {
> static void register_vfio_pci_dev_type(void)
> {
> type_register_static(&vfio_pci_dev_info);
> + /*
> + * Register notifier when machine init is done, since we need
> + * check the configration manner after all vfio device are inited.
> + */
> + qemu_add_machine_init_done_notifier(&machine_notifier);
> }
>
> type_init(register_vfio_pci_dev_type)
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index e0c53f2..aff46c2 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -15,6 +15,7 @@
> #include "qemu-common.h"
> #include "exec/memory.h"
> #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> #include "hw/pci/pci_bridge.h"
> #include "hw/vfio/vfio-common.h"
> #include "qemu/event_notifier.h"
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not
2016-03-08 22:55 ` Alex Williamson
@ 2016-03-09 1:26 ` Chen Fan
0 siblings, 0 replies; 31+ messages in thread
From: Chen Fan @ 2016-03-09 1:26 UTC (permalink / raw)
To: Alex Williamson, Cao jin; +Cc: izumi.taku, qemu-devel, mst
On 03/09/2016 06:55 AM, Alex Williamson wrote:
> On Mon, 7 Mar 2016 11:22:58 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> when boot up a VM that assigning vfio devices with aer enabled, we
>> must check the vfio device whether support host bus reset. because
>> when one error occur. OS driver always recover the device by do a
>> bus reset, in order to recover the vfio device, qemu must to do a
>> host bus reset to reset the device to default status. and for all
>> affected devices by the bus reset. we must check them whether all
>> are assigned to the VM.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>> hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> hw/vfio/pci.h | 1 +
>> 2 files changed, 212 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 8ec9b25..0898e34 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>> return 0;
>> }
>>
>> +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1,
>> + PCIHostDeviceAddress *host2)
>> +{
>> + return (host1->domain == host2->domain && host1->bus == host2->bus &&
>> + host1->slot == host2->slot);
>> +}
>> +
>> +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
>> + PCIHostDeviceAddress *host2)
>> +{
>> + return (vfio_pci_host_slot_match(host1, host2) &&
>> + host1->function == host2->function);
>> +}
>> +
>> +struct VFIODeviceFind {
>> + PCIDevice *pdev;
>> + bool found;
>> +};
>> +
>> +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev,
>> + void *opaque)
>> +{
>> + DeviceState *dev = DEVICE(pdev);
>> + DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> + VFIOPCIDevice *vdev;
>> + struct VFIODeviceFind *find = opaque;
>> +
>> + if (find->found) {
>> + return;
>> + }
>> +
>> + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>> + if (!dc->reset) {
>> + goto found;
>> + }
>> + return;
>> + }
>> + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
>> + !vdev->vbasedev.reset_works) {
>> + goto found;
>> + }
>> +
>> + return;
>> +found:
>> + find->pdev = pdev;
>> + find->found = true;
>> +}
>> +
>> +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
>> +{
>> + PCIBus *bus = vdev->pdev.bus;
>> + struct vfio_pci_hot_reset_info *info = NULL;
>> + struct vfio_pci_dependent_device *devices;
>> + VFIOGroup *group;
>> + struct VFIODeviceFind find;
>> + int ret, i;
>> +
>> + ret = vfio_get_hot_reset_info(vdev, &info);
>> + if (ret) {
>> + error_setg(errp, "vfio: Cannot enable AER for device %s,"
>> + " device does not support hot reset.",
>> + vdev->vbasedev.name);
>> + return;
>> + }
>> +
>> + /* List all affected devices by bus reset */
>> + devices = &info->devices[0];
>> +
>> + /* Verify that we have all the groups required */
>> + for (i = 0; i < info->count; i++) {
>> + PCIHostDeviceAddress host;
>> + VFIOPCIDevice *tmp;
>> + VFIODevice *vbasedev_iter;
>> + bool found = false;
>> +
>> + host.domain = devices[i].segment;
>> + host.bus = devices[i].bus;
>> + host.slot = PCI_SLOT(devices[i].devfn);
>> + host.function = PCI_FUNC(devices[i].devfn);
>> +
>> + /* Skip the current device */
>> + if (vfio_pci_host_match(&host, &vdev->host)) {
>> + continue;
>> + }
>> +
>> + /* Ensure we own the group of the affected device */
>> + QLIST_FOREACH(group, &vfio_group_list, next) {
>> + if (group->groupid == devices[i].group_id) {
>> + break;
>> + }
>> + }
>> +
>> + if (!group) {
>> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
>> + "depends on group %d which is not owned.",
>> + vdev->vbasedev.name, devices[i].group_id);
>> + goto out;
>> + }
>> +
>> + /* Ensure affected devices for reset on the same slot */
>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
>> + continue;
>> + }
>> + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
>> + if (vfio_pci_host_match(&host, &tmp->host)) {
>> + /*
>> + * AER errors may be broadcast to all functions of a multi-
>> + * function endpoint. If any of those sibling functions are
>> + * also assigned, they need to have AER enabled or else an
>> + * error may continue to cause a vm_stop condition. IOW,
>> + * AER setup of this function would be pointless.
>> + */
>> + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) &&
>> + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) {
>> + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot"
>> + " the dependent device %s which does not enable AER.",
>> + vdev->vbasedev.name, tmp->vbasedev.name);
>> + goto out;
>> + }
>> +
>> + if (tmp->pdev.bus != bus) {
>> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
>> + "the dependent device %s is not on the same bus",
>> + vdev->vbasedev.name, tmp->vbasedev.name);
>> + goto out;
>> + }
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + /* Ensure all affected devices assigned to VM */
>> + if (!found) {
>> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
>> + "the dependent device %04x:%02x:%02x.%x "
>> + "is not assigned to VM.",
>> + vdev->vbasedev.name, host.domain, host.bus,
>> + host.slot, host.function);
>> + goto out;
>> + }
>> + }
>> +
>> + /*
>> + * Check the all pci devices on or below the target bus
>> + * have a reset mechanism at least.
>> + */
>> + find.pdev = NULL;
>> + find.found = false;
>> + pci_for_each_device(bus, pci_bus_num(bus),
> I'm not fully convinced this does what it says that it does. Bus
> numbers are under guest control, but using pci_bus_num(bus) here will
> cause us to always take the path in pci_find_bus_nr() where it simply
> returns bus. Thus we call pci_for_each_device_under_bus() with the bus
> originally provided in pci_for_each_device(). But
> pci_for_each_device_under_bus() only iterates the devfns immediately on
> that bus, despite the name that would imply otherwise. So it seems
> like the pci_for_each_device() callback would need to itself call
> pci_for_each_device() if it finds a bridge. Am I missing something or
> is that correct?
>
> We could also just go on to require that there are no subordinate buses
> to this bus, which seems like a reasonable thing to do. Thanks,
yes. this should be enough for us.
Thanks,
Chen
>
> Alex
>
>> + vfio_check_device_noreset, &find);
>> + if (find.found) {
>> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
>> + "the affected device %s does not have a reset mechanism.",
>> + vdev->vbasedev.name, find.pdev->name);
>> + goto out;
>> + }
>> +
>> +out:
>> + g_free(info);
>> + return;
>> +}
>> +
>> +static void vfio_check_devices_host_bus_reset(Error **errp)
>> +{
>> + VFIOGroup *group;
>> + VFIODevice *vbasedev;
>> + VFIOPCIDevice *vdev;
>> + Error *local_err = NULL;
>> +
>> + /* Check All vfio-pci devices if have bus reset capability */
>> + QLIST_FOREACH(group, &vfio_group_list, next) {
>> + QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
>> + continue;
>> + }
>> + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> + if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
>> + vfio_check_host_bus_reset(vdev, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> + }
>> + }
>> + }
>> +
>> + return;
>> +}
>> +
>> static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
>> int pos, uint16_t size)
>> {
>> @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>> vfio_intx_enable(vdev);
>> }
>>
>> -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
>> - PCIHostDeviceAddress *host2)
>> -{
>> - return (host1->domain == host2->domain && host1->bus == host2->bus &&
>> - host1->slot == host2->slot && host1->function == host2->function);
>> -}
>> -
>> static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>> {
>> VFIOGroup *group;
>> @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>> vdev->req_enabled = false;
>> }
>>
>> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>> +{
>> + Error *local_err = NULL;
>> +
>> + vfio_check_devices_host_bus_reset(&local_err);
>> + if (local_err) {
>> + error_report_err(local_err);
>> + exit(1);
>> + }
>> +}
>> +
>> +static Notifier machine_notifier = {
>> + .notify = vfio_pci_machine_done_notify,
>> +};
>> +
>> static int vfio_initfn(PCIDevice *pdev)
>> {
>> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = {
>> static void register_vfio_pci_dev_type(void)
>> {
>> type_register_static(&vfio_pci_dev_info);
>> + /*
>> + * Register notifier when machine init is done, since we need
>> + * check the configration manner after all vfio device are inited.
>> + */
>> + qemu_add_machine_init_done_notifier(&machine_notifier);
>> }
>>
>> type_init(register_vfio_pci_dev_type)
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index e0c53f2..aff46c2 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -15,6 +15,7 @@
>> #include "qemu-common.h"
>> #include "exec/memory.h"
>> #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bus.h"
>> #include "hw/pci/pci_bridge.h"
>> #include "hw/vfio/vfio-common.h"
>> #include "qemu/event_notifier.h"
>
>
> .
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not Cao jin
2016-03-08 22:55 ` Alex Williamson
@ 2016-03-09 16:47 ` Michael S. Tsirkin
2016-03-09 16:59 ` Alex Williamson
1 sibling, 1 reply; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 16:47 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, qemu-devel
On Mon, Mar 07, 2016 at 11:22:58AM +0800, Cao jin wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> when boot up a VM that assigning vfio devices with aer enabled, we
> must check the vfio device whether support host bus reset. because
> when one error occur. OS driver always recover the device by do a
> bus reset, in order to recover the vfio device, qemu must to do a
> host bus reset to reset the device to default status. and for all
> affected devices by the bus reset. we must check them whether all
> are assigned to the VM.
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
> hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> hw/vfio/pci.h | 1 +
> 2 files changed, 212 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8ec9b25..0898e34 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> return 0;
> }
>
> +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1,
> + PCIHostDeviceAddress *host2)
> +{
> + return (host1->domain == host2->domain && host1->bus == host2->bus &&
> + host1->slot == host2->slot);
> +}
> +
> +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> + PCIHostDeviceAddress *host2)
> +{
> + return (vfio_pci_host_slot_match(host1, host2) &&
> + host1->function == host2->function);
> +}
> +
> +struct VFIODeviceFind {
> + PCIDevice *pdev;
> + bool found;
> +};
> +
> +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev,
> + void *opaque)
> +{
> + DeviceState *dev = DEVICE(pdev);
> + DeviceClass *dc = DEVICE_GET_CLASS(dev);
> + VFIOPCIDevice *vdev;
> + struct VFIODeviceFind *find = opaque;
> +
> + if (find->found) {
> + return;
> + }
> +
> + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> + if (!dc->reset) {
> + goto found;
> + }
> + return;
> + }
> + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> + !vdev->vbasedev.reset_works) {
> + goto found;
> + }
> +
> + return;
> +found:
> + find->pdev = pdev;
> + find->found = true;
> +}
> +
> +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> +{
> + PCIBus *bus = vdev->pdev.bus;
> + struct vfio_pci_hot_reset_info *info = NULL;
> + struct vfio_pci_dependent_device *devices;
> + VFIOGroup *group;
> + struct VFIODeviceFind find;
> + int ret, i;
> +
> + ret = vfio_get_hot_reset_info(vdev, &info);
> + if (ret) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s,"
> + " device does not support hot reset.",
> + vdev->vbasedev.name);
> + return;
> + }
> +
> + /* List all affected devices by bus reset */
> + devices = &info->devices[0];
> +
> + /* Verify that we have all the groups required */
> + for (i = 0; i < info->count; i++) {
> + PCIHostDeviceAddress host;
> + VFIOPCIDevice *tmp;
> + VFIODevice *vbasedev_iter;
> + bool found = false;
> +
> + host.domain = devices[i].segment;
> + host.bus = devices[i].bus;
> + host.slot = PCI_SLOT(devices[i].devfn);
> + host.function = PCI_FUNC(devices[i].devfn);
> +
> + /* Skip the current device */
> + if (vfio_pci_host_match(&host, &vdev->host)) {
> + continue;
> + }
> +
> + /* Ensure we own the group of the affected device */
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + if (group->groupid == devices[i].group_id) {
> + break;
> + }
> + }
> +
> + if (!group) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "depends on group %d which is not owned.",
> + vdev->vbasedev.name, devices[i].group_id);
> + goto out;
> + }
> +
> + /* Ensure affected devices for reset on the same slot */
> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
So what you did here, first you let user build up
all functions, then when finally function 0 is added,
you go and check them all.
Result is rather messy.
Instead, make is_valid_func access two functions:
existing one and new one.
And this way each new function can validate that
all existing functions are setup correctly,
and each existing function can validate that
the new one does not conflict with it.
> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> + continue;
> + }
> + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> + if (vfio_pci_host_match(&host, &tmp->host)) {
> + /*
> + * AER errors may be broadcast to all functions of a multi-
> + * function endpoint. If any of those sibling functions are
> + * also assigned, they need to have AER enabled or else an
> + * error may continue to cause a vm_stop condition. IOW,
> + * AER setup of this function would be pointless.
> + */
> + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) &&
> + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot"
> + " the dependent device %s which does not enable AER.",
> + vdev->vbasedev.name, tmp->vbasedev.name);
> + goto out;
> + }
> +
> + if (tmp->pdev.bus != bus) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "the dependent device %s is not on the same bus",
> + vdev->vbasedev.name, tmp->vbasedev.name);
> + goto out;
> + }
> + found = true;
> + break;
> + }
> + }
> +
> + /* Ensure all affected devices assigned to VM */
> + if (!found) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "the dependent device %04x:%02x:%02x.%x "
> + "is not assigned to VM.",
> + vdev->vbasedev.name, host.domain, host.bus,
> + host.slot, host.function);
> + goto out;
> + }
> + }
> +
> + /*
> + * Check the all pci devices on or below the target bus
> + * have a reset mechanism at least.
> + */
> + find.pdev = NULL;
> + find.found = false;
> + pci_for_each_device(bus, pci_bus_num(bus),
> + vfio_check_device_noreset, &find);
> + if (find.found) {
> + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> + "the affected device %s does not have a reset mechanism.",
> + vdev->vbasedev.name, find.pdev->name);
> + goto out;
> + }
> +
> +out:
> + g_free(info);
> + return;
> +}
> +
> +static void vfio_check_devices_host_bus_reset(Error **errp)
> +{
> + VFIOGroup *group;
> + VFIODevice *vbasedev;
> + VFIOPCIDevice *vdev;
> + Error *local_err = NULL;
> +
> + /* Check All vfio-pci devices if have bus reset capability */
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + QLIST_FOREACH(vbasedev, &group->device_list, next) {
> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> + continue;
> + }
> + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> + if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
> + vfio_check_host_bus_reset(vdev, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + }
> + }
> + }
> +
> + return;
> +}
> +
> static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> int pos, uint16_t size)
> {
> @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
> vfio_intx_enable(vdev);
> }
>
> -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> - PCIHostDeviceAddress *host2)
> -{
> - return (host1->domain == host2->domain && host1->bus == host2->bus &&
> - host1->slot == host2->slot && host1->function == host2->function);
> -}
> -
> static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> {
> VFIOGroup *group;
> @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> vdev->req_enabled = false;
> }
>
> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> +{
> + Error *local_err = NULL;
> +
> + vfio_check_devices_host_bus_reset(&local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + exit(1);
> + }
> +}
> +
> +static Notifier machine_notifier = {
> + .notify = vfio_pci_machine_done_notify,
> +};
> +
> static int vfio_initfn(PCIDevice *pdev)
> {
> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = {
> static void register_vfio_pci_dev_type(void)
> {
> type_register_static(&vfio_pci_dev_info);
> + /*
> + * Register notifier when machine init is done, since we need
> + * check the configration manner after all vfio device are inited.
> + */
> + qemu_add_machine_init_done_notifier(&machine_notifier);
> }
>
> type_init(register_vfio_pci_dev_type)
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index e0c53f2..aff46c2 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -15,6 +15,7 @@
> #include "qemu-common.h"
> #include "exec/memory.h"
> #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> #include "hw/pci/pci_bridge.h"
> #include "hw/vfio/vfio-common.h"
> #include "qemu/event_notifier.h"
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not
2016-03-09 16:47 ` Michael S. Tsirkin
@ 2016-03-09 16:59 ` Alex Williamson
2016-03-09 17:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 31+ messages in thread
From: Alex Williamson @ 2016-03-09 16:59 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: chen.fan.fnst, izumi.taku, Cao jin, qemu-devel
On Wed, 9 Mar 2016 18:47:35 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 07, 2016 at 11:22:58AM +0800, Cao jin wrote:
> > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >
> > when boot up a VM that assigning vfio devices with aer enabled, we
> > must check the vfio device whether support host bus reset. because
> > when one error occur. OS driver always recover the device by do a
> > bus reset, in order to recover the vfio device, qemu must to do a
> > host bus reset to reset the device to default status. and for all
> > affected devices by the bus reset. we must check them whether all
> > are assigned to the VM.
> >
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> > hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > hw/vfio/pci.h | 1 +
> > 2 files changed, 212 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 8ec9b25..0898e34 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> > return 0;
> > }
> >
> > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1,
> > + PCIHostDeviceAddress *host2)
> > +{
> > + return (host1->domain == host2->domain && host1->bus == host2->bus &&
> > + host1->slot == host2->slot);
> > +}
> > +
> > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> > + PCIHostDeviceAddress *host2)
> > +{
> > + return (vfio_pci_host_slot_match(host1, host2) &&
> > + host1->function == host2->function);
> > +}
> > +
> > +struct VFIODeviceFind {
> > + PCIDevice *pdev;
> > + bool found;
> > +};
> > +
> > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev,
> > + void *opaque)
> > +{
> > + DeviceState *dev = DEVICE(pdev);
> > + DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > + VFIOPCIDevice *vdev;
> > + struct VFIODeviceFind *find = opaque;
> > +
> > + if (find->found) {
> > + return;
> > + }
> > +
> > + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> > + if (!dc->reset) {
> > + goto found;
> > + }
> > + return;
> > + }
> > + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> > + !vdev->vbasedev.reset_works) {
> > + goto found;
> > + }
> > +
> > + return;
> > +found:
> > + find->pdev = pdev;
> > + find->found = true;
> > +}
> > +
> > +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> > +{
> > + PCIBus *bus = vdev->pdev.bus;
> > + struct vfio_pci_hot_reset_info *info = NULL;
> > + struct vfio_pci_dependent_device *devices;
> > + VFIOGroup *group;
> > + struct VFIODeviceFind find;
> > + int ret, i;
> > +
> > + ret = vfio_get_hot_reset_info(vdev, &info);
> > + if (ret) {
> > + error_setg(errp, "vfio: Cannot enable AER for device %s,"
> > + " device does not support hot reset.",
> > + vdev->vbasedev.name);
> > + return;
> > + }
> > +
> > + /* List all affected devices by bus reset */
> > + devices = &info->devices[0];
> > +
> > + /* Verify that we have all the groups required */
> > + for (i = 0; i < info->count; i++) {
> > + PCIHostDeviceAddress host;
> > + VFIOPCIDevice *tmp;
> > + VFIODevice *vbasedev_iter;
> > + bool found = false;
> > +
> > + host.domain = devices[i].segment;
> > + host.bus = devices[i].bus;
> > + host.slot = PCI_SLOT(devices[i].devfn);
> > + host.function = PCI_FUNC(devices[i].devfn);
> > +
> > + /* Skip the current device */
> > + if (vfio_pci_host_match(&host, &vdev->host)) {
> > + continue;
> > + }
> > +
> > + /* Ensure we own the group of the affected device */
> > + QLIST_FOREACH(group, &vfio_group_list, next) {
> > + if (group->groupid == devices[i].group_id) {
> > + break;
> > + }
> > + }
> > +
> > + if (!group) {
> > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > + "depends on group %d which is not owned.",
> > + vdev->vbasedev.name, devices[i].group_id);
> > + goto out;
> > + }
> > +
> > + /* Ensure affected devices for reset on the same slot */
> > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>
>
>
> So what you did here, first you let user build up
> all functions, then when finally function 0 is added,
> you go and check them all.
>
> Result is rather messy.
>
> Instead, make is_valid_func access two functions:
> existing one and new one.
> And this way each new function can validate that
> all existing functions are setup correctly,
> and each existing function can validate that
> the new one does not conflict with it.
Same problem as what you're proposed for 09/11, when bus reset is
involved, there may be no valid configuration of a subset of the
devices. Only when the entire set of devices is added does it become
valid. Thanks,
Alex
> > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> > + continue;
> > + }
> > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> > + if (vfio_pci_host_match(&host, &tmp->host)) {
> > + /*
> > + * AER errors may be broadcast to all functions of a multi-
> > + * function endpoint. If any of those sibling functions are
> > + * also assigned, they need to have AER enabled or else an
> > + * error may continue to cause a vm_stop condition. IOW,
> > + * AER setup of this function would be pointless.
> > + */
> > + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) &&
> > + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) {
> > + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot"
> > + " the dependent device %s which does not enable AER.",
> > + vdev->vbasedev.name, tmp->vbasedev.name);
> > + goto out;
> > + }
> > +
> > + if (tmp->pdev.bus != bus) {
> > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > + "the dependent device %s is not on the same bus",
> > + vdev->vbasedev.name, tmp->vbasedev.name);
> > + goto out;
> > + }
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + /* Ensure all affected devices assigned to VM */
> > + if (!found) {
> > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > + "the dependent device %04x:%02x:%02x.%x "
> > + "is not assigned to VM.",
> > + vdev->vbasedev.name, host.domain, host.bus,
> > + host.slot, host.function);
> > + goto out;
> > + }
> > + }
> > +
> > + /*
> > + * Check the all pci devices on or below the target bus
> > + * have a reset mechanism at least.
> > + */
> > + find.pdev = NULL;
> > + find.found = false;
> > + pci_for_each_device(bus, pci_bus_num(bus),
> > + vfio_check_device_noreset, &find);
> > + if (find.found) {
> > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > + "the affected device %s does not have a reset mechanism.",
> > + vdev->vbasedev.name, find.pdev->name);
> > + goto out;
> > + }
> > +
> > +out:
> > + g_free(info);
> > + return;
> > +}
> > +
> > +static void vfio_check_devices_host_bus_reset(Error **errp)
> > +{
> > + VFIOGroup *group;
> > + VFIODevice *vbasedev;
> > + VFIOPCIDevice *vdev;
> > + Error *local_err = NULL;
> > +
> > + /* Check All vfio-pci devices if have bus reset capability */
> > + QLIST_FOREACH(group, &vfio_group_list, next) {
> > + QLIST_FOREACH(vbasedev, &group->device_list, next) {
> > + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> > + continue;
> > + }
> > + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> > + if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
> > + vfio_check_host_bus_reset(vdev, &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + return;
> > + }
> > + }
> > + }
> > + }
> > +
> > + return;
> > +}
> > +
> > static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> > int pos, uint16_t size)
> > {
> > @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
> > vfio_intx_enable(vdev);
> > }
> >
> > -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> > - PCIHostDeviceAddress *host2)
> > -{
> > - return (host1->domain == host2->domain && host1->bus == host2->bus &&
> > - host1->slot == host2->slot && host1->function == host2->function);
> > -}
> > -
> > static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> > {
> > VFIOGroup *group;
> > @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> > vdev->req_enabled = false;
> > }
> >
> > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> > +{
> > + Error *local_err = NULL;
> > +
> > + vfio_check_devices_host_bus_reset(&local_err);
> > + if (local_err) {
> > + error_report_err(local_err);
> > + exit(1);
> > + }
> > +}
> > +
> > +static Notifier machine_notifier = {
> > + .notify = vfio_pci_machine_done_notify,
> > +};
> > +
> > static int vfio_initfn(PCIDevice *pdev)
> > {
> > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = {
> > static void register_vfio_pci_dev_type(void)
> > {
> > type_register_static(&vfio_pci_dev_info);
> > + /*
> > + * Register notifier when machine init is done, since we need
> > + * check the configration manner after all vfio device are inited.
> > + */
> > + qemu_add_machine_init_done_notifier(&machine_notifier);
> > }
> >
> > type_init(register_vfio_pci_dev_type)
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index e0c53f2..aff46c2 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -15,6 +15,7 @@
> > #include "qemu-common.h"
> > #include "exec/memory.h"
> > #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bus.h"
> > #include "hw/pci/pci_bridge.h"
> > #include "hw/vfio/vfio-common.h"
> > #include "qemu/event_notifier.h"
> > --
> > 1.9.3
> >
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not
2016-03-09 16:59 ` Alex Williamson
@ 2016-03-09 17:21 ` Michael S. Tsirkin
0 siblings, 0 replies; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 17:21 UTC (permalink / raw)
To: Alex Williamson; +Cc: chen.fan.fnst, izumi.taku, Cao jin, qemu-devel
On Wed, Mar 09, 2016 at 09:59:04AM -0700, Alex Williamson wrote:
> On Wed, 9 Mar 2016 18:47:35 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Mar 07, 2016 at 11:22:58AM +0800, Cao jin wrote:
> > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > >
> > > when boot up a VM that assigning vfio devices with aer enabled, we
> > > must check the vfio device whether support host bus reset. because
> > > when one error occur. OS driver always recover the device by do a
> > > bus reset, in order to recover the vfio device, qemu must to do a
> > > host bus reset to reset the device to default status. and for all
> > > affected devices by the bus reset. we must check them whether all
> > > are assigned to the VM.
> > >
> > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > ---
> > > hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > hw/vfio/pci.h | 1 +
> > > 2 files changed, 212 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index 8ec9b25..0898e34 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> > > return 0;
> > > }
> > >
> > > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1,
> > > + PCIHostDeviceAddress *host2)
> > > +{
> > > + return (host1->domain == host2->domain && host1->bus == host2->bus &&
> > > + host1->slot == host2->slot);
> > > +}
> > > +
> > > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> > > + PCIHostDeviceAddress *host2)
> > > +{
> > > + return (vfio_pci_host_slot_match(host1, host2) &&
> > > + host1->function == host2->function);
> > > +}
> > > +
> > > +struct VFIODeviceFind {
> > > + PCIDevice *pdev;
> > > + bool found;
> > > +};
> > > +
> > > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev,
> > > + void *opaque)
> > > +{
> > > + DeviceState *dev = DEVICE(pdev);
> > > + DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > > + VFIOPCIDevice *vdev;
> > > + struct VFIODeviceFind *find = opaque;
> > > +
> > > + if (find->found) {
> > > + return;
> > > + }
> > > +
> > > + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> > > + if (!dc->reset) {
> > > + goto found;
> > > + }
> > > + return;
> > > + }
> > > + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> > > + !vdev->vbasedev.reset_works) {
> > > + goto found;
> > > + }
> > > +
> > > + return;
> > > +found:
> > > + find->pdev = pdev;
> > > + find->found = true;
> > > +}
> > > +
> > > +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> > > +{
> > > + PCIBus *bus = vdev->pdev.bus;
> > > + struct vfio_pci_hot_reset_info *info = NULL;
> > > + struct vfio_pci_dependent_device *devices;
> > > + VFIOGroup *group;
> > > + struct VFIODeviceFind find;
> > > + int ret, i;
> > > +
> > > + ret = vfio_get_hot_reset_info(vdev, &info);
> > > + if (ret) {
> > > + error_setg(errp, "vfio: Cannot enable AER for device %s,"
> > > + " device does not support hot reset.",
> > > + vdev->vbasedev.name);
> > > + return;
> > > + }
> > > +
> > > + /* List all affected devices by bus reset */
> > > + devices = &info->devices[0];
> > > +
> > > + /* Verify that we have all the groups required */
> > > + for (i = 0; i < info->count; i++) {
> > > + PCIHostDeviceAddress host;
> > > + VFIOPCIDevice *tmp;
> > > + VFIODevice *vbasedev_iter;
> > > + bool found = false;
> > > +
> > > + host.domain = devices[i].segment;
> > > + host.bus = devices[i].bus;
> > > + host.slot = PCI_SLOT(devices[i].devfn);
> > > + host.function = PCI_FUNC(devices[i].devfn);
> > > +
> > > + /* Skip the current device */
> > > + if (vfio_pci_host_match(&host, &vdev->host)) {
> > > + continue;
> > > + }
> > > +
> > > + /* Ensure we own the group of the affected device */
> > > + QLIST_FOREACH(group, &vfio_group_list, next) {
> > > + if (group->groupid == devices[i].group_id) {
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!group) {
> > > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > > + "depends on group %d which is not owned.",
> > > + vdev->vbasedev.name, devices[i].group_id);
> > > + goto out;
> > > + }
> > > +
> > > + /* Ensure affected devices for reset on the same slot */
> > > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> >
> >
> >
> > So what you did here, first you let user build up
> > all functions, then when finally function 0 is added,
> > you go and check them all.
> >
> > Result is rather messy.
> >
> > Instead, make is_valid_func access two functions:
> > existing one and new one.
> > And this way each new function can validate that
> > all existing functions are setup correctly,
> > and each existing function can validate that
> > the new one does not conflict with it.
>
> Same problem as what you're proposed for 09/11, when bus reset is
> involved, there may be no valid configuration of a subset of the
> devices. Only when the entire set of devices is added does it become
> valid. Thanks,
>
> Alex
There are two issues:
1. make sure virtual device includes all host device functions
- you can not detect this until you have the full virtual
device, new callback when device is ready makes sense
2. make sure virtual device does not include two functions from different host devices
- you can detect this on realize of function which conflicts,
so you probably should.
checking this in this callback makes no sense.
> > > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> > > + continue;
> > > + }
> > > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> > > + if (vfio_pci_host_match(&host, &tmp->host)) {
> > > + /*
> > > + * AER errors may be broadcast to all functions of a multi-
> > > + * function endpoint. If any of those sibling functions are
> > > + * also assigned, they need to have AER enabled or else an
> > > + * error may continue to cause a vm_stop condition. IOW,
> > > + * AER setup of this function would be pointless.
> > > + */
> > > + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) &&
> > > + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) {
> > > + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot"
> > > + " the dependent device %s which does not enable AER.",
> > > + vdev->vbasedev.name, tmp->vbasedev.name);
> > > + goto out;
> > > + }
> > > +
> > > + if (tmp->pdev.bus != bus) {
> > > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > > + "the dependent device %s is not on the same bus",
> > > + vdev->vbasedev.name, tmp->vbasedev.name);
> > > + goto out;
> > > + }
> > > + found = true;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + /* Ensure all affected devices assigned to VM */
> > > + if (!found) {
> > > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > > + "the dependent device %04x:%02x:%02x.%x "
> > > + "is not assigned to VM.",
> > > + vdev->vbasedev.name, host.domain, host.bus,
> > > + host.slot, host.function);
> > > + goto out;
> > > + }
> > > + }
> > > +
> > > + /*
> > > + * Check the all pci devices on or below the target bus
> > > + * have a reset mechanism at least.
> > > + */
> > > + find.pdev = NULL;
> > > + find.found = false;
> > > + pci_for_each_device(bus, pci_bus_num(bus),
> > > + vfio_check_device_noreset, &find);
> > > + if (find.found) {
> > > + error_setg(errp, "vfio: Cannot enable AER for device %s, "
> > > + "the affected device %s does not have a reset mechanism.",
> > > + vdev->vbasedev.name, find.pdev->name);
> > > + goto out;
> > > + }
> > > +
> > > +out:
> > > + g_free(info);
> > > + return;
> > > +}
> > > +
> > > +static void vfio_check_devices_host_bus_reset(Error **errp)
> > > +{
> > > + VFIOGroup *group;
> > > + VFIODevice *vbasedev;
> > > + VFIOPCIDevice *vdev;
> > > + Error *local_err = NULL;
> > > +
> > > + /* Check All vfio-pci devices if have bus reset capability */
> > > + QLIST_FOREACH(group, &vfio_group_list, next) {
> > > + QLIST_FOREACH(vbasedev, &group->device_list, next) {
> > > + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> > > + continue;
> > > + }
> > > + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> > > + if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
> > > + vfio_check_host_bus_reset(vdev, &local_err);
> > > + if (local_err) {
> > > + error_propagate(errp, local_err);
> > > + return;
> > > + }
> > > + }
> > > + }
> > > + }
> > > +
> > > + return;
> > > +}
> > > +
> > > static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> > > int pos, uint16_t size)
> > > {
> > > @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
> > > vfio_intx_enable(vdev);
> > > }
> > >
> > > -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> > > - PCIHostDeviceAddress *host2)
> > > -{
> > > - return (host1->domain == host2->domain && host1->bus == host2->bus &&
> > > - host1->slot == host2->slot && host1->function == host2->function);
> > > -}
> > > -
> > > static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> > > {
> > > VFIOGroup *group;
> > > @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> > > vdev->req_enabled = false;
> > > }
> > >
> > > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> > > +{
> > > + Error *local_err = NULL;
> > > +
> > > + vfio_check_devices_host_bus_reset(&local_err);
> > > + if (local_err) {
> > > + error_report_err(local_err);
> > > + exit(1);
> > > + }
> > > +}
> > > +
> > > +static Notifier machine_notifier = {
> > > + .notify = vfio_pci_machine_done_notify,
> > > +};
> > > +
> > > static int vfio_initfn(PCIDevice *pdev)
> > > {
> > > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = {
> > > static void register_vfio_pci_dev_type(void)
> > > {
> > > type_register_static(&vfio_pci_dev_info);
> > > + /*
> > > + * Register notifier when machine init is done, since we need
> > > + * check the configration manner after all vfio device are inited.
> > > + */
> > > + qemu_add_machine_init_done_notifier(&machine_notifier);
> > > }
> > >
> > > type_init(register_vfio_pci_dev_type)
> > > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > > index e0c53f2..aff46c2 100644
> > > --- a/hw/vfio/pci.h
> > > +++ b/hw/vfio/pci.h
> > > @@ -15,6 +15,7 @@
> > > #include "qemu-common.h"
> > > #include "exec/memory.h"
> > > #include "hw/pci/pci.h"
> > > +#include "hw/pci/pci_bus.h"
> > > #include "hw/pci/pci_bridge.h"
> > > #include "hw/vfio/vfio-common.h"
> > > #include "qemu/event_notifier.h"
> > > --
> > > 1.9.3
> > >
> > >
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (4 preceding siblings ...)
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 05/11] vfio: add check host bus reset is support or not Cao jin
@ 2016-03-07 3:22 ` Cao jin
2016-03-09 16:22 ` Michael S. Tsirkin
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 07/11] vfio: add check aer functionality for hotplug device Cao jin
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:22 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++
include/hw/pci/pci.h | 1 +
2 files changed, 40 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d940f79..72650c5 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1836,6 +1836,31 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
return bus->devices[devfn];
}
+static void pci_bus_check_device(PCIDevice *pdev, Error **errp)
+{
+ PCIBus *bus = pdev->bus;
+ PCIDeviceClass *pc;
+ int i;
+ Error *local_err = NULL;
+
+ for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
+ if (!bus->devices[i]) {
+ continue;
+ }
+
+ pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
+ if (!pc->is_valid_func) {
+ continue;
+ }
+
+ pc->is_valid_func(bus->devices[i], &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ }
+}
+
static void pci_qdev_realize(DeviceState *qdev, Error **errp)
{
PCIDevice *pci_dev = (PCIDevice *)qdev;
@@ -1878,6 +1903,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
pci_qdev_unrealize(DEVICE(pci_dev), NULL);
return;
}
+
+ /*
+ * If the function is func 0, indicate the closure of the slot.
+ * then we get the chance to check all functions on same device
+ * if valid.
+ */
+ if (pci_get_function_0(pci_dev) == pci_dev) {
+ pci_bus_check_device(pci_dev, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ pci_qdev_unrealize(DEVICE(pci_dev), NULL);
+ return;
+ }
+ }
}
static void pci_default_realize(PCIDevice *dev, Error **errp)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index dedf277..4e56256 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
void (*realize)(PCIDevice *dev, Error **errp);
int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
+ void (*is_valid_func)(PCIDevice *dev, Error **errp);
PCIUnregisterFunc *exit;
PCIConfigReadFunc *config_read;
PCIConfigWriteFunc *config_write;
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete Cao jin
@ 2016-03-09 16:22 ` Michael S. Tsirkin
2016-03-09 16:50 ` Alex Williamson
0 siblings, 1 reply; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 16:22 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, qemu-devel
On Mon, Mar 07, 2016 at 11:22:59AM +0800, Cao jin wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
So if you create a mess, you discover it when
you later add function 0.
Why not call this when function is added?
O(N^2) on # of functions, but that # is up to 256 so maybe
that is not too bad.
> ---
> hw/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/hw/pci/pci.h | 1 +
> 2 files changed, 40 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index d940f79..72650c5 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1836,6 +1836,31 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
> return bus->devices[devfn];
> }
>
> +static void pci_bus_check_device(PCIDevice *pdev, Error **errp)
> +{
> + PCIBus *bus = pdev->bus;
> + PCIDeviceClass *pc;
> + int i;
> + Error *local_err = NULL;
> +
> + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> + if (!bus->devices[i]) {
> + continue;
> + }
> +
> + pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
> + if (!pc->is_valid_func) {
> + continue;
> + }
> +
> + pc->is_valid_func(bus->devices[i], &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + }
> +}
> +
> static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> {
> PCIDevice *pci_dev = (PCIDevice *)qdev;
> @@ -1878,6 +1903,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> return;
> }
> +
> + /*
> + * If the function is func 0, indicate the closure of the slot.
> + * then we get the chance to check all functions on same device
> + * if valid.
> + */
> + if (pci_get_function_0(pci_dev) == pci_dev) {
> + pci_bus_check_device(pci_dev, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> + return;
> + }
> + }
> }
>
> static void pci_default_realize(PCIDevice *dev, Error **errp)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index dedf277..4e56256 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
>
> void (*realize)(PCIDevice *dev, Error **errp);
> int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> + void (*is_valid_func)(PCIDevice *dev, Error **errp);
> PCIUnregisterFunc *exit;
> PCIConfigReadFunc *config_read;
> PCIConfigWriteFunc *config_write;
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete
2016-03-09 16:22 ` Michael S. Tsirkin
@ 2016-03-09 16:50 ` Alex Williamson
2016-03-09 17:14 ` Michael S. Tsirkin
0 siblings, 1 reply; 31+ messages in thread
From: Alex Williamson @ 2016-03-09 16:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: chen.fan.fnst, izumi.taku, Cao jin, qemu-devel
On Wed, 9 Mar 2016 18:22:24 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 07, 2016 at 11:22:59AM +0800, Cao jin wrote:
> > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> So if you create a mess, you discover it when
> you later add function 0.
> Why not call this when function is added?
> O(N^2) on # of functions, but that # is up to 256 so maybe
> that is not too bad.
Because the configuration isn't valid until the slot is closed. Take a
dual port NIC example again, after we add the first function, the
configuration is invalid because we have an AER indicated device that
can't do a bus reset because the second function, which may be in a
separate IOMMU group, hasn't been added yet. Therefore we can only
check the configuration when the slot is complete. Thanks,
Alex
> > ---
> > hw/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++
> > include/hw/pci/pci.h | 1 +
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index d940f79..72650c5 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1836,6 +1836,31 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
> > return bus->devices[devfn];
> > }
> >
> > +static void pci_bus_check_device(PCIDevice *pdev, Error **errp)
> > +{
> > + PCIBus *bus = pdev->bus;
> > + PCIDeviceClass *pc;
> > + int i;
> > + Error *local_err = NULL;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> > + if (!bus->devices[i]) {
> > + continue;
> > + }
> > +
> > + pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
> > + if (!pc->is_valid_func) {
> > + continue;
> > + }
> > +
> > + pc->is_valid_func(bus->devices[i], &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + return;
> > + }
> > + }
> > +}
> > +
> > static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > {
> > PCIDevice *pci_dev = (PCIDevice *)qdev;
> > @@ -1878,6 +1903,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> > return;
> > }
> > +
> > + /*
> > + * If the function is func 0, indicate the closure of the slot.
> > + * then we get the chance to check all functions on same device
> > + * if valid.
> > + */
> > + if (pci_get_function_0(pci_dev) == pci_dev) {
> > + pci_bus_check_device(pci_dev, &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> > + return;
> > + }
> > + }
> > }
> >
> > static void pci_default_realize(PCIDevice *dev, Error **errp)
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index dedf277..4e56256 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
> >
> > void (*realize)(PCIDevice *dev, Error **errp);
> > int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> > + void (*is_valid_func)(PCIDevice *dev, Error **errp);
> > PCIUnregisterFunc *exit;
> > PCIConfigReadFunc *config_read;
> > PCIConfigWriteFunc *config_write;
> > --
> > 1.9.3
> >
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete
2016-03-09 16:50 ` Alex Williamson
@ 2016-03-09 17:14 ` Michael S. Tsirkin
2016-03-10 2:00 ` Chen Fan
0 siblings, 1 reply; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 17:14 UTC (permalink / raw)
To: Alex Williamson; +Cc: chen.fan.fnst, izumi.taku, Cao jin, qemu-devel
On Wed, Mar 09, 2016 at 09:50:31AM -0700, Alex Williamson wrote:
> On Wed, 9 Mar 2016 18:22:24 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Mar 07, 2016 at 11:22:59AM +0800, Cao jin wrote:
> > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > >
> > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >
> > So if you create a mess, you discover it when
> > you later add function 0.
> > Why not call this when function is added?
> > O(N^2) on # of functions, but that # is up to 256 so maybe
> > that is not too bad.
>
> Because the configuration isn't valid until the slot is closed. Take a
> dual port NIC example again, after we add the first function, the
> configuration is invalid because we have an AER indicated device that
> can't do a bus reset because the second function, which may be in a
> separate IOMMU group, hasn't been added yet. Therefore we can only
> check the configuration when the slot is complete. Thanks,
>
> Alex
I see. The name mislead me. So what you want to do is validate a
multi-function device, not the bus.
> > > ---
> > > hw/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++
> > > include/hw/pci/pci.h | 1 +
> > > 2 files changed, 40 insertions(+)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index d940f79..72650c5 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -1836,6 +1836,31 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
> > > return bus->devices[devfn];
> > > }
> > >
> > > +static void pci_bus_check_device(PCIDevice *pdev, Error **errp)
Is not it true that what you are really after is validating
functions of the given device?
Pls rename this pci_check_valid_functions or something
like this, and change it to only scan functions of the device,
not all devices on the bus.
> > > +{
> > > + PCIBus *bus = pdev->bus;
> > > + PCIDeviceClass *pc;
> > > + int i;
> > > + Error *local_err = NULL;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> > > + if (!bus->devices[i]) {
> > > + continue;
> > > + }
> > > +
> > > + pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
> > > + if (!pc->is_valid_func) {
> > > + continue;
> > > + }
> > > +
> > > + pc->is_valid_func(bus->devices[i], &local_err);
> > > + if (local_err) {
> > > + error_propagate(errp, local_err);
> > > + return;
> > > + }
> > > + }
> > > +}
> > > +
> > > static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > > {
> > > PCIDevice *pci_dev = (PCIDevice *)qdev;
> > > @@ -1878,6 +1903,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > > pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> > > return;
> > > }
> > > +
> > > + /*
> > > + * If the function is func 0, indicate the closure of the slot.
> > > + * then we get the chance to check all functions on same device
> > > + * if valid.
> > > + */
> > > + if (pci_get_function_0(pci_dev) == pci_dev) {
> > > + pci_bus_check_device(pci_dev, &local_err);
> > > + if (local_err) {
> > > + error_propagate(errp, local_err);
> > > + pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> > > + return;
> > > + }
> > > + }
> > > }
> > >
> > > static void pci_default_realize(PCIDevice *dev, Error **errp)
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index dedf277..4e56256 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
> > >
> > > void (*realize)(PCIDevice *dev, Error **errp);
> > > int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> > > + void (*is_valid_func)(PCIDevice *dev, Error **errp);
> > > PCIUnregisterFunc *exit;
> > > PCIConfigReadFunc *config_read;
> > > PCIConfigWriteFunc *config_write;
> > > --
> > > 1.9.3
> > >
> > >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete
2016-03-09 17:14 ` Michael S. Tsirkin
@ 2016-03-10 2:00 ` Chen Fan
0 siblings, 0 replies; 31+ messages in thread
From: Chen Fan @ 2016-03-10 2:00 UTC (permalink / raw)
To: Michael S. Tsirkin, Alex Williamson; +Cc: izumi.taku, Cao jin, qemu-devel
On 03/10/2016 01:14 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 09, 2016 at 09:50:31AM -0700, Alex Williamson wrote:
>> On Wed, 9 Mar 2016 18:22:24 +0200
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Mon, Mar 07, 2016 at 11:22:59AM +0800, Cao jin wrote:
>>>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>> So if you create a mess, you discover it when
>>> you later add function 0.
>>> Why not call this when function is added?
>>> O(N^2) on # of functions, but that # is up to 256 so maybe
>>> that is not too bad.
>> Because the configuration isn't valid until the slot is closed. Take a
>> dual port NIC example again, after we add the first function, the
>> configuration is invalid because we have an AER indicated device that
>> can't do a bus reset because the second function, which may be in a
>> separate IOMMU group, hasn't been added yet. Therefore we can only
>> check the configuration when the slot is complete. Thanks,
>>
>> Alex
> I see. The name mislead me. So what you want to do is validate a
> multi-function device, not the bus.
>
>>>> ---
>>>> hw/pci/pci.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>> include/hw/pci/pci.h | 1 +
>>>> 2 files changed, 40 insertions(+)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index d940f79..72650c5 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -1836,6 +1836,31 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
>>>> return bus->devices[devfn];
>>>> }
>>>>
>>>> +static void pci_bus_check_device(PCIDevice *pdev, Error **errp)
> Is not it true that what you are really after is validating
> functions of the given device?
> Pls rename this pci_check_valid_functions or something
> like this, and change it to only scan functions of the device,
> not all devices on the bus.
thanks, will do that.
Chen
>
>
>>>> +{
>>>> + PCIBus *bus = pdev->bus;
>>>> + PCIDeviceClass *pc;
>>>> + int i;
>>>> + Error *local_err = NULL;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
>>>> + if (!bus->devices[i]) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
>>>> + if (!pc->is_valid_func) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + pc->is_valid_func(bus->devices[i], &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return;
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>> {
>>>> PCIDevice *pci_dev = (PCIDevice *)qdev;
>>>> @@ -1878,6 +1903,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>> pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>>>> return;
>>>> }
>>>> +
>>>> + /*
>>>> + * If the function is func 0, indicate the closure of the slot.
>>>> + * then we get the chance to check all functions on same device
>>>> + * if valid.
>>>> + */
>>>> + if (pci_get_function_0(pci_dev) == pci_dev) {
>>>> + pci_bus_check_device(pci_dev, &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>>>> + return;
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> static void pci_default_realize(PCIDevice *dev, Error **errp)
>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>> index dedf277..4e56256 100644
>>>> --- a/include/hw/pci/pci.h
>>>> +++ b/include/hw/pci/pci.h
>>>> @@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
>>>>
>>>> void (*realize)(PCIDevice *dev, Error **errp);
>>>> int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
>>>> + void (*is_valid_func)(PCIDevice *dev, Error **errp);
>>>> PCIUnregisterFunc *exit;
>>>> PCIConfigReadFunc *config_read;
>>>> PCIConfigWriteFunc *config_write;
>>>> --
>>>> 1.9.3
>>>>
>>>>
>
> .
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 07/11] vfio: add check aer functionality for hotplug device
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (5 preceding siblings ...)
2016-03-07 3:22 ` [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete Cao jin
@ 2016-03-07 3:23 ` Cao jin
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 08/11] pci: introduce pci bus pre reset Cao jin
` (3 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:23 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 0898e34..24848c9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3034,6 +3034,20 @@ post_reset:
vfio_pci_post_reset(vdev);
}
+static void vfio_pci_is_valid(PCIDevice *dev, Error **errp)
+{
+ VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, dev);
+ Error *local_err = NULL;
+
+ if (DEVICE(dev)->hotplugged &&
+ (vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+ vfio_check_host_bus_reset(vdev, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ }
+ }
+}
+
static void vfio_instance_init(Object *obj)
{
PCIDevice *pci_dev = PCI_DEVICE(obj);
@@ -3087,6 +3101,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
pdc->init = vfio_initfn;
pdc->exit = vfio_exitfn;
+ pdc->is_valid_func = vfio_pci_is_valid;
pdc->config_read = vfio_pci_read_config;
pdc->config_write = vfio_pci_write_config;
pdc->is_express = 1; /* We might be */
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 08/11] pci: introduce pci bus pre reset
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (6 preceding siblings ...)
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 07/11] vfio: add check aer functionality for hotplug device Cao jin
@ 2016-03-07 3:23 ` Cao jin
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset Cao jin
` (2 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:23 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
in order to distinguish a hot reset with a normal reset. we add this
pre reset call back to notice that we should do a hot reset for all devices.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/core/qdev.c | 4 ++--
hw/pci/pci.c | 11 +++++++++++
hw/pci/pci_bridge.c | 5 ++++-
include/hw/pci/pci_bus.h | 5 +++++
include/hw/qdev-core.h | 3 +++
5 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 779de2b..e36fa07 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -304,14 +304,14 @@ void qdev_unplug(DeviceState *dev, Error **errp)
}
}
-static int qdev_reset_one(DeviceState *dev, void *opaque)
+int qdev_reset_one(DeviceState *dev, void *opaque)
{
device_reset(dev);
return 0;
}
-static int qbus_reset_one(BusState *bus, void *opaque)
+int qbus_reset_one(BusState *bus, void *opaque)
{
BusClass *bc = BUS_GET_CLASS(bus);
if (bc->reset) {
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 72650c5..7c79767 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -257,6 +257,15 @@ void pci_device_reset(PCIDevice *dev)
pci_do_device_reset(dev);
}
+int pcibus_pre_reset(BusState *qbus, void *opaque)
+{
+ PCIBus *pci_bus = DO_UPCAST(PCIBus, qbus, qbus);
+
+ pci_bus->in_hot_reset = true;
+
+ return 0;
+}
+
/*
* Trigger pci bus reset under a given bus.
* Called via qbus_reset_all on RST# assert, after the devices
@@ -276,6 +285,8 @@ static void pcibus_reset(BusState *qbus)
for (i = 0; i < bus->nirq; i++) {
assert(bus->irq_count[i] == 0);
}
+
+ bus->in_hot_reset = false;
}
static void pci_host_bus_register(PCIBus *bus, DeviceState *parent)
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 7eab9d5..6c35171 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -269,7 +269,10 @@ void pci_bridge_write_config(PCIDevice *d,
newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
/* Trigger hot reset on 0->1 transition. */
- qbus_reset_all(&s->sec_bus.qbus);
+ qbus_walk_children(&s->sec_bus.qbus, NULL,
+ pcibus_pre_reset,
+ qdev_reset_one,
+ qbus_reset_one, &s->sec_bus.qbus);
}
}
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 403fec6..1a87179 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -39,8 +39,13 @@ struct PCIBus {
Keep a count of the number of devices with raised IRQs. */
int nirq;
int *irq_count;
+
+ /* The bus need to do a hot reset */
+ bool in_hot_reset;
};
+int pcibus_pre_reset(BusState *qbus, void *opaque);
+
typedef struct PCIBridgeWindows PCIBridgeWindows;
/*
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index abcdee8..3d71c17 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -401,4 +401,7 @@ static inline bool qbus_is_hotpluggable(BusState *bus)
void device_listener_register(DeviceListener *listener);
void device_listener_unregister(DeviceListener *listener);
+int qdev_reset_one(DeviceState *dev, void *opaque);
+int qbus_reset_one(BusState *bus, void *opaque);
+
#endif
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (7 preceding siblings ...)
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 08/11] pci: introduce pci bus pre reset Cao jin
@ 2016-03-07 3:23 ` Cao jin
2016-03-09 10:07 ` Michael S. Tsirkin
` (2 more replies)
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 10/11] vfio-pci: pass the aer error to guest Cao jin
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 11/11] vfio: add 'aer' property to expose aercap Cao jin
10 siblings, 3 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:23 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/vfio/pci.h | 1 +
2 files changed, 58 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 24848c9..8e902d2 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
/* List all affected devices by bus reset */
devices = &info->devices[0];
+ vdev->single_depend_dev = (info->count == 1);
+
/* Verify that we have all the groups required */
for (i = 0; i < info->count; i++) {
PCIHostDeviceAddress host;
@@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
vfio_unregister_bars(vdev);
}
+static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
+{
+ struct vfio_pci_hot_reset_info *info = NULL;
+ struct vfio_pci_dependent_device *devices;
+ VFIOGroup *group;
+ VFIODevice *vbasedev_iter;
+ int ret;
+
+ ret = vfio_get_hot_reset_info(vdev, &info);
+ if (ret) {
+ error_report("vfio: Cannot enable AER for device %s,"
+ " device does not support hot reset.",
+ vdev->vbasedev.name);
+ return NULL;
+ }
+
+ devices = &info->devices[0];
+
+ QLIST_FOREACH(group, &vfio_group_list, next) {
+ if (group->groupid == devices[0].group_id) {
+ break;
+ }
+ }
+
+ if (!group) {
+ error_report("vfio: Cannot enable AER for device %s, "
+ "depends on group %d which is not owned.",
+ vdev->vbasedev.name, devices[0].group_id);
+ return NULL;
+ }
+
+ QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
+ if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
+ !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+ continue;
+ }
+
+ return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
+ }
+
+ return NULL;
+}
+
static void vfio_pci_reset(DeviceState *dev)
{
PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
@@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
trace_vfio_pci_reset(vdev->vbasedev.name);
+ if (pdev->bus->in_hot_reset) {
+ VFIOPCIDevice *tmp;
+
+ tmp = vfio_pci_get_do_reset_device(vdev);
+ if (tmp) {
+ if (tmp == vdev) {
+ vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
+ }
+ return;
+ }
+ }
+
vfio_pci_pre_reset(vdev);
if (vdev->resetfn && !vdev->resetfn(vdev)) {
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index aff46c2..32bd31f 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
bool no_kvm_intx;
bool no_kvm_msi;
bool no_kvm_msix;
+ bool single_depend_dev;
} VFIOPCIDevice;
uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset Cao jin
@ 2016-03-09 10:07 ` Michael S. Tsirkin
2016-03-09 16:37 ` Alex Williamson
2016-03-09 16:39 ` Michael S. Tsirkin
2 siblings, 0 replies; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 10:07 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, qemu-devel
On Mon, Mar 07, 2016 at 11:23:02AM +0800, Cao jin wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
> hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/vfio/pci.h | 1 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 24848c9..8e902d2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> /* List all affected devices by bus reset */
> devices = &info->devices[0];
>
> + vdev->single_depend_dev = (info->count == 1);
> +
> /* Verify that we have all the groups required */
> for (i = 0; i < info->count; i++) {
> PCIHostDeviceAddress host;
> @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> vfio_unregister_bars(vdev);
> }
>
> +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> +{
> + struct vfio_pci_hot_reset_info *info = NULL;
> + struct vfio_pci_dependent_device *devices;
> + VFIOGroup *group;
> + VFIODevice *vbasedev_iter;
> + int ret;
> +
> + ret = vfio_get_hot_reset_info(vdev, &info);
> + if (ret) {
> + error_report("vfio: Cannot enable AER for device %s,"
> + " device does not support hot reset.",
> + vdev->vbasedev.name);
> + return NULL;
> + }
> +
> + devices = &info->devices[0];
> +
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + if (group->groupid == devices[0].group_id) {
> + break;
> + }
> + }
> +
> + if (!group) {
> + error_report("vfio: Cannot enable AER for device %s, "
> + "depends on group %d which is not owned.",
> + vdev->vbasedev.name, devices[0].group_id);
> + return NULL;
> + }
> +
> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> + !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> + continue;
> + }
> +
> + return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> + }
> +
> + return NULL;
> +}
> +
> static void vfio_pci_reset(DeviceState *dev)
> {
> PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
>
> trace_vfio_pci_reset(vdev->vbasedev.name);
>
> + if (pdev->bus->in_hot_reset) {
So the full system (fundamental) reset is a subset of the hot reset?
This seems to make no sense. I expect that the fundamental reset
is stronger. What did I miss?
> + VFIOPCIDevice *tmp;
> +
> + tmp = vfio_pci_get_do_reset_device(vdev);
> + if (tmp) {
> + if (tmp == vdev) {
> + vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> + }
> + return;
> + }
> + }
> +
> vfio_pci_pre_reset(vdev);
>
> if (vdev->resetfn && !vdev->resetfn(vdev)) {
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index aff46c2..32bd31f 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> bool no_kvm_intx;
> bool no_kvm_msi;
> bool no_kvm_msix;
> + bool single_depend_dev;
> } VFIOPCIDevice;
>
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset Cao jin
2016-03-09 10:07 ` Michael S. Tsirkin
@ 2016-03-09 16:37 ` Alex Williamson
2016-03-10 6:15 ` Chen Fan
2016-03-09 16:39 ` Michael S. Tsirkin
2 siblings, 1 reply; 31+ messages in thread
From: Alex Williamson @ 2016-03-09 16:37 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, qemu-devel, mst
On Mon, 7 Mar 2016 11:23:02 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
> hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/vfio/pci.h | 1 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 24848c9..8e902d2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> /* List all affected devices by bus reset */
> devices = &info->devices[0];
>
> + vdev->single_depend_dev = (info->count == 1);
> +
> /* Verify that we have all the groups required */
> for (i = 0; i < info->count; i++) {
> PCIHostDeviceAddress host;
> @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> vfio_unregister_bars(vdev);
> }
>
> +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> +{
> + struct vfio_pci_hot_reset_info *info = NULL;
> + struct vfio_pci_dependent_device *devices;
> + VFIOGroup *group;
> + VFIODevice *vbasedev_iter;
> + int ret;
> +
> + ret = vfio_get_hot_reset_info(vdev, &info);
> + if (ret) {
> + error_report("vfio: Cannot enable AER for device %s,"
> + " device does not support hot reset.",
> + vdev->vbasedev.name);
> + return NULL;
Isn't this path called for all devices, not just those trying to
support AER?
> + }
> +
> + devices = &info->devices[0];
> +
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + if (group->groupid == devices[0].group_id) {
> + break;
> + }
> + }
> +
> + if (!group) {
> + error_report("vfio: Cannot enable AER for device %s, "
> + "depends on group %d which is not owned.",
> + vdev->vbasedev.name, devices[0].group_id);
> + return NULL;
Same here, we're not in an AER specific code path and we haven't even
tested if the device is trying to support AER.
> + }
> +
> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> + !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> + continue;
> + }
> +
> + return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
So the reset device is simply the first device in the group list, this
needs to be documented, but DMA isolation (grouping) and bus isolation
are separate things and we can have more than one group per bus...
often an IOMMU group is a single device, so have we really solved
anything here? If the next device is on the same bus but in a separate
group, we're going to hot reset again, right? Thanks,
Alex
> + }
> +
> + return NULL;
> +}
> +
> static void vfio_pci_reset(DeviceState *dev)
> {
> PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
>
> trace_vfio_pci_reset(vdev->vbasedev.name);
>
> + if (pdev->bus->in_hot_reset) {
> + VFIOPCIDevice *tmp;
> +
> + tmp = vfio_pci_get_do_reset_device(vdev);
> + if (tmp) {
> + if (tmp == vdev) {
> + vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> + }
> + return;
> + }
> + }
> +
> vfio_pci_pre_reset(vdev);
>
> if (vdev->resetfn && !vdev->resetfn(vdev)) {
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index aff46c2..32bd31f 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> bool no_kvm_intx;
> bool no_kvm_msi;
> bool no_kvm_msix;
> + bool single_depend_dev;
> } VFIOPCIDevice;
>
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-09 16:37 ` Alex Williamson
@ 2016-03-10 6:15 ` Chen Fan
2016-03-10 14:16 ` Michael S. Tsirkin
0 siblings, 1 reply; 31+ messages in thread
From: Chen Fan @ 2016-03-10 6:15 UTC (permalink / raw)
To: Alex Williamson, Cao jin; +Cc: izumi.taku, qemu-devel, mst
On 03/10/2016 12:37 AM, Alex Williamson wrote:
> On Mon, 7 Mar 2016 11:23:02 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>> hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/vfio/pci.h | 1 +
>> 2 files changed, 58 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 24848c9..8e902d2 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
>> /* List all affected devices by bus reset */
>> devices = &info->devices[0];
>>
>> + vdev->single_depend_dev = (info->count == 1);
>> +
>> /* Verify that we have all the groups required */
>> for (i = 0; i < info->count; i++) {
>> PCIHostDeviceAddress host;
>> @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
>> vfio_unregister_bars(vdev);
>> }
>>
>> +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
>> +{
>> + struct vfio_pci_hot_reset_info *info = NULL;
>> + struct vfio_pci_dependent_device *devices;
>> + VFIOGroup *group;
>> + VFIODevice *vbasedev_iter;
>> + int ret;
>> +
>> + ret = vfio_get_hot_reset_info(vdev, &info);
>> + if (ret) {
>> + error_report("vfio: Cannot enable AER for device %s,"
>> + " device does not support hot reset.",
>> + vdev->vbasedev.name);
>> + return NULL;
> Isn't this path called for all devices, not just those trying to
> support AER?
>
>> + }
>> +
>> + devices = &info->devices[0];
>> +
>> + QLIST_FOREACH(group, &vfio_group_list, next) {
>> + if (group->groupid == devices[0].group_id) {
>> + break;
>> + }
>> + }
>> +
>> + if (!group) {
>> + error_report("vfio: Cannot enable AER for device %s, "
>> + "depends on group %d which is not owned.",
>> + vdev->vbasedev.name, devices[0].group_id);
>> + return NULL;
> Same here, we're not in an AER specific code path and we haven't even
> tested if the device is trying to support AER.
>
>> + }
>> +
>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
>> + !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>> + continue;
>> + }
>> +
>> + return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> So the reset device is simply the first device in the group list, this
> needs to be documented, but DMA isolation (grouping) and bus isolation
> are separate things and we can have more than one group per bus...
> often an IOMMU group is a single device, so have we really solved
> anything here? If the next device is on the same bus but in a separate
> group, we're going to hot reset again, right? Thanks,
You are right, I recall my original idea was not something what like
this. :)
I tend to favor MST's ideas which said in V1 series patchset:
"If the assumption that vfio functions are combined
in the same way as on the host, then this is not needed:
just reset when function 0 is reset."
so, seems we could implement this in next version.
1. requiring functions on same host bus must were assigned on the same
virtual
bus. here we should not have a subordinate bus. because nowadays bridge
do not support pass through. so we can simply reset the vfio device
with the
smallest devfn in a bus reset as long as one device has aer on the bus.
with this, we don't need to care the groups isolation, because we
only need the devices
by a bus hot reset, if the devices all on one virtual bus. reset
anyone is sufficient.
2. in order to identify the reset is from a normal reset or aer recovery
reset, we could
add a flags in vfio device to mark the aer have occurred in
err_notifier_handle, if
subsequent reset is coming with the is_bus_rst in parent bus is
true. we can
directly do the hot reset.
Thanks,
Chen
>
> Alex
>
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static void vfio_pci_reset(DeviceState *dev)
>> {
>> PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
>> @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
>>
>> trace_vfio_pci_reset(vdev->vbasedev.name);
>>
>> + if (pdev->bus->in_hot_reset) {
>> + VFIOPCIDevice *tmp;
>> +
>> + tmp = vfio_pci_get_do_reset_device(vdev);
>> + if (tmp) {
>> + if (tmp == vdev) {
>> + vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
>> + }
>> + return;
>> + }
>> + }
>> +
>> vfio_pci_pre_reset(vdev);
>>
>> if (vdev->resetfn && !vdev->resetfn(vdev)) {
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index aff46c2..32bd31f 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
>> bool no_kvm_intx;
>> bool no_kvm_msi;
>> bool no_kvm_msix;
>> + bool single_depend_dev;
>> } VFIOPCIDevice;
>>
>> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
>
>
> .
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-10 6:15 ` Chen Fan
@ 2016-03-10 14:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-10 14:16 UTC (permalink / raw)
To: Chen Fan; +Cc: izumi.taku, Alex Williamson, Cao jin, qemu-devel
On Thu, Mar 10, 2016 at 02:15:53PM +0800, Chen Fan wrote:
>
> On 03/10/2016 12:37 AM, Alex Williamson wrote:
> >On Mon, 7 Mar 2016 11:23:02 +0800
> >Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> >
> >>From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >>
> >>Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >>---
> >> hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> hw/vfio/pci.h | 1 +
> >> 2 files changed, 58 insertions(+)
> >>
> >>diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >>index 24848c9..8e902d2 100644
> >>--- a/hw/vfio/pci.c
> >>+++ b/hw/vfio/pci.c
> >>@@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> >> /* List all affected devices by bus reset */
> >> devices = &info->devices[0];
> >>+ vdev->single_depend_dev = (info->count == 1);
> >>+
> >> /* Verify that we have all the groups required */
> >> for (i = 0; i < info->count; i++) {
> >> PCIHostDeviceAddress host;
> >>@@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> >> vfio_unregister_bars(vdev);
> >> }
> >>+static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> >>+{
> >>+ struct vfio_pci_hot_reset_info *info = NULL;
> >>+ struct vfio_pci_dependent_device *devices;
> >>+ VFIOGroup *group;
> >>+ VFIODevice *vbasedev_iter;
> >>+ int ret;
> >>+
> >>+ ret = vfio_get_hot_reset_info(vdev, &info);
> >>+ if (ret) {
> >>+ error_report("vfio: Cannot enable AER for device %s,"
> >>+ " device does not support hot reset.",
> >>+ vdev->vbasedev.name);
> >>+ return NULL;
> >Isn't this path called for all devices, not just those trying to
> >support AER?
> >
> >>+ }
> >>+
> >>+ devices = &info->devices[0];
> >>+
> >>+ QLIST_FOREACH(group, &vfio_group_list, next) {
> >>+ if (group->groupid == devices[0].group_id) {
> >>+ break;
> >>+ }
> >>+ }
> >>+
> >>+ if (!group) {
> >>+ error_report("vfio: Cannot enable AER for device %s, "
> >>+ "depends on group %d which is not owned.",
> >>+ vdev->vbasedev.name, devices[0].group_id);
> >>+ return NULL;
> >Same here, we're not in an AER specific code path and we haven't even
> >tested if the device is trying to support AER.
> >
> >>+ }
> >>+
> >>+ QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> >>+ if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> >>+ !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> >>+ continue;
> >>+ }
> >>+
> >>+ return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> >So the reset device is simply the first device in the group list, this
> >needs to be documented, but DMA isolation (grouping) and bus isolation
> >are separate things and we can have more than one group per bus...
> >often an IOMMU group is a single device, so have we really solved
> >anything here? If the next device is on the same bus but in a separate
> >group, we're going to hot reset again, right? Thanks,
> You are right, I recall my original idea was not something what like this.
> :)
> I tend to favor MST's ideas which said in V1 series patchset:
> "If the assumption that vfio functions are combined
> in the same way as on the host, then this is not needed:
> just reset when function 0 is reset."
If you do this, you can do your checks on realize, without
need for a separate validate callback.
>
> so, seems we could implement this in next version.
> 1. requiring functions on same host bus must were assigned on the same
> virtual
> bus. here we should not have a subordinate bus. because nowadays bridge
> do not support pass through. so we can simply reset the vfio device with
> the
> smallest devfn in a bus reset as long as one device has aer on the bus.
>
> with this, we don't need to care the groups isolation, because we only
> need the devices
> by a bus hot reset, if the devices all on one virtual bus. reset anyone
> is sufficient.
>
> 2. in order to identify the reset is from a normal reset or aer recovery
> reset, we could
> add a flags in vfio device to mark the aer have occurred in
> err_notifier_handle, if
> subsequent reset is coming with the is_bus_rst in parent bus is true. we
> can
> directly do the hot reset.
>
> Thanks,
> Chen
>
> >
> >Alex
> >
> >>+ }
> >>+
> >>+ return NULL;
> >>+}
> >>+
> >> static void vfio_pci_reset(DeviceState *dev)
> >> {
> >> PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> >>@@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
> >> trace_vfio_pci_reset(vdev->vbasedev.name);
> >>+ if (pdev->bus->in_hot_reset) {
> >>+ VFIOPCIDevice *tmp;
> >>+
> >>+ tmp = vfio_pci_get_do_reset_device(vdev);
> >>+ if (tmp) {
> >>+ if (tmp == vdev) {
> >>+ vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> >>+ }
> >>+ return;
> >>+ }
> >>+ }
> >>+
> >> vfio_pci_pre_reset(vdev);
> >> if (vdev->resetfn && !vdev->resetfn(vdev)) {
> >>diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> >>index aff46c2..32bd31f 100644
> >>--- a/hw/vfio/pci.h
> >>+++ b/hw/vfio/pci.h
> >>@@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> >> bool no_kvm_intx;
> >> bool no_kvm_msi;
> >> bool no_kvm_msix;
> >>+ bool single_depend_dev;
> >> } VFIOPCIDevice;
> >> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> >
> >
> >.
> >
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset Cao jin
2016-03-09 10:07 ` Michael S. Tsirkin
2016-03-09 16:37 ` Alex Williamson
@ 2016-03-09 16:39 ` Michael S. Tsirkin
2016-03-09 17:09 ` Alex Williamson
2 siblings, 1 reply; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 16:39 UTC (permalink / raw)
To: Cao jin; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, qemu-devel
On Mon, Mar 07, 2016 at 11:23:02AM +0800, Cao jin wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
> hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/vfio/pci.h | 1 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 24848c9..8e902d2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> /* List all affected devices by bus reset */
> devices = &info->devices[0];
>
> + vdev->single_depend_dev = (info->count == 1);
> +
> /* Verify that we have all the groups required */
> for (i = 0; i < info->count; i++) {
> PCIHostDeviceAddress host;
> @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> vfio_unregister_bars(vdev);
> }
>
> +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> +{
> + struct vfio_pci_hot_reset_info *info = NULL;
> + struct vfio_pci_dependent_device *devices;
> + VFIOGroup *group;
> + VFIODevice *vbasedev_iter;
> + int ret;
> +
> + ret = vfio_get_hot_reset_info(vdev, &info);
> + if (ret) {
> + error_report("vfio: Cannot enable AER for device %s,"
> + " device does not support hot reset.",
> + vdev->vbasedev.name);
> + return NULL;
> + }
> +
> + devices = &info->devices[0];
> +
> + QLIST_FOREACH(group, &vfio_group_list, next) {
> + if (group->groupid == devices[0].group_id) {
> + break;
> + }
> + }
> +
> + if (!group) {
> + error_report("vfio: Cannot enable AER for device %s, "
> + "depends on group %d which is not owned.",
> + vdev->vbasedev.name, devices[0].group_id);
> + return NULL;
> + }
> +
> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> + !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> + continue;
> + }
> +
> + return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> + }
> +
> + return NULL;
> +}
> +
> static void vfio_pci_reset(DeviceState *dev)
> {
> PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
>
> trace_vfio_pci_reset(vdev->vbasedev.name);
>
> + if (pdev->bus->in_hot_reset) {
After discussing this with Alex, I think what you
really are looking for is
if (dev->realized)
in order to skip hot reset when device is plugged-in
initially.
> + VFIOPCIDevice *tmp;
> +
> + tmp = vfio_pci_get_do_reset_device(vdev);
> + if (tmp) {
> + if (tmp == vdev) {
> + vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> + }
> + return;
> + }
> + }
> +
> vfio_pci_pre_reset(vdev);
>
> if (vdev->resetfn && !vdev->resetfn(vdev)) {
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index aff46c2..32bd31f 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> bool no_kvm_intx;
> bool no_kvm_msi;
> bool no_kvm_msix;
> + bool single_depend_dev;
> } VFIOPCIDevice;
>
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-09 16:39 ` Michael S. Tsirkin
@ 2016-03-09 17:09 ` Alex Williamson
2016-03-09 17:31 ` Michael S. Tsirkin
0 siblings, 1 reply; 31+ messages in thread
From: Alex Williamson @ 2016-03-09 17:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: chen.fan.fnst, izumi.taku, Cao jin, qemu-devel
On Wed, 9 Mar 2016 18:39:51 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 07, 2016 at 11:23:02AM +0800, Cao jin wrote:
> > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> > hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/vfio/pci.h | 1 +
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 24848c9..8e902d2 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> > /* List all affected devices by bus reset */
> > devices = &info->devices[0];
> >
> > + vdev->single_depend_dev = (info->count == 1);
> > +
> > /* Verify that we have all the groups required */
> > for (i = 0; i < info->count; i++) {
> > PCIHostDeviceAddress host;
> > @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> > vfio_unregister_bars(vdev);
> > }
> >
> > +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> > +{
> > + struct vfio_pci_hot_reset_info *info = NULL;
> > + struct vfio_pci_dependent_device *devices;
> > + VFIOGroup *group;
> > + VFIODevice *vbasedev_iter;
> > + int ret;
> > +
> > + ret = vfio_get_hot_reset_info(vdev, &info);
> > + if (ret) {
> > + error_report("vfio: Cannot enable AER for device %s,"
> > + " device does not support hot reset.",
> > + vdev->vbasedev.name);
> > + return NULL;
> > + }
> > +
> > + devices = &info->devices[0];
> > +
> > + QLIST_FOREACH(group, &vfio_group_list, next) {
> > + if (group->groupid == devices[0].group_id) {
> > + break;
> > + }
> > + }
> > +
> > + if (!group) {
> > + error_report("vfio: Cannot enable AER for device %s, "
> > + "depends on group %d which is not owned.",
> > + vdev->vbasedev.name, devices[0].group_id);
> > + return NULL;
> > + }
> > +
> > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> > + !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> > + continue;
> > + }
> > +
> > + return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > static void vfio_pci_reset(DeviceState *dev)
> > {
> > PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> > @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
> >
> > trace_vfio_pci_reset(vdev->vbasedev.name);
> >
> > + if (pdev->bus->in_hot_reset) {
>
> After discussing this with Alex, I think what you
> really are looking for is
> if (dev->realized)
> in order to skip hot reset when device is plugged-in
> initially.
Seems fragile, device_reset() also gets called via pci_device_reset()
which would happen if we hooked up FLR to the device. That's clearly
intended to be a function local reset, as is I believe the dc->reset
itself, so assuming behavior based on states like dev->realized is
likely to break. Either we need some external hints, like what's added
here, or we need a separate callback for a bus reset. Thanks,
Alex
> > + VFIOPCIDevice *tmp;
> > +
> > + tmp = vfio_pci_get_do_reset_device(vdev);
> > + if (tmp) {
> > + if (tmp == vdev) {
> > + vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> > + }
> > + return;
> > + }
> > + }
> > +
> > vfio_pci_pre_reset(vdev);
> >
> > if (vdev->resetfn && !vdev->resetfn(vdev)) {
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index aff46c2..32bd31f 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> > bool no_kvm_intx;
> > bool no_kvm_msi;
> > bool no_kvm_msix;
> > + bool single_depend_dev;
> > } VFIOPCIDevice;
> >
> > uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> > --
> > 1.9.3
> >
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset
2016-03-09 17:09 ` Alex Williamson
@ 2016-03-09 17:31 ` Michael S. Tsirkin
0 siblings, 0 replies; 31+ messages in thread
From: Michael S. Tsirkin @ 2016-03-09 17:31 UTC (permalink / raw)
To: Alex Williamson; +Cc: chen.fan.fnst, izumi.taku, Cao jin, qemu-devel
On Wed, Mar 09, 2016 at 10:09:11AM -0700, Alex Williamson wrote:
> On Wed, 9 Mar 2016 18:39:51 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Mar 07, 2016 at 11:23:02AM +0800, Cao jin wrote:
> > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > >
> > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > ---
> > > hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > hw/vfio/pci.h | 1 +
> > > 2 files changed, 58 insertions(+)
> > >
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index 24848c9..8e902d2 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp)
> > > /* List all affected devices by bus reset */
> > > devices = &info->devices[0];
> > >
> > > + vdev->single_depend_dev = (info->count == 1);
> > > +
> > > /* Verify that we have all the groups required */
> > > for (i = 0; i < info->count; i++) {
> > > PCIHostDeviceAddress host;
> > > @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> > > vfio_unregister_bars(vdev);
> > > }
> > >
> > > +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> > > +{
> > > + struct vfio_pci_hot_reset_info *info = NULL;
> > > + struct vfio_pci_dependent_device *devices;
> > > + VFIOGroup *group;
> > > + VFIODevice *vbasedev_iter;
> > > + int ret;
> > > +
> > > + ret = vfio_get_hot_reset_info(vdev, &info);
> > > + if (ret) {
> > > + error_report("vfio: Cannot enable AER for device %s,"
> > > + " device does not support hot reset.",
> > > + vdev->vbasedev.name);
> > > + return NULL;
> > > + }
> > > +
> > > + devices = &info->devices[0];
> > > +
> > > + QLIST_FOREACH(group, &vfio_group_list, next) {
> > > + if (group->groupid == devices[0].group_id) {
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!group) {
> > > + error_report("vfio: Cannot enable AER for device %s, "
> > > + "depends on group %d which is not owned.",
> > > + vdev->vbasedev.name, devices[0].group_id);
> > > + return NULL;
> > > + }
> > > +
> > > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> > > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> > > + !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> > > + continue;
> > > + }
> > > +
> > > + return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > static void vfio_pci_reset(DeviceState *dev)
> > > {
> > > PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> > > @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
> > >
> > > trace_vfio_pci_reset(vdev->vbasedev.name);
> > >
> > > + if (pdev->bus->in_hot_reset) {
> >
> > After discussing this with Alex, I think what you
> > really are looking for is
> > if (dev->realized)
> > in order to skip hot reset when device is plugged-in
> > initially.
>
> Seems fragile, device_reset() also gets called via pci_device_reset()
> which would happen if we hooked up FLR to the device.
You can check that as well (through config space, no need
for new flags).
There's also PM reset.
> That's clearly
> intended to be a function local reset, as is I believe the dc->reset
> itself, so assuming behavior based on states like dev->realized is
> likely to break. Either we need some external hints, like what's added
> here, or we need a separate callback for a bus reset. Thanks,
>
> Alex
So I'm fine with a new flag is_bus_rst, or an API pci_is_bus_rst.
But it has to be set whenever bus is reset, *not* just on secondary bus reset.
> > > + VFIOPCIDevice *tmp;
> > > +
> > > + tmp = vfio_pci_get_do_reset_device(vdev);
> > > + if (tmp) {
> > > + if (tmp == vdev) {
> > > + vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> > > + }
> > > + return;
> > > + }
> > > + }
> > > +
> > > vfio_pci_pre_reset(vdev);
> > >
> > > if (vdev->resetfn && !vdev->resetfn(vdev)) {
> > > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > > index aff46c2..32bd31f 100644
> > > --- a/hw/vfio/pci.h
> > > +++ b/hw/vfio/pci.h
> > > @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> > > bool no_kvm_intx;
> > > bool no_kvm_msi;
> > > bool no_kvm_msix;
> > > + bool single_depend_dev;
> > > } VFIOPCIDevice;
> > >
> > > uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> > > --
> > > 1.9.3
> > >
> > >
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 10/11] vfio-pci: pass the aer error to guest
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (8 preceding siblings ...)
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset Cao jin
@ 2016-03-07 3:23 ` Cao jin
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 11/11] vfio: add 'aer' property to expose aercap Cao jin
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:23 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
when the vfio device encounters an uncorrectable error in host,
the vfio_pci driver will signal the eventfd registered by this
vfio device, resulting in the qemu eventfd handler getting
invoked.
this patch is to pass the error to guest and let the guest driver
recover from the error.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8e902d2..0ac5a70 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2552,18 +2552,63 @@ static void vfio_put_device(VFIOPCIDevice *vdev)
static void vfio_err_notifier_handler(void *opaque)
{
VFIOPCIDevice *vdev = opaque;
+ PCIDevice *dev = &vdev->pdev;
+ Error *local_err = NULL;
+ PCIEAERMsg msg = {
+ .severity = 0,
+ .source_id = (pci_bus_num(dev->bus) << 8) | dev->devfn,
+ };
if (!event_notifier_test_and_clear(&vdev->err_notifier)) {
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.
+ * in case the real hardware configuration has been changed,
+ * here we should recheck the bus reset capability.
+ */
+ if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
+ vfio_check_host_bus_reset(vdev, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ }
+ goto stop;
+ }
+ /*
+ * 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) &&
+ dev->exp.aer_cap) {
+ uint8_t *aer_cap = dev->config + dev->exp.aer_cap;
+ uint32_t uncor_status;
+ bool isfatal;
+
+ uncor_status = vfio_pci_read_config(dev,
+ dev->exp.aer_cap + PCI_ERR_UNCOR_STATUS, 4);
+
+ /*
+ * if the error is not emitted by this device, we can
+ * just ignore it.
+ */
+ if (!(uncor_status & ~0UL)) {
+ return;
+ }
+
+ 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;
+ }
+
+stop:
+ /*
+ * 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
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 11/11] vfio: add 'aer' property to expose aercap
2016-03-07 3:22 [Qemu-devel] [PATCH v2 Resend 00/11] vfio-pci: pass the aer error to guest, part2 Cao jin
` (9 preceding siblings ...)
2016-03-07 3:23 ` [Qemu-devel] [PATCH v2 10/11] vfio-pci: pass the aer error to guest Cao jin
@ 2016-03-07 3:23 ` Cao jin
10 siblings, 0 replies; 31+ messages in thread
From: Cao jin @ 2016-03-07 3:23 UTC (permalink / raw)
To: qemu-devel; +Cc: chen.fan.fnst, izumi.taku, alex.williamson, mst
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
add 'aer' property to let user able to decide whether expose
the aer capability. by default we should disable aer feature,
because it needs configuration restrictions.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
hw/vfio/pci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 0ac5a70..87903af 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3178,6 +3178,8 @@ static Property vfio_pci_dev_properties[] = {
sub_vendor_id, PCI_ANY_ID),
DEFINE_PROP_UINT32("x-pci-sub-device-id", VFIOPCIDevice,
sub_device_id, PCI_ANY_ID),
+ DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
+ VFIO_FEATURE_ENABLE_AER_BIT, false),
/*
* TODO - support passed fds... is this necessary?
* DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread