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: Fri, 19 Jul 2013 17:07:04 +0100 Message-ID: <20130719160704.GA25318@citrix.com> References: <20130719150726.GA25302@citrix.com> <20130719152326.GB13009@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20130719152326.GB13009@localhost.localdomain> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: keir@xen.org, Ian.Campbell@citrix.com, andrew.cooper3@citrix.com, zhenzhong.duan@oracle.com, xen-devel@lists.xen.org, JBeulich@suse.com, malcolm.crossley@citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, Jul 19, 2013 at 11:23:26AM -0400, Konrad Rzeszutek Wilk wrote: > 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). > > So.. if we do it won't that be potentially dangerous? Yes it can be. That's why we are reading the value from MSI-X vector control register and only changing 0th bit and retaining the rest. > > + */ > > 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