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;
}
