From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joby Poriyath Subject: Re: [PATCH v2] interrupts: allow guest to set and clear MSI-X mask bit Date: Tue, 23 Jul 2013 11:54:41 +0100 Message-ID: <20130723105441.GE31939@citrix.com> References: <20130719150726.GA25302@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20130719150726.GA25302@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org Cc: andrew.cooper3@citrix.com, malcolm.crossley@citrix.com, keir@xen.org, Ian.Campbell@citrix.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org Ping... Could you kindly review this? Many 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 down > ifconfig 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 > --- > 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 >