Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.