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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 1/25/24 07:33, Roger Pau Monné wrote:
> On Thu, Jan 25, 2024 at 12:23:05PM +0100, Jan Beulich wrote:
>> On 25.01.2024 10:05, Roger Pau Monné wrote:
>>> On Thu, Jan 25, 2024 at 08:43:05AM +0100, Jan Beulich wrote:
>>>> 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.
>>>
>>> The new lock is taken in read mode in most contexts, and hence can't
>>> be used to indirectly gain exclusive access to domain related
>>> structures in a safe way.
>>
>> Oh, right - I keep being misled by rw_is_locked(). This is a fair
>> argument. Irrespective it would feel better to me if an abstraction
>> construct was introduced; but seeing you don't like the idea I guess
>> I won't insist.
> 
> TBH I'm not going to argue against it if you and Stewart think it's
> clearer, but I also won't request the addition of such wrapper myself.
> 
> Thanks, Roger.

Overall, I think there are two sources of confusion:

    1. This patch is using the odd-looking ASSERTs to verify that it is safe to 
*read* d->pdev_list, and/or ensure a pdev does not go away or get reassigned. 
The purpose of these ASSERTs is not immediately obvious due to inadequate 
description.

    2. At first glance, the patch appears to be doing two things: using 
d->pci_lock for d->pdev_list/pdev protection, and using d->pci_lock for 
pdev->vpci protection.

Regarding #1, while the review experience could have been improved by 
introducing a wrapper construct, I think it would also (more importantly) be 
valuable to have such a wrapper for the sake of code readability. I think it is 
important to get this right and hopefully avoid/reduce potential future 
confusion. I'll add something like this in v13, e.g. in sched.h:

/* Ensure pdevs do not go away or get assigned to other domains. */
#define pdev_list_is_read_locked(d) ({                           \

        struct domain *d_ = (d);                                 \

        pcidevs_locked() || (d_ && rw_is_locked(&d_->pci_lock)); \

    })

Example use:

    ASSERT(pdev_list_is_read_locked(d));

Regarding #2, the patch description primarily talks about protecting the 
pdev->vpci field, and the d->pdev_list read / pdev reassignment protection 
seems an afterthought. However, the use of pcidevs_lock() for pdev protection 
is pre-existing. Now that vPCI callers are going to use d->pci_lock (instead of 
pcidevs_lock()), we are simultaneously changing the allowable mechanism for 
protecting d->pdev_list reads and pdevs going away or getting reassigned. I 
briefly experimented with splitting this into two separate patches, but I chose 
not to pursue this further because then we'd have a brief odd intermediate 
state, not to mention the additional test/review burden of evaluating each 
separate change independently. Keep in mind this patch as a whole has already 
been through much test/review, and at this point my primary focus is to improve 
readability and avoid confusion. I will plan to add appropriate description and 
rationale for v13.

Since I will be changing to use a wrapper construct and updating the 
descriptions, I will plan to drop Roger's R-b tag on this patch for v13.

Lastly, as has already been mentioned in the cover letter and reiterated in 
discussions here, for the non-vPCI code paths already using pcidevs_lock() I 
will plan to keep them using pcidevs_lock().