[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH v4] x86/msi: fix locking for SR-IOV devices
In commit 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci structure") a lock moved from allocate_and_map_msi_pirq() to the caller and changed from pcidevs_lock() to read_lock(&d->pci_lock). However, one call path wasn't updated to reflect the change, leading to a failed assertion observed under the following conditions: * PV dom0 * Debug build (debug=y) of Xen * There is an SR-IOV device in the system with one or more VFs enabled * Dom0 has loaded the driver for the VF and enabled MSI-X (XEN) Assertion 'd || pcidevs_locked()' failed at drivers/passthrough/pci.c:535 (XEN) ----[ Xen-4.20-unstable x86_64 debug=y Not tainted ]---- ... (XEN) Xen call trace: (XEN) [<ffff82d040284da8>] R pci_get_pdev+0x4c/0xab (XEN) [<ffff82d040344f5c>] F arch/x86/msi.c#read_pci_mem_bar+0x58/0x272 (XEN) [<ffff82d04034530e>] F arch/x86/msi.c#msix_capability_init+0x198/0x755 (XEN) [<ffff82d040345dad>] F arch/x86/msi.c#__pci_enable_msix+0x82/0xe8 (XEN) [<ffff82d0403463e5>] F pci_enable_msi+0x3f/0x78 (XEN) [<ffff82d04034be2b>] F map_domain_pirq+0x2a4/0x6dc (XEN) [<ffff82d04034d4d5>] F allocate_and_map_msi_pirq+0x103/0x262 (XEN) [<ffff82d04035da5d>] F physdev_map_pirq+0x210/0x259 (XEN) [<ffff82d04035e798>] F do_physdev_op+0x9c3/0x1454 (XEN) [<ffff82d040329475>] F pv_hypercall+0x5ac/0x6af (XEN) [<ffff82d0402012d3>] F lstar_enter+0x143/0x150 In read_pci_mem_bar(), the VF obtains the struct pci_dev pointer for its associated PF to access the vf_rlen array. This array is initialized in pci_add_device() and is only populated in the associated PF's struct pci_dev. Add links between the VF's struct pci_dev and associated PF struct pci_dev, ensuring the PF's struct doesn't get deallocated until all its VFs have been removed. Access the vf_rlen array via the new link to the PF, and remove the troublesome call to pci_get_pdev(). Add a call to pci_get_pdev() inside the pcidevs_lock()-locked section of pci_add_device() to set up the link from VF to PF. In case the new pci_get_pdev() invocation fails to find the associated PF (returning NULL), return an error. In pci_remove_device(), handle an issue with a corner case when a PF is removed with VFs enabled, then re-added with VFs disabled. During PF removal, a buggy PF driver may fail to disable SR-IOV, leaving stale dangling VFs. At least Linux warns in this case: [ 106.500334] 0000:01:00.0: driver left SR-IOV enabled after remove If the PF is subsequently re-added with VFs disabled, the previously associated VFs in Xen would not be backed by a device. Any attempt to access the config space of the stale VF would be invalid. Avoid issues in this case by removing the VFs right away when removing the PF. This also happens to simplify the maintenance of the PF<->VF links since the associated PF will always exist during the lifetime of a VF. Note, however, if a PF is removed before the VFs, Xen will return an error for the VF removals since they were already removed. Fixes: 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci structure") Reported-by: Teddy Astie <teddy.astie@xxxxxxxxxx> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx> --- Candidate for backport to 4.19 v3->v4: * handle case when PF is removed with VFs enabled, then re-added with VFs disabled v2->v3: * link from VF to PF's struct pci_dev * v1->v2: * remove call to pci_get_pdev() --- When I tested removing a PF with VFs enabled, then re-adding with VFs disabled, I observed the following Xen crash when dom0 attempted to access the config space of a stale VF: (XEN) Assertion 'pos' failed at arch/x86/msi.c:1275 (XEN) ----[ Xen-4.20-unstable x86_64 debug=y Tainted: C ]---- ... (XEN) Xen call trace: (XEN) [<ffff82d040346834>] R pci_msi_conf_write_intercept+0xa2/0x1de (XEN) [<ffff82d04035d6b4>] F pci_conf_write_intercept+0x68/0x78 (XEN) [<ffff82d0403264e5>] F arch/x86/pv/emul-priv-op.c#pci_cfg_ok+0xa0/0x114 (XEN) [<ffff82d04032660e>] F arch/x86/pv/emul-priv-op.c#guest_io_write+0xb5/0x1c8 (XEN) [<ffff82d0403267bb>] F arch/x86/pv/emul-priv-op.c#write_io+0x9a/0xe0 (XEN) [<ffff82d04037c77a>] F x86_emulate+0x100e5/0x25f1e (XEN) [<ffff82d0403941a8>] F x86_emulate_wrapper+0x29/0x64 (XEN) [<ffff82d04032802b>] F pv_emulate_privileged_op+0x12e/0x217 (XEN) [<ffff82d040369f12>] F do_general_protection+0xc2/0x1b8 (XEN) [<ffff82d040201aa7>] F x86_64/entry.S#handle_exception_saved+0x2b/0x8c This crash is avoided by removing the VFs right away with the PF. --- xen/arch/x86/msi.c | 33 +++++++++++---------- xen/drivers/passthrough/pci.c | 55 ++++++++++++++++++++++++++++++++--- xen/include/xen/pci.h | 13 ++++++++- 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index 0c97fbb3fc03..8d54f268dbbf 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -662,34 +662,31 @@ static int msi_capability_init(struct pci_dev *dev, return 0; } -static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf) +static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf, + const struct pf_info *pf_info) { + pci_sbdf_t sbdf = PCI_SBDF(seg, bus, slot, func); u8 limit; u32 addr, base = PCI_BASE_ADDRESS_0; u64 disp = 0; if ( vf >= 0 ) { - struct pci_dev *pdev = pci_get_pdev(NULL, - PCI_SBDF(seg, bus, slot, func)); unsigned int pos; uint16_t ctrl, num_vf, offset, stride; - if ( !pdev ) - return 0; - - pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_SRIOV); - ctrl = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_CTRL); - num_vf = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_NUM_VF); - offset = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_VF_OFFSET); - stride = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_VF_STRIDE); + pos = pci_find_ext_capability(sbdf, PCI_EXT_CAP_ID_SRIOV); + ctrl = pci_conf_read16(sbdf, pos + PCI_SRIOV_CTRL); + num_vf = pci_conf_read16(sbdf, pos + PCI_SRIOV_NUM_VF); + offset = pci_conf_read16(sbdf, pos + PCI_SRIOV_VF_OFFSET); + stride = pci_conf_read16(sbdf, pos + PCI_SRIOV_VF_STRIDE); if ( !pos || !(ctrl & PCI_SRIOV_CTRL_VFE) || !(ctrl & PCI_SRIOV_CTRL_MSE) || !num_vf || !offset || (num_vf > 1 && !stride) || bir >= PCI_SRIOV_NUM_BARS || - !pdev->vf_rlen[bir] ) + !pf_info->vf_rlen[bir] ) return 0; base = pos + PCI_SRIOV_BAR; vf -= PCI_BDF(bus, slot, func) + offset; @@ -703,8 +700,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf) } if ( vf >= num_vf ) return 0; - BUILD_BUG_ON(ARRAY_SIZE(pdev->vf_rlen) != PCI_SRIOV_NUM_BARS); - disp = vf * pdev->vf_rlen[bir]; + BUILD_BUG_ON(ARRAY_SIZE(pf_info->vf_rlen) != PCI_SRIOV_NUM_BARS); + disp = vf * pf_info->vf_rlen[bir]; limit = PCI_SRIOV_NUM_BARS; } else switch ( pci_conf_read8(PCI_SBDF(seg, bus, slot, func), @@ -813,6 +810,7 @@ static int msix_capability_init(struct pci_dev *dev, int vf; paddr_t pba_paddr; unsigned int pba_offset; + const struct pf_info *pf_info; if ( !dev->info.is_virtfn ) { @@ -820,6 +818,7 @@ static int msix_capability_init(struct pci_dev *dev, pslot = slot; pfunc = func; vf = -1; + pf_info = NULL; } else { @@ -827,9 +826,11 @@ static int msix_capability_init(struct pci_dev *dev, pslot = PCI_SLOT(dev->info.physfn.devfn); pfunc = PCI_FUNC(dev->info.physfn.devfn); vf = dev->sbdf.bdf; + pf_info = &dev->virtfn.pf_pdev->physfn; } - table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf); + table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf, + pf_info); WARN_ON(msi && msi->table_base != table_paddr); if ( !table_paddr ) { @@ -852,7 +853,7 @@ static int msix_capability_init(struct pci_dev *dev, pba_offset = pci_conf_read32(dev->sbdf, msix_pba_offset_reg(pos)); bir = (u8)(pba_offset & PCI_MSIX_BIRMASK); - pba_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf); + pba_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf, pf_info); WARN_ON(!pba_paddr); pba_paddr += pba_offset & ~PCI_MSIX_BIRMASK; diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 5a446d3dcee0..97b3417252f7 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -341,6 +341,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) list_add(&pdev->alldevs_list, &pseg->alldevs_list); + INIT_LIST_HEAD(&pdev->physfn.vf_list); + /* update bus2bridge */ switch ( pdev->type = pdev_type(pseg->nr, bus, devfn) ) { @@ -446,6 +448,10 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev) list_del(&pdev->alldevs_list); pdev_msi_deinit(pdev); + + if ( pdev->info.is_virtfn ) + list_del(&pdev->virtfn.entry); + xfree(pdev); } @@ -700,10 +706,31 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, * extended function. */ if ( pdev->info.is_virtfn ) + { + struct pci_dev *pf_pdev; + pdev->info.is_extfn = pf_is_extfn; + pf_pdev = pci_get_pdev(NULL, + PCI_SBDF(seg, info->physfn.bus, + info->physfn.devfn)); + if ( pf_pdev ) + { + pdev->virtfn.pf_pdev = pf_pdev; + list_add(&pdev->virtfn.entry, &pf_pdev->physfn.vf_list); + } + else + { + printk(XENLOG_WARNING "Failed to add SR-IOV device PF %pp for VF %pp\n", + &PCI_SBDF(seg, info->physfn.bus, info->physfn.devfn), + &pdev->sbdf); + free_pdev(pseg, pdev); + ret = -ENODEV; + goto out; + } + } } - if ( !pdev->info.is_virtfn && !pdev->vf_rlen[0] ) + if ( !pdev->info.is_virtfn && !pdev->physfn.vf_rlen[0] ) { unsigned int pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_SRIOV); @@ -715,7 +742,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, { unsigned int i; - BUILD_BUG_ON(ARRAY_SIZE(pdev->vf_rlen) != PCI_SRIOV_NUM_BARS); + BUILD_BUG_ON(ARRAY_SIZE(pdev->physfn.vf_rlen) != + PCI_SRIOV_NUM_BARS); + for ( i = 0; i < PCI_SRIOV_NUM_BARS; ) { unsigned int idx = pos + PCI_SRIOV_BAR + i * 4; @@ -730,7 +759,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, continue; } ret = pci_size_mem_bar(pdev->sbdf, idx, NULL, - &pdev->vf_rlen[i], + &pdev->physfn.vf_rlen[i], PCI_BAR_VF | ((i == PCI_SRIOV_NUM_BARS - 1) ? PCI_BAR_LAST : 0)); @@ -818,6 +847,24 @@ 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 ) { + ret = 0; + + if ( !pdev->info.is_virtfn ) + { + struct pci_dev *vf_pdev, *tmp; + + /* + * If a PF with VFs enabled is removed, then re-added without + * VFs enabled, the previously associated VFs would no longer be + * backed by a device. Remove the associated VFs right away. + */ + list_for_each_entry_safe(vf_pdev, tmp, &pdev->physfn.vf_list, + virtfn.entry) + ret = pci_remove_device(vf_pdev->sbdf.seg, + vf_pdev->sbdf.bus, + vf_pdev->sbdf.devfn) ?: ret; + } + if ( pdev->domain ) { write_lock(&pdev->domain->pci_lock); @@ -826,7 +873,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) write_unlock(&pdev->domain->pci_lock); } pci_cleanup_msi(pdev); - ret = iommu_remove_device(pdev); + ret = iommu_remove_device(pdev) ?: ret; printk(XENLOG_DEBUG "PCI remove device %pp\n", &pdev->sbdf); free_pdev(pseg, pdev); break; diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 63e49f0117e9..a24026d25148 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -150,7 +150,18 @@ struct pci_dev { unsigned int count; #define PT_FAULT_THRESHOLD 10 } fault; - u64 vf_rlen[6]; + union { + struct pf_info { + /* Only populated for PFs (info.is_virtfn == false) */ + struct list_head vf_list; /* List head */ + uint64_t vf_rlen[PCI_SRIOV_NUM_BARS]; + } physfn; + struct { + /* Only populated for VFs (info.is_virtfn == true) */ + struct list_head entry; /* Entry in vf_list */ + struct pci_dev *pf_pdev; /* Link from VF to PF */ + } virtfn; + }; /* Data for vPCI. */ struct vpci *vpci; base-commit: b8cdfac2be38c98dd3ad0e911a3f7f787f5bcf6b -- 2.46.0
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |