Re: [Xen-devel] Re: [PATCH][RFC] fix some spinlock issues in vmsi

[Qing He <qing.he@xxxxxxxxx>]

On Fri, 2009-03-06 at 15:31 +0800, Kouya Shimura wrote:
> Qing He writes:
> > No, it's not redundant. msixtbl_list_lock is to protect msixtbl_list,
> > which may contain entries for multiple irqs, consider if two
> > pt_bind_irq are called simultaneously, irq_desc->lock wouldn't be
> > enough to protect the list.
> 
> I see. Although two pt_bind_irq are hardly called simultaneously.

The problem here and in many other place is whether we can safely assume
that dom0 operations are completely within expectation

> 
> > > - ASSERT(spin_is_locked(&pcidevs_lock)) is not needed.
> > >   At first, I intended to add the enclosure of pcidevs_lock.
> > >   But this assertion seems pointless. pcidevs_lock is
> > >   for alldevs_list and msixtbl_pt_xxx functions never refer it.
> > 
> > I was thinking that pcidevs_lock is already held for msixtbl_pt_xxx, but
> > obviously I was wrong:-) However, it's not that pointless, msixtbl_pt_xxx
> > refers to the content of pci_dev, the use of pcidevs_lock is intended to
> > protect it against destructive modification, like an improper
> > PHYDEVOP_manage_pci_remove. It may not be neccessary for a well-written
> > device model, but that's something we can't rely on.
> 
> Okay, one more question.
> If ASSERT(spin_is_locked(&pcidevs_lock)) is needed,
> msixtbl_list_lock becomes redundant?
> (i.e. pcidevs_lock must covers it)
> Any way, msixtbl_list_lock would better be remained for the future...

I'm OK with that, though I think msixtbl_list_lock is more closer to the
list. Are you going to use something named like this?

Thanks,
Qing

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel