Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Jason Gunthorpe]

On Tue, May 17, 2022 at 02:08:51PM -0400, Matthew Rosato wrote:
> Rather than relying on a notifier for associating the KVM with
> the group, let's assume that the association has already been
> made prior to device_open.  The first time a device is opened
> associate the group KVM with the device.

This also fixes a user triggerable oops in gvt
 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>

You can add my signed-off-by as well

> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index cfcff7764403..c5d421eda275 100644
> +++ b/drivers/vfio/vfio.c
> @@ -10,6 +10,7 @@
>   * Author: Tom Lyon, pugs@cisco.com
>   */
>  
> +#include "linux/kvm_host.h"

This is the wrong format of include (editor automation, sigh)

> @@ -1083,6 +1084,13 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  
>  	mutex_lock(&device->dev_set->lock);
>  	device->open_count++;
> +	down_write(&device->group->group_rwsem);

Read I suppose

> +	if (device->open_count == 1 && device->group->kvm) {
> +		device->kvm = device->group->kvm;
> +		kvm_get_kvm(device->kvm);
> +	}
> +	up_write(&device->group->group_rwsem);

Yeah, this is OK, not very elegant though

I was looking at this - but it could come later too:

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 8e11d9119418be..c5d8dfe8314108 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -10,7 +10,6 @@
  * Author: Tom Lyon, pugs@cisco.com
  */
 
-#include "linux/kvm_host.h"
 #include <linux/cdev.h>
 #include <linux/compat.h>
 #include <linux/device.h>
@@ -33,6 +32,7 @@
 #include <linux/vfio.h>
 #include <linux/wait.h>
 #include <linux/sched/signal.h>
+#include <linux/kvm_host.h>
 #include "vfio.h"
 
 #define DRIVER_VERSION	"0.3"
@@ -1059,93 +1059,71 @@ static int vfio_device_assign_container(struct vfio_device *device)
 
 static void vfio_device_unassign_container(struct vfio_device *device)
 {
-	down_write(&device->group->group_rwsem);
+	lockdep_assert_held(&device->group->group_rwsem);
+
 	WARN_ON(device->group->container_users <= 1);
 	device->group->container_users--;
 	fput(device->group->opened_file);
-	up_write(&device->group->group_rwsem);
 }
 
-static struct file *vfio_device_open(struct vfio_device *device)
+static int vfio_device_open(struct vfio_device *device)
 {
-	struct file *filep;
 	int ret;
 
+	lockdep_assert_held(&device->dev_set->lock);
+
+	if (!try_module_get(device->dev->driver->owner))
+		return -ENODEV;
+
 	down_write(&device->group->group_rwsem);
 	ret = vfio_device_assign_container(device);
-	if (ret) {
-		up_write(&device->group->group_rwsem);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto err_unlock;
 
 	if (device->ops->flags & VFIO_DEVICE_NEEDS_KVM)
 	{
-		if (!device->group->kvm) {
-			up_write(&device->group->group_rwsem);
+		if (!device->group->kvm)
 			goto err_unassign_container;
-		}
 		device->kvm = device->group->kvm;
 		kvm_get_kvm(device->kvm);
 	}
 	up_write(&device->group->group_rwsem);
 
-	if (!try_module_get(device->dev->driver->owner)) {
-		ret = -ENODEV;
-		goto err_put_kvm;
-	}
-
-	mutex_lock(&device->dev_set->lock);
-	device->open_count++;
-	if (device->open_count == 1 && device->ops->open_device) {
+	if (device->ops->open_device) {
 		ret = device->ops->open_device(device);
 		if (ret)
-			goto err_undo_count;
+			goto err_put_kvm;
 	}
-	mutex_unlock(&device->dev_set->lock);
+	return 0;
 
-	/*
-	 * We can't use anon_inode_getfd() because we need to modify
-	 * the f_mode flags directly to allow more than just ioctls
-	 */
-	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
-				   device, O_RDWR);
-	if (IS_ERR(filep)) {
-		ret = PTR_ERR(filep);
-		goto err_close_device;
+err_put_kvm:
+	if (device->kvm) {
+		kvm_put_kvm(device->kvm);
+		device->kvm = NULL;
 	}
+	down_write(&device->group->group_rwsem);
+err_unassign_container:
+	vfio_device_unassign_container(device);
+err_unlock:
+	up_write(&device->group->group_rwsem);
+	module_put(device->dev->driver->owner);
+	return ret;
+}
 
-	/*
-	 * TODO: add an anon_inode interface to do this.
-	 * Appears to be missing by lack of need rather than
-	 * explicitly prevented.  Now there's need.
-	 */
-	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
-
-	if (device->group->type == VFIO_NO_IOMMU)
-		dev_warn(device->dev, "vfio-noiommu device opened by user "
-			 "(%s:%d)\n", current->comm, task_pid_nr(current));
-	/*
-	 * On success the ref of device is moved to the file and
-	 * put in vfio_device_fops_release()
-	 */
-	return filep;
+static void vfio_device_close(struct vfio_device *device)
+{
+	lockdep_assert_held(&device->dev_set->lock);
 
-err_close_device:
-	mutex_lock(&device->dev_set->lock);
-	if (device->open_count == 1 && device->ops->close_device)
+	if (device->ops->close_device)
 		device->ops->close_device(device);
-err_undo_count:
-	device->open_count--;
-	mutex_unlock(&device->dev_set->lock);
-	module_put(device->dev->driver->owner);
-err_put_kvm:
 	if (device->kvm) {
 		kvm_put_kvm(device->kvm);
 		device->kvm = NULL;
 	}
-err_unassign_container:
+	down_write(&device->group->group_rwsem);
 	vfio_device_unassign_container(device);
-	return ERR_PTR(ret);
+	up_write(&device->group->group_rwsem);
+	module_put(device->dev->driver->owner);
 }
 
 static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
@@ -1159,23 +1137,61 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	if (IS_ERR(device))
 		return PTR_ERR(device);
 
-	fdno = get_unused_fd_flags(O_CLOEXEC);
-	if (fdno < 0) {
-		ret = fdno;
-		goto err_put_device;
+	mutex_lock(&device->dev_set->lock);
+	device->open_count++;
+	if (device->open_count == 1) {
+		ret = vfio_device_open(device);
+		if (ret) {
+			device->open_count--;
+			mutex_unlock(&device->dev_set->lock);
+			goto err_put_device;
+		}
 	}
+	mutex_unlock(&device->dev_set->lock);
 
-	filep = vfio_device_open(device);
+	/*
+	 * We can't use anon_inode_getfd() because we need to modify
+	 * the f_mode flags directly to allow more than just ioctls
+	 */
+	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops, device,
+				   O_RDWR);
 	if (IS_ERR(filep)) {
 		ret = PTR_ERR(filep);
-		goto err_put_fdno;
+		goto err_close_device;
+	}
+
+	/*
+	 * TODO: add an anon_inode interface to do this.
+	 * Appears to be missing by lack of need rather than
+	 * explicitly prevented.  Now there's need.
+	 */
+	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
+
+	fdno = get_unused_fd_flags(O_CLOEXEC);
+	if (fdno < 0) {
+		ret = fdno;
+		goto err_put_file;
 	}
 
+	if (device->group->type == VFIO_NO_IOMMU)
+		dev_warn(device->dev, "vfio-noiommu device opened by user "
+			 "(%s:%d)\n", current->comm, task_pid_nr(current));
+
+	/*
+	 * On success the ref of device is moved to the file and put in
+	 * vfio_device_fops_release().
+	 */
 	fd_install(fdno, filep);
 	return fdno;
 
-err_put_fdno:
-	put_unused_fd(fdno);
+err_put_file:
+	fput(filep);
+err_close_device:
+	mutex_lock(&device->dev_set->lock);
+	if (device->open_count == 1)
+		vfio_device_close(device);
+	device->open_count--;
+	mutex_unlock(&device->dev_set->lock);
 err_put_device:
 	vfio_device_put(device);
 	return ret;
@@ -1333,19 +1349,11 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 
 	mutex_lock(&device->dev_set->lock);
 	vfio_assert_device_open(device);
-	if (device->open_count == 1 && device->ops->close_device)
-		device->ops->close_device(device);
+	if (device->open_count == 1)
+		vfio_device_close(device);
 	device->open_count--;
 	mutex_unlock(&device->dev_set->lock);
 
-	module_put(device->dev->driver->owner);
-
-	if (device->kvm) {
-		kvm_put_kvm(device->kvm);
-		device->kvm = NULL;
-	}
-	vfio_device_unassign_container(device);
-
 	vfio_device_put(device);
 
 	return 0;

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Patchwork]

== Series Details ==

Series: series starting with [1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM
URL   : https://patchwork.freedesktop.org/series/104101/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/104101/revisions/1/mbox/ not applied
Applying: vfio: remove VFIO_GROUP_NOTIFY_SET_KVM
error: sha1 information is lacking or useless (drivers/vfio/vfio.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 vfio: remove VFIO_GROUP_NOTIFY_SET_KVM
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Christoph Hellwig]

>  	if (device->ops->flags & VFIO_DEVICE_NEEDS_KVM)
>  	{

Nit: this is not the normal brace placement.

But what is you diff against anyway?  The one Matthew sent did away
with the VFIO_DEVICE_NEEDS_KVM flags, which does the wrong thing for
zpci, so it can't be that..

Also if we want to do major code movement, it really needs to go into
a separate patch or patches, as the combinations of all these moves
with actual code changes is almost unreadable.

Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Christoph Hellwig]

With this the release_work in gvt can go away as well:

diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 633acfcf76bf2..aee1a45da74bc 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -227,7 +227,6 @@ struct intel_vgpu {
 	struct mutex cache_lock;
 
 	struct notifier_block iommu_notifier;
-	struct work_struct release_work;
 	atomic_t released;
 
 	struct kvm_page_track_notifier_node track_node;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index b317ae4cc7d2d..917617d7599a9 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -228,8 +228,6 @@ static void intel_gvt_cleanup_vgpu_type_groups(struct intel_gvt *gvt)
 	}
 }
 
-static void intel_vgpu_release_work(struct work_struct *work);
-
 static void gvt_unpin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn,
 		unsigned long size)
 {
@@ -850,8 +848,9 @@ static void intel_vgpu_release_msi_eventfd_ctx(struct intel_vgpu *vgpu)
 	}
 }
 
-static void __intel_vgpu_release(struct intel_vgpu *vgpu)
+static void intel_vgpu_close_device(struct vfio_device *vfio_dev)
 {
+	struct intel_vgpu *vgpu = vfio_dev_to_vgpu(vfio_dev);
 	struct drm_i915_private *i915 = vgpu->gvt->gt->i915;
 	int ret;
 
@@ -880,19 +879,6 @@ static void __intel_vgpu_release(struct intel_vgpu *vgpu)
 	vgpu->attached = false;
 }
 
-static void intel_vgpu_close_device(struct vfio_device *vfio_dev)
-{
-	__intel_vgpu_release(vfio_dev_to_vgpu(vfio_dev));
-}
-
-static void intel_vgpu_release_work(struct work_struct *work)
-{
-	struct intel_vgpu *vgpu =
-		container_of(work, struct intel_vgpu, release_work);
-
-	__intel_vgpu_release(vgpu);
-}
-
 static u64 intel_vgpu_get_bar_addr(struct intel_vgpu *vgpu, int bar)
 {
 	u32 start_lo, start_hi;
@@ -1639,7 +1625,6 @@ static int intel_vgpu_probe(struct mdev_device *mdev)
 		return PTR_ERR(vgpu);
 	}
 
-	INIT_WORK(&vgpu->release_work, intel_vgpu_release_work);
 	vfio_init_group_dev(&vgpu->vfio_device, &mdev->dev,
 			    &intel_vgpu_dev_ops);
 

Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Jason Gunthorpe]

On Wed, May 18, 2022 at 12:39:41AM -0700, Christoph Hellwig wrote:
> >  	if (device->ops->flags & VFIO_DEVICE_NEEDS_KVM)
> >  	{
> 
> Nit: this is not the normal brace placement.
> 
> But what is you diff against anyway?  The one Matthew sent did away
> with the VFIO_DEVICE_NEEDS_KVM flags, which does the wrong thing for
> zpci, so it can't be that..

Against what I sent before, I did this before Matthew sent his

> Also if we want to do major code movement, it really needs to go into
> a separate patch or patches, as the combinations of all these moves
> with actual code changes is almost unreadable.

Sure, just checking how things could look at this point

Matthew's version first followed by the code motion is probably a good
approach.

Jason

Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Jason Gunthorpe]

On Wed, May 18, 2022 at 10:37:48AM -0400, Matthew Rosato wrote:
> On 5/17/22 2:08 PM, Matthew Rosato wrote:
> > Rather than relying on a notifier for associating the KVM with
> > the group, let's assume that the association has already been
> > made prior to device_open.  The first time a device is opened
> > associate the group KVM with the device.
> > 
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> ...
> 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index cfcff7764403..c5d421eda275 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -10,6 +10,7 @@
> >    * Author: Tom Lyon, pugs@cisco.com
> >    */
> > +#include "linux/kvm_host.h"
> >   #include <linux/cdev.h>
> >   #include <linux/compat.h>
> >   #include <linux/device.h>
> > @@ -1083,6 +1084,13 @@ static struct file *vfio_device_open(struct vfio_device *device)
> >   	mutex_lock(&device->dev_set->lock);
> >   	device->open_count++;
> > +	down_write(&device->group->group_rwsem);
> > +	if (device->open_count == 1 && device->group->kvm) {
> > +		device->kvm = device->group->kvm;
> > +		kvm_get_kvm(device->kvm);
> 
> Did some more compile testing, since vfio has no hard kvm dependency,
> kvm_get_kvm and kvm_put_kvm are an issue if KVM is a module while vfio is
> built-in...

Ugh, my other plan was to have the driver itself capture the kvm, ie
we lock the group_rwsem to keep the group->kvm valid and then pass the
kvm to open_device in some way, then the driver can kvm_get_kvm() it

Alternatively, I don't know why kvm_get_kvm() is an exported symbol
when it is just calling refcount_inc() - inlining it would be an
improvement I think.

Jason

Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Jason Gunthorpe]

On Wed, May 18, 2022 at 11:33:37AM -0400, Matthew Rosato wrote:
> On 5/18/22 11:12 AM, Jason Gunthorpe wrote:
> > On Wed, May 18, 2022 at 10:37:48AM -0400, Matthew Rosato wrote:
> > > On 5/17/22 2:08 PM, Matthew Rosato wrote:
> > > > Rather than relying on a notifier for associating the KVM with
> > > > the group, let's assume that the association has already been
> > > > made prior to device_open.  The first time a device is opened
> > > > associate the group KVM with the device.
> > > > 
> > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > > 
> > > ...
> > > 
> > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > > > index cfcff7764403..c5d421eda275 100644
> > > > +++ b/drivers/vfio/vfio.c
> > > > @@ -10,6 +10,7 @@
> > > >     * Author: Tom Lyon, pugs@cisco.com
> > > >     */
> > > > +#include "linux/kvm_host.h"
> > > >    #include <linux/cdev.h>
> > > >    #include <linux/compat.h>
> > > >    #include <linux/device.h>
> > > > @@ -1083,6 +1084,13 @@ static struct file *vfio_device_open(struct vfio_device *device)
> > > >    	mutex_lock(&device->dev_set->lock);
> > > >    	device->open_count++;
> > > > +	down_write(&device->group->group_rwsem);
> > > > +	if (device->open_count == 1 && device->group->kvm) {
> > > > +		device->kvm = device->group->kvm;
> > > > +		kvm_get_kvm(device->kvm);
> > > 
> > > Did some more compile testing, since vfio has no hard kvm dependency,
> > > kvm_get_kvm and kvm_put_kvm are an issue if KVM is a module while vfio is
> > > built-in...
> > 
> > Ugh, my other plan was to have the driver itself capture the kvm, ie
> > we lock the group_rwsem to keep the group->kvm valid and then pass the
> > kvm to open_device in some way, then the driver can kvm_get_kvm() it
> > 
> 
> Hrm... If we did that we would have to re-evaluate some other usage of the
> rwsem e.g. if driver open_device calls vfio_register_iommu_notifier it will
> try to get the rwsem but it's already locked.

Ugh, yes, it means removing the other notifier callback too, which I
was expecting to do as well

Maybe we could split the lock for just this patch though.

> > Alternatively, I don't know why kvm_get_kvm() is an exported symbol
> > when it is just calling refcount_inc() - inlining it would be an
> > improvement I think.
> 
> I think that would work for kvm_get_kvm, but kvm_put_kvm (which we also
> need) calls kvm_destroy_kvm after the refcount_dec and that can't be inlined

Indeed.

Jason