On Thu, 1 Feb 2024 20:57:09 -0800
Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
> vfio_pci_set_intx_trigger() and vfio_pci_set_msi_trigger() have
> flows that can be shared.
>
> For INTx, MSI, and MSI-X interrupt management to share the
> same enable/disable flow the interrupt specific enable and
> disable functions should have the same signatures.
>
> Let vfio_intx_enable() and vfio_msi_enable() use the same
> parameters by passing "start" and "count" to these functions
> instead of letting the (what will eventually be) common code
> interpret these values.
>
> Providing "start" and "count" to vfio_intx_enable()
> enables the INTx specific check of these parameters to move into
> the INTx specific vfio_intx_enable(). Similarly, providing "start"
> and "count" to vfio_msi_enable() enables the MSI/MSI-X specific
> code to initialize number of vectors needed.
>
> The shared MSI/MSI-X code needs the interrupt index. Provide
> the interrupt index (clearly marked as unused) to the INTx code
> to use the same signatures.
>
> With interrupt type specific code using the same parameters it
> is possible to have common code that calls the enable/disable
> code for different interrupt types.
>
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 37065623d286..9217fea3f636 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -257,13 +257,18 @@ static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
> return ret;
> }
>
> -static int vfio_intx_enable(struct vfio_pci_core_device *vdev)
> +static int vfio_intx_enable(struct vfio_pci_core_device *vdev,
> + unsigned int start, unsigned int count,
> + unsigned int __always_unused index)
> {
> struct vfio_pci_irq_ctx *ctx;
>
> if (!is_irq_none(vdev))
> return -EINVAL;
>
> + if (start != 0 || count != 1)
> + return -EINVAL;
> +
> if (!vdev->pdev->irq)
> return -ENODEV;
>
> @@ -332,7 +337,8 @@ static char *vfio_intx_device_name(struct vfio_pci_core_device *vdev,
> return kasprintf(GFP_KERNEL_ACCOUNT, "vfio-intx(%s)", pci_name(pdev));
> }
>
> -static void vfio_intx_disable(struct vfio_pci_core_device *vdev)
> +static void vfio_intx_disable(struct vfio_pci_core_device *vdev,
> + unsigned int __always_unused index)
> {
> struct vfio_pci_irq_ctx *ctx;
>
> @@ -358,17 +364,20 @@ 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,
> +static int vfio_msi_enable(struct vfio_pci_core_device *vdev,
> + unsigned int start, unsigned int count,
> unsigned int index)
> {
> struct pci_dev *pdev = vdev->pdev;
> unsigned int flag;
> - int ret;
> + int ret, nvec;
> u16 cmd;
>
> if (!is_irq_none(vdev))
> return -EINVAL;
>
> + nvec = start + count;
> +
> 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: */
> @@ -701,11 +710,11 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> unsigned int i;
>
> if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> - vfio_intx_disable(vdev);
> + vfio_intx_disable(vdev, index);
> return 0;
> }
>
> - if (!(is_intx(vdev) || is_irq_none(vdev)) || start != 0 || count != 1)
> + if (!(is_intx(vdev) || is_irq_none(vdev)))
> return -EINVAL;
>
> if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> @@ -715,13 +724,13 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> if (is_intx(vdev))
> return vfio_irq_set_block(vdev, start, count, fds, index);
>
> - ret = vfio_intx_enable(vdev);
> + ret = vfio_intx_enable(vdev, start, count, index);
Please trace what happens when a user calls SET_IRQS to setup a trigger
eventfd with start = 0, count = 1, followed by any other combination of
start and count values once is_intx() is true. vfio_intx_enable()
cannot be the only place we bounds check the user, all of the INTx
callbacks should be an error or nop if vector != 0. Thanks,
Alex
> if (ret)
> return ret;
>
> ret = vfio_irq_set_block(vdev, start, count, fds, index);
> if (ret)
> - vfio_intx_disable(vdev);
> + vfio_intx_disable(vdev, index);
>
> return ret;
> }
> @@ -771,7 +780,7 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
> return vfio_irq_set_block(vdev, start, count,
> fds, index);
>
> - ret = vfio_msi_enable(vdev, start + count, index);
> + ret = vfio_msi_enable(vdev, start, count, index);
> if (ret)
> return ret;
>
