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_andrushche...@epam.com>
>>
>> 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
