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

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

[Joby Poriyath]

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.

Signed-off-by: Joby Poriyath <joby.poriyath@citrix.com>
---
 xen/arch/x86/hvm/vmsi.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 36de312..6d9892a 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -169,6 +169,7 @@ struct msixtbl_entry
         uint32_t msi_ad[3];	/* Shadow of address low, high and data */
     } gentries[MAX_MSIX_ACC_ENTRIES];
     struct rcu_head rcu;
+    struct pirq *pirq;
 };
 
 static DEFINE_RCU_READ_LOCK(msixtbl_rcu_lock);
@@ -254,6 +255,8 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
     void *virt;
     unsigned int nr_entry, index;
     int r = X86EMUL_UNHANDLEABLE;
+    unsigned long flags;
+    struct irq_desc *desc;
 
     if ( len != 4 || (address & 3) )
         return r;
@@ -283,20 +286,13 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
     if ( !virt )
         goto out;
 
-    /* Do not allow the mask bit to be changed. */
-#if 0 /* XXX
-       * 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;
     writel(val, virt);
     spin_unlock_irqrestore(&desc->lock, flags);
-#endif
 
     r = X86EMUL_OKAY;
 out:
@@ -328,7 +324,8 @@ const struct hvm_mmio_handler msixtbl_mmio_handler = {
 static void add_msixtbl_entry(struct domain *d,
                               struct pci_dev *pdev,
                               uint64_t gtable,
-                              struct msixtbl_entry *entry)
+                              struct msixtbl_entry *entry,
+                              struct pirq *pirq)
 {
     u32 len;
 
@@ -342,6 +339,7 @@ static void add_msixtbl_entry(struct domain *d,
     entry->table_len = len;
     entry->pdev = pdev;
     entry->gtable = (unsigned long) gtable;
+    entry->pirq = pirq;
 
     list_add_rcu(&entry->list, &d->arch.hvm_domain.msixtbl_list);
 }
@@ -404,7 +402,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
 
     entry = new_entry;
     new_entry = NULL;
-    add_msixtbl_entry(d, pdev, gtable, entry);
+    add_msixtbl_entry(d, pdev, gtable, entry, pirq);
 
 found:
     atomic_inc(&entry->refcnt);
-- 
1.7.10.4

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

[Ian Campbell]

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.

> -       * 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.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Joby Poriyath]

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