From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] interrupts: allow guest to set and clear MSI-X mask bit Date: Tue, 23 Jul 2013 14:21:40 +0100 Message-ID: <51EE8364.1070905@citrix.com> References: <20130719150726.GA25302@citrix.com> <20130723105441.GE31939@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130723105441.GE31939@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: Joby Poriyath Cc: malcolm.crossley@citrix.com, keir@xen.org, Ian.Campbell@citrix.com, JBeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 23/07/13 11:54, Joby Poriyath wrote: > 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 Reviewed-by: Andrew Cooper This should be backported to all stable versions (perhaps including 4.1) as the buggy changeset 74c213c50 was backported in the name of security/general fixes. ~Andrew >> --- >> 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 >>