| Message ID | 3-v2-6a528653a750+1578a-vfio_kvm_no_group_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove vfio_group from the struct file facing VFIO API | expand |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, April 21, 2022 3:23 AM > > The only user wants to get a pointer to the struct iommu_group associated > with the VFIO group file being used. Instead of returning the group ID > then searching sysfs for that string just directly return the iommu_group > pointer already held by the vfio_group struct. > > It already has a safe lifetime due to the struct file kref, the vfio_group > and thus the iommu_group cannot be destroyed while the group file is open. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/vfio.c | 21 ++++++++++++++------- > include/linux/vfio.h | 2 +- > virt/kvm/vfio.c | 37 ++++++++++++------------------------- > 3 files changed, 27 insertions(+), 33 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index a4555014bd1e72..3444d36714e933 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1919,10 +1919,7 @@ static const struct file_operations > vfio_device_fops = { > * increments the container user counter to prevent > * the VFIO group from disposal before KVM exits. > * > - * 3. The external user calls vfio_external_user_iommu_id() > - * to know an IOMMU ID. > - * > - * 4. When the external KVM finishes, it calls > + * 3. When the external KVM finishes, it calls > * vfio_group_put_external_user() to release the VFIO group. > * This call decrements the container user counter. > */ > @@ -2001,11 +1998,21 @@ bool vfio_external_group_match_file(struct > vfio_group *test_group, > } > EXPORT_SYMBOL_GPL(vfio_external_group_match_file); > > -int vfio_external_user_iommu_id(struct vfio_group *group) > +/** > + * vfio_file_iommu_group - Return the struct iommu_group for the vfio > group file > + * @file: VFIO group file > + * > + * The returned iommu_group is valid as long as a ref is held on the file. > + */ > +struct iommu_group *vfio_file_iommu_group(struct file *file) > { > - return iommu_group_id(group->iommu_group); > + struct vfio_group *group = file->private_data; > + > + if (file->f_op != &vfio_group_fops) > + return NULL; > + return group->iommu_group; > } > -EXPORT_SYMBOL_GPL(vfio_external_user_iommu_id); > +EXPORT_SYMBOL_GPL(vfio_file_iommu_group); > > long vfio_external_check_extension(struct vfio_group *group, unsigned long > arg) > { > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 66dda06ec42d1b..8b53fd9920d24a 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -144,7 +144,7 @@ extern struct vfio_group > *vfio_group_get_external_user_from_dev(struct device > *dev); > extern bool vfio_external_group_match_file(struct vfio_group *group, > struct file *filep); > -extern int vfio_external_user_iommu_id(struct vfio_group *group); > +extern struct iommu_group *vfio_file_iommu_group(struct file *file); > extern long vfio_external_check_extension(struct vfio_group *group, > unsigned long arg); > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 07ee54a62b560d..1655d3aebd16b4 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -108,43 +108,31 @@ static bool kvm_vfio_group_is_coherent(struct > vfio_group *vfio_group) > } > > #ifdef CONFIG_SPAPR_TCE_IOMMU > -static int kvm_vfio_external_user_iommu_id(struct vfio_group *vfio_group) > +static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) > { > - int (*fn)(struct vfio_group *); > - int ret = -EINVAL; > + struct iommu_group *(*fn)(struct file *file); > + struct iommu_group *ret; > > - fn = symbol_get(vfio_external_user_iommu_id); > + fn = symbol_get(vfio_file_iommu_group); > if (!fn) > - return ret; > + return NULL; > > - ret = fn(vfio_group); > + ret = fn(file); > > - symbol_put(vfio_external_user_iommu_id); > + symbol_put(vfio_file_iommu_group); > > return ret; > } > > -static struct iommu_group *kvm_vfio_group_get_iommu_group( > - struct vfio_group *group) > -{ > - int group_id = kvm_vfio_external_user_iommu_id(group); > - > - if (group_id < 0) > - return NULL; > - > - return iommu_group_get_by_id(group_id); > -} > - > static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm, > - struct vfio_group *vfio_group) > + struct kvm_vfio_group *kvg) > { > - struct iommu_group *grp = > kvm_vfio_group_get_iommu_group(vfio_group); > + struct iommu_group *grp = kvm_vfio_file_iommu_group(kvg->file); > > if (WARN_ON_ONCE(!grp)) > return; > > kvm_spapr_tce_release_iommu_group(kvm, grp); > - iommu_group_put(grp); > } > #endif > > @@ -258,7 +246,7 @@ static int kvm_vfio_group_del(struct kvm_device > *dev, unsigned int fd) > list_del(&kvg->node); > kvm_arch_end_assignment(dev->kvm); > #ifdef CONFIG_SPAPR_TCE_IOMMU > - kvm_spapr_tce_release_vfio_group(dev->kvm, kvg- > >vfio_group); > + kvm_spapr_tce_release_vfio_group(dev->kvm, kvg); > #endif > kvm_vfio_group_set_kvm(kvg->vfio_group, NULL); > kvm_vfio_group_put_external_user(kvg->vfio_group); > @@ -304,7 +292,7 @@ static int kvm_vfio_group_set_spapr_tce(struct > kvm_device *dev, > if (kvg->file != f.file) > continue; > > - grp = kvm_vfio_group_get_iommu_group(kvg->vfio_group); > + grp = kvm_vfio_file_iommu_group(kvg->file); > if (WARN_ON_ONCE(!grp)) { > ret = -EIO; > goto err_fdput; > @@ -312,7 +300,6 @@ static int kvm_vfio_group_set_spapr_tce(struct > kvm_device *dev, > > ret = kvm_spapr_tce_attach_iommu_group(dev->kvm, > param.tablefd, > grp); > - iommu_group_put(grp); > break; > } > > @@ -386,7 +373,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) > > list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { > #ifdef CONFIG_SPAPR_TCE_IOMMU > - kvm_spapr_tce_release_vfio_group(dev->kvm, kvg- > >vfio_group); > + kvm_spapr_tce_release_vfio_group(dev->kvm, kvg); > #endif > kvm_vfio_group_set_kvm(kvg->vfio_group, NULL); > kvm_vfio_group_put_external_user(kvg->vfio_group); > -- > 2.36.0
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 2022/4/21 03:23, Jason Gunthorpe wrote: > The only user wants to get a pointer to the struct iommu_group associated > with the VFIO group file being used. Not native speaker, but above line is a little bit difficlut to interpret. "What user wants is to get a pointer to the struct iommu_group associated with the VFIO group file being used." Reviewed-by: Yi Liu <yi.l.liu@intel.com> > Instead of returning the group ID > then searching sysfs for that string just directly return the iommu_group > pointer already held by the vfio_group struct. > > It already has a safe lifetime due to the struct file kref, the vfio_group > and thus the iommu_group cannot be destroyed while the group file is open. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 21 ++++++++++++++------- > include/linux/vfio.h | 2 +- > virt/kvm/vfio.c | 37 ++++++++++++------------------------- > 3 files changed, 27 insertions(+), 33 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index a4555014bd1e72..3444d36714e933 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1919,10 +1919,7 @@ static const struct file_operations vfio_device_fops = { > * increments the container user counter to prevent > * the VFIO group from disposal before KVM exits. > * > - * 3. The external user calls vfio_external_user_iommu_id() > - * to know an IOMMU ID. > - * > - * 4. When the external KVM finishes, it calls > + * 3. When the external KVM finishes, it calls > * vfio_group_put_external_user() to release the VFIO group. > * This call decrements the container user counter. > */ > @@ -2001,11 +1998,21 @@ bool vfio_external_group_match_file(struct vfio_group *test_group, > } > EXPORT_SYMBOL_GPL(vfio_external_group_match_file); > > -int vfio_external_user_iommu_id(struct vfio_group *group) > +/** > + * vfio_file_iommu_group - Return the struct iommu_group for the vfio group file > + * @file: VFIO group file > + * > + * The returned iommu_group is valid as long as a ref is held on the file. > + */ > +struct iommu_group *vfio_file_iommu_group(struct file *file) > { > - return iommu_group_id(group->iommu_group); > + struct vfio_group *group = file->private_data; > + > + if (file->f_op != &vfio_group_fops) > + return NULL; > + return group->iommu_group; > } > -EXPORT_SYMBOL_GPL(vfio_external_user_iommu_id); > +EXPORT_SYMBOL_GPL(vfio_file_iommu_group); > > long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > { > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 66dda06ec42d1b..8b53fd9920d24a 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -144,7 +144,7 @@ extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device > *dev); > extern bool vfio_external_group_match_file(struct vfio_group *group, > struct file *filep); > -extern int vfio_external_user_iommu_id(struct vfio_group *group); > +extern struct iommu_group *vfio_file_iommu_group(struct file *file); > extern long vfio_external_check_extension(struct vfio_group *group, > unsigned long arg); > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 07ee54a62b560d..1655d3aebd16b4 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -108,43 +108,31 @@ static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) > } > > #ifdef CONFIG_SPAPR_TCE_IOMMU > -static int kvm_vfio_external_user_iommu_id(struct vfio_group *vfio_group) > +static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) > { > - int (*fn)(struct vfio_group *); > - int ret = -EINVAL; > + struct iommu_group *(*fn)(struct file *file); > + struct iommu_group *ret; > > - fn = symbol_get(vfio_external_user_iommu_id); > + fn = symbol_get(vfio_file_iommu_group); > if (!fn) > - return ret; > + return NULL; > > - ret = fn(vfio_group); > + ret = fn(file); > > - symbol_put(vfio_external_user_iommu_id); > + symbol_put(vfio_file_iommu_group); > > return ret; > } > > -static struct iommu_group *kvm_vfio_group_get_iommu_group( > - struct vfio_group *group) > -{ > - int group_id = kvm_vfio_external_user_iommu_id(group); > - > - if (group_id < 0) > - return NULL; > - > - return iommu_group_get_by_id(group_id); > -} > - > static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm, > - struct vfio_group *vfio_group) > + struct kvm_vfio_group *kvg) > { > - struct iommu_group *grp = kvm_vfio_group_get_iommu_group(vfio_group); > + struct iommu_group *grp = kvm_vfio_file_iommu_group(kvg->file); > > if (WARN_ON_ONCE(!grp)) > return; > > kvm_spapr_tce_release_iommu_group(kvm, grp); > - iommu_group_put(grp); > } > #endif > > @@ -258,7 +246,7 @@ static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd) > list_del(&kvg->node); > kvm_arch_end_assignment(dev->kvm); > #ifdef CONFIG_SPAPR_TCE_IOMMU > - kvm_spapr_tce_release_vfio_group(dev->kvm, kvg->vfio_group); > + kvm_spapr_tce_release_vfio_group(dev->kvm, kvg); > #endif > kvm_vfio_group_set_kvm(kvg->vfio_group, NULL); > kvm_vfio_group_put_external_user(kvg->vfio_group); > @@ -304,7 +292,7 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev, > if (kvg->file != f.file) > continue; > > - grp = kvm_vfio_group_get_iommu_group(kvg->vfio_group); > + grp = kvm_vfio_file_iommu_group(kvg->file); > if (WARN_ON_ONCE(!grp)) { > ret = -EIO; > goto err_fdput; > @@ -312,7 +300,6 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev, > > ret = kvm_spapr_tce_attach_iommu_group(dev->kvm, param.tablefd, > grp); > - iommu_group_put(grp); > break; > } > > @@ -386,7 +373,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) > > list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { > #ifdef CONFIG_SPAPR_TCE_IOMMU > - kvm_spapr_tce_release_vfio_group(dev->kvm, kvg->vfio_group); > + kvm_spapr_tce_release_vfio_group(dev->kvm, kvg); > #endif > kvm_vfio_group_set_kvm(kvg->vfio_group, NULL); > kvm_vfio_group_put_external_user(kvg->vfio_group);
On Thu, Apr 21, 2022 at 10:57:06PM +0800, Yi Liu wrote: > On 2022/4/21 03:23, Jason Gunthorpe wrote: > > The only user wants to get a pointer to the struct iommu_group associated > > with the VFIO group file being used. > > Not native speaker, but above line is a little bit difficlut to interpret. > > "What user wants is to get a pointer to the struct iommu_group associated > with the VFIO group file being used." How about this: The only caller wants to get a pointer to the struct iommu_group associated with the VFIO group file. Instead of returning the group ID then searching sysfs for that string to get the struct iommu_group just directly return the iommu_group pointer already held by the vfio_group struct. Jason
On 2022/4/23 01:05, Jason Gunthorpe wrote: > On Thu, Apr 21, 2022 at 10:57:06PM +0800, Yi Liu wrote: >> On 2022/4/21 03:23, Jason Gunthorpe wrote: >>> The only user wants to get a pointer to the struct iommu_group associated >>> with the VFIO group file being used. >> >> Not native speaker, but above line is a little bit difficlut to interpret. >> >> "What user wants is to get a pointer to the struct iommu_group associated >> with the VFIO group file being used." > > How about this: > > The only caller wants to get a pointer to the struct iommu_group > associated with the VFIO group file. Instead of returning the group ID > then searching sysfs for that string to get the struct iommu_group just > directly return the iommu_group pointer already held by the vfio_group > struct. clear now. :-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index a4555014bd1e72..3444d36714e933 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -1919,10 +1919,7 @@ static const struct file_operations vfio_device_fops = { * increments the container user counter to prevent * the VFIO group from disposal before KVM exits. * - * 3. The external user calls vfio_external_user_iommu_id() - * to know an IOMMU ID. - * - * 4. When the external KVM finishes, it calls + * 3. When the external KVM finishes, it calls * vfio_group_put_external_user() to release the VFIO group. * This call decrements the container user counter. */ @@ -2001,11 +1998,21 @@ bool vfio_external_group_match_file(struct vfio_group *test_group, } EXPORT_SYMBOL_GPL(vfio_external_group_match_file); -int vfio_external_user_iommu_id(struct vfio_group *group) +/** + * vfio_file_iommu_group - Return the struct iommu_group for the vfio group file + * @file: VFIO group file + * + * The returned iommu_group is valid as long as a ref is held on the file. + */ +struct iommu_group *vfio_file_iommu_group(struct file *file) { - return iommu_group_id(group->iommu_group); + struct vfio_group *group = file->private_data; + + if (file->f_op != &vfio_group_fops) + return NULL; + return group->iommu_group; } -EXPORT_SYMBOL_GPL(vfio_external_user_iommu_id); +EXPORT_SYMBOL_GPL(vfio_file_iommu_group); long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) { diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 66dda06ec42d1b..8b53fd9920d24a 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -144,7 +144,7 @@ extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device *dev); extern bool vfio_external_group_match_file(struct vfio_group *group, struct file *filep); -extern int vfio_external_user_iommu_id(struct vfio_group *group); +extern struct iommu_group *vfio_file_iommu_group(struct file *file); extern long vfio_external_check_extension(struct vfio_group *group, unsigned long arg); diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 07ee54a62b560d..1655d3aebd16b4 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -108,43 +108,31 @@ static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) } #ifdef CONFIG_SPAPR_TCE_IOMMU -static int kvm_vfio_external_user_iommu_id(struct vfio_group *vfio_group) +static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file) { - int (*fn)(struct vfio_group *); - int ret = -EINVAL; + struct iommu_group *(*fn)(struct file *file); + struct iommu_group *ret; - fn = symbol_get(vfio_external_user_iommu_id); + fn = symbol_get(vfio_file_iommu_group); if (!fn) - return ret; + return NULL; - ret = fn(vfio_group); + ret = fn(file); - symbol_put(vfio_external_user_iommu_id); + symbol_put(vfio_file_iommu_group); return ret; } -static struct iommu_group *kvm_vfio_group_get_iommu_group( - struct vfio_group *group) -{ - int group_id = kvm_vfio_external_user_iommu_id(group); - - if (group_id < 0) - return NULL; - - return iommu_group_get_by_id(group_id); -} - static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm, - struct vfio_group *vfio_group) + struct kvm_vfio_group *kvg) { - struct iommu_group *grp = kvm_vfio_group_get_iommu_group(vfio_group); + struct iommu_group *grp = kvm_vfio_file_iommu_group(kvg->file); if (WARN_ON_ONCE(!grp)) return; kvm_spapr_tce_release_iommu_group(kvm, grp); - iommu_group_put(grp); } #endif @@ -258,7 +246,7 @@ static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd) list_del(&kvg->node); kvm_arch_end_assignment(dev->kvm); #ifdef CONFIG_SPAPR_TCE_IOMMU - kvm_spapr_tce_release_vfio_group(dev->kvm, kvg->vfio_group); + kvm_spapr_tce_release_vfio_group(dev->kvm, kvg); #endif kvm_vfio_group_set_kvm(kvg->vfio_group, NULL); kvm_vfio_group_put_external_user(kvg->vfio_group); @@ -304,7 +292,7 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev, if (kvg->file != f.file) continue; - grp = kvm_vfio_group_get_iommu_group(kvg->vfio_group); + grp = kvm_vfio_file_iommu_group(kvg->file); if (WARN_ON_ONCE(!grp)) { ret = -EIO; goto err_fdput; @@ -312,7 +300,6 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev, ret = kvm_spapr_tce_attach_iommu_group(dev->kvm, param.tablefd, grp); - iommu_group_put(grp); break; } @@ -386,7 +373,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { #ifdef CONFIG_SPAPR_TCE_IOMMU - kvm_spapr_tce_release_vfio_group(dev->kvm, kvg->vfio_group); + kvm_spapr_tce_release_vfio_group(dev->kvm, kvg); #endif kvm_vfio_group_set_kvm(kvg->vfio_group, NULL); kvm_vfio_group_put_external_user(kvg->vfio_group);
The only user wants to get a pointer to the struct iommu_group associated with the VFIO group file being used. Instead of returning the group ID then searching sysfs for that string just directly return the iommu_group pointer already held by the vfio_group struct. It already has a safe lifetime due to the struct file kref, the vfio_group and thus the iommu_group cannot be destroyed while the group file is open. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio.c | 21 ++++++++++++++------- include/linux/vfio.h | 2 +- virt/kvm/vfio.c | 37 ++++++++++++------------------------- 3 files changed, 27 insertions(+), 33 deletions(-)