| Message ID | ca249ad459c07a36cd64137ee1cc107dcd8b6f4c.1706849424.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Remove duplicate code and logic from VFIO PCI interrupt management | expand |
On Thu, 1 Feb 2024 20:57:00 -0800 Reinette Chatre <reinette.chatre@intel.com> wrote: > vfio_pci_set_intx_trigger() and vfio_pci_set_msi_trigger() have similar > enough flows that can be converged into one shared flow instead of > duplicated. > > To share code between management of interrupt types it is necessary that > the type of the interrupt is only interpreted by the code specific to > the interrupt type being managed. > > Remove interrupt type interpretation from what will eventually be > shared code (vfio_pci_set_msi_trigger(), vfio_msi_set_block()) by > pushing this interpretation to be within the interrupt > type specific code (vfio_msi_enable(), > (temporary) vfio_msi_set_vector_signal()). > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > Note to maintainers: > Originally formed part of the IMS submission below, but is not > specific to IMS. > https://lore.kernel.org/lkml/cover.1696609476.git.reinette.chatre@intel.com > > drivers/vfio/pci/vfio_pci_intrs.c | 38 +++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 97a3bb22b186..31f73c70fcd2 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -346,16 +346,19 @@ static irqreturn_t vfio_msihandler(int irq, void *arg) > return IRQ_HANDLED; > } > > -static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msix) > +static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, > + unsigned int index) > { > struct pci_dev *pdev = vdev->pdev; > - unsigned int flag = msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI; > + unsigned int flag; > int ret; > u16 cmd; > > if (!is_irq_none(vdev)) > return -EINVAL; > > + flag = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? PCI_IRQ_MSIX : PCI_IRQ_MSI; > + > /* return the number of supported vectors if we can't get all: */ > cmd = vfio_pci_memory_lock_and_enable(vdev); > ret = pci_alloc_irq_vectors(pdev, 1, nvec, flag); > @@ -367,10 +370,9 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi > } > vfio_pci_memory_unlock_and_restore(vdev, cmd); > > - vdev->irq_type = msix ? VFIO_PCI_MSIX_IRQ_INDEX : > - VFIO_PCI_MSI_IRQ_INDEX; > + vdev->irq_type = index; > > - if (!msix) { > + if (index == VFIO_PCI_MSI_IRQ_INDEX) { > /* > * Compute the virtual hardware field for max msi vectors - > * it is the log base 2 of the number of vectors. > @@ -413,8 +415,10 @@ static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev, > } > > static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > - unsigned int vector, int fd, bool msix) > + unsigned int vector, int fd, > + unsigned int index) > { > + bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; Cleanup the unnecessary ternary while here, this can just be: bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX); Thanks, Alex > struct pci_dev *pdev = vdev->pdev; > struct vfio_pci_irq_ctx *ctx; > struct eventfd_ctx *trigger; > @@ -505,25 +509,26 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > > static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, > unsigned int start, unsigned int count, > - int32_t *fds, bool msix) > + int32_t *fds, unsigned int index) > { > unsigned int i, j; > int ret = 0; > > for (i = 0, j = start; i < count && !ret; i++, j++) { > int fd = fds ? fds[i] : -1; > - ret = vfio_msi_set_vector_signal(vdev, j, fd, msix); > + ret = vfio_msi_set_vector_signal(vdev, j, fd, index); > } > > if (ret) { > for (i = start; i < j; i++) > - vfio_msi_set_vector_signal(vdev, i, -1, msix); > + vfio_msi_set_vector_signal(vdev, i, -1, index); > } > > return ret; > } > > -static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) > +static void vfio_msi_disable(struct vfio_pci_core_device *vdev, > + unsigned int index) > { > struct pci_dev *pdev = vdev->pdev; > struct vfio_pci_irq_ctx *ctx; > @@ -533,7 +538,7 @@ static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) > xa_for_each(&vdev->ctx, i, ctx) { > vfio_virqfd_disable(&ctx->unmask); > vfio_virqfd_disable(&ctx->mask); > - vfio_msi_set_vector_signal(vdev, i, -1, msix); > + vfio_msi_set_vector_signal(vdev, i, -1, index); > } > > cmd = vfio_pci_memory_lock_and_enable(vdev); > @@ -656,10 +661,9 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev, > { > struct vfio_pci_irq_ctx *ctx; > unsigned int i; > - bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; > > if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) { > - vfio_msi_disable(vdev, msix); > + vfio_msi_disable(vdev, index); > return 0; > } > > @@ -672,15 +676,15 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev, > > if (vdev->irq_type == index) > return vfio_msi_set_block(vdev, start, count, > - fds, msix); > + fds, index); > > - ret = vfio_msi_enable(vdev, start + count, msix); > + ret = vfio_msi_enable(vdev, start + count, index); > if (ret) > return ret; > > - ret = vfio_msi_set_block(vdev, start, count, fds, msix); > + ret = vfio_msi_set_block(vdev, start, count, fds, index); > if (ret) > - vfio_msi_disable(vdev, msix); > + vfio_msi_disable(vdev, index); > > return ret; > }
Hi Alex, On 2/5/2024 2:35 PM, Alex Williamson wrote: > On Thu, 1 Feb 2024 20:57:00 -0800 > Reinette Chatre <reinette.chatre@intel.com> wrote: >> @@ -413,8 +415,10 @@ static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev, >> } >> >> static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, >> - unsigned int vector, int fd, bool msix) >> + unsigned int vector, int fd, >> + unsigned int index) >> { >> + bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; > > Cleanup the unnecessary ternary while here, this can just be: > > bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX); > Will do. Thank you. Reinette
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 97a3bb22b186..31f73c70fcd2 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -346,16 +346,19 @@ static irqreturn_t vfio_msihandler(int irq, void *arg) return IRQ_HANDLED; } -static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msix) +static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, + unsigned int index) { struct pci_dev *pdev = vdev->pdev; - unsigned int flag = msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI; + unsigned int flag; int ret; u16 cmd; if (!is_irq_none(vdev)) return -EINVAL; + flag = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? PCI_IRQ_MSIX : PCI_IRQ_MSI; + /* return the number of supported vectors if we can't get all: */ cmd = vfio_pci_memory_lock_and_enable(vdev); ret = pci_alloc_irq_vectors(pdev, 1, nvec, flag); @@ -367,10 +370,9 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi } vfio_pci_memory_unlock_and_restore(vdev, cmd); - vdev->irq_type = msix ? VFIO_PCI_MSIX_IRQ_INDEX : - VFIO_PCI_MSI_IRQ_INDEX; + vdev->irq_type = index; - if (!msix) { + if (index == VFIO_PCI_MSI_IRQ_INDEX) { /* * Compute the virtual hardware field for max msi vectors - * it is the log base 2 of the number of vectors. @@ -413,8 +415,10 @@ static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev, } static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, - unsigned int vector, int fd, bool msix) + unsigned int vector, int fd, + unsigned int index) { + bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; struct pci_dev *pdev = vdev->pdev; struct vfio_pci_irq_ctx *ctx; struct eventfd_ctx *trigger; @@ -505,25 +509,26 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned int start, unsigned int count, - int32_t *fds, bool msix) + int32_t *fds, unsigned int index) { unsigned int i, j; int ret = 0; for (i = 0, j = start; i < count && !ret; i++, j++) { int fd = fds ? fds[i] : -1; - ret = vfio_msi_set_vector_signal(vdev, j, fd, msix); + ret = vfio_msi_set_vector_signal(vdev, j, fd, index); } if (ret) { for (i = start; i < j; i++) - vfio_msi_set_vector_signal(vdev, i, -1, msix); + vfio_msi_set_vector_signal(vdev, i, -1, index); } return ret; } -static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) +static void vfio_msi_disable(struct vfio_pci_core_device *vdev, + unsigned int index) { struct pci_dev *pdev = vdev->pdev; struct vfio_pci_irq_ctx *ctx; @@ -533,7 +538,7 @@ static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) xa_for_each(&vdev->ctx, i, ctx) { vfio_virqfd_disable(&ctx->unmask); vfio_virqfd_disable(&ctx->mask); - vfio_msi_set_vector_signal(vdev, i, -1, msix); + vfio_msi_set_vector_signal(vdev, i, -1, index); } cmd = vfio_pci_memory_lock_and_enable(vdev); @@ -656,10 +661,9 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev, { struct vfio_pci_irq_ctx *ctx; unsigned int i; - bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) { - vfio_msi_disable(vdev, msix); + vfio_msi_disable(vdev, index); return 0; } @@ -672,15 +676,15 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev, if (vdev->irq_type == index) return vfio_msi_set_block(vdev, start, count, - fds, msix); + fds, index); - ret = vfio_msi_enable(vdev, start + count, msix); + ret = vfio_msi_enable(vdev, start + count, index); if (ret) return ret; - ret = vfio_msi_set_block(vdev, start, count, fds, msix); + ret = vfio_msi_set_block(vdev, start, count, fds, index); if (ret) - vfio_msi_disable(vdev, msix); + vfio_msi_disable(vdev, index); return ret; }
vfio_pci_set_intx_trigger() and vfio_pci_set_msi_trigger() have similar enough flows that can be converged into one shared flow instead of duplicated. To share code between management of interrupt types it is necessary that the type of the interrupt is only interpreted by the code specific to the interrupt type being managed. Remove interrupt type interpretation from what will eventually be shared code (vfio_pci_set_msi_trigger(), vfio_msi_set_block()) by pushing this interpretation to be within the interrupt type specific code (vfio_msi_enable(), (temporary) vfio_msi_set_vector_signal()). Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- Note to maintainers: Originally formed part of the IMS submission below, but is not specific to IMS. https://lore.kernel.org/lkml/cover.1696609476.git.reinette.chatre@intel.com drivers/vfio/pci/vfio_pci_intrs.c | 38 +++++++++++++++++-------------- 1 file changed, 21 insertions(+), 17 deletions(-)