diff mbox series

[v2,08/14] vfio/pci: Move to the device set infrastructure

Message ID 8-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Provide core infrastructure for managing open/release | expand

Commit Message

Jason Gunthorpe July 20, 2021, 5:42 p.m. UTC 
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(-)

Comments

Christoph Hellwig July 23, 2021, 7:47 a.m. UTC | #1 
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>
Jason Gunthorpe July 23, 2021, 12:59 p.m. UTC | #2 
On Fri, Jul 23, 2021 at 09:47:49AM +0200, Christoph Hellwig wrote:
> > @@ -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.
> 	 */

I like the comment

> 	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);

The consensus idiom here is the !:

drivers/pci/pci.c:      return (!pci_probe_reset_slot(pdev->slot)) ?
drivers/vfio/pci/vfio_pci.c:            if (!pci_probe_reset_slot(vdev->pdev->slot))
drivers/vfio/pci/vfio_pci.c:            if (!pci_probe_reset_slot(vdev->pdev->slot))
drivers/vfio/pci/vfio_pci.c:    bool slot = !pci_probe_reset_slot(vdev->pdev->slot);
drivers/vfio/pci/vfio_pci.c:    if (!pci_probe_reset_slot(vdev->pdev->slot))

It reads quite poorly either way, IMHO, I'd rather switch it to a
readable bool, but not for this series

Thanks,
Jason
diff mbox series

Patch

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);
 	if (ret)
 		goto out_uninit;
 	ret = vfio_pci_vf_init(vdev);
 	if (ret)
-		goto out_reflck;
+		goto out_uninit;
 	ret = vfio_pci_vga_init(vdev);
 	if (ret)
 		goto out_vf;
@@ -2057,8 +2046,6 @@  static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		vfio_pci_set_power_state(vdev, PCI_D0);
 out_vf:
 	vfio_pci_vf_uninit(vdev);
-out_reflck:
-	vfio_pci_reflck_put(vdev->reflck);
 out_uninit:
 	vfio_uninit_group_dev(&vdev->vdev);
 	kfree(vdev->pm_save);
@@ -2077,7 +2064,6 @@  static void vfio_pci_remove(struct pci_dev *pdev)
 	vfio_unregister_group_dev(&vdev->vdev);
 
 	vfio_pci_vf_uninit(vdev);
-	vfio_pci_reflck_put(vdev->reflck);
 	vfio_uninit_group_dev(&vdev->vdev);
 	vfio_pci_vga_uninit(vdev);
 
@@ -2153,86 +2139,6 @@  static struct pci_driver vfio_pci_driver = {
 	.err_handler		= &vfio_err_handlers,
 };
 
-static DEFINE_MUTEX(reflck_lock);
-
-static struct vfio_pci_reflck *vfio_pci_reflck_alloc(void)
-{
-	struct vfio_pci_reflck *reflck;
-
-	reflck = kzalloc(sizeof(*reflck), GFP_KERNEL);
-	if (!reflck)
-		return ERR_PTR(-ENOMEM);
-
-	kref_init(&reflck->kref);
-	mutex_init(&reflck->lock);
-
-	return reflck;
-}
-
-static void vfio_pci_reflck_get(struct vfio_pci_reflck *reflck)
-{
-	kref_get(&reflck->kref);
-}
-
-static int vfio_pci_reflck_find(struct pci_dev *pdev, void *data)
-{
-	struct vfio_pci_reflck **preflck = data;
-	struct vfio_device *device;
-	struct vfio_pci_device *vdev;
-
-	device = vfio_device_get_from_dev(&pdev->dev);
-	if (!device)
-		return 0;
-
-	if (pci_dev_driver(pdev) != &vfio_pci_driver) {
-		vfio_device_put(device);
-		return 0;
-	}
-
-	vdev = container_of(device, struct vfio_pci_device, vdev);
-
-	if (vdev->reflck) {
-		vfio_pci_reflck_get(vdev->reflck);
-		*preflck = vdev->reflck;
-		vfio_device_put(device);
-		return 1;
-	}
-
-	vfio_device_put(device);
-	return 0;
-}
-
-static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
-{
-	bool slot = !pci_probe_reset_slot(vdev->pdev->slot);
-
-	mutex_lock(&reflck_lock);
-
-	if (pci_is_root_bus(vdev->pdev->bus) ||
-	    vfio_pci_for_each_slot_or_bus(vdev->pdev, vfio_pci_reflck_find,
-					  &vdev->reflck, slot) <= 0)
-		vdev->reflck = vfio_pci_reflck_alloc();
-
-	mutex_unlock(&reflck_lock);
-
-	return PTR_ERR_OR_ZERO(vdev->reflck);
-}
-
-static void vfio_pci_reflck_release(struct kref *kref)
-{
-	struct vfio_pci_reflck *reflck = container_of(kref,
-						      struct vfio_pci_reflck,
-						      kref);
-
-	kfree(reflck);
-	mutex_unlock(&reflck_lock);
-}
-
-static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck)
-{
-	kref_put_mutex(&reflck->kref, vfio_pci_reflck_release, &reflck_lock);
-}
-
 static int vfio_pci_get_unused_devs(struct pci_dev *pdev, void *data)
 {
 	struct vfio_devices *devs = data;
@@ -2254,7 +2160,7 @@  static int vfio_pci_get_unused_devs(struct pci_dev *pdev, void *data)
 	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	/* Fault if the device is not unused */
-	if (vdev->refcnt) {
+	if (device->open_count) {
 		vfio_device_put(device);
 		return -EBUSY;
 	}
@@ -2303,7 +2209,7 @@  static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
  *  - At least one of the affected devices is marked dirty via
  *    needs_reset (such as by lack of FLR support)
  * Then attempt to perform that bus or slot reset.  Callers are required
- * to hold vdev->reflck->lock, protecting the bus/slot reset group from
+ * to hold vdev->dev_set->lock, protecting the bus/slot reset group from
  * concurrent opens.  A vfio_device reference is acquired for each device
  * to prevent unbinds during the reset operation.
  *
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 5a36272cecbf94..ae83c2eada3a64 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -83,11 +83,6 @@  struct vfio_pci_dummy_resource {
 	struct list_head	res_next;
 };
 
-struct vfio_pci_reflck {
-	struct kref		kref;
-	struct mutex		lock;
-};
-
 struct vfio_pci_vf_token {
 	struct mutex		lock;
 	uuid_t			uuid;
@@ -130,8 +125,6 @@  struct vfio_pci_device {
 	bool			needs_pm_restore;
 	struct pci_saved_state	*pci_saved_state;
 	struct pci_saved_state	*pm_save;
-	struct vfio_pci_reflck	*reflck;
-	int			refcnt;
 	int			ioeventfds_nr;
 	struct eventfd_ctx	*err_trigger;
 	struct eventfd_ctx	*req_trigger;