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

[Kouya Shimura <kouya@xxxxxxxxxxxxxx>]

Hi Qing,

Thank you for review and elaboration.

Qing He writes:
> > - d->arch.hvm_domain.msixtbl_list_lock is redundant.
> >   irq_desc->lock covers it, thus remove the spinlock.
> 
> 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.

> > - 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...

Thanks,
Kouya

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