On Wed, Sep 20, 2023 at 03:16:00PM -0400, Stewart Hildebrand wrote:
> 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?
I guess there was a reason to expect a path with v == NULL, but would
need to go trough the call paths that lead here.
Another option might be use use:
ASSERT(pcidevs_locked() || (v && rw_is_locked(&v->domain->pci_lock)));
But we would need some understanding of the call site of
vmx_pi_update_irte().
>
> >> 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));
Hm, it would be better to perform the ASSERT before possibly accessing
the pdev list without holding any locks, but it's just an assert so
that might be the best option.
>
> >> @@ -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.
Hm, yes, that's likely fine, but would want a small mention in the
commit message.
> >> 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));
Hm, strictly speaking iterating over the pseg list while just holding
the d->pci_lock is not safe, we should instead iterate over d->pdev_list.
We might have to slightly modify pci_enable_msi() to take a pdev so
that the search can be done by the caller (holding the right lock).
Thanks, Roger.
