On Sat, 1 Apr 2023 07:44:25 -0700
Yi Liu <yi.l....@intel.com> wrote:
> No functional change is intended.
>
> Reviewed-by: Kevin Tian <kevin.t...@intel.com>
> Reviewed-by: Jason Gunthorpe <j...@nvidia.com>
> Tested-by: Yanting Jiang <yanting.ji...@intel.com>
> Signed-off-by: Yi Liu <yi.l....@intel.com>
> ---
> drivers/vfio/pci/vfio_pci_core.c | 52 ++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c
> b/drivers/vfio/pci/vfio_pci_core.c
> index 2a510b71edcb..da6325008872 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -177,10 +177,10 @@ static void vfio_pci_probe_mmaps(struct
> vfio_pci_core_device *vdev)
> }
> }
>
> -struct vfio_pci_group_info;
> +struct vfio_pci_file_info;
> static void vfio_pci_dev_set_try_reset(struct vfio_device_set *dev_set);
> static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
> - struct vfio_pci_group_info *groups,
> + struct vfio_pci_file_info *info,
> struct iommufd_ctx *iommufd_ctx);
>
> /*
> @@ -800,7 +800,7 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void
> *data)
> return 0;
> }
>
> -struct vfio_pci_group_info {
> +struct vfio_pci_file_info {
> int count;
> struct file **files;
> };
> @@ -1257,14 +1257,14 @@ static int vfio_pci_ioctl_get_pci_hot_reset_info(
> }
>
> static int
> -vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev,
> - struct vfio_pci_hot_reset *hdr,
> - bool slot,
> - struct vfio_pci_hot_reset __user *arg)
> +vfio_pci_ioctl_pci_hot_reset_files(struct vfio_pci_core_device *vdev,
> + struct vfio_pci_hot_reset *hdr,
> + bool slot,
> + struct vfio_pci_hot_reset __user *arg)
> {
> - int32_t *group_fds;
> + int32_t *fds;
> struct file **files;
> - struct vfio_pci_group_info info;
> + struct vfio_pci_file_info info;
> int file_idx, count = 0, ret = 0;
>
> /*
> @@ -1281,17 +1281,17 @@ vfio_pci_ioctl_pci_hot_reset_groups(struct
> vfio_pci_core_device *vdev,
> if (hdr->count > count)
> return -EINVAL;
>
> - group_fds = kcalloc(hdr->count, sizeof(*group_fds), GFP_KERNEL);
> + fds = kcalloc(hdr->count, sizeof(*fds), GFP_KERNEL);
> files = kcalloc(hdr->count, sizeof(*files), GFP_KERNEL);
> - if (!group_fds || !files) {
> - kfree(group_fds);
> + if (!fds || !files) {
> + kfree(fds);
> kfree(files);
> return -ENOMEM;
> }
>
> - if (copy_from_user(group_fds, arg->group_fds,
> - hdr->count * sizeof(*group_fds))) {
> - kfree(group_fds);
> + if (copy_from_user(fds, arg->group_fds,
> + hdr->count * sizeof(*fds))) {
> + kfree(fds);
> kfree(files);
> return -EFAULT;
> }
> @@ -1301,7 +1301,7 @@ vfio_pci_ioctl_pci_hot_reset_groups(struct
> vfio_pci_core_device *vdev,
> * the reset
> */
> for (file_idx = 0; file_idx < hdr->count; file_idx++) {
> - struct file *file = fget(group_fds[file_idx]);
> + struct file *file = fget(fds[file_idx]);
>
> if (!file) {
> ret = -EBADF;
> @@ -1318,9 +1318,9 @@ vfio_pci_ioctl_pci_hot_reset_groups(struct
> vfio_pci_core_device *vdev,
> files[file_idx] = file;
> }
>
> - kfree(group_fds);
> + kfree(fds);
>
> - /* release reference to groups on error */
> + /* release reference to fds on error */
> if (ret)
> goto hot_reset_release;
>
> @@ -1358,7 +1358,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct
> vfio_pci_core_device *vdev,
> return -ENODEV;
>
> if (hdr.count)
> - return vfio_pci_ioctl_pci_hot_reset_groups(vdev, &hdr, slot,
> arg);
> + return vfio_pci_ioctl_pci_hot_reset_files(vdev, &hdr, slot,
> arg);
>
> iommufd = vfio_iommufd_physical_ictx(&vdev->vdev);
>
> @@ -2329,16 +2329,16 @@ const struct pci_error_handlers
> vfio_pci_core_err_handlers = {
> };
> EXPORT_SYMBOL_GPL(vfio_pci_core_err_handlers);
>
> -static bool vfio_dev_in_groups(struct vfio_pci_core_device *vdev,
> - struct vfio_pci_group_info *groups)
> +static bool vfio_dev_in_files(struct vfio_pci_core_device *vdev,
> + struct vfio_pci_file_info *info)
> {
> unsigned int i;
>
> - if (!groups)
> + if (!info)
> return false;
>
> - for (i = 0; i < groups->count; i++)
> - if (vfio_file_has_dev(groups->files[i], &vdev->vdev))
> + for (i = 0; i < info->count; i++)
> + if (vfio_file_has_dev(info->files[i], &vdev->vdev))
> return true;
> return false;
> }
> @@ -2429,7 +2429,7 @@ static bool vfio_dev_in_iommufd_ctx(struct
> vfio_pci_core_device *vdev,
> * get each memory_lock.
> */
> static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
> - struct vfio_pci_group_info *groups,
> + struct vfio_pci_file_info *info,
> struct iommufd_ctx *iommufd_ctx)
> {
> struct vfio_pci_core_device *cur_mem;
> @@ -2478,7 +2478,7 @@ static int vfio_pci_dev_set_hot_reset(struct
> vfio_device_set *dev_set,
> * the calling device is in a singleton dev_set.
> */
> if (cur_vma->vdev.open_count &&
> - !vfio_dev_in_groups(cur_vma, groups) &&
> + !vfio_dev_in_files(cur_vma, info) &&
> !vfio_dev_in_iommufd_ctx(cur_vma, iommufd_ctx) &&
> (dev_set->device_count > 1)) {
> ret = -EINVAL;
At this point, vfio_dev_in_files() supports both group and cdev fds and
these can be used for both regular IOMMU protected and no-IOMMU
devices AFAICT. We only add this 1-off dev_set device count test and
its subtle side-effects in order to support the null-array mode, which
IMO really has yet to be shown as a requirement.
IIRC, we were wanting to add that mode as part of the cdev interface so
that the existence of cdevs implies this support, but now we're already
making use of vfio_pci_hot_reset_info.flags to indicate group-id vs
dev-id in the output, so does anything prevent us from setting another
bit there if/when this feature proves itself useful and error free, to
indicate it's an available mode for the hot-reset ioctl?
With that I think we could drop patches 4 & 5 with a plan for
introducing them later without trying to strong arm the feature in
without a proven and available use case now. Thanks,
Alex
