On 1/24/24 00:00, Stewart Hildebrand wrote: > On 1/23/24 10:07, Roger Pau Monné wrote: >> On Tue, Jan 23, 2024 at 03:32:12PM +0100, Jan Beulich wrote: >>> On 15.01.2024 20:43, Stewart Hildebrand wrote: >>>> @@ -2888,6 +2888,8 @@ int allocate_and_map_msi_pirq(struct domain *d, int >>>> index, int *pirq_p, >>>> { >>>> int irq, pirq, ret; >>>> >>>> + ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock)); >>> >>> If either lock is sufficient to hold here, ... >>> >>>> --- a/xen/arch/x86/physdev.c >>>> +++ b/xen/arch/x86/physdev.c >>>> @@ -123,7 +123,9 @@ int physdev_map_pirq(domid_t domid, int type, int >>>> *index, int *pirq_p, >>>> >>>> case MAP_PIRQ_TYPE_MSI: >>>> case MAP_PIRQ_TYPE_MULTI_MSI: >>>> + pcidevs_lock(); >>>> ret = allocate_and_map_msi_pirq(d, *index, pirq_p, type, msi); >>>> + pcidevs_unlock(); >>>> break; >>> >>> ... why is it the global lock that's being acquired here? >>> >> >> IIRC (Stewart can further comment) this is done holding the pcidevs >> lock to keep the path unmodified, as there's no need to hold the >> per-domain rwlock. >> > > Although allocate_and_map_msi_pirq() was itself acquiring the global > pcidevs_lock() before this patch, we could just as well use > read_lock(&d->pci_lock) here instead now. It seems like a good optimization > to make, so if there aren't any objections I'll change it to > read_lock(&d->pci_lock). >
Actually, I take this back. As mentioned in the cover letter of this series, and has been reiterated in recent discussions, the goal with this is to keep existing (non-vPCI) code paths as unmodified as possible. So I'll keep it as pcidevs_lock() here.