Re: Bug 43328 - VT-d/SR-IOV doesn't work in guest applies also to Mellanox ConnectX SRIOV driver

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jun 08, 2012 at 03:53:50PM +0300, Jack Morgenstein wrote:
> Adding KVM mailing list
> 
> This regards Bug 43328 and the RedHat discussion thread
>   https://lkml.org/lkml/2012/6/1/261
> regarding
>   [PATCH] KVM: Use IRQF_ONESHOT for assigned device MSI interrupts 
> 
> Alex Williamson proposed a patch to fix the bug, but 
> I noticed (after reading the entire thread) that
> Red Hat did not accept this fix as-is internally, because of
> a possible performance impact on interrupt handling.
> 
> Do you have any idea how soon you will have a correct fix?
> 
> In the meantime, our upstream kernel ConnectX SRIOV driver
> does not work on guests because of this issue (Bugzilla 43328).
>  
> How do you suggest that we proceed with our SRIOV development
> and upstream kernel submissions?
> 
> (I guess we can, temporarily, as a hack, take Alex Williamson's
> patch internally to continue our SRIOV development. What about
> kernel submission, though? )
> 
> Any help/suggestions will be most appreciated.
> 
> Thanks!
> -Jack

This need not block upstream submission of any driver - you can test
it natively meanhwhile.

> > On Friday 08 June 2012 10:32, Or Gerlitz wrote:
> > > >> Maybe get in touch with Michael Tsirkin to push this upstream?
> > > 
> > > Yes, lets try that! 
> > > 
> > > 
> > > Hi Michael, Gleb, Dor, 
> > > 
> > > SB upstream commit  c6c69525b40eb76de8adf039409722015927dc3
> > > "genirq: Reject bogus threaded irq requests" 
> > > gives us hardtime, we can't get interrupts on devices mapped to guests,
> > > and we noted this thread https://lkml.org/lkml/2012/6/1/261 
> > > and the redhat patch, so what would you suggest? Is some redhat patch on its way upstream?
> > > 
> > > Or.
> > > ________________________________________
> > > From: Jack Morgenstein [jackm@dev.mellanox.co.il]
> > > Sent: 08 June 2012 06:59
> > > To: Or Gerlitz; Tziporet Koren
> > > Cc: Yevgeny Petrilin; Saeed Mahameed; Shlomo Pongratz
> > > Subject: Re: Broken EQs for guests
> > > 
> > > Problem is in the KVM module.
> > > RedHat has a fix in their code, but it has not yet been submitted upstream.
> > > 
> > > See link:
> > >    https://lkml.org/lkml/2012/6/1/261
> > > 
> > > Gist of this link is below (and if you look at the bugzilla bug
> > > description, you will see that this is the exact problem we are
> > > having with the upstream kernel!). Note that the RH fix is
> > > from only a week ago (June 1).
> > > 
> > > [PATCH] KVM: Use IRQF_ONESHOT for assigned device MSI interrupts
> > > Date Fri, 01 Jun 2012 10:16:19 -0600
> > > 
> > > The kernel no longer allows us to pass NULL for a hard interrupt
> > > handler without IRQF_ONESHOT.  Should have been using this flag
> > > anyway.
> > > 
> > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43328
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  virt/kvm/assigned-dev.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > index 01f572c..e804d14 100644
> > > --- a/virt/kvm/assigned-dev.c
> > > +++ b/virt/kvm/assigned-dev.c
> > > @@ -347,7 +347,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
> > > 
> > >         dev->host_irq = dev->dev->irq;
> > >         if (request_threaded_irq(dev->host_irq, NULL,
> > > -                                kvm_assigned_dev_thread_msi, 0,
> > > +                                kvm_assigned_dev_thread_msi, IRQF_ONESHOT,
> > >                                  dev->irq_name, dev)) {
> > >                 pci_disable_msi(dev->dev);
> > >                 return -EIO;
> > > @@ -375,7 +375,7 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
> > >         for (i = 0; i < dev->entries_nr; i++) {
> > >                 r = request_threaded_irq(dev->host_msix_entries[i].vector,
> > >                                          NULL, kvm_assigned_dev_thread_msix,
> > > -                                        0, dev->irq_name, dev);
> > > +                                        IRQF_ONESHOT, dev->irq_name, dev);
> > >                 if (r)
> > >                         goto err;
> > >         }
> > > ================
> > > If you check the upstream kernel, you will see that this commit is missing from
> > > upstream (for procedures assigned_device_enable_host_msix and assigned_device_enable_host_msi),
> > > so kvm still calls request_threaded_irq with a NULL handler and flags=0.
> > > 
> > > We NEED this patch for kvm, or guests will not work with SRIOV on the upstream kernel.
> > > 
> > > -Jack
> > > On Thursday 07 June 2012 23:19, Or Gerlitz wrote:
> > > > > Will check this on Sunday, The worse case scenario is that the check they added is valid
> > > > > and we might have issue with our FW
> > > >
> > > > Yevgeny,
> > > >
> > > > Can you explain how this patch relates to firmware?
> > > >
> > > > Also, I check and it was merged for 3.5-rc1, so if we would have run regression on
> > > > each released kernel and each -rc1 as I suggested we could have spot this...
> > > >
> > > > Or.
> > > >
> > > >
> > > > ommit 1c6c69525b40eb76de8adf039409722015927dc3
> > > > Author: Thomas Gleixner <tglx@linutronix.de>
> > > > Date:   Thu Apr 19 10:35:17 2012 +0200
> > > >
> > > >     genirq: Reject bogus threaded irq requests
> > > >
> > > >     Requesting a threaded interrupt without a primary handler and without
> > > >     IRQF_ONESHOT set is dangerous.
> > > >
> > > >     The core will use the default primary handler for it, which merily
> > > >     wakes the thread. For a level type interrupt this results in an
> > > >     interrupt storm, because the interrupt line is reenabled after the
> > > >     primary handler runs. The device has still the line asserted, which
> > > >     brings us back into the primary handler.
> > > >
> > > >     While this works for edge type interrupts, we play it safe and reject
> > > >     unconditionally because we can't say for sure which type this
> > > >     interrupt really has. The type flags are unreliable as the underlying
> > > >     chip implementation can override them. And we cannot assume that
> > > >     developers using that interface know what they are doing.
> > > >
> > > >     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > >
> > > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > > > index 89a3ea8..9a35ace 100644
> > > > --- a/kernel/irq/manage.c
> > > > +++ b/kernel/irq/manage.c
> > > > @@ -1031,6 +1031,27 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> > > >                  * all existing action->thread_mask bits.
> > > >                  */
> > > >                 new->thread_mask = 1 << ffz(thread_mask);
> > > > +
> > > > +       } else if (new->handler == irq_default_primary_handler) {
> > > > +               /*
> > > > +                * The interrupt was requested with handler = NULL, so
> > > > +                * we use the default primary handler for it. But it
> > > > +                * does not have the oneshot flag set. In combination
> > > > +                * with level interrupts this is deadly, because the
> > > > +                * default primary handler just wakes the thread, then
> > > > +                * the irq lines is reenabled, but the device still
> > > > +                * has the level irq asserted. Rinse and repeat....
> > > > +                *
> > > > +                * While this works for edge type interrupts, we play
> > > > +                * it safe and reject unconditionally because we can't
> > > > +                * say for sure which type this interrupt really
> > > > +                * has. The type flags are unreliable as the
> > > > +                * underlying chip implementation can override them.
> > > > +                */
> > > > +               pr_err("genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq %d\n",
> > > > +                      irq);
> > > > +               ret = -EINVAL;
> > > > +               goto out_mask;
> > > >         }
> > > >
> > > >         if (!shared) {
> > > >
> > > >
> > > >
> > > >
> > > 
> >