From mboxrd@z Thu Jan  1 00:00:00 1970
From: Joby Poriyath <joby.poriyath@citrix.com>
Subject: Re: [PATCH v7] interrupts: allow guest to set/clear
	MSI-X mask bit
Date: Wed, 4 Sep 2013 13:01:06 +0100
Message-ID: <20130904120105.GA14816@citrix.com>
References: <20130830161240.GA14360@citrix.com>
	<52270D1902000078000F03EB@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <pcj@citrix.com>) id 1VHBlm-0000j6-QF
	for xen-devel@lists.xenproject.org; Wed, 04 Sep 2013 12:01:11 +0000
Content-Disposition: inline
In-Reply-To: <52270D1902000078000F03EB@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: andrew.cooper3@citrix.com, xen-devel <xen-devel@lists.xenproject.org>, Sander Eikelenboom <linux@eikelenboom.it>
List-Id: xen-devel@lists.xenproject.org

On Wed, Sep 04, 2013 at 09:36:09AM +0100, Jan Beulich wrote:
> >>> On 30.08.13 at 18:12, Joby Poriyath <joby.poriyath@citrix.com> wrote:
> > +static struct msi_desc *virt_to_msi_desc(struct pci_dev *dev, void *virt)
> > +{
> > +    struct msi_desc *desc;
> > +
> > +    list_for_each_entry( desc, &dev->msi_list, list )
> > +        if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX  &&
> > +             desc->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET 
> > +                == virt )
> 
> To match the function's name I'd prefer this to hit on the full 16-byte
> range rather than just the control word.
> 

Sorry Jan, I didn't quite understand this. virt points to the control word 
in this function. So how can we do a match on (low | high) address and data words?

> >  static int msixtbl_write(struct vcpu *v, unsigned long address,
> > -                        unsigned long len, unsigned long val)
> > +                         unsigned long len, unsigned long val)
> >  {
> >      unsigned long offset;
> >      struct msixtbl_entry *entry;
> > +    struct msi_desc *m_desc;
> 
> Please name this msi_desc, consistent with other variables of this
> type in this file. And afaict this could (once again) be const.

I'll add the const and also rename m_desc to msi_desc.

> 
> > +    m_desc = virt_to_msi_desc(entry->pdev, virt);
> > +    if ( !m_desc || m_desc->irq < 0 )
> > +        goto out;
> > +    
> > +    desc = irq_to_desc(m_desc->irq);
> > +    if ( !desc )
> > +        goto out;
> > +
> >      spin_lock_irqsave(&desc->lock, flags);
> > +
> > +    if ( !desc->msi_desc )
> > +        goto unlock;
> 
> I'd again strongly recommend adding an ASSERT() here, checking
> desc->msi_desc against (as it's currently named) m_desc.

ASSERT makes sense. I'll add it.

> 
> But overall this looks much better than the earlier, no reverted
> variant.
> 
> Jan
> 

Thanks,
Joby