Re: [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 26 Aug 2021 15:34:14 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Factor out a helper to find or allocate the vfio_group to reduce the
> spagetthi code in vfio_register_group_dev a little.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 59 ++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 18e4c7906d1b3f..852fe22125520d 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -823,10 +823,38 @@ void vfio_uninit_group_dev(struct vfio_device *device)
>  }
>  EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
>  
> +struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
> +{
> +	struct iommu_group *iommu_group;
> +	struct vfio_group *group;
> +
> +	iommu_group = vfio_iommu_group_get(dev);
> +	if (!iommu_group)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* a found vfio_group already holds a reference to the iommu_group */
> +	group = vfio_group_get_from_iommu(iommu_group);
> +	if (group)
> +		goto out_put;
> +
> +	/* a newly created vfio_group keeps the reference. */
> +	group = vfio_create_group(iommu_group);
> +	if (IS_ERR(group))
> +		goto out_put;
> +	return group;
> +
> +out_put:
> +#ifdef CONFIG_VFIO_NOIOMMU
> +	if (iommu_group_get_iommudata(iommu_group) == &noiommu)
> +		iommu_group_remove_device(dev);
> +#endif

When we get here via the first goto above, it doesn't match the code
we're removing below.  I stared at the below logic from patch 01 for a
while and came to the conclusion that the only way we take that else
branch is if the iommu_group already existed and was not created
because how else could we find a vfio group for that iommu group
otherwise?  Therefore we only put the iommu group reference without
removing the device, because we didn't add the device.

The above assumes we created the iommu group and added the device.  So
I think there needs to be a separate goto target below that's reached in
place of the first goto above.  Thanks,

Alex

> +	iommu_group_put(iommu_group);
> +	return group;
> +}
> +
>  int vfio_register_group_dev(struct vfio_device *device)
>  {
>  	struct vfio_device *existing_device;
> -	struct iommu_group *iommu_group;
>  	struct vfio_group *group;
>  
>  	/*
> @@ -836,36 +864,17 @@ int vfio_register_group_dev(struct vfio_device *device)
>  	if (!device->dev_set)
>  		vfio_assign_device_set(device, device);
>  
> -	iommu_group = vfio_iommu_group_get(device->dev);
> -	if (!iommu_group)
> -		return -EINVAL;
> -
> -	group = vfio_group_get_from_iommu(iommu_group);
> -	if (!group) {
> -		group = vfio_create_group(iommu_group);
> -		if (IS_ERR(group)) {
> -#ifdef CONFIG_VFIO_NOIOMMU
> -			if (iommu_group_get_iommudata(iommu_group) == &noiommu)
> -				iommu_group_remove_device(device->dev);
> -#endif
> -			iommu_group_put(iommu_group);
> -			return PTR_ERR(group);
> -		}
> -	} else {
> -		/*
> -		 * A found vfio_group already holds a reference to the
> -		 * iommu_group.  A created vfio_group keeps the reference.
> -		 */
> -		iommu_group_put(iommu_group);
> -	}
> +	group = vfio_group_find_or_alloc(device->dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
>  
>  	existing_device = vfio_group_get_device(group, device->dev);
>  	if (existing_device) {
>  		dev_WARN(device->dev, "Device already exists on group %d\n",
> -			 iommu_group_id(iommu_group));
> +			 iommu_group_id(group->iommu_group));
>  		vfio_device_put(existing_device);
>  #ifdef CONFIG_VFIO_NOIOMMU
> -		if (iommu_group_get_iommudata(iommu_group) == &noiommu)
> +		if (iommu_group_get_iommudata(group->iommu_group) == &noiommu)
>  			iommu_group_remove_device(device->dev);
>  #endif
>  		vfio_group_put(group);