Re: [PATCH] vfio/pci: Check return value of vfio_set_irq_signaling()

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 26 Oct 2023 09:10:43 +0200
Cédric Le Goater <clg@redhat.com> wrote:

> and drop warning when ENOTTY is returned. Only useful for the mdev-mtty
> driver today, which has partial support for INTx: the AUTOMASK
> behavior is not implemented.

FWIW, I prefer not to carry a sentence through from subject to commit
log, I find it harder to follow.

Anyway, I'm not sure it's a great idea to suppress this warning.  We
really want drivers to implement the eventfd channel for INTx
unmasking.  I think we're only putting up with it for mtty because it's
a sample driver and it's a step forward versus the botched
implementation of the SET_IRQS ioctl that it previously had.  We could
implement the unmask eventfd channel for mtty, but it might be better
from a test coverage perspective to have it as a driver that forces the
QEMU INTx path to be exercised.

If we suppress this warning, as the de facto userspace driver for vfio
devices, we're declaring it ok to implement INTx without UNMASK eventfd
support when there's no technical reason it couldn't be implemented.

Maybe we should just let QEMU continue to complain about mtty?  Thanks,

Alex

> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/vfio/pci.c | 46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b27011cee72a0fb3b2d57d297c0b5c2ccff9d9a6..5cbc771e55d83561011785e54a38dea042fc834c 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -114,15 +114,16 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  }
>  
> -static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
> +static int vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
> +    int ret = 0;
>  #ifdef CONFIG_KVM
>      int irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
>  
>      if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
>          vdev->intx.route.mode != PCI_INTX_ENABLED ||
>          !kvm_resamplefds_enabled()) {
> -        return;
> +        return 0;
>      }
>  
>      /* Get to a known interrupt state */
> @@ -132,23 +133,26 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>      pci_irq_deassert(&vdev->pdev);
>  
>      /* Get an eventfd for resample/unmask */
> -    if (event_notifier_init(&vdev->intx.unmask, 0)) {
> +    ret = event_notifier_init(&vdev->intx.unmask, 0);
> +    if (ret) {
>          error_setg(errp, "event_notifier_init failed eoi");
>          goto fail;
>      }
>  
> -    if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> -                                           &vdev->intx.interrupt,
> -                                           &vdev->intx.unmask,
> -                                           vdev->intx.route.irq)) {
> +    ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> +                                             &vdev->intx.interrupt,
> +                                             &vdev->intx.unmask,
> +                                             vdev->intx.route.irq);
> +    if (ret) {
>          error_setg_errno(errp, errno, "failed to setup resample irqfd");
>          goto fail_irqfd;
>      }
>  
> -    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
> -                               VFIO_IRQ_SET_ACTION_UNMASK,
> -                               event_notifier_get_fd(&vdev->intx.unmask),
> -                               errp)) {
> +    ret = vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
> +                                 VFIO_IRQ_SET_ACTION_UNMASK,
> +                                 event_notifier_get_fd(&vdev->intx.unmask),
> +                                 errp);
> +    if (ret) {
>          goto fail_vfio;
>      }
>  
> @@ -159,7 +163,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  
>      trace_vfio_intx_enable_kvm(vdev->vbasedev.name);
>  
> -    return;
> +    return 0;
>  
>  fail_vfio:
>      kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &vdev->intx.interrupt,
> @@ -170,6 +174,7 @@ fail:
>      qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev);
>      vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  #endif
> +    return ret;
>  }
>  
>  static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
> @@ -212,6 +217,7 @@ static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
>  static void vfio_intx_update(VFIOPCIDevice *vdev, PCIINTxRoute *route)
>  {
>      Error *err = NULL;
> +    int ret;
>  
>      trace_vfio_intx_update(vdev->vbasedev.name,
>                             vdev->intx.route.irq, route->irq);
> @@ -224,9 +230,13 @@ static void vfio_intx_update(VFIOPCIDevice *vdev, PCIINTxRoute *route)
>          return;
>      }
>  
> -    vfio_intx_enable_kvm(vdev, &err);
> +    ret = vfio_intx_enable_kvm(vdev, &err);
>      if (err) {
> -        warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        if (ret != -ENOTTY) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        } else {
> +            error_free(err);
> +        }
>      }
>  
>      /* Re-enable the interrupt in cased we missed an EOI */
> @@ -300,9 +310,13 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>          return -errno;
>      }
>  
> -    vfio_intx_enable_kvm(vdev, &err);
> +    ret = vfio_intx_enable_kvm(vdev, &err);
>      if (err) {
> -        warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        if (ret != -ENOTTY) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        } else {
> +            error_free(err);
> +        }
>      }
>  
>      vdev->interrupt = VFIO_INT_INTx;