In commit 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci
structure") a lock was 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 (CONFIG_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.
Access the vf_rlen array via the link to the PF, and remove the
troublesome call to pci_get_pdev().
Fixes: 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci structure")
Reported-by: Teddy Astie <teddy.astie@vates.tech>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Candidate for backport to 4.19
Patch #2 ("xen/pci: introduce PF<->VF links") is pre-requisite
v4->v5:
* split the PF<->VF links to a pre-requisite patch
* pass pci_sbdf_t to read_pci_mem_bar()
* use stdint.h types on changed lines
* re-add NULL check for pf_info in read_pci_mem_bar(), as pf_info could
be NULL
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()
---
xen/arch/x86/msi.c | 37 ++++++++++++++++++++++-------------
xen/drivers/passthrough/pci.c | 8 +++++---
xen/include/xen/pci.h | 15 +++++++++-----
3 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index fbb07fe821b5..254a403dfb51 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -662,34 +662,35 @@ 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 uint64_t read_pci_mem_bar(pci_sbdf_t sbdf, uint8_t bir, int vf,
+ const struct pf_info *pf_info)
{
+ uint16_t seg = sbdf.seg;
+ uint8_t bus = sbdf.bus, slot = sbdf.dev, func = sbdf.fn;
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 )
+ if ( !pf_info )
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 +704,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 +814,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 +822,7 @@ static int msix_capability_init(struct pci_dev *dev,
pslot = slot;
pfunc = func;
vf = -1;
+ pf_info = NULL;
}
else
{
@@ -827,9 +830,14 @@ 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;
+ if ( dev->virtfn.pf_pdev )
+ pf_info = &dev->virtfn.pf_pdev->physfn;
+ else
+ pf_info = NULL;
}
- table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
+ table_paddr = read_pci_mem_bar(PCI_SBDF(seg, pbus, pslot, pfunc), bir,
+ vf, pf_info);
WARN_ON(msi && msi->table_base != table_paddr);
if ( !table_paddr )
{
@@ -852,7 +860,8 @@ 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(PCI_SBDF(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 95a8ed850efd..8209efdebecf 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -736,7 +736,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
}
}
- 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);
@@ -748,7 +748,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;
@@ -763,7 +765,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));
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index f9435b7f4eb9..12f1c599e467 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -156,11 +156,16 @@ struct pci_dev {
* List entry if info.is_virtfn == true
*/
struct list_head vf_list;
- u64 vf_rlen[6];
- struct {
- /* Only populated for VFs (info.is_virtfn == true) */
- const struct pci_dev *pf_pdev; /* Link from VF to PF */
- } virtfn;
+ union {
+ struct pf_info {
+ /* Only populated for PFs (info.is_virtfn == false) */
+ uint64_t vf_rlen[PCI_SRIOV_NUM_BARS];
+ } physfn;
+ struct {
+ /* Only populated for VFs (info.is_virtfn == true) */
+ const struct pci_dev *pf_pdev; /* Link from VF to PF */
+ } virtfn;
+ };
/* Data for vPCI. */
struct vpci *vpci;
--
2.47.0On 11.10.2024 17:27, Stewart Hildebrand wrote:
> In commit 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci
> structure") a lock was 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 (CONFIG_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.
>
> Access the vf_rlen array via the link to the PF, and remove the
> troublesome call to pci_get_pdev().
>
> Fixes: 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci structure")
> Reported-by: Teddy Astie <teddy.astie@vates.tech>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -736,7 +736,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> }
> }
>
> - if ( !pdev->info.is_virtfn && !pdev->vf_rlen[0] )
> + if ( !pdev->info.is_virtfn && !pdev->physfn.vf_rlen[0] )
Unrelated to your change: Now that I look at this again, it seems slightly
wrong to me to use array slot 0 as "have we populated the array already"
indicator. If BAR0 was unused, we may end up doing this more than once.
Jan
© 2016 - 2024 Red Hat, Inc.