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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2023 02:22, Volodymyr Babchuk wrote:
> 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".

I guess this may indeed be the "best" approach for now - introduce
refcounting to use where relevant for new work, and then slowly see about
replacing (dropping) locking where a refcount suffices when one is held.

Jan