Hello,

This is yet another take to a PCI locking rework. This approach
was suggest by Jan Beulich who proposed to use a reference
counter to control lifetime of pci_dev objects.

When I started added reference counting it quickly became clear
that this approach can provide more granular locking insted of
huge pcidevs_lock() which is used right now. I studied how this
lock used and what it protects. And found the following:

0. Comment in pci.h states the following:

 153 /*
 154  * The pcidevs_lock protect alldevs_list, and the assignment for the
 155  * devices, it also sync the access to the msi capability that is not
 156  * interrupt handling related (the mask bit register).
 157  */

But in reality it does much more. Here is what I found:

1. Lifetime of pci_dev struct

2. Access to pseg->alldevs_list

3. Access to domain->pdev_list

4. Access to iommu->ats_list

5. Access to MSI capability

6. Some obsucure stuff in IOMMU drivers: there are places that
are guarded by pcidevs_lock() but it seems that nothing
PCI-related happens there.

7. Something that I probably overlooked

Anyways, I tried to get rid of global mighty pcidevs_lock() by
reworking items 1-5.

This patch series does exactly this: adds separate lock for each
of the lists, lock for struct pci_dev itself, adds reference
counting, then removes pcidevs_lock() entirely. I do understand
that I should not remove locks when there are locking fixes for
items 6-7. But this is why it is an RFC. I want to discuss if my
approach is legit and get some guidance from maintainers on what
should be done in addition to the presented changes.


Volodymyr Babchuk (10):
  xen: pci: add per-domain pci list lock
  xen: pci: add pci_seg->alldevs_lock
  xen: pci: introduce ats_list_lock
  xen: add reference counter support
  xen: pci: introduce reference counting for pdev
  xen: pci: print reference counter when dumping pci_devs
  xen: pci: add per-device locking
  xen: pci: remove pcidev_[un]lock[ed] calls
  [RFC only] xen: iommu: remove last  pcidevs_lock() calls in iommu
  [RFC only] xen: pci: remove pcidev_lock() function

 xen/arch/x86/domctl.c                       |   8 -
 xen/arch/x86/hvm/vioapic.c                  |   2 -
 xen/arch/x86/hvm/vmsi.c                     |  20 +-
 xen/arch/x86/irq.c                          |  11 +-
 xen/arch/x86/msi.c                          |  68 ++++-
 xen/arch/x86/pci.c                          |   8 +-
 xen/arch/x86/physdev.c                      |  24 +-
 xen/common/domain.c                         |   1 +
 xen/common/sysctl.c                         |   7 +-
 xen/drivers/char/ns16550.c                  |   4 -
 xen/drivers/passthrough/amd/iommu.h         |   1 +
 xen/drivers/passthrough/amd/iommu_cmd.c     |   4 +-
 xen/drivers/passthrough/amd/iommu_detect.c  |   1 +
 xen/drivers/passthrough/amd/iommu_init.c    |  19 +-
 xen/drivers/passthrough/amd/iommu_map.c     |  11 +-
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  19 +-
 xen/drivers/passthrough/msi.c               |   8 +-
 xen/drivers/passthrough/pci.c               | 267 +++++++++++---------
 xen/drivers/passthrough/vtd/intremap.c      |   2 -
 xen/drivers/passthrough/vtd/iommu.c         |  33 +--
 xen/drivers/passthrough/vtd/iommu.h         |   1 +
 xen/drivers/passthrough/vtd/qinval.c        |   3 +
 xen/drivers/passthrough/vtd/quirks.c        |   2 +
 xen/drivers/passthrough/vtd/x86/ats.c       |   3 +
 xen/drivers/passthrough/x86/iommu.c         |   5 -
 xen/drivers/video/vga.c                     |  12 +-
 xen/drivers/vpci/header.c                   |   3 +
 xen/drivers/vpci/msi.c                      |   7 +-
 xen/drivers/vpci/vpci.c                     |  10 +-
 xen/include/xen/pci.h                       |  36 ++-
 xen/include/xen/refcnt.h                    |  28 ++
 xen/include/xen/sched.h                     |   1 +
 32 files changed, 380 insertions(+), 249 deletions(-)
 create mode 100644 xen/include/xen/refcnt.h

-- 
2.36.1

On 31.08.2022 16:10, Volodymyr Babchuk wrote:
> Hello,
> 
> This is yet another take to a PCI locking rework. This approach
> was suggest by Jan Beulich who proposed to use a reference
> counter to control lifetime of pci_dev objects.
> 
> When I started added reference counting it quickly became clear
> that this approach can provide more granular locking insted of
> huge pcidevs_lock() which is used right now. I studied how this
> lock used and what it protects. And found the following:
> 
> 0. Comment in pci.h states the following:
> 
>  153 /*
>  154  * The pcidevs_lock protect alldevs_list, and the assignment for the
>  155  * devices, it also sync the access to the msi capability that is not
>  156  * interrupt handling related (the mask bit register).
>  157  */
> 
> But in reality it does much more. Here is what I found:
> 
> 1. Lifetime of pci_dev struct
> 
> 2. Access to pseg->alldevs_list
> 
> 3. Access to domain->pdev_list
> 
> 4. Access to iommu->ats_list
> 
> 5. Access to MSI capability
> 
> 6. Some obsucure stuff in IOMMU drivers: there are places that
> are guarded by pcidevs_lock() but it seems that nothing
> PCI-related happens there.

Right - the lock being held was (ab)used in IOMMU code in a number of
places. This likely needs to change in the course of this re-work;
patch titles don't suggest this is currently part of the series.

> 7. Something that I probably overlooked

And this is the main risk here. The huge scope of the original lock
means that many things are serialized now but won't be anymore once
the lock is gone.

But yes - thanks for the work. To be honest I don't expect to be able
to look at this series in detail until after the Xen Summit. And even
then it may take a while ...

Jan

Hi Jan,

On 06/09/2022 11:32, Jan Beulich wrote:
> On 31.08.2022 16:10, Volodymyr Babchuk wrote:
>> Hello,
>>
>> This is yet another take to a PCI locking rework. This approach
>> was suggest by Jan Beulich who proposed to use a reference
>> counter to control lifetime of pci_dev objects.
>>
>> When I started added reference counting it quickly became clear
>> that this approach can provide more granular locking insted of
>> huge pcidevs_lock() which is used right now. I studied how this
>> lock used and what it protects. And found the following:
>>
>> 0. Comment in pci.h states the following:
>>
>>   153 /*
>>   154  * The pcidevs_lock protect alldevs_list, and the assignment for the
>>   155  * devices, it also sync the access to the msi capability that is not
>>   156  * interrupt handling related (the mask bit register).
>>   157  */
>>
>> But in reality it does much more. Here is what I found:
>>
>> 1. Lifetime of pci_dev struct
>>
>> 2. Access to pseg->alldevs_list
>>
>> 3. Access to domain->pdev_list
>>
>> 4. Access to iommu->ats_list
>>
>> 5. Access to MSI capability
>>
>> 6. Some obsucure stuff in IOMMU drivers: there are places that
>> are guarded by pcidevs_lock() but it seems that nothing
>> PCI-related happens there.
> 
> Right - the lock being held was (ab)used in IOMMU code in a number of
> places. This likely needs to change in the course of this re-work;
> patch titles don't suggest this is currently part of the series.
> 
>> 7. Something that I probably overlooked
> 
> And this is the main risk here. The huge scope of the original lock
> means that many things are serialized now but won't be anymore once
> the lock is gone.
> 
> But yes - thanks for the work. To be honest I don't expect to be able
> to look at this series in detail until after the Xen Summit. And even
> then it may take a while ...

I was wondering if this is still in your list to review?

Cheers,

-- 
Julien Grall

On 18.01.2023 19:21, Julien Grall wrote:
> On 06/09/2022 11:32, Jan Beulich wrote:
>> On 31.08.2022 16:10, Volodymyr Babchuk wrote:
>>> Hello,
>>>
>>> This is yet another take to a PCI locking rework. This approach
>>> was suggest by Jan Beulich who proposed to use a reference
>>> counter to control lifetime of pci_dev objects.
>>>
>>> When I started added reference counting it quickly became clear
>>> that this approach can provide more granular locking insted of
>>> huge pcidevs_lock() which is used right now. I studied how this
>>> lock used and what it protects. And found the following:
>>>
>>> 0. Comment in pci.h states the following:
>>>
>>>   153 /*
>>>   154  * The pcidevs_lock protect alldevs_list, and the assignment for the
>>>   155  * devices, it also sync the access to the msi capability that is not
>>>   156  * interrupt handling related (the mask bit register).
>>>   157  */
>>>
>>> But in reality it does much more. Here is what I found:
>>>
>>> 1. Lifetime of pci_dev struct
>>>
>>> 2. Access to pseg->alldevs_list
>>>
>>> 3. Access to domain->pdev_list
>>>
>>> 4. Access to iommu->ats_list
>>>
>>> 5. Access to MSI capability
>>>
>>> 6. Some obsucure stuff in IOMMU drivers: there are places that
>>> are guarded by pcidevs_lock() but it seems that nothing
>>> PCI-related happens there.
>>
>> Right - the lock being held was (ab)used in IOMMU code in a number of
>> places. This likely needs to change in the course of this re-work;
>> patch titles don't suggest this is currently part of the series.
>>
>>> 7. Something that I probably overlooked
>>
>> And this is the main risk here. The huge scope of the original lock
>> means that many things are serialized now but won't be anymore once
>> the lock is gone.
>>
>> But yes - thanks for the work. To be honest I don't expect to be able
>> to look at this series in detail until after the Xen Summit. And even
>> then it may take a while ...
> 
> I was wondering if this is still in your list to review?

Yes, it certainly is. But as before no predictions when I might get to it.

Jan