[PATCH v2 0/8] Remove vfio_group from the struct file facing VFIO API

[PATCH v2 0/8] Remove vfio_group from the struct file facing VFIO API

[Jason Gunthorpe]

This is the other half of removing the vfio_group from the externally
facing VFIO API.

VFIO provides an API to manipulate its struct file *'s for use by KVM and
VFIO PCI. Instead of converting the struct file into a ref counted struct
vfio_group simply use the struct file as the handle throughout the API.

Along the way some of the APIs are simplified to be more direct about what
they are trying to do with an eye to making future iommufd implementations
for all of them.

This also simplifies the container_users ref counting by not holding a
users refcount while KVM holds the group file.

Removing vfio_group from the external facing API is part of the iommufd
work to modualize and compartmentalize the VFIO container and group object
to be entirely internal to VFIO itself.

This is on github: https://github.com/jgunthorpe/linux/commits/vfio_kvm_no_group

v2:
- s/filp/file/ s/filep/file/
- Drop patch to allow ppc to be compile tested
- Keep symbol_get's Christoph has an alternative approach
v1: https://lore.kernel.org/r/0-v1-33906a626da1+16b0-vfio_kvm_no_group_jgg@nvidia.com

Jason Gunthorpe (8):
  kvm/vfio: Move KVM_DEV_VFIO_GROUP_* ioctls into functions
  kvm/vfio: Store the struct file in the kvm_vfio_group
  vfio: Change vfio_external_user_iommu_id() to vfio_file_iommu_group()
  vfio: Remove vfio_external_group_match_file()
  vfio: Change vfio_external_check_extension() to
    vfio_file_enforced_coherent()
  vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()
  kvm/vfio: Remove vfio_group from kvm
  vfio/pci: Use the struct file as the handle not the vfio_group

 drivers/vfio/pci/vfio_pci_core.c |  42 ++--
 drivers/vfio/vfio.c              | 146 ++++++------
 include/linux/vfio.h             |  14 +-
 virt/kvm/vfio.c                  | 377 ++++++++++++++-----------------
 4 files changed, 270 insertions(+), 309 deletions(-)


base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e
-- 
2.36.0

[PATCH v2 1/8] kvm/vfio: Move KVM_DEV_VFIO_GROUP_* ioctls into functions

[Jason Gunthorpe]

To make it easier to read and change in following patches.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 virt/kvm/vfio.c | 271 ++++++++++++++++++++++++++----------------------
 1 file changed, 146 insertions(+), 125 deletions(-)

This is best viewed with 'git show -b'

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 8fcbc50221c2d2..a1167ab7a2246f 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -181,149 +181,170 @@ static void kvm_vfio_update_coherency(struct kvm_device *dev)
 	mutex_unlock(&kv->lock);
 }
 
-static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
+static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 {
 	struct kvm_vfio *kv = dev->private;
 	struct vfio_group *vfio_group;
 	struct kvm_vfio_group *kvg;
-	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
 	struct fd f;
-	int32_t fd;
 	int ret;
 
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+
+	vfio_group = kvm_vfio_group_get_external_user(f.file);
+	fdput(f);
+
+	if (IS_ERR(vfio_group))
+		return PTR_ERR(vfio_group);
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (kvg->vfio_group == vfio_group) {
+			ret = -EEXIST;
+			goto err_unlock;
+		}
+	}
+
+	kvg = kzalloc(sizeof(*kvg), GFP_KERNEL_ACCOUNT);
+	if (!kvg) {
+		ret = -ENOMEM;
+		goto err_unlock;
+	}
+
+	list_add_tail(&kvg->node, &kv->group_list);
+	kvg->vfio_group = vfio_group;
+
+	kvm_arch_start_assignment(dev->kvm);
+
+	mutex_unlock(&kv->lock);
+
+	kvm_vfio_group_set_kvm(vfio_group, dev->kvm);
+	kvm_vfio_update_coherency(dev);
+
+	return 0;
+err_unlock:
+	mutex_unlock(&kv->lock);
+	kvm_vfio_group_put_external_user(vfio_group);
+	return ret;
+}
+
+static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg;
+	struct fd f;
+	int ret;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+
+	ret = -ENOENT;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_group_match_file(kvg->vfio_group,
+							f.file))
+			continue;
+
+		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);
+#endif
+		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		kfree(kvg);
+		ret = 0;
+		break;
+	}
+
+	mutex_unlock(&kv->lock);
+
+	fdput(f);
+
+	kvm_vfio_update_coherency(dev);
+
+	return ret;
+}
+
+#ifdef CONFIG_SPAPR_TCE_IOMMU
+static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
+					void __user *arg)
+{
+	struct kvm_vfio_spapr_tce param;
+	struct kvm_vfio *kv = dev->private;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	struct fd f;
+	struct iommu_group *grp;
+	int ret;
+
+	if (copy_from_user(&param, arg, sizeof(struct kvm_vfio_spapr_tce)))
+		return -EFAULT;
+
+	f = fdget(param.groupfd);
+	if (!f.file)
+		return -EBADF;
+
+	vfio_group = kvm_vfio_group_get_external_user(f.file);
+	fdput(f);
+
+	if (IS_ERR(vfio_group))
+		return PTR_ERR(vfio_group);
+
+	grp = kvm_vfio_group_get_iommu_group(vfio_group);
+	if (WARN_ON_ONCE(!grp)) {
+		ret = -EIO;
+		goto err_put_external;
+	}
+
+	ret = -ENOENT;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (kvg->vfio_group != vfio_group)
+			continue;
+
+		ret = kvm_spapr_tce_attach_iommu_group(dev->kvm, param.tablefd,
+						       grp);
+		break;
+	}
+
+	mutex_unlock(&kv->lock);
+
+	iommu_group_put(grp);
+err_put_external:
+	kvm_vfio_group_put_external_user(vfio_group);
+	return ret;
+}
+#endif
+
+static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
+{
+	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
+	int32_t fd;
+
 	switch (attr) {
 	case KVM_DEV_VFIO_GROUP_ADD:
 		if (get_user(fd, argp))
 			return -EFAULT;
-
-		f = fdget(fd);
-		if (!f.file)
-			return -EBADF;
-
-		vfio_group = kvm_vfio_group_get_external_user(f.file);
-		fdput(f);
-
-		if (IS_ERR(vfio_group))
-			return PTR_ERR(vfio_group);
-
-		mutex_lock(&kv->lock);
-
-		list_for_each_entry(kvg, &kv->group_list, node) {
-			if (kvg->vfio_group == vfio_group) {
-				mutex_unlock(&kv->lock);
-				kvm_vfio_group_put_external_user(vfio_group);
-				return -EEXIST;
-			}
-		}
-
-		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL_ACCOUNT);
-		if (!kvg) {
-			mutex_unlock(&kv->lock);
-			kvm_vfio_group_put_external_user(vfio_group);
-			return -ENOMEM;
-		}
-
-		list_add_tail(&kvg->node, &kv->group_list);
-		kvg->vfio_group = vfio_group;
-
-		kvm_arch_start_assignment(dev->kvm);
-
-		mutex_unlock(&kv->lock);
-
-		kvm_vfio_group_set_kvm(vfio_group, dev->kvm);
-
-		kvm_vfio_update_coherency(dev);
-
-		return 0;
+		return kvm_vfio_group_add(dev, fd);
 
 	case KVM_DEV_VFIO_GROUP_DEL:
 		if (get_user(fd, argp))
 			return -EFAULT;
+		return kvm_vfio_group_del(dev, fd);
 
-		f = fdget(fd);
-		if (!f.file)
-			return -EBADF;
-
-		ret = -ENOENT;
-
-		mutex_lock(&kv->lock);
-
-		list_for_each_entry(kvg, &kv->group_list, node) {
-			if (!kvm_vfio_external_group_match_file(kvg->vfio_group,
-								f.file))
-				continue;
-
-			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);
+	case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE:
+		return kvm_vfio_group_set_spapr_tce(dev, (void __user *)arg);
 #endif
-			kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
-			kvm_vfio_group_put_external_user(kvg->vfio_group);
-			kfree(kvg);
-			ret = 0;
-			break;
-		}
-
-		mutex_unlock(&kv->lock);
-
-		fdput(f);
-
-		kvm_vfio_update_coherency(dev);
-
-		return ret;
-
-#ifdef CONFIG_SPAPR_TCE_IOMMU
-	case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE: {
-		struct kvm_vfio_spapr_tce param;
-		struct kvm_vfio *kv = dev->private;
-		struct vfio_group *vfio_group;
-		struct kvm_vfio_group *kvg;
-		struct fd f;
-		struct iommu_group *grp;
-
-		if (copy_from_user(&param, (void __user *)arg,
-				sizeof(struct kvm_vfio_spapr_tce)))
-			return -EFAULT;
-
-		f = fdget(param.groupfd);
-		if (!f.file)
-			return -EBADF;
-
-		vfio_group = kvm_vfio_group_get_external_user(f.file);
-		fdput(f);
-
-		if (IS_ERR(vfio_group))
-			return PTR_ERR(vfio_group);
-
-		grp = kvm_vfio_group_get_iommu_group(vfio_group);
-		if (WARN_ON_ONCE(!grp)) {
-			kvm_vfio_group_put_external_user(vfio_group);
-			return -EIO;
-		}
-
-		ret = -ENOENT;
-
-		mutex_lock(&kv->lock);
-
-		list_for_each_entry(kvg, &kv->group_list, node) {
-			if (kvg->vfio_group != vfio_group)
-				continue;
-
-			ret = kvm_spapr_tce_attach_iommu_group(dev->kvm,
-					param.tablefd, grp);
-			break;
-		}
-
-		mutex_unlock(&kv->lock);
-
-		iommu_group_put(grp);
-		kvm_vfio_group_put_external_user(vfio_group);
-
-		return ret;
-	}
-#endif /* CONFIG_SPAPR_TCE_IOMMU */
 	}
 
 	return -ENXIO;
-- 
2.36.0

Re: [PATCH v2 1/8] kvm/vfio: Move KVM_DEV_VFIO_GROUP_* ioctls into functions

[Cornelia Huck]

On Wed, Apr 20 2022, Jason Gunthorpe <jgg@nvidia.com> wrote:

> To make it easier to read and change in following patches.
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  virt/kvm/vfio.c | 271 ++++++++++++++++++++++++++----------------------
>  1 file changed, 146 insertions(+), 125 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2 1/8] kvm/vfio: Move KVM_DEV_VFIO_GROUP_* ioctls into functions

[Yi Liu]

On 2022/4/21 03:23, Jason Gunthorpe wrote:
> To make it easier to read and change in following patches.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   virt/kvm/vfio.c | 271 ++++++++++++++++++++++++++----------------------
>   1 file changed, 146 insertions(+), 125 deletions(-)
> 
> This is best viewed with 'git show -b'


Reviewed-by: Yi Liu <yi.l.liu@intel.com>

-- 
Regards,
Yi Liu

Re: [PATCH v2 1/8] kvm/vfio: Move KVM_DEV_VFIO_GROUP_* ioctls into functions

[Jason Gunthorpe]

On Wed, Apr 20, 2022 at 04:23:10PM -0300, Jason Gunthorpe wrote:
> To make it easier to read and change in following patches.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  virt/kvm/vfio.c | 271 ++++++++++++++++++++++++++----------------------
>  1 file changed, 146 insertions(+), 125 deletions(-)
> 
> This is best viewed with 'git show -b'

Zero day hit a warning on 32bit, I'll fold this into v3:

--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -259,9 +259,10 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
 }
 #endif
 
-static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
+static int kvm_vfio_set_group(struct kvm_device *dev, long attr,
+			      void __user *arg)
 {
-	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
+	int32_t __user *argp = arg;
 	int32_t fd;
 
 	switch (attr) {
@@ -277,7 +278,7 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
 
 #ifdef CONFIG_SPAPR_TCE_IOMMU
 	case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE:
-		return kvm_vfio_group_set_spapr_tce(dev, (void __user *)arg);
+		return kvm_vfio_group_set_spapr_tce(dev, arg);
 #endif
 	}
 
@@ -289,7 +290,8 @@ static int kvm_vfio_set_attr(struct kvm_device *dev,
 {
 	switch (attr->group) {
 	case KVM_DEV_VFIO_GROUP:
-		return kvm_vfio_set_group(dev, attr->attr, attr->addr);
+		return kvm_vfio_set_group(dev, attr->attr,
+					  u64_to_user_ptr(attr->addr));
 	}
 
 	return -ENXIO;

[PATCH v2 2/8] kvm/vfio: Store the struct file in the kvm_vfio_group

[Jason Gunthorpe]

Following patches will change the APIs to use the struct file as the handle
instead of the vfio_group, so hang on to a reference to it with the same
duration of as the vfio_group.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 virt/kvm/vfio.c | 59 ++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index a1167ab7a2246f..07ee54a62b560d 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -23,6 +23,7 @@
 
 struct kvm_vfio_group {
 	struct list_head node;
+	struct file *file;
 	struct vfio_group *vfio_group;
 };
 
@@ -186,23 +187,17 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 	struct kvm_vfio *kv = dev->private;
 	struct vfio_group *vfio_group;
 	struct kvm_vfio_group *kvg;
-	struct fd f;
+	struct file *filp;
 	int ret;
 
-	f = fdget(fd);
-	if (!f.file)
+	filp = fget(fd);
+	if (!filp)
 		return -EBADF;
 
-	vfio_group = kvm_vfio_group_get_external_user(f.file);
-	fdput(f);
-
-	if (IS_ERR(vfio_group))
-		return PTR_ERR(vfio_group);
-
 	mutex_lock(&kv->lock);
 
 	list_for_each_entry(kvg, &kv->group_list, node) {
-		if (kvg->vfio_group == vfio_group) {
+		if (kvg->file == filp) {
 			ret = -EEXIST;
 			goto err_unlock;
 		}
@@ -214,6 +209,13 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 		goto err_unlock;
 	}
 
+	vfio_group = kvm_vfio_group_get_external_user(filp);
+	if (IS_ERR(vfio_group)) {
+		ret = PTR_ERR(vfio_group);
+		goto err_free;
+	}
+
+	kvg->file = filp;
 	list_add_tail(&kvg->node, &kv->group_list);
 	kvg->vfio_group = vfio_group;
 
@@ -225,9 +227,11 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 	kvm_vfio_update_coherency(dev);
 
 	return 0;
+err_free:
+	kfree(kvg);
 err_unlock:
 	mutex_unlock(&kv->lock);
-	kvm_vfio_group_put_external_user(vfio_group);
+	fput(filp);
 	return ret;
 }
 
@@ -258,6 +262,7 @@ static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
 #endif
 		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		fput(kvg->file);
 		kfree(kvg);
 		ret = 0;
 		break;
@@ -278,10 +283,8 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
 {
 	struct kvm_vfio_spapr_tce param;
 	struct kvm_vfio *kv = dev->private;
-	struct vfio_group *vfio_group;
 	struct kvm_vfio_group *kvg;
 	struct fd f;
-	struct iommu_group *grp;
 	int ret;
 
 	if (copy_from_user(&param, arg, sizeof(struct kvm_vfio_spapr_tce)))
@@ -291,36 +294,31 @@ static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
 	if (!f.file)
 		return -EBADF;
 
-	vfio_group = kvm_vfio_group_get_external_user(f.file);
-	fdput(f);
-
-	if (IS_ERR(vfio_group))
-		return PTR_ERR(vfio_group);
-
-	grp = kvm_vfio_group_get_iommu_group(vfio_group);
-	if (WARN_ON_ONCE(!grp)) {
-		ret = -EIO;
-		goto err_put_external;
-	}
-
 	ret = -ENOENT;
 
 	mutex_lock(&kv->lock);
 
 	list_for_each_entry(kvg, &kv->group_list, node) {
-		if (kvg->vfio_group != vfio_group)
+		struct iommu_group *grp;
+
+		if (kvg->file != f.file)
 			continue;
 
+		grp = kvm_vfio_group_get_iommu_group(kvg->vfio_group);
+		if (WARN_ON_ONCE(!grp)) {
+			ret = -EIO;
+			goto err_fdput;
+		}
+
 		ret = kvm_spapr_tce_attach_iommu_group(dev->kvm, param.tablefd,
 						       grp);
+		iommu_group_put(grp);
 		break;
 	}
 
 	mutex_unlock(&kv->lock);
-
-	iommu_group_put(grp);
-err_put_external:
-	kvm_vfio_group_put_external_user(vfio_group);
+err_fdput:
+	fdput(f);
 	return ret;
 }
 #endif
@@ -392,6 +390,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
 #endif
 		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		fput(kvg->file);
 		list_del(&kvg->node);
 		kfree(kvg);
 		kvm_arch_end_assignment(dev->kvm);
-- 
2.36.0

Re: [PATCH v2 2/8] kvm/vfio: Store the struct file in the kvm_vfio_group

[Yi Liu]

On 2022/4/21 03:23, Jason Gunthorpe wrote:
> Following patches will change the APIs to use the struct file as the handle
> instead of the vfio_group, so hang on to a reference to it with the same
> duration of as the vfio_group.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   virt/kvm/vfio.c | 59 ++++++++++++++++++++++++-------------------------
>   1 file changed, 29 insertions(+), 30 deletions(-)


Reviewed-by: Yi Liu <yi.l.liu@intel.com>


-- 
Regards,
Yi Liu

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

[Jason Gunthorpe]

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

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

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

[Tian, Kevin]

> 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

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Yi Liu]

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

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

[Jason Gunthorpe]

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

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

[Yi Liu]

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. :-)

-- 
Regards,
Yi Liu

[PATCH v2 4/8] vfio: Remove vfio_external_group_match_file()

[Jason Gunthorpe]

vfio_group_fops_open() ensures there is only ever one struct file open for
any struct vfio_group at any time:

	/* Do we need multiple instances of the group open?  Seems not. */
	opened = atomic_cmpxchg(&group->opened, 0, 1);
	if (opened) {
		vfio_group_put(group);
		return -EBUSY;

Therefor the struct file * can be used directly to search the list of VFIO
groups that KVM keeps instead of using the
vfio_external_group_match_file() callback to try to figure out if the
passed in FD matches the list or not.

Delete vfio_external_group_match_file().

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c  |  9 ---------
 include/linux/vfio.h |  2 --
 virt/kvm/vfio.c      | 19 +------------------
 3 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 3444d36714e933..c9122c84583aa1 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1989,15 +1989,6 @@ void vfio_group_put_external_user(struct vfio_group *group)
 }
 EXPORT_SYMBOL_GPL(vfio_group_put_external_user);
 
-bool vfio_external_group_match_file(struct vfio_group *test_group,
-				    struct file *filep)
-{
-	struct vfio_group *group = filep->private_data;
-
-	return (filep->f_op == &vfio_group_fops) && (group == test_group);
-}
-EXPORT_SYMBOL_GPL(vfio_external_group_match_file);
-
 /**
  * vfio_file_iommu_group - Return the struct iommu_group for the vfio group file
  * @file: VFIO group file
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 8b53fd9920d24a..132cf3e7cda8db 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -142,8 +142,6 @@ extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
 extern void vfio_group_put_external_user(struct vfio_group *group);
 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 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 1655d3aebd16b4..50193ae270faca 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -49,22 +49,6 @@ static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
 	return vfio_group;
 }
 
-static bool kvm_vfio_external_group_match_file(struct vfio_group *group,
-					       struct file *filep)
-{
-	bool ret, (*fn)(struct vfio_group *, struct file *);
-
-	fn = symbol_get(vfio_external_group_match_file);
-	if (!fn)
-		return false;
-
-	ret = fn(group, filep);
-
-	symbol_put(vfio_external_group_match_file);
-
-	return ret;
-}
-
 static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
 {
 	void (*fn)(struct vfio_group *);
@@ -239,8 +223,7 @@ static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
 	mutex_lock(&kv->lock);
 
 	list_for_each_entry(kvg, &kv->group_list, node) {
-		if (!kvm_vfio_external_group_match_file(kvg->vfio_group,
-							f.file))
+		if (kvg->file != f.file)
 			continue;
 
 		list_del(&kvg->node);
-- 
2.36.0

Re: [PATCH v2 4/8] vfio: Remove vfio_external_group_match_file()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 4/8] vfio: Remove vfio_external_group_match_file()

[Yi Liu]

On 2022/4/21 03:23, Jason Gunthorpe wrote:
> vfio_group_fops_open() ensures there is only ever one struct file open for
> any struct vfio_group at any time:
> 
> 	/* Do we need multiple instances of the group open?  Seems not. */
> 	opened = atomic_cmpxchg(&group->opened, 0, 1);
> 	if (opened) {
> 		vfio_group_put(group);
> 		return -EBUSY;
> 
> Therefor the struct file * can be used directly to search the list of VFIO
> groups that KVM keeps instead of using the
> vfio_external_group_match_file() callback to try to figure out if the
> passed in FD matches the list or not.
> 
> Delete vfio_external_group_match_file().
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/vfio.c  |  9 ---------
>   include/linux/vfio.h |  2 --
>   virt/kvm/vfio.c      | 19 +------------------
>   3 files changed, 1 insertion(+), 29 deletions(-)

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

-- 
Regards,
Yi Liu

[PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Jason Gunthorpe]

Instead of a general extension check change the function into a limited
test if the iommu_domain has enforced coherency, which is the only thing
kvm needs to query.

Make the new op self contained by properly refcounting the container
before touching it.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c  | 30 +++++++++++++++++++++++++++---
 include/linux/vfio.h |  3 +--
 virt/kvm/vfio.c      | 16 ++++++++--------
 3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index c9122c84583aa1..ae3e802991edf2 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2005,11 +2005,35 @@ struct iommu_group *vfio_file_iommu_group(struct file *file)
 }
 EXPORT_SYMBOL_GPL(vfio_file_iommu_group);
 
-long vfio_external_check_extension(struct vfio_group *group, unsigned long arg)
+/**
+ * vfio_file_enforced_coherent - True if the DMA associated with the VFIO file
+ *        is always CPU cache coherent
+ * @file: VFIO group file
+ *
+ * Enforced coherency means that the IOMMU ignores things like the PCIe no-snoop
+ * bit in DMA transactions. A return of false indicates that the user has
+ * rights to access additional instructions such as wbinvd on x86.
+ */
+bool vfio_file_enforced_coherent(struct file *file)
 {
-	return vfio_ioctl_check_extension(group->container, arg);
+	struct vfio_group *group = file->private_data;
+	bool ret;
+
+	if (file->f_op != &vfio_group_fops)
+		return true;
+
+	/*
+	 * Since the coherency state is determined only once a container is
+	 * attached the user must do so before they can prove they have
+	 * permission.
+	 */
+	if (vfio_group_add_container_user(group))
+		return true;
+	ret = vfio_ioctl_check_extension(group->container, VFIO_DMA_CC_IOMMU);
+	vfio_group_try_dissolve_container(group);
+	return ret;
 }
-EXPORT_SYMBOL_GPL(vfio_external_check_extension);
+EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
 
 /*
  * Sub-module support
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 132cf3e7cda8db..7f022ae126a392 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -143,8 +143,7 @@ extern void vfio_group_put_external_user(struct vfio_group *group);
 extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device
 								*dev);
 extern struct iommu_group *vfio_file_iommu_group(struct file *file);
-extern long vfio_external_check_extension(struct vfio_group *group,
-					  unsigned long arg);
+extern bool vfio_file_enforced_coherent(struct file *file);
 
 #define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned long))
 
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 50193ae270faca..2330b0c272e671 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -75,20 +75,20 @@ static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
 	symbol_put(vfio_group_set_kvm);
 }
 
-static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group)
+static bool kvm_vfio_file_enforced_coherent(struct file *file)
 {
-	long (*fn)(struct vfio_group *, unsigned long);
-	long ret;
+	bool (*fn)(struct file *file);
+	bool ret;
 
-	fn = symbol_get(vfio_external_check_extension);
+	fn = symbol_get(vfio_file_enforced_coherent);
 	if (!fn)
 		return false;
 
-	ret = fn(vfio_group, VFIO_DMA_CC_IOMMU);
+	ret = fn(file);
 
-	symbol_put(vfio_external_check_extension);
+	symbol_put(vfio_file_enforced_coherent);
 
-	return ret > 0;
+	return ret;
 }
 
 #ifdef CONFIG_SPAPR_TCE_IOMMU
@@ -136,7 +136,7 @@ static void kvm_vfio_update_coherency(struct kvm_device *dev)
 	mutex_lock(&kv->lock);
 
 	list_for_each_entry(kvg, &kv->group_list, node) {
-		if (!kvm_vfio_group_is_coherent(kvg->vfio_group)) {
+		if (!kvm_vfio_file_enforced_coherent(kvg->file)) {
 			noncoherent = true;
 			break;
 		}
-- 
2.36.0

RE: [PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 21, 2022 3:23 AM
> 
> Instead of a general extension check change the function into a limited
> test if the iommu_domain has enforced coherency, which is the only thing
> kvm needs to query.
> 
> Make the new op self contained by properly refcounting the container
> before touching it.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c  | 30 +++++++++++++++++++++++++++---
>  include/linux/vfio.h |  3 +--
>  virt/kvm/vfio.c      | 16 ++++++++--------
>  3 files changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index c9122c84583aa1..ae3e802991edf2 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -2005,11 +2005,35 @@ struct iommu_group
> *vfio_file_iommu_group(struct file *file)
>  }
>  EXPORT_SYMBOL_GPL(vfio_file_iommu_group);
> 
> -long vfio_external_check_extension(struct vfio_group *group, unsigned long
> arg)
> +/**
> + * vfio_file_enforced_coherent - True if the DMA associated with the VFIO
> file
> + *        is always CPU cache coherent
> + * @file: VFIO group file
> + *
> + * Enforced coherency means that the IOMMU ignores things like the PCIe
> no-snoop
> + * bit in DMA transactions. A return of false indicates that the user has
> + * rights to access additional instructions such as wbinvd on x86.
> + */
> +bool vfio_file_enforced_coherent(struct file *file)
>  {
> -	return vfio_ioctl_check_extension(group->container, arg);
> +	struct vfio_group *group = file->private_data;
> +	bool ret;
> +
> +	if (file->f_op != &vfio_group_fops)
> +		return true;
> +
> +	/*
> +	 * Since the coherency state is determined only once a container is
> +	 * attached the user must do so before they can prove they have
> +	 * permission.
> +	 */
> +	if (vfio_group_add_container_user(group))
> +		return true;

IMHO I still think returning an error here is better for 'user must
do so' than telling inaccurate info and leading to a situation 
where lacking of wbinvd may incur various cache problem which
is hard to debug. Yes, it's user's own problem but having a place
to capture this problem early is still a nice thing to do.

> +	ret = vfio_ioctl_check_extension(group->container,
> VFIO_DMA_CC_IOMMU);
> +	vfio_group_try_dissolve_container(group);
> +	return ret;
>  }
> -EXPORT_SYMBOL_GPL(vfio_external_check_extension);
> +EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
> 
>  /*
>   * Sub-module support
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 132cf3e7cda8db..7f022ae126a392 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -143,8 +143,7 @@ extern void vfio_group_put_external_user(struct
> vfio_group *group);
>  extern struct vfio_group *vfio_group_get_external_user_from_dev(struct
> device
>  								*dev);
>  extern struct iommu_group *vfio_file_iommu_group(struct file *file);
> -extern long vfio_external_check_extension(struct vfio_group *group,
> -					  unsigned long arg);
> +extern bool vfio_file_enforced_coherent(struct file *file);
> 
>  #define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned
> long))
> 
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 50193ae270faca..2330b0c272e671 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -75,20 +75,20 @@ static void kvm_vfio_group_set_kvm(struct
> vfio_group *group, struct kvm *kvm)
>  	symbol_put(vfio_group_set_kvm);
>  }
> 
> -static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group)
> +static bool kvm_vfio_file_enforced_coherent(struct file *file)
>  {
> -	long (*fn)(struct vfio_group *, unsigned long);
> -	long ret;
> +	bool (*fn)(struct file *file);
> +	bool ret;
> 
> -	fn = symbol_get(vfio_external_check_extension);
> +	fn = symbol_get(vfio_file_enforced_coherent);
>  	if (!fn)
>  		return false;
> 
> -	ret = fn(vfio_group, VFIO_DMA_CC_IOMMU);
> +	ret = fn(file);
> 
> -	symbol_put(vfio_external_check_extension);
> +	symbol_put(vfio_file_enforced_coherent);
> 
> -	return ret > 0;
> +	return ret;
>  }
> 
>  #ifdef CONFIG_SPAPR_TCE_IOMMU
> @@ -136,7 +136,7 @@ static void kvm_vfio_update_coherency(struct
> kvm_device *dev)
>  	mutex_lock(&kv->lock);
> 
>  	list_for_each_entry(kvg, &kv->group_list, node) {
> -		if (!kvm_vfio_group_is_coherent(kvg->vfio_group)) {
> +		if (!kvm_vfio_file_enforced_coherent(kvg->file)) {
>  			noncoherent = true;
>  			break;
>  		}
> --
> 2.36.0

Re: [PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Christoph Hellwig]

I can see why a specific error might be nice for some cases and am
open to that, but as a simple transformation this already looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

RE: [PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Tian, Kevin]

> From: Christoph Hellwig <hch@lst.de>
> Sent: Thursday, April 21, 2022 1:41 PM
> 
> I can see why a specific error might be nice for some cases and am
> open to that, but as a simple transformation this already looks good:
> 

There is a slight semantics change together with patch7.

Before patch7 the container must be attached before calling
KVM_DEV_VFIO_GROUP_ADD, otherwise vfio_group_get_external_user()
will fail. In this case the result of cache coherency for a group is
deterministic, either true or false.

After patch7 vfio_group_get_external_user() is not called. It's possible
that KVM_DEV_VFIO_GROUP_ADD is called before a container is attached
by the group. In this case cache coherency of the group cannot be
determined at that point. I prefer to returning an error in this callback
so KVM can still fail adding the group, instead of letting the inaccurate
coherency info to bit the user in a much latter point...

Thanks
Kevin

RE: [PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Friday, April 22, 2022 8:13 AM
> 
> > From: Christoph Hellwig <hch@lst.de>
> > Sent: Thursday, April 21, 2022 1:41 PM
> >
> > I can see why a specific error might be nice for some cases and am
> > open to that, but as a simple transformation this already looks good:
> >
> 
> There is a slight semantics change together with patch7.
> 
> Before patch7 the container must be attached before calling
> KVM_DEV_VFIO_GROUP_ADD, otherwise vfio_group_get_external_user()
> will fail. In this case the result of cache coherency for a group is
> deterministic, either true or false.
> 
> After patch7 vfio_group_get_external_user() is not called. It's possible
> that KVM_DEV_VFIO_GROUP_ADD is called before a container is attached
> by the group. In this case cache coherency of the group cannot be
> determined at that point. I prefer to returning an error in this callback
> so KVM can still fail adding the group, instead of letting the inaccurate
> coherency info to bit the user in a much latter point...
> 

More accurately: s/cache coherency/enforced coherency/ in above.

Re: [PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Jason Gunthorpe]

On Fri, Apr 22, 2022 at 12:32:58AM +0000, Tian, Kevin wrote:
> > From: Tian, Kevin
> > Sent: Friday, April 22, 2022 8:13 AM
> > 
> > > From: Christoph Hellwig <hch@lst.de>
> > > Sent: Thursday, April 21, 2022 1:41 PM
> > >
> > > I can see why a specific error might be nice for some cases and am
> > > open to that, but as a simple transformation this already looks good:
> > >
> > 
> > There is a slight semantics change together with patch7.
> > 
> > Before patch7 the container must be attached before calling
> > KVM_DEV_VFIO_GROUP_ADD, otherwise vfio_group_get_external_user()
> > will fail. In this case the result of cache coherency for a group is
> > deterministic, either true or false.

No, it isn't. The coherency is a propery of the iommu_domain/container
and it can change when more groups are attached to the same
domain. The best KVM can say is that if it is reporting coherency
enforced it won't stop reporting that - which doesn't change in this
series.

> > After patch7 vfio_group_get_external_user() is not called. It's possible
> > that KVM_DEV_VFIO_GROUP_ADD is called before a container is attached
> > by the group. In this case cache coherency of the group cannot be
> > determined at that point. 

groups don't have cache coherency, only iommu_domains do. In this case
it is correct to report that no non-coherent DMA is possible becuase
it isn't possible at that instant when no domain is attached.

> I prefer to returning an error in this callback > so KVM can still
> fail adding the group, instead of letting the inaccurate > coherency
> info to bit the user in a much latter point...
> 
> More accurately: s/cache coherency/enforced coherency/ in above.

An error here will not cause KVM to fail adding the group without more
changes to how it works.

I'll check, it would be nice to preserve the ABI behavior of rejecting
groups with no container.

Jason

RE: [PATCH v2 5/8] vfio: Change vfio_external_check_extension() to vfio_file_enforced_coherent()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, April 23, 2022 12:59 AM
> 
> On Fri, Apr 22, 2022 at 12:32:58AM +0000, Tian, Kevin wrote:
> > > From: Tian, Kevin
> > > Sent: Friday, April 22, 2022 8:13 AM
> > >
> > > > From: Christoph Hellwig <hch@lst.de>
> > > > Sent: Thursday, April 21, 2022 1:41 PM
> > > >
> > > > I can see why a specific error might be nice for some cases and am
> > > > open to that, but as a simple transformation this already looks good:
> > > >
> > >
> > > There is a slight semantics change together with patch7.
> > >
> > > Before patch7 the container must be attached before calling
> > > KVM_DEV_VFIO_GROUP_ADD, otherwise vfio_group_get_external_user()
> > > will fail. In this case the result of cache coherency for a group is
> > > deterministic, either true or false.
> 
> No, it isn't. The coherency is a propery of the iommu_domain/container
> and it can change when more groups are attached to the same
> domain. The best KVM can say is that if it is reporting coherency
> enforced it won't stop reporting that - which doesn't change in this
> series.

You are right. I thought this interface returns the domain property
which should be locked down once being attached by a group but
it actually checks the container property which is dynamic as you
pointed out.

[PATCH v2 6/8] vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()

[Jason Gunthorpe]

Just change the argument from struct vfio_group to struct file *.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c  | 29 +++++++++++++++++++++--------
 include/linux/vfio.h |  5 +++--
 virt/kvm/vfio.c      | 16 ++++++++--------
 3 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index ae3e802991edf2..7d0fad02936f69 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2035,6 +2035,27 @@ bool vfio_file_enforced_coherent(struct file *file)
 }
 EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
 
+/**
+ * vfio_file_set_kvm - Link a kvm with VFIO drivers
+ * @file: VFIO group file
+ * @kvm: KVM to link
+ *
+ * The kvm pointer will be forwarded to all the vfio_device's attached to the
+ * VFIO file via the VFIO_GROUP_NOTIFY_SET_KVM notifier.
+ */
+void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
+{
+	struct vfio_group *group = file->private_data;
+
+	if (file->f_op != &vfio_group_fops)
+		return;
+
+	group->kvm = kvm;
+	blocking_notifier_call_chain(&group->notifier,
+				     VFIO_GROUP_NOTIFY_SET_KVM, kvm);
+}
+EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
+
 /*
  * Sub-module support
  */
@@ -2446,14 +2467,6 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
 	return ret;
 }
 
-void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
-{
-	group->kvm = kvm;
-	blocking_notifier_call_chain(&group->notifier,
-				VFIO_GROUP_NOTIFY_SET_KVM, kvm);
-}
-EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
-
 static int vfio_register_group_notifier(struct vfio_group *group,
 					unsigned long *events,
 					struct notifier_block *nb)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 7f022ae126a392..cbd9103b5c1223 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -15,6 +15,8 @@
 #include <linux/poll.h>
 #include <uapi/linux/vfio.h>
 
+struct kvm;
+
 /*
  * VFIO devices can be placed in a set, this allows all devices to share this
  * structure and the VFIO core will provide a lock that is held around
@@ -144,6 +146,7 @@ extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device
 								*dev);
 extern struct iommu_group *vfio_file_iommu_group(struct file *file);
 extern bool vfio_file_enforced_coherent(struct file *file);
+extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
 
 #define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned long))
 
@@ -183,8 +186,6 @@ extern int vfio_unregister_notifier(struct device *dev,
 				    enum vfio_notify_type type,
 				    struct notifier_block *nb);
 
-struct kvm;
-extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
 
 /*
  * Sub-module helpers
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 2330b0c272e671..2aeb53247001cc 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -62,17 +62,17 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
 	symbol_put(vfio_group_put_external_user);
 }
 
-static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
+static void kvm_vfio_file_set_kvm(struct file *file, struct kvm *kvm)
 {
-	void (*fn)(struct vfio_group *, struct kvm *);
+	void (*fn)(struct file *file, struct kvm *kvm);
 
-	fn = symbol_get(vfio_group_set_kvm);
+	fn = symbol_get(vfio_file_set_kvm);
 	if (!fn)
 		return;
 
-	fn(group, kvm);
+	fn(file, kvm);
 
-	symbol_put(vfio_group_set_kvm);
+	symbol_put(vfio_file_set_kvm);
 }
 
 static bool kvm_vfio_file_enforced_coherent(struct file *file)
@@ -195,7 +195,7 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 
 	mutex_unlock(&kv->lock);
 
-	kvm_vfio_group_set_kvm(vfio_group, dev->kvm);
+	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
 	kvm_vfio_update_coherency(dev);
 
 	return 0;
@@ -231,7 +231,7 @@ static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
 #ifdef CONFIG_SPAPR_TCE_IOMMU
 		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
 #endif
-		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
+		kvm_vfio_file_set_kvm(kvg->file, NULL);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		fput(kvg->file);
 		kfree(kvg);
@@ -358,7 +358,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
 #ifdef CONFIG_SPAPR_TCE_IOMMU
 		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
 #endif
-		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
+		kvm_vfio_file_set_kvm(kvg->file, NULL);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		fput(kvg->file);
 		list_del(&kvg->node);
-- 
2.36.0

RE: [PATCH v2 6/8] vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 21, 2022 3:23 AM
> 
> Just change the argument from struct vfio_group to struct file *.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/vfio.c  | 29 +++++++++++++++++++++--------
>  include/linux/vfio.h |  5 +++--
>  virt/kvm/vfio.c      | 16 ++++++++--------
>  3 files changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index ae3e802991edf2..7d0fad02936f69 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -2035,6 +2035,27 @@ bool vfio_file_enforced_coherent(struct file *file)
>  }
>  EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
> 
> +/**
> + * vfio_file_set_kvm - Link a kvm with VFIO drivers
> + * @file: VFIO group file
> + * @kvm: KVM to link
> + *
> + * The kvm pointer will be forwarded to all the vfio_device's attached to the
> + * VFIO file via the VFIO_GROUP_NOTIFY_SET_KVM notifier.
> + */
> +void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> +{
> +	struct vfio_group *group = file->private_data;
> +
> +	if (file->f_op != &vfio_group_fops)
> +		return;
> +
> +	group->kvm = kvm;
> +	blocking_notifier_call_chain(&group->notifier,
> +				     VFIO_GROUP_NOTIFY_SET_KVM, kvm);
> +}
> +EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
> +
>  /*
>   * Sub-module support
>   */
> @@ -2446,14 +2467,6 @@ static int vfio_unregister_iommu_notifier(struct
> vfio_group *group,
>  	return ret;
>  }
> 
> -void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> -{
> -	group->kvm = kvm;
> -	blocking_notifier_call_chain(&group->notifier,
> -				VFIO_GROUP_NOTIFY_SET_KVM, kvm);
> -}
> -EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
> -
>  static int vfio_register_group_notifier(struct vfio_group *group,
>  					unsigned long *events,
>  					struct notifier_block *nb)
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 7f022ae126a392..cbd9103b5c1223 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -15,6 +15,8 @@
>  #include <linux/poll.h>
>  #include <uapi/linux/vfio.h>
> 
> +struct kvm;
> +
>  /*
>   * VFIO devices can be placed in a set, this allows all devices to share this
>   * structure and the VFIO core will provide a lock that is held around
> @@ -144,6 +146,7 @@ extern struct vfio_group
> *vfio_group_get_external_user_from_dev(struct device
>  								*dev);
>  extern struct iommu_group *vfio_file_iommu_group(struct file *file);
>  extern bool vfio_file_enforced_coherent(struct file *file);
> +extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
> 
>  #define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned
> long))
> 
> @@ -183,8 +186,6 @@ extern int vfio_unregister_notifier(struct device *dev,
>  				    enum vfio_notify_type type,
>  				    struct notifier_block *nb);
> 
> -struct kvm;
> -extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
> 
>  /*
>   * Sub-module helpers
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 2330b0c272e671..2aeb53247001cc 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -62,17 +62,17 @@ static void kvm_vfio_group_put_external_user(struct
> vfio_group *vfio_group)
>  	symbol_put(vfio_group_put_external_user);
>  }
> 
> -static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm
> *kvm)
> +static void kvm_vfio_file_set_kvm(struct file *file, struct kvm *kvm)
>  {
> -	void (*fn)(struct vfio_group *, struct kvm *);
> +	void (*fn)(struct file *file, struct kvm *kvm);
> 
> -	fn = symbol_get(vfio_group_set_kvm);
> +	fn = symbol_get(vfio_file_set_kvm);
>  	if (!fn)
>  		return;
> 
> -	fn(group, kvm);
> +	fn(file, kvm);
> 
> -	symbol_put(vfio_group_set_kvm);
> +	symbol_put(vfio_file_set_kvm);
>  }
> 
>  static bool kvm_vfio_file_enforced_coherent(struct file *file)
> @@ -195,7 +195,7 @@ static int kvm_vfio_group_add(struct kvm_device
> *dev, unsigned int fd)
> 
>  	mutex_unlock(&kv->lock);
> 
> -	kvm_vfio_group_set_kvm(vfio_group, dev->kvm);
> +	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
>  	kvm_vfio_update_coherency(dev);
> 
>  	return 0;
> @@ -231,7 +231,7 @@ static int kvm_vfio_group_del(struct kvm_device
> *dev, unsigned int fd)
>  #ifdef CONFIG_SPAPR_TCE_IOMMU
>  		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
>  #endif
> -		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
> +		kvm_vfio_file_set_kvm(kvg->file, NULL);
>  		kvm_vfio_group_put_external_user(kvg->vfio_group);
>  		fput(kvg->file);
>  		kfree(kvg);
> @@ -358,7 +358,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
>  #ifdef CONFIG_SPAPR_TCE_IOMMU
>  		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
>  #endif
> -		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
> +		kvm_vfio_file_set_kvm(kvg->file, NULL);
>  		kvm_vfio_group_put_external_user(kvg->vfio_group);
>  		fput(kvg->file);
>  		list_del(&kvg->node);
> --
> 2.36.0

Re: [PATCH v2 6/8] vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 7/8] kvm/vfio: Remove vfio_group from kvm

[Jason Gunthorpe]

None of the VFIO APIs take in the vfio_group anymore, so we can remove it
completely.

This has a subtle side effect on the enforced coherency tracking. The
vfio_group_get_external_user() was holding on to the container_users which
would prevent the iommu_domain and thus the enforced coherency value from
changing while the group is registered with kvm.

It changes the security proof slightly into 'user must hold a group FD
that has a device that cannot enforce DMA coherence'. As opening the group
FD, not attaching the container, is the privileged operation this doesn't
change the security properties much.

On the flip side it paves the way to changing the iommu_domain/container
attached to a group at runtime which is something that will be required to
support nested translation.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 virt/kvm/vfio.c | 51 ++++++++-----------------------------------------
 1 file changed, 8 insertions(+), 43 deletions(-)

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 2aeb53247001cc..f78c2fe3659c1a 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -24,7 +24,6 @@
 struct kvm_vfio_group {
 	struct list_head node;
 	struct file *file;
-	struct vfio_group *vfio_group;
 };
 
 struct kvm_vfio {
@@ -33,35 +32,6 @@ struct kvm_vfio {
 	bool noncoherent;
 };
 
-static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
-{
-	struct vfio_group *vfio_group;
-	struct vfio_group *(*fn)(struct file *);
-
-	fn = symbol_get(vfio_group_get_external_user);
-	if (!fn)
-		return ERR_PTR(-EINVAL);
-
-	vfio_group = fn(filep);
-
-	symbol_put(vfio_group_get_external_user);
-
-	return vfio_group;
-}
-
-static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
-{
-	void (*fn)(struct vfio_group *);
-
-	fn = symbol_get(vfio_group_put_external_user);
-	if (!fn)
-		return;
-
-	fn(vfio_group);
-
-	symbol_put(vfio_group_put_external_user);
-}
-
 static void kvm_vfio_file_set_kvm(struct file *file, struct kvm *kvm)
 {
 	void (*fn)(struct file *file, struct kvm *kvm);
@@ -91,7 +61,6 @@ static bool kvm_vfio_file_enforced_coherent(struct file *file)
 	return ret;
 }
 
-#ifdef CONFIG_SPAPR_TCE_IOMMU
 static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file)
 {
 	struct iommu_group *(*fn)(struct file *file);
@@ -108,6 +77,7 @@ static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file)
 	return ret;
 }
 
+#ifdef CONFIG_SPAPR_TCE_IOMMU
 static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm,
 					     struct kvm_vfio_group *kvg)
 {
@@ -157,7 +127,6 @@ static void kvm_vfio_update_coherency(struct kvm_device *dev)
 static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 {
 	struct kvm_vfio *kv = dev->private;
-	struct vfio_group *vfio_group;
 	struct kvm_vfio_group *kvg;
 	struct file *filp;
 	int ret;
@@ -166,6 +135,12 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 	if (!filp)
 		return -EBADF;
 
+	/* Ensure the FD is a vfio group FD.*/
+	if (!kvm_vfio_file_iommu_group(filp)) {
+		ret = -EINVAL;
+		goto err_fput;
+	}
+
 	mutex_lock(&kv->lock);
 
 	list_for_each_entry(kvg, &kv->group_list, node) {
@@ -181,15 +156,8 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 		goto err_unlock;
 	}
 
-	vfio_group = kvm_vfio_group_get_external_user(filp);
-	if (IS_ERR(vfio_group)) {
-		ret = PTR_ERR(vfio_group);
-		goto err_free;
-	}
-
 	kvg->file = filp;
 	list_add_tail(&kvg->node, &kv->group_list);
-	kvg->vfio_group = vfio_group;
 
 	kvm_arch_start_assignment(dev->kvm);
 
@@ -199,10 +167,9 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
 	kvm_vfio_update_coherency(dev);
 
 	return 0;
-err_free:
-	kfree(kvg);
 err_unlock:
 	mutex_unlock(&kv->lock);
+err_fput:
 	fput(filp);
 	return ret;
 }
@@ -232,7 +199,6 @@ static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
 		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
 #endif
 		kvm_vfio_file_set_kvm(kvg->file, NULL);
-		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		fput(kvg->file);
 		kfree(kvg);
 		ret = 0;
@@ -359,7 +325,6 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
 		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
 #endif
 		kvm_vfio_file_set_kvm(kvg->file, NULL);
-		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		fput(kvg->file);
 		list_del(&kvg->node);
 		kfree(kvg);
-- 
2.36.0

RE: [PATCH v2 7/8] kvm/vfio: Remove vfio_group from kvm

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 21, 2022 3:23 AM
> 
> None of the VFIO APIs take in the vfio_group anymore, so we can remove it
> completely.
> 
> This has a subtle side effect on the enforced coherency tracking. The
> vfio_group_get_external_user() was holding on to the container_users which
> would prevent the iommu_domain and thus the enforced coherency value
> from
> changing while the group is registered with kvm.
> 
> It changes the security proof slightly into 'user must hold a group FD
> that has a device that cannot enforce DMA coherence'. As opening the group
> FD, not attaching the container, is the privileged operation this doesn't
> change the security properties much.
> 
> On the flip side it paves the way to changing the iommu_domain/container
> attached to a group at runtime which is something that will be required to
> support nested translation.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Other than whether to check error on enforced coherency:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  virt/kvm/vfio.c | 51 ++++++++-----------------------------------------
>  1 file changed, 8 insertions(+), 43 deletions(-)
> 
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 2aeb53247001cc..f78c2fe3659c1a 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -24,7 +24,6 @@
>  struct kvm_vfio_group {
>  	struct list_head node;
>  	struct file *file;
> -	struct vfio_group *vfio_group;
>  };
> 
>  struct kvm_vfio {
> @@ -33,35 +32,6 @@ struct kvm_vfio {
>  	bool noncoherent;
>  };
> 
> -static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
> -{
> -	struct vfio_group *vfio_group;
> -	struct vfio_group *(*fn)(struct file *);
> -
> -	fn = symbol_get(vfio_group_get_external_user);
> -	if (!fn)
> -		return ERR_PTR(-EINVAL);
> -
> -	vfio_group = fn(filep);
> -
> -	symbol_put(vfio_group_get_external_user);
> -
> -	return vfio_group;
> -}
> -
> -static void kvm_vfio_group_put_external_user(struct vfio_group
> *vfio_group)
> -{
> -	void (*fn)(struct vfio_group *);
> -
> -	fn = symbol_get(vfio_group_put_external_user);
> -	if (!fn)
> -		return;
> -
> -	fn(vfio_group);
> -
> -	symbol_put(vfio_group_put_external_user);
> -}
> -
>  static void kvm_vfio_file_set_kvm(struct file *file, struct kvm *kvm)
>  {
>  	void (*fn)(struct file *file, struct kvm *kvm);
> @@ -91,7 +61,6 @@ static bool kvm_vfio_file_enforced_coherent(struct file
> *file)
>  	return ret;
>  }
> 
> -#ifdef CONFIG_SPAPR_TCE_IOMMU
>  static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file)
>  {
>  	struct iommu_group *(*fn)(struct file *file);
> @@ -108,6 +77,7 @@ static struct iommu_group
> *kvm_vfio_file_iommu_group(struct file *file)
>  	return ret;
>  }
> 
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
>  static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm,
>  					     struct kvm_vfio_group *kvg)
>  {
> @@ -157,7 +127,6 @@ static void kvm_vfio_update_coherency(struct
> kvm_device *dev)
>  static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
>  {
>  	struct kvm_vfio *kv = dev->private;
> -	struct vfio_group *vfio_group;
>  	struct kvm_vfio_group *kvg;
>  	struct file *filp;
>  	int ret;
> @@ -166,6 +135,12 @@ static int kvm_vfio_group_add(struct kvm_device
> *dev, unsigned int fd)
>  	if (!filp)
>  		return -EBADF;
> 
> +	/* Ensure the FD is a vfio group FD.*/
> +	if (!kvm_vfio_file_iommu_group(filp)) {
> +		ret = -EINVAL;
> +		goto err_fput;
> +	}
> +
>  	mutex_lock(&kv->lock);
> 
>  	list_for_each_entry(kvg, &kv->group_list, node) {
> @@ -181,15 +156,8 @@ static int kvm_vfio_group_add(struct kvm_device
> *dev, unsigned int fd)
>  		goto err_unlock;
>  	}
> 
> -	vfio_group = kvm_vfio_group_get_external_user(filp);
> -	if (IS_ERR(vfio_group)) {
> -		ret = PTR_ERR(vfio_group);
> -		goto err_free;
> -	}
> -
>  	kvg->file = filp;
>  	list_add_tail(&kvg->node, &kv->group_list);
> -	kvg->vfio_group = vfio_group;
> 
>  	kvm_arch_start_assignment(dev->kvm);
> 
> @@ -199,10 +167,9 @@ static int kvm_vfio_group_add(struct kvm_device
> *dev, unsigned int fd)
>  	kvm_vfio_update_coherency(dev);
> 
>  	return 0;
> -err_free:
> -	kfree(kvg);
>  err_unlock:
>  	mutex_unlock(&kv->lock);
> +err_fput:
>  	fput(filp);
>  	return ret;
>  }
> @@ -232,7 +199,6 @@ static int kvm_vfio_group_del(struct kvm_device
> *dev, unsigned int fd)
>  		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
>  #endif
>  		kvm_vfio_file_set_kvm(kvg->file, NULL);
> -		kvm_vfio_group_put_external_user(kvg->vfio_group);
>  		fput(kvg->file);
>  		kfree(kvg);
>  		ret = 0;
> @@ -359,7 +325,6 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
>  		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg);
>  #endif
>  		kvm_vfio_file_set_kvm(kvg->file, NULL);
> -		kvm_vfio_group_put_external_user(kvg->vfio_group);
>  		fput(kvg->file);
>  		list_del(&kvg->node);
>  		kfree(kvg);
> --
> 2.36.0

Re: [PATCH v2 7/8] kvm/vfio: Remove vfio_group from kvm

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 8/8] vfio/pci: Use the struct file as the handle not the vfio_group

[Jason Gunthorpe]

VFIO PCI does a security check as part of hot reset to prove that the user
has permission to manipulate all the devices that will be impacted by the
reset.

Use a new API vfio_file_has_dev() to perform this security check against
the struct file directly and remove the vfio_group from VFIO PCI.

Since VFIO PCI was the last user of vfio_group_get_external_user() remove
it as well.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 42 ++++++++++-----------
 drivers/vfio/vfio.c              | 63 +++++++++-----------------------
 include/linux/vfio.h             |  2 +-
 3 files changed, 40 insertions(+), 67 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index b7bb16f92ac628..465c42f53fd2fc 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -577,7 +577,7 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void *data)
 
 struct vfio_pci_group_info {
 	int count;
-	struct vfio_group **groups;
+	struct file **files;
 };
 
 static bool vfio_pci_dev_below_slot(struct pci_dev *pdev, struct pci_slot *slot)
@@ -1039,10 +1039,10 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 	} else if (cmd == VFIO_DEVICE_PCI_HOT_RESET) {
 		struct vfio_pci_hot_reset hdr;
 		int32_t *group_fds;
-		struct vfio_group **groups;
+		struct file **files;
 		struct vfio_pci_group_info info;
 		bool slot = false;
-		int group_idx, count = 0, ret = 0;
+		int file_idx, count = 0, ret = 0;
 
 		minsz = offsetofend(struct vfio_pci_hot_reset, count);
 
@@ -1075,17 +1075,17 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 			return -EINVAL;
 
 		group_fds = kcalloc(hdr.count, sizeof(*group_fds), GFP_KERNEL);
-		groups = kcalloc(hdr.count, sizeof(*groups), GFP_KERNEL);
-		if (!group_fds || !groups) {
+		files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL);
+		if (!group_fds || !files) {
 			kfree(group_fds);
-			kfree(groups);
+			kfree(files);
 			return -ENOMEM;
 		}
 
 		if (copy_from_user(group_fds, (void __user *)(arg + minsz),
 				   hdr.count * sizeof(*group_fds))) {
 			kfree(group_fds);
-			kfree(groups);
+			kfree(files);
 			return -EFAULT;
 		}
 
@@ -1094,22 +1094,22 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 		 * user interface and store the group and iommu ID.  This
 		 * ensures the group is held across the reset.
 		 */
-		for (group_idx = 0; group_idx < hdr.count; group_idx++) {
-			struct vfio_group *group;
-			struct fd f = fdget(group_fds[group_idx]);
-			if (!f.file) {
+		for (file_idx = 0; file_idx < hdr.count; file_idx++) {
+			struct file *file = fget(group_fds[file_idx]);
+
+			if (!file) {
 				ret = -EBADF;
 				break;
 			}
 
-			group = vfio_group_get_external_user(f.file);
-			fdput(f);
-			if (IS_ERR(group)) {
-				ret = PTR_ERR(group);
+			/* Ensure the FD is a vfio group FD.*/
+			if (!vfio_file_iommu_group(file)) {
+				fput(file);
+				ret = -EINVAL;
 				break;
 			}
 
-			groups[group_idx] = group;
+			files[file_idx] = file;
 		}
 
 		kfree(group_fds);
@@ -1119,15 +1119,15 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 			goto hot_reset_release;
 
 		info.count = hdr.count;
-		info.groups = groups;
+		info.files = files;
 
 		ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info);
 
 hot_reset_release:
-		for (group_idx--; group_idx >= 0; group_idx--)
-			vfio_group_put_external_user(groups[group_idx]);
+		for (file_idx--; file_idx >= 0; file_idx--)
+			fput(files[file_idx]);
 
-		kfree(groups);
+		kfree(files);
 		return ret;
 	} else if (cmd == VFIO_DEVICE_IOEVENTFD) {
 		struct vfio_device_ioeventfd ioeventfd;
@@ -1964,7 +1964,7 @@ static bool vfio_dev_in_groups(struct vfio_pci_core_device *vdev,
 	unsigned int i;
 
 	for (i = 0; i < groups->count; i++)
-		if (groups->groups[i] == vdev->vdev.group)
+		if (vfio_file_has_dev(groups->files[i], &vdev->vdev))
 			return true;
 	return false;
 }
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 7d0fad02936f69..ff5f6e0f285faa 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1899,51 +1899,6 @@ static const struct file_operations vfio_device_fops = {
 	.mmap		= vfio_device_fops_mmap,
 };
 
-/*
- * External user API, exported by symbols to be linked dynamically.
- *
- * The protocol includes:
- *  1. do normal VFIO init operation:
- *	- opening a new container;
- *	- attaching group(s) to it;
- *	- setting an IOMMU driver for a container.
- * When IOMMU is set for a container, all groups in it are
- * considered ready to use by an external user.
- *
- * 2. User space passes a group fd to an external user.
- * The external user calls vfio_group_get_external_user()
- * to verify that:
- *	- the group is initialized;
- *	- IOMMU is set for it.
- * If both checks passed, vfio_group_get_external_user()
- * increments the container user counter to prevent
- * the VFIO group from disposal before KVM exits.
- *
- * 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.
- */
-struct vfio_group *vfio_group_get_external_user(struct file *filep)
-{
-	struct vfio_group *group = filep->private_data;
-	int ret;
-
-	if (filep->f_op != &vfio_group_fops)
-		return ERR_PTR(-EINVAL);
-
-	ret = vfio_group_add_container_user(group);
-	if (ret)
-		return ERR_PTR(ret);
-
-	/*
-	 * Since the caller holds the fget on the file group->users must be >= 1
-	 */
-	vfio_group_get(group);
-
-	return group;
-}
-EXPORT_SYMBOL_GPL(vfio_group_get_external_user);
-
 /*
  * External user API, exported by symbols to be linked dynamically.
  * The external user passes in a device pointer
@@ -2056,6 +2011,24 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
 }
 EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
 
+/**
+ * vfio_file_has_dev - True if the VFIO file is a handle for device
+ * @file: VFIO file to check
+ * @device: Device that must be part of the file
+ *
+ * Returns true if given file has permission to manipulate the given device.
+ */
+bool vfio_file_has_dev(struct file *file, struct vfio_device *device)
+{
+	struct vfio_group *group = file->private_data;
+
+	if (file->f_op != &vfio_group_fops)
+		return false;
+
+	return group == device->group;
+}
+EXPORT_SYMBOL_GPL(vfio_file_has_dev);
+
 /*
  * Sub-module support
  */
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index cbd9103b5c1223..e8be8ec40f2b50 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -140,13 +140,13 @@ int vfio_mig_get_next_state(struct vfio_device *device,
 /*
  * External user API
  */
-extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
 extern void vfio_group_put_external_user(struct vfio_group *group);
 extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device
 								*dev);
 extern struct iommu_group *vfio_file_iommu_group(struct file *file);
 extern bool vfio_file_enforced_coherent(struct file *file);
 extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
+extern bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
 
 #define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned long))
 
-- 
2.36.0

RE: [PATCH v2 8/8] vfio/pci: Use the struct file as the handle not the vfio_group

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 21, 2022 3:23 AM
> 
> VFIO PCI does a security check as part of hot reset to prove that the user
> has permission to manipulate all the devices that will be impacted by the
> reset.
> 
> Use a new API vfio_file_has_dev() to perform this security check against
> the struct file directly and remove the vfio_group from VFIO PCI.
> 
> Since VFIO PCI was the last user of vfio_group_get_external_user() remove
> it as well.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/pci/vfio_pci_core.c | 42 ++++++++++-----------
>  drivers/vfio/vfio.c              | 63 +++++++++-----------------------
>  include/linux/vfio.h             |  2 +-
>  3 files changed, 40 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index b7bb16f92ac628..465c42f53fd2fc 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -577,7 +577,7 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void
> *data)
> 
>  struct vfio_pci_group_info {
>  	int count;
> -	struct vfio_group **groups;
> +	struct file **files;
>  };
> 
>  static bool vfio_pci_dev_below_slot(struct pci_dev *pdev, struct pci_slot
> *slot)
> @@ -1039,10 +1039,10 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
>  	} else if (cmd == VFIO_DEVICE_PCI_HOT_RESET) {
>  		struct vfio_pci_hot_reset hdr;
>  		int32_t *group_fds;
> -		struct vfio_group **groups;
> +		struct file **files;
>  		struct vfio_pci_group_info info;
>  		bool slot = false;
> -		int group_idx, count = 0, ret = 0;
> +		int file_idx, count = 0, ret = 0;
> 
>  		minsz = offsetofend(struct vfio_pci_hot_reset, count);
> 
> @@ -1075,17 +1075,17 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
>  			return -EINVAL;
> 
>  		group_fds = kcalloc(hdr.count, sizeof(*group_fds),
> GFP_KERNEL);
> -		groups = kcalloc(hdr.count, sizeof(*groups), GFP_KERNEL);
> -		if (!group_fds || !groups) {
> +		files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL);
> +		if (!group_fds || !files) {
>  			kfree(group_fds);
> -			kfree(groups);
> +			kfree(files);
>  			return -ENOMEM;
>  		}
> 
>  		if (copy_from_user(group_fds, (void __user *)(arg + minsz),
>  				   hdr.count * sizeof(*group_fds))) {
>  			kfree(group_fds);
> -			kfree(groups);
> +			kfree(files);
>  			return -EFAULT;
>  		}
> 
> @@ -1094,22 +1094,22 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
>  		 * user interface and store the group and iommu ID.  This
>  		 * ensures the group is held across the reset.
>  		 */
> -		for (group_idx = 0; group_idx < hdr.count; group_idx++) {
> -			struct vfio_group *group;
> -			struct fd f = fdget(group_fds[group_idx]);
> -			if (!f.file) {
> +		for (file_idx = 0; file_idx < hdr.count; file_idx++) {
> +			struct file *file = fget(group_fds[file_idx]);
> +
> +			if (!file) {
>  				ret = -EBADF;
>  				break;
>  			}
> 
> -			group = vfio_group_get_external_user(f.file);
> -			fdput(f);
> -			if (IS_ERR(group)) {
> -				ret = PTR_ERR(group);
> +			/* Ensure the FD is a vfio group FD.*/
> +			if (!vfio_file_iommu_group(file)) {
> +				fput(file);
> +				ret = -EINVAL;
>  				break;
>  			}
> 
> -			groups[group_idx] = group;
> +			files[file_idx] = file;
>  		}
> 
>  		kfree(group_fds);
> @@ -1119,15 +1119,15 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
>  			goto hot_reset_release;
> 
>  		info.count = hdr.count;
> -		info.groups = groups;
> +		info.files = files;
> 
>  		ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info);
> 
>  hot_reset_release:
> -		for (group_idx--; group_idx >= 0; group_idx--)
> -			vfio_group_put_external_user(groups[group_idx]);
> +		for (file_idx--; file_idx >= 0; file_idx--)
> +			fput(files[file_idx]);
> 
> -		kfree(groups);
> +		kfree(files);
>  		return ret;
>  	} else if (cmd == VFIO_DEVICE_IOEVENTFD) {
>  		struct vfio_device_ioeventfd ioeventfd;
> @@ -1964,7 +1964,7 @@ static bool vfio_dev_in_groups(struct
> vfio_pci_core_device *vdev,
>  	unsigned int i;
> 
>  	for (i = 0; i < groups->count; i++)
> -		if (groups->groups[i] == vdev->vdev.group)
> +		if (vfio_file_has_dev(groups->files[i], &vdev->vdev))
>  			return true;
>  	return false;
>  }
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 7d0fad02936f69..ff5f6e0f285faa 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1899,51 +1899,6 @@ static const struct file_operations
> vfio_device_fops = {
>  	.mmap		= vfio_device_fops_mmap,
>  };
> 
> -/*
> - * External user API, exported by symbols to be linked dynamically.
> - *
> - * The protocol includes:
> - *  1. do normal VFIO init operation:
> - *	- opening a new container;
> - *	- attaching group(s) to it;
> - *	- setting an IOMMU driver for a container.
> - * When IOMMU is set for a container, all groups in it are
> - * considered ready to use by an external user.
> - *
> - * 2. User space passes a group fd to an external user.
> - * The external user calls vfio_group_get_external_user()
> - * to verify that:
> - *	- the group is initialized;
> - *	- IOMMU is set for it.
> - * If both checks passed, vfio_group_get_external_user()
> - * increments the container user counter to prevent
> - * the VFIO group from disposal before KVM exits.
> - *
> - * 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.
> - */
> -struct vfio_group *vfio_group_get_external_user(struct file *filep)
> -{
> -	struct vfio_group *group = filep->private_data;
> -	int ret;
> -
> -	if (filep->f_op != &vfio_group_fops)
> -		return ERR_PTR(-EINVAL);
> -
> -	ret = vfio_group_add_container_user(group);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
> -	/*
> -	 * Since the caller holds the fget on the file group->users must be >= 1
> -	 */
> -	vfio_group_get(group);
> -
> -	return group;
> -}
> -EXPORT_SYMBOL_GPL(vfio_group_get_external_user);
> -
>  /*
>   * External user API, exported by symbols to be linked dynamically.
>   * The external user passes in a device pointer
> @@ -2056,6 +2011,24 @@ void vfio_file_set_kvm(struct file *file, struct kvm
> *kvm)
>  }
>  EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
> 
> +/**
> + * vfio_file_has_dev - True if the VFIO file is a handle for device
> + * @file: VFIO file to check
> + * @device: Device that must be part of the file
> + *
> + * Returns true if given file has permission to manipulate the given device.
> + */
> +bool vfio_file_has_dev(struct file *file, struct vfio_device *device)
> +{
> +	struct vfio_group *group = file->private_data;
> +
> +	if (file->f_op != &vfio_group_fops)
> +		return false;
> +
> +	return group == device->group;
> +}
> +EXPORT_SYMBOL_GPL(vfio_file_has_dev);
> +
>  /*
>   * Sub-module support
>   */
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index cbd9103b5c1223..e8be8ec40f2b50 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -140,13 +140,13 @@ int vfio_mig_get_next_state(struct vfio_device
> *device,
>  /*
>   * External user API
>   */
> -extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
>  extern void vfio_group_put_external_user(struct vfio_group *group);
>  extern struct vfio_group *vfio_group_get_external_user_from_dev(struct
> device
>  								*dev);
>  extern struct iommu_group *vfio_file_iommu_group(struct file *file);
>  extern bool vfio_file_enforced_coherent(struct file *file);
>  extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
> +extern bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
> 
>  #define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned
> long))
> 
> --
> 2.36.0

Re: [PATCH v2 8/8] vfio/pci: Use the struct file as the handle not the vfio_group

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 0/8] Remove vfio_group from the struct file facing VFIO API

[Yi Liu]

On 2022/4/21 03:23, Jason Gunthorpe wrote:
> This is the other half of removing the vfio_group from the externally
> facing VFIO API.
> 
> VFIO provides an API to manipulate its struct file *'s for use by KVM and
> VFIO PCI. Instead of converting the struct file into a ref counted struct
> vfio_group simply use the struct file as the handle throughout the API.
> 
> Along the way some of the APIs are simplified to be more direct about what
> they are trying to do with an eye to making future iommufd implementations
> for all of them.
> 
> This also simplifies the container_users ref counting by not holding a
> users refcount while KVM holds the group file.
> 
> Removing vfio_group from the external facing API is part of the iommufd
> work to modualize and compartmentalize the VFIO container and group object
> to be entirely internal to VFIO itself.
> 
> This is on github: https://github.com/jgunthorpe/linux/commits/vfio_kvm_no_group

passthrough a device from singleton-group passed. will do it test against
non-singleton group as well.

> v2:
> - s/filp/file/ s/filep/file/
> - Drop patch to allow ppc to be compile tested
> - Keep symbol_get's Christoph has an alternative approach
> v1: https://lore.kernel.org/r/0-v1-33906a626da1+16b0-vfio_kvm_no_group_jgg@nvidia.com
> 
> Jason Gunthorpe (8):
>    kvm/vfio: Move KVM_DEV_VFIO_GROUP_* ioctls into functions
>    kvm/vfio: Store the struct file in the kvm_vfio_group
>    vfio: Change vfio_external_user_iommu_id() to vfio_file_iommu_group()
>    vfio: Remove vfio_external_group_match_file()
>    vfio: Change vfio_external_check_extension() to
>      vfio_file_enforced_coherent()
>    vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()
>    kvm/vfio: Remove vfio_group from kvm
>    vfio/pci: Use the struct file as the handle not the vfio_group
> 
>   drivers/vfio/pci/vfio_pci_core.c |  42 ++--
>   drivers/vfio/vfio.c              | 146 ++++++------
>   include/linux/vfio.h             |  14 +-
>   virt/kvm/vfio.c                  | 377 ++++++++++++++-----------------
>   4 files changed, 270 insertions(+), 309 deletions(-)
> 
> 
> base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e

-- 
Regards,
Yi Liu