On Thu, 1 Feb 2024 20:57:00 -0800 Reinette Chatre <reinette.chatre@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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@xxxxxxxxx > > 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; > }
