Re: [PATCH] interrupts: allow guest to set and clear MSI-X mask bit

[Joby Poriyath <joby.poriyath@citrix.com>]

Thanks Ian.

On Fri, Jul 19, 2013 at 09:58:48AM +0100, Ian Campbell wrote:
> On Thu, 2013-07-18 at 18:44 +0100, Joby Poriyath wrote:
> > Guest needs the ability to enable and disable MSI-X interrupts
> > by setting the MSI-X control bit. Currently, a write to MSI-X
> > mask bit by the guest is silently ignored.
> > 
> > A likely scenario is where we have a 82599 SR-IOV nic passed
> > through to a guest. From the guest if you do
> > 
> >   ifconfig <ETH_DEV> down
> >   ifconfig <ETH_DEV> up
> > 
> > the interrupts remain masked.  The the mask bit for the VF is
> > being set by the PF performing a reset (at the request of the VF).
> > However, interrupts are enabled by VF driver by clearing the mask
> > bit by writing directly to BAR3 region containing the MSI-X table.
> > 
> > From dom0, we can verify that
> > interrupts are being masked using 'xl debug-keys M'.
> > 
> > Intially, guest was allowed to modify MSI-X bit.
> > Later this behaviour was changed.
> > See changeset 74c213c506afcd74a8556dd092995fd4dc38b225.
> 
> That commit message says:
>     - the interrupt mask bit was permitted to be written by the guest
>       (while Xen's interrupt flow control routines need to control it)
> I guess it's not entirely clear that simply reversing this is
> sufficient. The above doesn't give much to go on but I would have
> naïvely thought that any change to allow the guest to control this bit
> would be accompanied by some sort of call to Xen's interrupt flow
> control routines.
> 

I would have thought so. May be there is a interrupt flow control routine
that I ought to have called. But looking through the code it wasn't
obvious.

The write to mask bit was allowed so as to reduce the load on qemu-dm
if guest updates the mask bit frequently.
See changeset 34097f0d30802ecdc6da79658090fab9479a0c1c which introduced
this.

However I wasn't sure why this was disabled. Perhaps Jan could comment
on this. 

> > -       * As the mask bit is the only defined bit in the word, and as the
> > -       * host MSI-X code doesn't preserve the other bits anyway, doing
> > -       * this is pointless. So for now just discard the write (also
> > -       * saving us from having to determine the matching irq_desc).
> > -       */
> > -    spin_lock_irqsave(&desc->lock, flags);
> > -    orig = readl(virt);
> > -    val &= ~PCI_MSIX_VECTOR_BITMASK;
> > -    val |= orig & PCI_MSIX_VECTOR_BITMASK;
> > +    desc = pirq_spin_lock_irq_desc(entry->pirq, &flags);
> > +    if ( !desc )
> > +        goto out;
> > +
> > +    val &= PCI_MSIX_VECTOR_BITMASK;
> 
> I think you need to at least retain the bits of the comment which
> explain why we don't preserve the other bits, or actually preserve them.

I should have preserved the comments. Moreover I should only change the 
0th bit. Remaing 31 bit are reserved. Data sheet for 82599 clearly says
that the reserved bits should be preserved (as a general rule).

I'll send the updated patch for more comments.

> 
> Ian.
> 

Thanks,
Joby