Re: [Qemu-devel] [PATCH] vfio: Fix regression in MSI routing configuration

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

On Thu, 15 Sep 2016 16:11:48 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> d1f6af6 "kvm-irqchip: simplify kvm_irqchip_add_msi_route" was a cleanup
> of kvmchip routing configuration, that was mostly intended for x86.
> However, it also contains a subtle change in behaviour which breaks EEH[1]
> error recovery on certain VFIO passthrough devices on spapr guests.  So far
> it's only been seen on a BCM5719 NIC on a POWER8 server, but there may be
> other hardware with the same problem.  It's also possible there could be
> circumstances where it causes a bug on x86 as well, though I don't know of
> any obvious candidates.
> 
> Prior to d1f6af6, both vfio_msix_vector_do_use() and
> vfio_add_kvm_msi_virq() used msg == NULL as a special flag to mark this
> as the "dummy" vector used to make the host hardware state sync with the
> guest expected hardware state in terms of MSI configuration.
> 
> Specifically that flag caused vfio_add_kvm_msi_virq() to become a no-op,
> meaning the dummy irq would always be delivered via qemu. d1f6af6 changed
> vfio_add_kvm_msi_virq() so it takes a vector number instead of the msg
> parameter, and determines the correct message itself.  The test for !msg
> was removed, and not replaced with anything there or in the caller.
> 
> With an spapr guest which has a VFIO device, if an EEH error occurs on the
> host hardware, then the device will be isolated then reset.  This is a
> combination of host and guest action, mediated by some EEH related
> hypercalls.  I haven't fully traced the mechanics, but somehow installing
> the kvm irqchip route for the dummy irq on the BCM5719 means that after EEH
> reset and recovery, at least some irqs are no longer delivered to the
> guest.
> 
> In particular, the guest never gets the link up event, and so the NIC is
> effectively dead.

I suspect what's happening is that we're getting a virq setup with
something bogus about the state and then we reuse that bogus state when
the first vector is actually configured and it probably never works.
Anyway, I'm concerned that d1f6af6 is going to cause a regression with
b0223e29afdc ("vfio-pci: Make host MSI-X enable track guest"), which is
where that dummy NULL msg call comes from, so I'm going to mark this for
stable.  Thanks!

Alex

> 
> [1] EEH (Enhanced Error Handling) is an IBM POWER server specific PCI-*
>     error reporting and recovery mechanism.  The concept is somewhat
>     similar to PCI-E AER, but the details are different.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1373802
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Gavin Shan <gwshan@au1.ibm.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/vfio/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7bfa17c..a5a620a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -496,7 +496,9 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>              vfio_update_kvm_msi_virq(vector, *msg, pdev);
>          }
>      } else {
> -        vfio_add_kvm_msi_virq(vdev, vector, nr, true);
> +        if (msg) {
> +            vfio_add_kvm_msi_virq(vdev, vector, nr, true);
> +        }
>      }
>  
>      /*