Re: [PATCH v3 31/37] vfio/pci: Adapt vfio pci hot reset support with iommufd BE

["Cédric Le Goater" <clg@redhat.com>]

On 10/26/23 12:30, Zhenzhong Duan wrote:
> As pci hot reset path need to reference pci specific functions
> and data structures, adding container level callback functions
> for legacy and iommufd BE and referencing those pci specific
> func/data is no better than implementing reset support with
> iommufd BE directly in pci.c

yes but it includes a large section of IOMMUFD code in pci.c
which is ugly. Please make this an VFIOIOMMUOps handler instead.

Thanks,

C.

> 
> This way we can also share the common bus reset and system reset
> path for both BEs.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/vfio/pci.c        | 156 ++++++++++++++++++++++++++++++++++++++++++-
>   hw/vfio/trace-events |   1 +
>   2 files changed, 156 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c17e1f4376..d7a41c8def 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -42,6 +42,7 @@
>   #include "qapi/error.h"
>   #include "migration/blocker.h"
>   #include "migration/qemu-file.h"
> +#include "linux/iommufd.h"
>   
>   #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
>   
> @@ -2497,7 +2498,7 @@ static int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
>       return 0;
>   }
>   
> -static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> +static int vfio_pci_hot_reset_legacy(VFIOPCIDevice *vdev, bool single)
>   {
>       VFIOGroup *group;
>       struct vfio_pci_hot_reset_info *info = NULL;
> @@ -2661,6 +2662,159 @@ out_single:
>       return ret;
>   }
>   
> +#ifdef CONFIG_IOMMUFD
> +static VFIODevice *vfio_pci_find_by_iommufd_devid(__u32 devid)
> +{
> +    VFIODevice *vbasedev_iter;
> +
> +    QLIST_FOREACH(vbasedev_iter, &vfio_device_list, global_next) {
> +        if (vbasedev_iter->bcontainer->ops != &vfio_iommufd_ops) {
> +            continue;
> +        }
> +        if (devid == vbasedev_iter->devid) {
> +            return vbasedev_iter;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int vfio_pci_hot_reset_iommufd(VFIOPCIDevice *vdev, bool single)
> +{
> +    struct vfio_pci_hot_reset_info *info = NULL;
> +    struct vfio_pci_dependent_device *devices;
> +    struct vfio_pci_hot_reset *reset;
> +    int ret, i;
> +    bool multi = false;
> +
> +    trace_vfio_pci_hot_reset(vdev->vbasedev.name, single ? "one" : "multi");
> +
> +    if (!single) {
> +        vfio_pci_pre_reset(vdev);
> +    }
> +    vdev->vbasedev.needs_reset = false;
> +
> +    ret = vfio_pci_get_pci_hot_reset_info(vdev, &info);
> +
> +    if (ret) {
> +        goto out_single;
> +    }
> +
> +    assert(info->flags & VFIO_PCI_HOT_RESET_FLAG_DEV_ID);
> +
> +    devices = &info->devices[0];
> +
> +    if (!(info->flags & VFIO_PCI_HOT_RESET_FLAG_DEV_ID_OWNED)) {
> +        if (!vdev->has_pm_reset) {
> +            for (i = 0; i < info->count; i++) {
> +                if (devices[i].devid == VFIO_PCI_DEVID_NOT_OWNED) {
> +                    error_report("vfio: Cannot reset device %s, "
> +                                 "depends on device %04x:%02x:%02x.%x "
> +                                 "which is not owned.",
> +                                 vdev->vbasedev.name, devices[i].segment,
> +                                 devices[i].bus, PCI_SLOT(devices[i].devfn),
> +                                 PCI_FUNC(devices[i].devfn));
> +                }
> +            }
> +        }
> +        ret = -EPERM;
> +        goto out_single;
> +    }
> +
> +    trace_vfio_pci_hot_reset_has_dep_devices(vdev->vbasedev.name);
> +
> +    for (i = 0; i < info->count; i++) {
> +        VFIOPCIDevice *tmp;
> +        VFIODevice *vbasedev_iter;
> +
> +        trace_vfio_pci_hot_reset_dep_devices_iommufd(devices[i].segment,
> +                                             devices[i].bus,
> +                                             PCI_SLOT(devices[i].devfn),
> +                                             PCI_FUNC(devices[i].devfn),
> +                                             devices[i].devid);
> +
> +        /*
> +         * If a VFIO cdev device is resettable, all the dependent devices
> +         * are either bound to same iommufd or within same iommu_groups as
> +         * one of the iommufd bound devices.
> +         */
> +        assert(devices[i].devid != VFIO_PCI_DEVID_NOT_OWNED);
> +
> +        if (devices[i].devid == vdev->vbasedev.devid ||
> +            devices[i].devid == VFIO_PCI_DEVID_OWNED) {
> +            continue;
> +        }
> +
> +        vbasedev_iter = vfio_pci_find_by_iommufd_devid(devices[i].devid);
> +        if (!vbasedev_iter || !vbasedev_iter->dev->realized ||
> +            vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> +            continue;
> +        }
> +        tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> +        if (single) {
> +            ret = -EINVAL;
> +            goto out_single;
> +        }
> +        vfio_pci_pre_reset(tmp);
> +        tmp->vbasedev.needs_reset = false;
> +        multi = true;
> +    }
> +
> +    if (!single && !multi) {
> +        ret = -EINVAL;
> +        goto out_single;
> +    }
> +
> +    /* Use zero length array for hot reset with iommufd backend */
> +    reset = g_malloc0(sizeof(*reset));
> +    reset->argsz = sizeof(*reset);
> +
> +     /* Bus reset! */
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
> +    g_free(reset);
> +
> +    trace_vfio_pci_hot_reset_result(vdev->vbasedev.name,
> +                                    ret ? strerror(errno) : "Success");
> +
> +    /* Re-enable INTx on affected devices */
> +    for (i = 0; i < info->count; i++) {
> +        VFIOPCIDevice *tmp;
> +        VFIODevice *vbasedev_iter;
> +
> +        if (devices[i].devid == vdev->vbasedev.devid ||
> +            devices[i].devid == VFIO_PCI_DEVID_OWNED) {
> +            continue;
> +        }
> +
> +        vbasedev_iter = vfio_pci_find_by_iommufd_devid(devices[i].devid);
> +        if (!vbasedev_iter || !vbasedev_iter->dev->realized ||
> +            vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> +            continue;
> +        }
> +        tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> +        vfio_pci_post_reset(tmp);
> +    }
> +out_single:
> +    if (!single) {
> +        vfio_pci_post_reset(vdev);
> +    }
> +    g_free(info);
> +
> +    return ret;
> +}
> +#endif
> +
> +static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> +{
> +#ifdef CONFIG_IOMMUFD
> +    if (vdev->vbasedev.iommufd) {
> +        return vfio_pci_hot_reset_iommufd(vdev, single);
> +    } else
> +#endif
> +    {
> +        return vfio_pci_hot_reset_legacy(vdev, single);
> +    }
> +}> +
>   /*
>    * We want to differentiate hot reset of multiple in-use devices vs hot reset
>    * of a single in-use device.  VFIO_DEVICE_RESET will already handle the case
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 9b180cf77c..71c5840636 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -34,6 +34,7 @@ vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"
>   vfio_pci_hot_reset(const char *name, const char *type) " (%s) %s"
>   vfio_pci_hot_reset_has_dep_devices(const char *name) "%s: hot reset dependent devices:"
>   vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int group_id) "\t%04x:%02x:%02x.%x group %d"
> +vfio_pci_hot_reset_dep_devices_iommufd(int domain, int bus, int slot, int function, int dev_id) "\t%04x:%02x:%02x.%x devid %d"
>   vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s"
>   vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n  size: 0x%lx, offset: 0x%lx, flags: 0x%lx"
>   vfio_populate_device_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s"