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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2024 18:51, Roger Pau Monné wrote:
> On Wed, Jan 24, 2024 at 12:34:10PM +0100, Jan Beulich wrote:
>> On 24.01.2024 10:24, Roger Pau Monné wrote:
>>> On Wed, Jan 24, 2024 at 09:48:35AM +0100, Jan Beulich wrote:
>>>> On 23.01.2024 16: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;
>>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Yet why would we prefer to acquire a global lock when a per-domain one
>>>> suffices?
>>>
>>> I was hoping to introduce less changes, specially if they are not
>>> strictly required, as it's less risk.  I'm always quite worry of
>>> locking changes.
>>
>> In which case more description / code commenting is needed. The pattern
>> of the assertions looks dangerous.
> 
> Is such dangerousness perception because you fear some of the pcidevs
> lock usage might be there not just for preventing the pdev from going
> away, but also to guarantee exclusive access to certain state?

Indeed. In my view the main purpose of locks is to guard state. Their
use here to guard against devices here is imo rather an abuse; as
mentioned before this should instead be achieved e.g via refcounting.
And it's bad enough already that pcidevs_lock() alone has been abused
this way, without proper marking (leaving us to guess in many places).
It gets worse when a second lock can now also serve this same purpose.

>> Even if (as you say in a later reply)
>> this is only temporary, we all know how long "temporary" can be. It
>> might even be advisable to introduce a helper construct.
> 
> The aim here was to modify as little as possible, in order to avoid
> having to analyze all possible users of pcidevs lock, and thus not
> block the vPCI work on the probably lengthy and difficult analysis.
> 
> Not sure adding a construct makes is much better, as I didn't want to
> give the impression all checks for the pcidevs lock can merely be
> replaced by the new construct.

Of course such a construct could only be used in places where it can
be shown to be appropriate.

Jan