diff mbox series

vfio: Remove vfio_group dev_counter

Message ID 0-v1-efa4029ed93d+22-vfio_dev_counter_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series vfio: Remove vfio_group dev_counter | expand

Commit Message

Jason Gunthorpe Aug. 15, 2022, 4:50 p.m. UTC 
This counts the number of devices attached to a vfio_group, ie the number
of items in the group->device_list.

It is only read in vfio_pin_pages(), however that function already does
vfio_assert_device_open(). Given an opened device has to already be
properly setup with a group, this test and variable are redundant. Remove
it.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio_main.c | 6 ------
 1 file changed, 6 deletions(-)


base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868

Comments

Liu, Yi L Aug. 16, 2022, 1:21 a.m. UTC | #1 
Hi Jason,

On 2022/8/16 00:50, Jason Gunthorpe wrote:
> This counts the number of devices attached to a vfio_group, ie the number
> of items in the group->device_list.

yes. This dev_counter is added to ensure only singleton vfio group supports
pin page. Although I don't think it is a good approach as it only counts 
the registered devices.

https://lore.kernel.org/kvm/1584035607-23166-8-git-send-email-kwankhede@nvidia.com/

> It is only read in vfio_pin_pages(), however that function already does
> vfio_assert_device_open(). Given an opened device has to already be
> properly setup with a group, this test and variable are redundant. Remove
> it.

But still have a doubt on your change. The vfio_assert_device_open() means
the input vfio_device is properly set up with a group. But it doesn't mean
the group is singleton. right? So your removal is not an apple to apple
replacement so far. Did I miss anything here? Perhaps this removal is good
cleanup but may be done with a different claim. :-)

> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/vfio_main.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 7cb56c382c97a2..76a73890d853e6 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -74,7 +74,6 @@ struct vfio_group {
>   	struct list_head		vfio_next;
>   	struct list_head		container_next;
>   	enum vfio_group_type		type;
> -	unsigned int			dev_counter;
>   	struct rw_semaphore		group_rwsem;
>   	struct kvm			*kvm;
>   	struct file			*opened_file;
> @@ -608,7 +607,6 @@ static int __vfio_register_dev(struct vfio_device *device,
>   
>   	mutex_lock(&group->device_lock);
>   	list_add(&device->group_next, &group->device_list);
> -	group->dev_counter++;
>   	mutex_unlock(&group->device_lock);
>   
>   	return 0;
> @@ -696,7 +694,6 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>   
>   	mutex_lock(&group->device_lock);
>   	list_del(&device->group_next);
> -	group->dev_counter--;
>   	mutex_unlock(&group->device_lock);
>   
>   	if (group->type == VFIO_NO_IOMMU || group->type == VFIO_EMULATED_IOMMU)
> @@ -1961,9 +1958,6 @@ int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,
>   	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
>   		return -E2BIG;
>   
> -	if (group->dev_counter > 1)
> -		return -EINVAL;
> -
>   	/* group->container cannot change while a vfio device is open */
>   	container = group->container;
>   	driver = container->iommu_driver;
> 
> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
Jason Gunthorpe Aug. 16, 2022, 12:29 p.m. UTC | #2 
On Tue, Aug 16, 2022 at 09:21:05AM +0800, Yi Liu wrote:
> Hi Jason,
> 
> On 2022/8/16 00:50, Jason Gunthorpe wrote:
> > This counts the number of devices attached to a vfio_group, ie the number
> > of items in the group->device_list.
> 
> yes. This dev_counter is added to ensure only singleton vfio group supports
> pin page. Although I don't think it is a good approach as it only counts the
> registered devices.

Ah, I missed explaining this, lets try again:

vfio: Remove vfio_group dev_counter

This counts the number of devices attached to a vfio_group, ie the number
of items in the group->device_list.

It has two purposes
 - To ensure the vfio_device is opened
 - To assert the group is singleton because the dirty tracking code in the
   type1 iommu has limitations

However, vfio_pin_pages() already calls vfio_assert_device_open() so the
first is taken care of, and all callers of vfio_pin_pages() use now use
vfio_register_emulated_iommu_dev() which guarentees single groups by
constrution.

So delete dev_counter and leave a note that vfio_pin_pages() can only be
used with vfio_register_emulated_iommu_dev().

Jason
Liu, Yi L Aug. 16, 2022, 3:12 p.m. UTC | #3 
On 2022/8/16 20:29, Jason Gunthorpe wrote:
> On Tue, Aug 16, 2022 at 09:21:05AM +0800, Yi Liu wrote:
>> Hi Jason,
>>
>> On 2022/8/16 00:50, Jason Gunthorpe wrote:
>>> This counts the number of devices attached to a vfio_group, ie the number
>>> of items in the group->device_list.
>>
>> yes. This dev_counter is added to ensure only singleton vfio group supports
>> pin page. Although I don't think it is a good approach as it only counts the
>> registered devices.
> 
> Ah, I missed explaining this, lets try again:
> 
> vfio: Remove vfio_group dev_counter
> 
> This counts the number of devices attached to a vfio_group, ie the number
> of items in the group->device_list.
> 
> It has two purposes
>   - To ensure the vfio_device is opened

hmmm, surely vfio_pin_pages() needs to ensure vfio_device is opened.
But it's not the purpose of adding dev_counter. So I guess this line
can be removed. :)

>   - To assert the group is singleton because the dirty tracking code in the
>     type1 iommu has limitations
> 
> However, vfio_pin_pages() already calls vfio_assert_device_open() so the
> first is taken care of, and all callers of vfio_pin_pages() use now use
> vfio_register_emulated_iommu_dev() which guarentees single groups by
> constrution.
> 
> So delete dev_counter and leave a note that vfio_pin_pages() can only be
> used with vfio_register_emulated_iommu_dev().

oh, yes. The fake group is allocated in vfio_register_emulated_iommu_dev().
So the group is surely singleton. Maybe for simple just say
vfio_register_emulated_iommu_dev() guarantees the group is singleton, and
vfio_pin_pages() can only be used with vfio_register_emulated_iommu_dev().
So no need to use dev_counter in vfio_pin_pages(). Such commit message may
be more accurate and no unnecessary distraction. :-) btw. The change looks
good to me.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>
Tian, Kevin Aug. 18, 2022, 7:46 a.m. UTC | #4 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, August 16, 2022 12:50 AM
> 
> This counts the number of devices attached to a vfio_group, ie the number
> of items in the group->device_list.
> 
> It is only read in vfio_pin_pages(), however that function already does
> vfio_assert_device_open(). Given an opened device has to already be
> properly setup with a group, this test and variable are redundant. Remove
> it.

I didn't get the rationale behind. The original check was for whether
the group is singleton. Why is it equivalent to the condition of an
opened device?

Though I do think this check is unnecessary. All the devices in the group
share the container and iommu domain which is what the pinning
operation applies to. I'm not sure why the singleton restriction was
added in the first place.

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio_main.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 7cb56c382c97a2..76a73890d853e6 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -74,7 +74,6 @@ struct vfio_group {
>  	struct list_head		vfio_next;
>  	struct list_head		container_next;
>  	enum vfio_group_type		type;
> -	unsigned int			dev_counter;
>  	struct rw_semaphore		group_rwsem;
>  	struct kvm			*kvm;
>  	struct file			*opened_file;
> @@ -608,7 +607,6 @@ static int __vfio_register_dev(struct vfio_device
> *device,
> 
>  	mutex_lock(&group->device_lock);
>  	list_add(&device->group_next, &group->device_list);
> -	group->dev_counter++;
>  	mutex_unlock(&group->device_lock);
> 
>  	return 0;
> @@ -696,7 +694,6 @@ void vfio_unregister_group_dev(struct vfio_device
> *device)
> 
>  	mutex_lock(&group->device_lock);
>  	list_del(&device->group_next);
> -	group->dev_counter--;
>  	mutex_unlock(&group->device_lock);
> 
>  	if (group->type == VFIO_NO_IOMMU || group->type ==
> VFIO_EMULATED_IOMMU)
> @@ -1961,9 +1958,6 @@ int vfio_pin_pages(struct vfio_device *device,
> dma_addr_t iova,
>  	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
>  		return -E2BIG;
> 
> -	if (group->dev_counter > 1)
> -		return -EINVAL;
> -
>  	/* group->container cannot change while a vfio device is open */
>  	container = group->container;
>  	driver = container->iommu_driver;
> 
> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> --
> 2.37.2
Liu, Yi L Aug. 18, 2022, 8:12 a.m. UTC | #5 
Hi Kevin,

On 2022/8/18 15:46, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Tuesday, August 16, 2022 12:50 AM
>>
>> This counts the number of devices attached to a vfio_group, ie the number
>> of items in the group->device_list.
>>
>> It is only read in vfio_pin_pages(), however that function already does
>> vfio_assert_device_open(). Given an opened device has to already be
>> properly setup with a group, this test and variable are redundant. Remove
>> it.
> 
> I didn't get the rationale behind. The original check was for whether
> the group is singleton. Why is it equivalent to the condition of an
> opened device?
> 
> Though I do think this check is unnecessary. All the devices in the group
> share the container and iommu domain which is what the pinning
> operation applies to. I'm not sure why the singleton restriction was
> added in the first place.

see if your confusion is addressed in below link?

https://lore.kernel.org/kvm/BN9PR11MB5276F9EB0295CBD485A58D308C6D9@BN9PR11MB5276.namprd11.prod.outlook.com/T/#m42eaf1548235810820d7e2ad1491092c4b0bbcba
Tian, Kevin Aug. 18, 2022, 8:19 a.m. UTC | #6 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, August 18, 2022 4:13 PM
> 
> Hi Kevin,
> 
> On 2022/8/18 15:46, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@nvidia.com>
> >> Sent: Tuesday, August 16, 2022 12:50 AM
> >>
> >> This counts the number of devices attached to a vfio_group, ie the
> number
> >> of items in the group->device_list.
> >>
> >> It is only read in vfio_pin_pages(), however that function already does
> >> vfio_assert_device_open(). Given an opened device has to already be
> >> properly setup with a group, this test and variable are redundant. Remove
> >> it.
> >
> > I didn't get the rationale behind. The original check was for whether
> > the group is singleton. Why is it equivalent to the condition of an
> > opened device?
> >
> > Though I do think this check is unnecessary. All the devices in the group
> > share the container and iommu domain which is what the pinning
> > operation applies to. I'm not sure why the singleton restriction was
> > added in the first place.
> 
> see if your confusion is addressed in below link?
> 
> https://lore.kernel.org/kvm/BN9PR11MB5276F9EB0295CBD485A58D308C6D
> 9@BN9PR11MB5276.namprd11.prod.outlook.com/T/#m42eaf154823581082
> 0d7e2ad1491092c4b0bbcba
> 

Ah, yes. I didn't note that this is already addressed.
diff mbox series

Patch

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 7cb56c382c97a2..76a73890d853e6 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -74,7 +74,6 @@  struct vfio_group {
 	struct list_head		vfio_next;
 	struct list_head		container_next;
 	enum vfio_group_type		type;
-	unsigned int			dev_counter;
 	struct rw_semaphore		group_rwsem;
 	struct kvm			*kvm;
 	struct file			*opened_file;
@@ -608,7 +607,6 @@  static int __vfio_register_dev(struct vfio_device *device,
 
 	mutex_lock(&group->device_lock);
 	list_add(&device->group_next, &group->device_list);
-	group->dev_counter++;
 	mutex_unlock(&group->device_lock);
 
 	return 0;
@@ -696,7 +694,6 @@  void vfio_unregister_group_dev(struct vfio_device *device)
 
 	mutex_lock(&group->device_lock);
 	list_del(&device->group_next);
-	group->dev_counter--;
 	mutex_unlock(&group->device_lock);
 
 	if (group->type == VFIO_NO_IOMMU || group->type == VFIO_EMULATED_IOMMU)
@@ -1961,9 +1958,6 @@  int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,
 	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
 		return -E2BIG;
 
-	if (group->dev_counter > 1)
-		return -EINVAL;
-
 	/* group->container cannot change while a vfio device is open */
 	container = group->container;
 	driver = container->iommu_driver;