Re: [RFC PATCH 03/10] xen: pci: introduce ats_list_lock

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hello Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 27.01.2023 00:56, Stefano Stabellini wrote:
>> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1641,6 +1641,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
>>>  {
>>>      pcidevs_lock();
>>>  
>>> +    /* iommu->ats_list_lock is taken by the caller of this function */
>> 
>> This is a locking inversion. In all other places we take pcidevs_lock
>> first, then ats_list_lock lock. For instance look at
>> xen/drivers/passthrough/pci.c:deassign_device that is called with
>> pcidevs_locked and then calls iommu_call(... reassign_device ...) which
>> ends up taking ats_list_lock.
>> 
>> This is the only exception. I think we need to move the
>> spin_lock(ats_list_lock) from qinval.c to here.
>
> First question here is what the lock is meant to protect: Just the list,
> or also the ATS state change (i.e. serializing enable and disable against
> one another). In the latter case the lock also wants naming differently.

My intention was to protect list only. But I believe you are right and
we should protect the whole state. I'll rename the lock to ats_lock.

> Second question is who is to acquire the lock. Why isn't this done _in_
> {en,dis}able_ats_device() themselves? That would then allow to further
> reduce the locked range, because at least the pci_find_ext_capability()
> call and the final logging can occur without the lock held.

You are right, I'll extended {en,dis}able_ats_device() API to pass
pointer to the lock.