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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Jan,

Jan Beulich <jbeulich@xxxxxxxx> writes:

> 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.

The more I dig into IOMMU code, the more I agree with Andrew. I can't
see how current PCI locking can be untangled in the IOMMU code. There
are just too many moving parts. I tried to play with static code
analysis tools, but I haven't find anything that can reliably analyze
locking in Xen. I even tried to write own tool tailored specifically for
PCI locking analysis. While it works on some synthetic tests, there is
too much work to support actual Xen code.

I am not able to rework x86 IOMMU code. So, I am inclined to drop this
patch series at all. My current plan is to take minimal refcounting from
this series to satisfy your comments for "vpci: use pcidevs locking to
protect MMIO handlers".

-- 
WBR, Volodymyr