Re: [PATCH v2 3/8] vfio: Change vfio_external_user_iommu_id() to vfio_file_iommu_group()

[Yi Liu <yi.l.liu@intel.com>]

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

-- 
Regards,
Yi Liu