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

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

Hi Konrad,

On Tue, Jul 23, 2013 at 09:28:42AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 23, 2013 at 11:54:41AM +0100, Joby Poriyath wrote:
> > Ping...
> > 
> > Could you kindly review this?
> 
> I believe Malcom reviewed and sent you an email with his response.

Andrew and Malcolm has reviewed the code.

> 
> Are you waiting for Andrew to look at it as well? Or Jan? Jan is
> out on vacation for a couple of weeks.

I was hoping that Jan would also review this, since this patch 
partly reverts his code. 

This patch makes an assumption that, for a passed through device, 
xen needs to mask the MSI-X control bit only when it does vcpu 
migration. Otherwise xen leaves it in the state guest has set it.

> 
> Anyhow if it is Andrew that you are asking to look over it I would
> suggest you put his name on the 'To' in the email in case he is
> using filters to stick non-To emails in some 'lower priority' mailbox.
> 
> > 
> > Many thanks,
> > Joby
> > 

Thanks,
Joby

> > On Fri, Jul 19, 2013 at 04:07:37PM +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'.
> > > 
> > > Initially, 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 |   32 +++++++++++++++++++-------------
> > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> > > index 36de312..97d9f93 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,9 @@ 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;
> > > +    unsigned long orig;
> > >  
> > >      if ( len != 4 || (address & 3) )
> > >          return r;
> > > @@ -283,20 +287,20 @@ 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);
> > > +    desc = pirq_spin_lock_irq_desc(entry->pirq, &flags);
> > > +    if ( !desc )
> > > +        goto out;
> > > +
> > > +   /* The mask bit is the only defined bit in the word. But we 
> > > +    * ought to preserve the reserved bits. Clearing the reserved 
> > > +    * bits can result in undefined behaviour (see PCI Local Bus
> > > +    * Specification revision 2.3).
> > > +    */
> > >      orig = readl(virt);
> > > -    val &= ~PCI_MSIX_VECTOR_BITMASK;
> > > -    val |= orig & PCI_MSIX_VECTOR_BITMASK;
> > > +    val &= PCI_MSIX_VECTOR_BITMASK;
> > > +    val |= ( orig & ~PCI_MSIX_VECTOR_BITMASK );
> > >      writel(val, virt);
> > >      spin_unlock_irqrestore(&desc->lock, flags);
> > > -#endif
> > >  
> > >      r = X86EMUL_OKAY;
> > >  out:
> > > @@ -328,7 +332,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 +347,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 +410,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
> > > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel