Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset

[Alex Williamson <alex.williamson@redhat.com>]

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