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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.