[Intel-gfx] [PATCH v12 18/24] vfio: Add VFIO_DEVICE_BIND_IOMMUFD

[Liu, Yi L]

> From: Liu, Yi L <yi.l.liu at intel.com>
> Sent: Monday, June 26, 2023 9:35 PM
> 
> > From: Jason Gunthorpe <jgg at nvidia.com>
> > Sent: Monday, June 26, 2023 8:56 PM
> >
> > On Mon, Jun 26, 2023 at 08:34:26AM +0000, Liu, Yi L wrote:
> > > > From: Jason Gunthorpe <jgg at nvidia.com>
> > > > Sent: Saturday, June 24, 2023 12:15 AM
> > >
> > > > >  }
> > > > >
> > > > > +static void vfio_device_get_kvm_safe(struct vfio_device_file *df)
> > > > > +{
> > > > > +	spin_lock(&df->kvm_ref_lock);
> > > > > +	if (df->kvm)
> > > > > +		_vfio_device_get_kvm_safe(df->device, df->kvm);
> > > > > +	spin_unlock(&df->kvm_ref_lock);
> > > > > +}
> > > >
> > > > I'm surprised symbol_get() can be called from a spinlock, but it sure
> > > > looks like it can..
> > > >
> > > > Also moving the if kvm is null test into _vfio_device_get_kvm_safe()
> > > > will save a few lines.
> > > >
> > > > Also shouldn't be called _vfio_device...
> > >
> > > Ah, any suggestion on the naming? How about vfio_device_get_kvm_safe_locked()?
> >
> > I thought you were using _df_ now for these functions?
> >
> 
> I see. Your point is passing df to _vfio_device_get_kvm_safe(() and
> test the df->kvm within it.  Hence rename it to be _df_. I think group
> path should be ok with this change as well. Let me make it.

To pass vfio_device_file to _vfio_device_get_kvm_safe(), needs to make
the below changes to the group path. If just wants to test null kvm in the
_vfio_device_get_kvm_safe(), maybe simpler to keep the current parameters
and just move the null kvm test within this function. Is it?

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index 41a09a2df690..c2e880c15c44 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -157,15 +157,15 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
 	return ret;
 }
 
-static void vfio_device_group_get_kvm_safe(struct vfio_device *device)
+static void vfio_device_group_get_kvm_safe(struct vfio_device_file *df)
 {
-	spin_lock(&device->group->kvm_ref_lock);
-	if (!device->group->kvm)
-		goto unlock;
-
-	_vfio_device_get_kvm_safe(device, device->group->kvm);
+	struct vfio_device *device = df->device;
 
-unlock:
+	spin_lock(&device->group->kvm_ref_lock);
+	spin_lock(&df->kvm_ref_lock);
+	df->kvm = device->group->kvm;
+	_vfio_df_get_kvm_safe(df);
+	spin_unlock(&df->kvm_ref_lock);
 	spin_unlock(&device->group->kvm_ref_lock);
 }
 
@@ -189,7 +189,7 @@ static int vfio_df_group_open(struct vfio_device_file *df)
 	 * the pointer in the device for use by drivers.
 	 */
 	if (device->open_count == 0)
-		vfio_device_group_get_kvm_safe(device);
+		vfio_device_group_get_kvm_safe(df);
 
 	df->iommufd = device->group->iommufd;
 	if (df->iommufd && vfio_device_is_noiommu(device) && device->open_count == 0) {
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index fb8f2fac3d23..066766d43bdc 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -340,11 +340,10 @@ enum { vfio_noiommu = false };
 #endif
 
 #ifdef CONFIG_HAVE_KVM
-void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm);
+void _vfio_df_get_kvm_safe(struct vfio_device_file *df);
 void vfio_device_put_kvm(struct vfio_device *device);
 #else
-static inline void _vfio_device_get_kvm_safe(struct vfio_device *device,
-					     struct kvm *kvm)
+static inline void _vfio_df_get_kvm_safe(struct vfio_device_file *df)
 {
 }
 
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 8a9ebcc6980b..4e6ea2943d28 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -373,14 +373,22 @@ void vfio_unregister_group_dev(struct vfio_device *device)
 EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
 
 #ifdef CONFIG_HAVE_KVM
-void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
+void _vfio_df_get_kvm_safe(struct vfio_device_file *df)
 {
+	struct vfio_device *device = df->device;
 	void (*pfn)(struct kvm *kvm);
 	bool (*fn)(struct kvm *kvm);
+	struct kvm *kvm;
 	bool ret;
 
+	lockdep_assert_held(&df->kvm_ref_lock);
 	lockdep_assert_held(&device->dev_set->lock);
 
+	kvm = df->kvm;
+
+	if (!kvm)
+		return;
+
 	pfn = symbol_get(kvm_put_kvm);
 	if (WARN_ON(!pfn))
 		return;