From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joby Poriyath Subject: Re: [PATCH] interrupts: allow guest to set and clear MSI-X mask bit Date: Fri, 19 Jul 2013 14:05:12 +0100 Message-ID: <20130719130512.GC29059@citrix.com> References: <20130718174407.GA28397@citrix.com> <1374224328.13645.8.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1374224328.13645.8.camel@kazak.uk.xensource.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: Ian Campbell Cc: andrew.cooper3@citrix.com, malcolm.crossley@citrix.com, keir@xen.org, JBeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 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'. > > = > > 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=EFvely 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 t= he > > - * 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 =3D readl(virt); > > - val &=3D ~PCI_MSIX_VECTOR_BITMASK; > > - val |=3D orig & PCI_MSIX_VECTOR_BITMASK; > > + desc =3D pirq_spin_lock_irq_desc(entry->pirq, &flags); > > + if ( !desc ) > > + goto out; > > + > > + val &=3D 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