Re: [PATCH] vfio: avoid SET_ACTION_TRIGGER ioctls

[Alex Williamson]

On Fri, 28 Feb 2020 13:08:00 +0100
Roman Kapl <address@hidden> wrote:

> For MSI-X interrupts, remember what the last used eventfd was (KVM
> bypass vs QEMU) and only call vfio_set_irq_signaling if it has changed.
> 
> This not only helps with performance, but it seems that interrupts can
> be lost during VFIO_IRQ_SET_ACTION_TRIGGER. With the 'x-no-kvm-msix'
> switch and this patch, SET_ACTION_TRIGGER is not called during
> mask/unmask. This really only affects guests that actively use MSI-X masking.
> 
> Signed-off-by: Roman Kapl <address@hidden>
> ---
> 
> This patch scratches my particular itch. I am able to get our guest (which 
> masks
> MSI on each interrupt) running, without getting randomly stuck on waiting for
> interrupt. However, the solution is far from perfect (x-no-kvm-msix is 
> required)
> and pretty slow. I would be interested in hearing any ideas how to improve 
> this.
> Some ideas:
> 
> 1) Fix the kernel so that SET_ACTION_TRIGGER does not loose interrupts (I 
> think
> the problem is there, but not 100% sure). I've tested on 5.3.0-40-generic
> #32~18.04.1-Ubuntu SMP.

I'd be curious if this (yet unmerged) series resolve this:

https://lore.kernel.org/lkml/address@hidden/
 
> 2) Add support for MASK/UNMASK for MSI-X in kernel and use that. But I don't
> know how to do PBA in that case. Another IOCTL? We could look at the real PBA
> array, if mapping is supported, but that seems hacky.

That lack of a masking API in the host kernel is part of the reason we
take the hacky approach of emulating the PBA in QEMU.  We could have
the PBA MemoryRegion do a pread() from the device, but if we're doing
some ioctls on every un/mask, we're probably already digging ourselves
out of a hole.

It would be interesting to see if the series above prevents dropping
interrupts, how it compares with the reduced ioctls + QEMU handling you
have here.

> 3) Twiddle the bits behing kernel's back, if it can be mapped?

I'm not sure what you're thinking here, you mean mask vectors directly
on the device w/o a host kernel masking API and then read the PBA
directly from the device, mapping it directly to the guest if possible?
The MSI-X MMIO space can be mmapped by QEMU, we rely on interrupt
remmappers to protect us from malicious users.  QEMU certainly
shouldn't touch the vector or data fields, but masking might be
reasonably safe, then we could leave the KVM route in place.  I'm
afraid it might be difficult to integrate with QEMU MSI-X support
though.

> Still, I think this patch does not hurt anything and could be applied if 
> no-one
> can think of a better way.
> 
> ---
> 
>  hw/vfio/pci.c | 32 ++++++++++++++++++++++----------
>  hw/vfio/pci.h |  2 ++
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e6569a7968..5f7ce91519 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -390,12 +390,16 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, 
> bool msix)
>           * MSI-X mask and pending bits are emulated, so we want to use the
>           * KVM signaling path only when configured and unmasked.
>           */
> -        if (vdev->msi_vectors[i].use) {
> -            if (vdev->msi_vectors[i].virq < 0 ||
> -                (msix && msix_is_masked(&vdev->pdev, i))) {
> -                fd = event_notifier_get_fd(&vdev->msi_vectors[i].interrupt);
> +        VFIOMSIVector *vector = &vdev->msi_vectors[i];
> +        if (vector->use) {
> +            if (vector->virq < 0 ||
> +                (msix && msix_is_masked(&vdev->pdev, i)))
> +            {
> +                vector->kvm_path_active = false;
> +                fd = event_notifier_get_fd(&vector->interrupt);
>              } else {
> -                fd = 
> event_notifier_get_fd(&vdev->msi_vectors[i].kvm_interrupt);
> +                vector->kvm_path_active = true;
> +                fd = event_notifier_get_fd(&vector->kvm_interrupt);
>              }
>          }
>  
> @@ -509,17 +513,23 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, 
> unsigned int nr,
>      } else {
>          Error *err = NULL;
>          int32_t fd;
> +        bool kvm_path;
>  
>          if (vector->virq >= 0) {
>              fd = event_notifier_get_fd(&vector->kvm_interrupt);
> +            kvm_path = true;
>          } else {
>              fd = event_notifier_get_fd(&vector->interrupt);
> +            kvm_path = false;
>          }
>  
> -        if (vfio_set_irq_signaling(&vdev->vbasedev,
> -                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
> -                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) 
> {
> -            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        if (vector->kvm_path_active != kvm_path) {
> +            if (vfio_set_irq_signaling(&vdev->vbasedev,
> +                                       VFIO_PCI_MSIX_IRQ_INDEX, nr,
> +                                       VFIO_IRQ_SET_ACTION_TRIGGER, fd, 
> &err)) {
> +                error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +            }
> +            vector->kvm_path_active = kvm_path;


Wouldn't this be more intuitive if we just cached the current fd on the
VFIOMSIVector object and created a vfio_set_irq_signaling() wrapper for
vectors that only calls through when the fd changes, updating the fd on
successful return otherwise?  AIUI, you're only trying to prevent
gratuitous calls to vfio_set_irq_signaling() when the eventfd remains
unchanged, which is the common case for your configuration of running
in QEMU interrupt mode.  Thanks,

Alex

>          }
>      }
>  
> @@ -555,13 +565,15 @@ static void vfio_msix_vector_release(PCIDevice *pdev, 
> unsigned int nr)
>       * core will mask the interrupt and set pending bits, allowing it to
>       * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
>       */
> -    if (vector->virq >= 0) {
> +    if (vector->virq >= 0 && vector->kvm_path_active) {
>          int32_t fd = event_notifier_get_fd(&vector->interrupt);
>          Error *err = NULL;
>  
>          if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, 
> nr,
>                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>              error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        } else {
> +            vector->kvm_path_active = false;
>          }
>      }
>  }
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index b329d50338..b01d2676cf 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -91,6 +91,8 @@ typedef struct VFIOMSIVector {
>       */
>      EventNotifier interrupt;
>      EventNotifier kvm_interrupt;
> +    /* Set when the trigger action is set to the KVM bypass FD */
> +    bool kvm_path_active;
>      struct VFIOPCIDevice *vdev; /* back pointer to device */
>      int virq;
>      bool use;