Re: [PATCH v2 08/14] vfio/pci: Move to the device set infrastructure
From: Christoph Hellwig <hch@lst.de>
Date: 2021-07-23 07:47:54
Also in:
intel-gfx, linux-doc, linux-s390
On Tue, Jul 20, 2021 at 02:42:54PM -0300, Jason Gunthorpe wrote:
quoted hunk ↗ jump to hunk
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>