Re: [PATCH v9 02/16] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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