Re: [PATCH v13 01/14] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 2/14/24 06:38, Jan Beulich wrote:
> On 02.02.2024 22:33, Stewart Hildebrand wrote:
>> --- 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;
> 
> I'm afraid I need to come back to this: This is the only place where this
> patch retains (moves) use of the global lock. By moving, its scope is
> actually extended. It was previously said that conversion doesn't happen
> to limit the scope of what is changing. But with allocate_and_map_msi_pirq()
> being happy about either lock being held, I'm having a hard time seeing why
> here the global lock would continue to need using. To me doing so suggests
> uncertainty whether the checking in the function is actually correct.

I understand the concern. I've gone back and forth on this one in
particular myself [1]. I've tested it both ways (i.e. as shown here with
pcidevs_lock() and replacing it with read_lock(&d->pci_lock)). In the
analysis I've done, I also cannot find a need to continue using
pcidevs_lock() here. read_lock(&d->pci_lock) is sufficient. Let's be
clear: both ways are correct. The only reason I left it at
pcidevs_lock() for v13 was to make sure the code was doing what the
commit description and notes in the cover letter say.

allocate_and_map_msi_pirq() acquires write_lock(&d->event_lock), and
accesses to non-domain-specific data fall in the category of reading
__read_mostly globals or are appropriately protected by desc->lock
and/or vector_lock and/or cmpxchg.

I'll change it to read_lock(&d->pci_lock) in this one particular
instance (and update the description appropriately).

[1] 
https://lore.kernel.org/xen-devel/3c1023c4-25fb-41f1-83eb-03cbc1c3720e@xxxxxxx/