Re: [PATCH v7] interrupts: allow guest to set/clear MSI-X mask bit

[Joby Poriyath <joby.poriyath@citrix.com>]

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