On 12.01.2024 19:14, Stewart Hildebrand wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com> > > Use the per-domain PCI read/write lock to protect the presence of the > pci device vpci field. This lock can be used (and in a few cases is used > right away) so that vpci removal can be performed while holding the lock > in write mode. Previously such removal could race with vpci_read for > example. > > When taking both d->pci_lock and pdev->vpci->lock, they should be > taken in this exact order: d->pci_lock then pdev->vpci->lock to avoid > possible deadlock situations. > > 1. Per-domain's pci_lock is used to protect pdev->vpci structure > from being removed. > > 2. Writing the command register and ROM BAR register may trigger > modify_bars to run, which in turn may access multiple pdevs while > checking for the existing BAR's overlap. The overlapping check, if > done under the read lock, requires vpci->lock to be acquired on both > devices being compared, which may produce a deadlock. It is not > possible to upgrade read lock to write lock in such a case. So, in > order to prevent the deadlock, use d->pci_lock in write mode instead. > > All other code, which doesn't lead to pdev->vpci destruction and does > not access multiple pdevs at the same time, can still use a > combination of the read lock and pdev->vpci->lock. > > 3. Drop const qualifier where the new rwlock is used and this is > appropriate. > > 4. Do not call process_pending_softirqs with any locks held. For that > unlock prior the call and re-acquire the locks after. After > re-acquiring the lock there is no need to check if pdev->vpci exists: > - in apply_map because of the context it is called (no race condition > possible) > - for MSI/MSI-X debug code because it is called at the end of > pdev->vpci access and no further access to pdev->vpci is made > > 5. Use d->pci_lock around for_each_pdev and pci_get_pdev_by_domain > while accessing pdevs in vpci code. > > Suggested-by: Roger Pau Monné <roger....@citrix.com> > Suggested-by: Jan Beulich <jbeul...@suse.com> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com> > Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com> > Reviewed-by: Roger Pau Monné <roger....@citrix.com>
While I know Roger did offer the tag with certain adjustments, ... > @@ -913,7 +911,12 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) > struct pci_dev *pdev = msix->pdev; > > spin_unlock(&msix->pdev->vpci->lock); > + read_unlock(&pdev->domain->pci_lock); > process_pending_softirqs(); > + > + if ( !read_trylock(&pdev->domain->pci_lock) ) > + return -EBUSY; > + > /* NB: we assume that pdev cannot go away for an alive domain. */ > if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) ) > return -EBUSY; ... I'm sure he was assuming you would get this right, in also dropping the 1st-try-acquired lock when this 2nd try-lock fails. Personally I feel this is the kind of change one would better not offer (or take) R-b ahead of time. I further think the respective comment in vpci_dump_msi() also wants adjusting from singular to plural. Jan
