Re: [Qemu-devel] [patch v3 5/9] vfio: add check host bus reset is support or not

[Alex Williamson]

On Tue, 15 Mar 2016 09:35:45 +0800
Cao jin  wrote:

> From: Chen Fan 
> 
> 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 able to do
> a host bus reset to recover 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 and on the same virtual bus. meanwhile, for
> simply done, the devices which don't affected by the host bus reset
> are not allowed to assign to the same virtual bus.
> 
> Signed-off-by: Chen Fan 
> ---
>  hw/vfio/pci.c | 219 
> --
>  hw/vfio/pci.h |   1 +
>  2 files changed, 213 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a77cc04..9670271 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);
> +}

This needs to be rebased to current qemu.git and account for:

7df9381 vfio: Add sysfsdev property for pci & platform

It's possible that vdev.host is not populated if the device is
specified via sysfsdev.  vbasedev.name is now the common data element
and I think for the purposes of a bus reset it's ok to expect it to
take the form of "%04x:%02x:%02x.%d".  The current vfio_pci_host_match
function builds a comparison string and does a strcmp.  You may want to
change that to build a PCIHostDeviceAddress to make masking out the
function number easier for a slot comparison.

If we have devices that don't match the "%04x:%02x:%02x.%d" format, the
expectation is that they won't support a hot reset ioctl.

> +
> +static void vfio_check_hot_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;
> +int ret, i, devfn;
> +
> +ret = vfio_get_hot_reset_info(vdev, );
> +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 = >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)) {
> +continue;
> +}
> +
> +/* Ensure we own the group of the affected device */
> +QLIST_FOREACH(group, _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 bus */
> +QLIST_FOREACH(vbasedev_iter, >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)) {
> +/*
> + * 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 

[Qemu-devel] [patch v3 5/9] vfio: add check host bus reset is support or not

[Cao jin]

From: Chen Fan 

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 able to do
a host bus reset to recover 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 and on the same virtual bus. meanwhile, for
simply done, the devices which don't affected by the host bus reset
are not allowed to assign to the same virtual bus.

Signed-off-by: Chen Fan 
---
 hw/vfio/pci.c | 219 --
 hw/vfio/pci.h |   1 +
 2 files changed, 213 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a77cc04..9670271 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);
+}
+
+static void vfio_check_hot_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;
+int ret, i, devfn;
+
+ret = vfio_get_hot_reset_info(vdev, );
+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 = >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)) {
+continue;
+}
+
+/* Ensure we own the group of the affected device */
+QLIST_FOREACH(group, _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 bus */
+QLIST_FOREACH(vbasedev_iter, >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)) {
+/*
+ * 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(>host, >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