On 9/19/23 11:39, Roger Pau Monné wrote: > On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote: >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >> index 1edc7f1e91..545a27796e 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -413,8 +413,6 @@ static int cf_check vmx_pi_update_irte(const struct vcpu >> *v, >> >> spin_unlock_irq(&desc->lock); >> >> - ASSERT(pcidevs_locked()); >> - > > Hm, this removal seems dubious, same with some of the removal below. > And I don't see any comment in the log message as to why removing the > asserts here and in __pci_enable_msi{,x}(), pci_prepare_msix() is > safe. >
I suspect we may want: ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock)); However, we don't have d. Using v->domain here is tricky because v may be NULL. How about passing struct domain *d as an arg to {hvm,vmx}_pi_update_irte()? Or ensuring that all callers pass a valid v? >> return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg); >> >> unlock_out: >> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c >> index d0bf63df1d..ba2963b7d2 100644 >> --- a/xen/arch/x86/msi.c >> +++ b/xen/arch/x86/msi.c >> @@ -613,7 +613,7 @@ static int msi_capability_init(struct pci_dev *dev, >> u8 slot = PCI_SLOT(dev->devfn); >> u8 func = PCI_FUNC(dev->devfn); >> >> - ASSERT(pcidevs_locked()); >> + ASSERT(pcidevs_locked() || rw_is_locked(&dev->domain->pci_lock)); >> pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI); >> if ( !pos ) >> return -ENODEV; >> @@ -783,7 +783,7 @@ static int msix_capability_init(struct pci_dev *dev, >> if ( !pos ) >> return -ENODEV; >> >> - ASSERT(pcidevs_locked()); >> + ASSERT(pcidevs_locked() || rw_is_locked(&dev->domain->pci_lock)); >> >> control = pci_conf_read16(dev->sbdf, msix_control_reg(pos)); >> /* >> @@ -1000,7 +1000,6 @@ static int __pci_enable_msi(struct msi_info *msi, >> struct msi_desc **desc) >> struct pci_dev *pdev; >> struct msi_desc *old_desc; >> >> - ASSERT(pcidevs_locked()); >> pdev = pci_get_pdev(NULL, msi->sbdf); >> if ( !pdev ) >> return -ENODEV; I think we can move the ASSERT here, after we obtain the pdev. Then we can add the pdev->domain->pci_lock check into the mix: ASSERT(pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock)); >> @@ -1055,7 +1054,6 @@ static int __pci_enable_msix(struct msi_info *msi, >> struct msi_desc **desc) >> struct pci_dev *pdev; >> struct msi_desc *old_desc; >> >> - ASSERT(pcidevs_locked()); >> pdev = pci_get_pdev(NULL, msi->sbdf); >> if ( !pdev || !pdev->msix ) >> return -ENODEV; Same here >> @@ -1170,8 +1168,6 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool >> off) >> */ >> int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc) >> { >> - ASSERT(pcidevs_locked()); >> - This removal inside pci_enable_msi() may be okay if both __pci_enable_msi() and __pci_enable_msix() have an appropriate ASSERT. >> if ( !use_msi ) >> return -EPERM; >> Related: in xen/drivers/passthrough/pci.c:pci_get_pdev() I run into an ASSERT with a PVH dom0: (XEN) Assertion 'd || pcidevs_locked()' failed at drivers/passthrough/pci.c:534 (XEN) ----[ Xen-4.18-unstable x86_64 debug=y Tainted: C ]---- ... (XEN) Xen call trace: (XEN) [<ffff82d040285a3b>] R pci_get_pdev+0x4c/0xab (XEN) [<ffff82d04034742e>] F arch/x86/msi.c#__pci_enable_msi+0x1d/0xb4 (XEN) [<ffff82d0403477b5>] F pci_enable_msi+0x20/0x28 (XEN) [<ffff82d04034cfa4>] F map_domain_pirq+0x2b0/0x718 (XEN) [<ffff82d04034e37c>] F allocate_and_map_msi_pirq+0xff/0x26b (XEN) [<ffff82d0402e088b>] F arch/x86/hvm/vmsi.c#vpci_msi_enable+0x53/0x9d (XEN) [<ffff82d0402e19d5>] F vpci_msi_arch_enable+0x36/0x6c (XEN) [<ffff82d04026f49d>] F drivers/vpci/msi.c#control_write+0x71/0x114 (XEN) [<ffff82d04026d050>] F drivers/vpci/vpci.c#vpci_write_helper+0x6f/0x7c (XEN) [<ffff82d04026de39>] F vpci_write+0x249/0x2f9 ... With the patch applied, it's valid to call pci_get_pdev() with only d->pci_lock held, so the ASSERT in pci_get_pdev() needs to be reworked too. Inside pci_get_pdev(), d may be null, so we can't easily add || rw_is_locked(&d->pci_lock) into the ASSERT. Instead I propose something like the following, which resolves the observed assertion failure: diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 572643abe412..2b4ad804510c 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -531,8 +531,6 @@ struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf) { struct pci_dev *pdev; - ASSERT(d || pcidevs_locked()); - /* * The hardware domain owns the majority of the devices in the system. * When there are multiple segments, traversing the per-segment list is @@ -549,12 +547,18 @@ struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf) list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list ) if ( pdev->sbdf.bdf == sbdf.bdf && (!d || pdev->domain == d) ) + { + ASSERT(d || pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock)); return pdev; + } } else list_for_each_entry ( pdev, &d->pdev_list, domain_list ) if ( pdev->sbdf.sbdf == sbdf.sbdf ) + { + ASSERT(d || pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock)); return pdev; + } return NULL; }