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