Re: [Intel-gfx] [PATCH v2 08/14] vfio/pci: Move to the device set infrastructure

[Christoph Hellwig <hch@lst.de>]

On Tue, Jul 20, 2021 at 02:42:54PM -0300, Jason Gunthorpe wrote:
> From: Yishai Hadas <yishaih@nvidia.com>
> 
> PCI wants to have the usual open/close_device() logic with the slight
> twist that the open/close_device() must be done under a singelton lock
> shared by all of the vfio_devices that are in the PCI "reset group".
> 
> The reset group, and thus the device set, is determined by what devices
> pci_reset_bus() touches, which is either the entire bus or only the slot.
> 
> Rely on the core code to do everything reflck was doing and delete reflck
> entirely.
> 
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c         | 156 ++++++----------------------
>  drivers/vfio/pci/vfio_pci_private.h |   7 --
>  2 files changed, 31 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index fab3715d60d4ba..22774e447b5f4a 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -530,53 +530,40 @@ static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
>  	vfio_device_put(&pf_vdev->vdev);
>  }
>  
> -static void vfio_pci_release(struct vfio_device *core_vdev)
> +static void vfio_pci_close_device(struct vfio_device *core_vdev)
>  {
>  	struct vfio_pci_device *vdev =
>  		container_of(core_vdev, struct vfio_pci_device, vdev);
>  
> -	mutex_lock(&vdev->reflck->lock);
> -
> -	if (!(--vdev->refcnt)) {
> -		vfio_pci_vf_token_user_add(vdev, -1);
> -		vfio_spapr_pci_eeh_release(vdev->pdev);
> -		vfio_pci_disable(vdev);
> +	vfio_pci_vf_token_user_add(vdev, -1);
> +	vfio_spapr_pci_eeh_release(vdev->pdev);
> +	vfio_pci_disable(vdev);
>  
> -		mutex_lock(&vdev->igate);
> -		if (vdev->err_trigger) {
> -			eventfd_ctx_put(vdev->err_trigger);
> -			vdev->err_trigger = NULL;
> -		}
> -		if (vdev->req_trigger) {
> -			eventfd_ctx_put(vdev->req_trigger);
> -			vdev->req_trigger = NULL;
> -		}
> -		mutex_unlock(&vdev->igate);
> +	mutex_lock(&vdev->igate);
> +	if (vdev->err_trigger) {
> +		eventfd_ctx_put(vdev->err_trigger);
> +		vdev->err_trigger = NULL;
>  	}
> -
> -	mutex_unlock(&vdev->reflck->lock);
> +	if (vdev->req_trigger) {
> +		eventfd_ctx_put(vdev->req_trigger);
> +		vdev->req_trigger = NULL;
> +	}
> +	mutex_unlock(&vdev->igate);
>  }
>  
> -static int vfio_pci_open(struct vfio_device *core_vdev)
> +static int vfio_pci_open_device(struct vfio_device *core_vdev)
>  {
>  	struct vfio_pci_device *vdev =
>  		container_of(core_vdev, struct vfio_pci_device, vdev);
>  	int ret = 0;
>  
> -	mutex_lock(&vdev->reflck->lock);
> -
> -	if (!vdev->refcnt) {
> -		ret = vfio_pci_enable(vdev);
> -		if (ret)
> -			goto error;
> +	ret = vfio_pci_enable(vdev);
> +	if (ret)
> +		return ret;
>  
> -		vfio_spapr_pci_eeh_open(vdev->pdev);
> -		vfio_pci_vf_token_user_add(vdev, 1);
> -	}
> -	vdev->refcnt++;
> -error:
> -	mutex_unlock(&vdev->reflck->lock);
> -	return ret;
> +	vfio_spapr_pci_eeh_open(vdev->pdev);
> +	vfio_pci_vf_token_user_add(vdev, 1);
> +	return 0;
>  }
>  
>  static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
> @@ -1870,8 +1857,8 @@ static int vfio_pci_match(struct vfio_device *core_vdev, char *buf)
>  
>  static const struct vfio_device_ops vfio_pci_ops = {
>  	.name		= "vfio-pci",
> -	.open		= vfio_pci_open,
> -	.release	= vfio_pci_release,
> +	.open_device	= vfio_pci_open_device,
> +	.close_device	= vfio_pci_close_device,
>  	.ioctl		= vfio_pci_ioctl,
>  	.read		= vfio_pci_read,
>  	.write		= vfio_pci_write,
> @@ -1880,9 +1867,6 @@ static const struct vfio_device_ops vfio_pci_ops = {
>  	.match		= vfio_pci_match,
>  };
>  
> -static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
> -static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
> -
>  static int vfio_pci_bus_notifier(struct notifier_block *nb,
>  				 unsigned long action, void *data)
>  {
> @@ -2020,12 +2004,17 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	INIT_LIST_HEAD(&vdev->vma_list);
>  	init_rwsem(&vdev->memory_lock);
>  
> -	ret = vfio_pci_reflck_attach(vdev);
> +	if (pci_is_root_bus(pdev->bus))
> +		ret = vfio_assign_device_set(&vdev->vdev, vdev);
> +	else if (!pci_probe_reset_slot(pdev->slot))
> +		ret = vfio_assign_device_set(&vdev->vdev, pdev->slot);
> +	else
> +		ret = vfio_assign_device_set(&vdev->vdev, pdev->bus);

Maybe invert this and add a comment:

	if (pci_is_root_bus(pdev->bus))
		ret = vfio_assign_device_set(&vdev->vdev, vdev);
	/*
	 * If there is no slot reset support for this device, the whole
	 * bus needs to be grouped together to support bus-wide resets.
	 */
	else if (pci_probe_reset_slot(pdev->slot) < 0)
		ret = vfio_assign_device_set(&vdev->vdev, pdev->bus);
	else
		ret = vfio_assign_device_set(&vdev->vdev, pdev->slot);

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx