On 29.02.2024 14:15, Roger Pau Monne wrote:
> Currently vpci_deassign_device() is called without holding the per-domain
> pci_lock in pci_remove_device(), which leads to:
>
> Assertion 'rw_is_write_locked(&pdev->domain->pci_lock)' failed at
> ../drivers/vpci/vpci.c:47
> [...]
> Xen call trace:
> [<ffff82d040260eac>] R vpci_deassign_device+0x10d/0x1b9
> [<ffff82d04027932f>] S pci_remove_device+0x2b1/0x380
> [<ffff82d040260bd0>] F pci_physdev_op+0x197/0x19e
> [<ffff82d04032272d>] F do_physdev_op+0x342/0x12aa
> [<ffff82d0402f067a>] F pv_hypercall+0x58e/0x62b
> [<ffff82d0402012ba>] F lstar_enter+0x13a/0x140
>
> Move the existing block that removes the device from the domain pdev_list
> ahead
> and also issue the call to vpci_deassign_device() there. It's fine to remove
> the device from the domain list of assigned devices, as further functions only
> care that the pdev domain field is correctly set to the owner of the device
> about to be removed.
>
> Moving the vpci_deassign_device() past the pci_cleanup_msi() call can be
> dangerous, as doing the MSI cleanup ahead of having removed the vPCI handlers
> could lead to stale data in vPCI MSI(-X) internal structures.
>
> Fixes: 4f78438b45e2 ('vpci: use per-domain PCI lock to protect vpci
> structure')
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -817,15 +817,15 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> if ( pdev->bus == bus && pdev->devfn == devfn )
> {
> - vpci_deassign_device(pdev);
> - pci_cleanup_msi(pdev);
> - ret = iommu_remove_device(pdev);
> if ( pdev->domain )
> {
> write_lock(&pdev->domain->pci_lock);
> + vpci_deassign_device(pdev);
> list_del(&pdev->domain_list);
> write_unlock(&pdev->domain->pci_lock);
This then also is properly symmetric with what pci_add_device() has.
Jan
> }
> + pci_cleanup_msi(pdev);
> + ret = iommu_remove_device(pdev);
> printk(XENLOG_DEBUG "PCI remove device %pp\n", &pdev->sbdf);
> free_pdev(pseg, pdev);
> break;
