[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure
On 10.02.22 18:16, Roger Pau Monné wrote: > On Wed, Feb 09, 2022 at 03:36:27PM +0200, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >> >> Introduce a per-domain read/write lock to check whether vpci is present, >> so we are sure there are no accesses to the contents of the vpci struct >> if not. 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. > Sadly there's still a race in the usage of pci_get_pdev_by_domain wrt > pci_remove_device, and likely when vPCI gets also used in > {de}assign_device I think. > How about the below? It seems to guarantee that we can access pdev without issues and without requiring pcidevs_lock to be used? diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index e8b09d77d880..fd464a58b3b3 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -937,8 +937,14 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, } devfn = pdev->devfn; +#ifdef CONFIG_HAS_VPCI + write_lock(&d->vpci_rwlock); +#endif ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn, pci_to_dev(pdev)); +#ifdef CONFIG_HAS_VPCI + write_unlock(&d->vpci_rwlock); +#endif if ( ret ) goto out; @@ -1474,6 +1480,9 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) const struct domain_iommu *hd = dom_iommu(d); struct pci_dev *pdev; int rc = 0; +#ifdef CONFIG_HAS_VPCI + struct domain *old_d; +#endif if ( !is_iommu_enabled(d) ) return 0; @@ -1487,15 +1496,34 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) ASSERT(pdev && (pdev->domain == hardware_domain || pdev->domain == dom_io)); +#ifdef CONFIG_HAS_VPCI + /* pdev->domain is either hwdom or dom_io. We do not want the later. */ + old_d = pdev->domain == hardware_domain ? pdev->domain : NULL; + if ( old_d ) + write_lock(&old_d->vpci_rwlock); +#endif + rc = pdev_msix_assign(d, pdev); if ( rc ) + { +#ifdef CONFIG_HAS_VPCI + if ( old_d ) + write_unlock(&old_d->vpci_rwlock); +#endif goto done; + } pdev->fault.count = 0; if ( (rc = iommu_call(hd->platform_ops, assign_device, d, devfn, pci_to_dev(pdev), flag)) ) + { +#ifdef CONFIG_HAS_VPCI + if ( old_d ) + write_unlock(&old_d->vpci_rwlock); +#endif goto done; + } for ( ; pdev->phantom_stride; rc = 0 ) { I think we don't care about pci_remove_device because: int pci_remove_device(u16 seg, u8 bus, u8 devfn) { [snip] pcidevs_lock(); list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list ) if ( pdev->bus == bus && pdev->devfn == devfn ) { vpci_remove_device(pdev); as vpci_remove_device will take care there are no readers and will safely remove vpci. Thank you, Oleksandr
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |