Re: [RFC PATCH 08/10] xen: pci: remove pcidev_[un]lock[ed] calls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2023 00:13, Volodymyr Babchuk wrote:
> Stefano Stabellini <sstabellini@xxxxxxxxxx> writes:
>> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
>>> As pci devices are refcounted now and all list that store them are
>>> protected by separate locks, we can safely drop global pcidevs_lock.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>>
>> Up until this patch this patch series introduces:
>> - d->pdevs_lock to protect d->pdev_list
>> - pci_seg->alldevs_lock to protect pci_seg->alldevs_list
>> - iommu->ats_list_lock to protect iommu->ats_devices
>> - pdev refcounting to detect a pdev in-use and when to free it
>> - pdev->lock to protect pdev->msi_list
>>
>> They cover a lot of ground.  Are they collectively covering everything
>> pcidevs_lock() was protecting?
> 
> Well, that is the question. Those patch are in RFC stage because I can't
> fully answer your question. I tried my best to introduce proper locking,
> but apparently missed couple of places, like
> 
>> deassign_device is not protected by pcidevs_lock anymore.
>> deassign_device accesses a number of pdev fields, including quarantine,
>> phantom_stride and fault.count.
>>
>> deassign_device could run at the same time as assign_device who sets
>> quarantine and other fields.
>>
> 
> I hope this is all, but problem is that PCI subsystem is old, large and
> complex. Fo example, as I wrote earlier, there are places that are
> protected with pcidevs_lock(), but do nothing with PCI. I just don't
> know what to do with such places. I have a hope that x86 maintainers
> would review my changes and give feedback on missed spots.

At the risk of it sounding unfair, at least initially: While review may
spot issues, you will want to keep in mind that none of the people who
originally wrote that code are around anymore. And even if they were,
it would be uncertain - just like for the x86 maintainers - that they
would recall (if they were aware at some time in the first place) all
the corner cases. Therefore I'm afraid that proving correctness and
safety of the proposed transformations can only be done by properly
auditing all involved code paths. Yet that's something that imo wants
to already have been done by the time patches are submitted for review.
Reviewers would then "merely" (hard enough perhaps) check the results
of that audit.

I might guess that this locking situation is one of the reasons why
Andrew in particular thinks (afaik) that the IOMMU code we have would
better be re-written almost from scratch. I assume it's clear to him
(it certainly is to me) that this is something that could only be
expected to happen in an ideal work: I see no-one taking on such an
exercise. We already have too little bandwidth.

Jan