| Message ID | 20230401144429.88673-9-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new methods for verifying ownership in vfio PCI hot reset | expand |
On Sat, 1 Apr 2023 07:44:25 -0700 Yi Liu <yi.l.liu@intel.com> wrote: > No functional change is intended. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Yanting Jiang <yanting.jiang@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@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
On 4/1/23 16:44, Yi Liu wrote: > No functional change is intended. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Yanting Jiang <yanting.jiang@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > --- > 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;
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;