On 09.03.2023 02:22, Volodymyr Babchuk wrote: > Jan Beulich <jbeul...@suse.com> writes: >> On 21.02.2023 00:13, Volodymyr Babchuk wrote: >>> Stefano Stabellini <sstabell...@kernel.org> 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_babc...@epam.com> >>>> >>>> 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
