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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2024 16:23, Roger Pau Monné wrote:
> On Tue, Jan 23, 2024 at 03:26:26PM +0100, Jan Beulich wrote:
>> On 15.01.2024 20:43, Stewart Hildebrand wrote:
>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -468,7 +468,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq 
>>> *pirq, uint64_t gtable)
>>>      struct msixtbl_entry *entry, *new_entry;
>>>      int r = -EINVAL;
>>>  
>>> -    ASSERT(pcidevs_locked());
>>> +    ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock));
>>>      ASSERT(rw_is_write_locked(&d->event_lock));
>>>  
>>>      if ( !msixtbl_initialised(d) )
>>> @@ -538,7 +538,7 @@ void msixtbl_pt_unregister(struct domain *d, struct 
>>> pirq *pirq)
>>>      struct pci_dev *pdev;
>>>      struct msixtbl_entry *entry;
>>>  
>>> -    ASSERT(pcidevs_locked());
>>> +    ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock));
>>>      ASSERT(rw_is_write_locked(&d->event_lock));
>>
>> I was hoping to just ack this patch, but the two changes above look
>> questionable to me: How can it be that holding _either_ lock is okay?
>> It's not obvious in this context that consumers have to hold both
>> locks now. In fact consumers looks to be the callers of
>> msixtbl_find_entry(), yet the list is RCU-protected. Whereas races
>> against themselves or against one another are avoided by holding
>> d->event_lock.
> 
> The reason for the change here is that msixtbl_pt_{un,}register() gets
> called by pt_irq_{create,destroy}_bind(), which is in turn called by
> vPCI code (pcidevs_locked()) that has been switched to not take the
> pcidevs lock anymore, and hence the ASSERT would trigger.

I understand this is the motivation for the change, but that doesn't
(alone) render the construct above sensible / correct.

>> My only guess then for the original need of holding pcidevs_lock is
>> the use of msi_desc->dev, with the desire for the device to not go
>> away. Yet the description doesn't talk about interactions of the per-
>> domain PCI lock with that one at all; it all circles around the
>> domain'd vPCI lock.
> 
> I do agree that it looks like the original intention of holding
> pcidevs_lock is to prevent msi_desc->dev from being removed - yet I'm
> not sure it's possible for the device to go away while the domain
> event_lock is hold, as device removal would need to take that same
> lock in order to destroy the irq_desc.

Yes, that matches an observation of mine as well. If we can simplify
(rather then complicate) locking, I'd prefer if we did. May need to
be a separate (prereq) patch, though.

>> Feels like I'm missing something that's obvious to everyone else.
>> Or maybe this part of the patch is actually unrelated, and should be
>> split off (with its own [proper] justification)? Or wouldn't it then
>> be better to also change the other paths leading here to acquire the
>> per-domain PCI lock?
> 
> Other paths in vPCI vpci_msi_update(), vpci_msi_arch_update(),
> vpci_msi_arch_enable()... are switched in this patch to use the
> per-domain pci_lock instead of pcidevs lock.

Hence my question: Can't we consolidate to all paths only using the
per-domain lock? That would make these odd-looking assertions become
normal-looking again.

Jan