[PATCH v2 0/5] x86: pass-through / {,v}PCI locking

[PATCH v2 0/5] x86: pass-through / {,v}PCI locking

[Jan Beulich]

Now that XSA-509 went out, here's what actually caused the issue to be
noticed, plus some follow-on cleanup.

1: x86/pass-through: alter locking around pt_irq_{create,destroy}_bind()
2: x86/vPCI: tighten locking assertions
3: x86/pass-through: use simpler locking primitives in pt_irq_{create,destroy}_bind()
4: x86/vPCI: make better use of "pdev" in vpci_msix_arch_print()
5: x86/vMSI: avoid "goto" in msixtbl_pt_unregister()

Jan

[PATCH v2 1/5] x86/pass-through: alter locking around pt_irq_{create,destroy}_bind()

[Jan Beulich]

The questionable use of pcidevs_lock() there was discussed more than once.
It really is pointless: The functions synchronize primarily via the per-
domain event lock. They also may already be called with the global PCI
devices lock not held: See hvm/vmsi.c:vpci_msi_update(),
hvm/vmsi.c:vpci_msi_arch_update(), and hvm/vmsi.c:vpci_msi_disable().

It's merely msixtbl_pt_{,un}register() which require the global or per-
domain PCI lock held. Switch to using the latter, which is also sufficient
to acquire in read mode. While strictly speaking this is needed only for
PT_IRQ_TYPE_MSI, keep it being unconditional. In vioapic_hwdom_map_gsi()
also unlock earlier.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Replace locking rather than dropping it altogether.

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -636,9 +636,9 @@ long arch_do_domctl(
             ret = -EPERM;
         else if ( is_iommu_enabled(d) )
         {
-            pcidevs_lock();
+            read_lock(&d->pci_lock);
             ret = pt_irq_create_bind(d, bind);
-            pcidevs_unlock();
+            read_unlock(&d->pci_lock);
 
             if ( ret < 0 )
                 printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for %pd\n",
@@ -670,9 +670,9 @@ long arch_do_domctl(
             ret = -EPERM;
         else if ( is_iommu_enabled(d) )
         {
-            pcidevs_lock();
+            read_lock(&d->pci_lock);
             ret = pt_irq_destroy_bind(d, bind);
-            pcidevs_unlock();
+            read_unlock(&d->pci_lock);
 
             if ( ret < 0 )
                 printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for %pd\n",
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -197,8 +197,9 @@ static int vioapic_hwdom_map_gsi(unsigne
         return ret;
     }
 
-    pcidevs_lock();
+    read_lock(&currd->pci_lock);
     ret = pt_irq_create_bind(currd, &pt_irq_bind);
+    read_unlock(&currd->pci_lock);
     if ( ret )
     {
         gprintk(XENLOG_WARNING, "vioapic: error binding GSI %u: %d\n",
@@ -207,7 +208,6 @@ static int vioapic_hwdom_map_gsi(unsigne
         unmap_domain_pirq(currd, pirq);
         write_unlock(&currd->event_lock);
     }
-    pcidevs_unlock();
 
     return ret;
 }

[PATCH v2 2/5] x86/vPCI: tighten locking assertions

[Jan Beulich]

Already when they were introduced, they seemed overly lax. In particular
anything invoked solely from vpci_{read,write}() can check that the per-
domain PCI r/w lock is held. There's no need to permit the alternative of
holding the global PCI devices lock.

vpci_msi_arch_update()'s sole call site is update_msi(), which in turn is
solely called from write handling hooks.

vpci_msi_update(), besides being called from vpci_msi_arch_update() (see
above), has two further call sites:
- vpci_msi_arch_enable(), called upon control register writes,
- vpci_msix_arch_enable_entry(), called solely from update_entry(), which
  in turn is again called upon control register writes, plus from
  msix_write(), which read-locks the domain's PCI lock.
Both arch_enable functions therefore can also have their assertions
adjusted.

vpci_msi_disable() is called from
- vpci_msi_arch_disable(), called upon control register writes, 
- vpci_msix_arch_enable_entry(), covered above,
- vpci_msix_arch_disable_entry(), called update_entry() (see above) and
  upon control register writes.

Furthermore msixtbl_pt_{,un}register() are solely called from
pt_irq_{create,destroy}_bind(), callers of which (create: when using
PT_IRQ_TYPE_MSI) acquire the per-domain lock.

As not using ASSERT_PDEV_LIST_IS_READ_LOCKED() isn't unusual anymore, drop
the comment near the top of vpci_msix_arch_print().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger@xenproject.org>
---
v2: Extend to msixtbl_pt_{,un}register(). Drop comment near the top of
    vpci_msix_arch_print().

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -621,7 +621,7 @@ int msixtbl_pt_register(struct domain *d
     struct msixtbl_entry *entry, *new_entry;
     int r = -EINVAL;
 
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(d);
+    ASSERT(rw_is_locked(&d->pci_lock));
     ASSERT(rw_is_write_locked(&d->event_lock));
 
     if ( !msixtbl_initialised(d) )
@@ -691,7 +691,7 @@ void msixtbl_pt_unregister(struct domain
     struct pci_dev *pdev;
     struct msixtbl_entry *entry;
 
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(d);
+    ASSERT(rw_is_locked(&d->pci_lock));
     ASSERT(rw_is_write_locked(&d->event_lock));
 
     if ( !msixtbl_initialised(d) )
@@ -837,7 +837,7 @@ static int vpci_msi_update(const struct
 {
     unsigned int i;
 
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(pdev->domain);
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
 
     if ( (address & MSI_ADDR_BASE_MASK) != MSI_ADDR_HEADER )
     {
@@ -878,7 +878,7 @@ void vpci_msi_arch_update(struct vpci_ms
     int rc;
 
     ASSERT(msi->arch.pirq != INVALID_PIRQ);
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(pdev->domain);
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
 
     for ( i = 0; i < msi->vectors && msi->arch.bound; i++ )
     {
@@ -930,7 +930,8 @@ int vpci_msi_arch_enable(struct vpci_msi
     int rc;
 
     ASSERT(msi->arch.pirq == INVALID_PIRQ);
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(pdev->domain);
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
+
     rc = vpci_msi_enable(pdev, vectors, 0);
     if ( rc < 0 )
         return rc;
@@ -948,7 +949,7 @@ static void vpci_msi_disable(const struc
     unsigned int i;
 
     ASSERT(pirq != INVALID_PIRQ);
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(pdev->domain);
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
 
     for ( i = 0; i < nr && bound; i++ )
     {
@@ -1004,7 +1005,8 @@ int vpci_msix_arch_enable_entry(struct v
     int rc;
 
     ASSERT(entry->arch.pirq == INVALID_PIRQ);
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(pdev->domain);
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
+
     rc = vpci_msi_enable(pdev, vmsix_entry_nr(pdev->vpci->msix, entry),
                          table_base);
     if ( rc < 0 )
@@ -1044,12 +1046,6 @@ int vpci_msix_arch_print(const struct vp
 {
     unsigned int i;
 
-    /*
-     * Assert that pdev_list doesn't change. ASSERT_PDEV_LIST_IS_READ_LOCKED
-     * is not suitable here because it allows either pcidevs_lock() or
-     * pci_lock to be held, but here we rely on pci_lock being held, not
-     * pcidevs_lock() (see the transient lock dropping further down).
-     */
     ASSERT(rw_is_locked(&msix->pdev->domain->pci_lock));
     ASSERT(spin_is_locked(&msix->pdev->vpci->lock));
 

[PATCH v2 3/5] x86/pass-through: use simpler locking primitives in pt_irq_{create,destroy}_bind()

[Jan Beulich]

Both functions already assume IRQs to be enabled upon entry, by e.g. their
acquiring of the domain's event channel lock. Hence like e.g.
hvm_migrate_pirq() (also called from here) does, saving/restoring of
EFLAGS.IF isn't necessary (because of the functions called, we can't
really avoid the saving there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger@xenproject.org>
---
v2: Add IRQ-s-enabled assertions.

--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -228,6 +228,8 @@ int pt_irq_create_bind(
     if ( pirq < 0 || pirq >= d->nr_pirqs )
         return -EINVAL;
 
+    ASSERT(local_irq_is_enabled());
+
  restart:
     write_lock(&d->event_lock);
 
@@ -400,8 +402,7 @@ int pt_irq_create_bind(
 
         if ( pt_irq_bind->u.msi.gflags & XEN_DOMCTL_VMSI_X86_UNMASKED )
         {
-            unsigned long flags;
-            struct irq_desc *desc = pirq_spin_lock_irq_desc(info, &flags);
+            struct irq_desc *desc = pirq_spin_lock_irq_desc(info, NULL);
 
             if ( !desc )
             {
@@ -411,7 +412,7 @@ int pt_irq_create_bind(
             }
 
             guest_mask_msi_irq(desc, false);
-            spin_unlock_irqrestore(&desc->lock, flags);
+            spin_unlock_irq(&desc->lock);
         }
 
         write_unlock(&d->event_lock);
@@ -588,6 +589,8 @@ int pt_irq_destroy_bind(
     struct pirq *pirq;
     const char *what = NULL;
 
+    ASSERT(local_irq_is_enabled());
+
     switch ( pt_irq_bind->irq_type )
     {
     case PT_IRQ_TYPE_PCI:
@@ -606,9 +609,7 @@ int pt_irq_destroy_bind(
         break;
     case PT_IRQ_TYPE_MSI:
     {
-        unsigned long flags;
-        struct irq_desc *desc = domain_spin_lock_irq_desc(d, machine_gsi,
-                                                          &flags);
+        struct irq_desc *desc = domain_spin_lock_irq_desc(d, machine_gsi, NULL);
 
         if ( !desc )
             return -EINVAL;
@@ -617,7 +618,7 @@ int pt_irq_destroy_bind(
          * pt_irq_create_bind is consistent across bind/unbinds.
          */
         guest_mask_msi_irq(desc, true);
-        spin_unlock_irqrestore(&desc->lock, flags);
+        spin_unlock_irq(&desc->lock);
         break;
     }
 

[PATCH v2 4/5] x86/vPCI: make better use of "pdev" in vpci_msix_arch_print()

[Jan Beulich]

Move the declaration to function scope, so that not only the (so far)
immediately following spin_unlock() can make use of it, but also the
assertions at the top. Also add const, as the pdev itself isn't supposed
to be altered here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -1044,10 +1044,11 @@ void vpci_msix_arch_init_entry(struct vp
 
 int vpci_msix_arch_print(const struct vpci_msix *msix)
 {
+    const struct pci_dev *pdev = msix->pdev;
     unsigned int i;
 
-    ASSERT(rw_is_locked(&msix->pdev->domain->pci_lock));
-    ASSERT(spin_is_locked(&msix->pdev->vpci->lock));
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
+    ASSERT(spin_is_locked(&pdev->vpci->lock));
 
     for ( i = 0; i < msix->max_entries; i++ )
     {
@@ -1064,9 +1065,7 @@ int vpci_msix_arch_print(const struct vp
                entry->masked, entry->arch.pirq);
         if ( i && !(i % 64) )
         {
-            struct pci_dev *pdev = msix->pdev;
-
-            spin_unlock(&msix->pdev->vpci->lock);
+            spin_unlock(&pdev->vpci->lock);
             read_unlock(&pdev->domain->pci_lock);
             process_pending_softirqs();
 

[PATCH v2 5/5] x86/vMSI: avoid "goto" in msixtbl_pt_unregister()

[Jan Beulich]

"goto" can be helpful in certain situations, yet where easily possible I
think it's better to avoid its use. With the re-arrangement also restrict
the scop of two of the local variables, and add const to one of them.

No functional changed intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -688,8 +688,6 @@ void msixtbl_pt_unregister(struct domain
 {
     struct irq_desc *irqd;
     struct msi_desc *msi_desc;
-    struct pci_dev *pdev;
-    struct msixtbl_entry *entry;
 
     ASSERT(rw_is_locked(&d->pci_lock));
     ASSERT(rw_is_write_locked(&d->event_lock));
@@ -702,22 +700,19 @@ void msixtbl_pt_unregister(struct domain
         return;
 
     msi_desc = irqd->msi_desc;
-    if ( !msi_desc )
-        goto out;
-
-    pdev = msi_desc->dev;
-
-    list_for_each_entry( entry, &d->arch.hvm.msixtbl_list, list )
-        if ( pdev == entry->pdev )
-            goto found;
-
-out:
-    spin_unlock_irq(&irqd->lock);
-    return;
-
-found:
-    if ( atomic_dec_and_test(&entry->refcnt) )
-        del_msixtbl_entry(entry);
+    if ( msi_desc )
+    {
+        const struct pci_dev *pdev = msi_desc->dev;
+        struct msixtbl_entry *entry;
+
+        list_for_each_entry( entry, &d->arch.hvm.msixtbl_list, list )
+            if ( pdev == entry->pdev )
+            {
+                if ( atomic_dec_and_test(&entry->refcnt) )
+                    del_msixtbl_entry(entry);
+                break;
+            }
+    }
 
     spin_unlock_irq(&irqd->lock);
 }

Re: [PATCH v2 1/5] x86/pass-through: alter locking around pt_irq_{create,destroy}_bind()

[Roger Pau Monné]

On Wed, Sep 30, 2026 at 01:42:29PM +0200, Jan Beulich wrote:
> The questionable use of pcidevs_lock() there was discussed more than once.
> It really is pointless: The functions synchronize primarily via the per-
> domain event lock. They also may already be called with the global PCI
> devices lock not held: See hvm/vmsi.c:vpci_msi_update(),
> hvm/vmsi.c:vpci_msi_arch_update(), and hvm/vmsi.c:vpci_msi_disable().
> 
> It's merely msixtbl_pt_{,un}register() which require the global or per-
> domain PCI lock held. Switch to using the latter, which is also sufficient
> to acquire in read mode. While strictly speaking this is needed only for
> PT_IRQ_TYPE_MSI, keep it being unconditional. In vioapic_hwdom_map_gsi()
> also unlock earlier.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Replace locking rather than dropping it altogether.
> 
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -636,9 +636,9 @@ long arch_do_domctl(
>              ret = -EPERM;
>          else if ( is_iommu_enabled(d) )
>          {
> -            pcidevs_lock();
> +            read_lock(&d->pci_lock);
>              ret = pt_irq_create_bind(d, bind);
> -            pcidevs_unlock();
> +            read_unlock(&d->pci_lock);
>  
>              if ( ret < 0 )
>                  printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for %pd\n",
> @@ -670,9 +670,9 @@ long arch_do_domctl(
>              ret = -EPERM;
>          else if ( is_iommu_enabled(d) )
>          {
> -            pcidevs_lock();
> +            read_lock(&d->pci_lock);
>              ret = pt_irq_destroy_bind(d, bind);
> -            pcidevs_unlock();
> +            read_unlock(&d->pci_lock);
>  
>              if ( ret < 0 )
>                  printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for %pd\n",
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -197,8 +197,9 @@ static int vioapic_hwdom_map_gsi(unsigne
>          return ret;
>      }
>  
> -    pcidevs_lock();
> +    read_lock(&currd->pci_lock);
>      ret = pt_irq_create_bind(currd, &pt_irq_bind);
> +    read_unlock(&currd->pci_lock);

I think you need to extend the ->pci_lock'ed region so it also covers
unmap_domain_pirq(), otherwise the ASSERT_PDEV_LIST_IS_READ_LOCKED()
in there will trigger?  IE: the read_unlock() needs to be where the
pcidevs_unlock() is?

Thanks, Roger.

Re: [PATCH v2 4/5] x86/vPCI: make better use of "pdev" in vpci_msix_arch_print()

[Roger Pau Monné]

On Wed, Sep 30, 2026 at 01:43:34PM +0200, Jan Beulich wrote:
> Move the declaration to function scope, so that not only the (so far)
> immediately following spin_unlock() can make use of it, but also the
> assertions at the top. Also add const, as the pdev itself isn't supposed
> to be altered here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH v2 5/5] x86/vMSI: avoid "goto" in msixtbl_pt_unregister()

[Roger Pau Monné]

On Wed, Sep 30, 2026 at 01:43:59PM +0200, Jan Beulich wrote:
> "goto" can be helpful in certain situations, yet where easily possible I
> think it's better to avoid its use. With the re-arrangement also restrict
> the scop of two of the local variables, and add const to one of them.
> 
> No functional changed intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH v2 1/5] x86/pass-through: alter locking around pt_irq_{create,destroy}_bind()

[Jan Beulich]

On 28.10.2026 15:33, Roger Pau Monné wrote:
> On Wed, Sep 30, 2026 at 01:42:29PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vioapic.c
>> +++ b/xen/arch/x86/hvm/vioapic.c
>> @@ -197,8 +197,9 @@ static int vioapic_hwdom_map_gsi(unsigne
>>          return ret;
>>      }
>>  
>> -    pcidevs_lock();
>> +    read_lock(&currd->pci_lock);
>>      ret = pt_irq_create_bind(currd, &pt_irq_bind);
>> +    read_unlock(&currd->pci_lock);
> 
> I think you need to extend the ->pci_lock'ed region so it also covers
> unmap_domain_pirq(), otherwise the ASSERT_PDEV_LIST_IS_READ_LOCKED()
> in there will trigger?  IE: the read_unlock() needs to be where the
> pcidevs_unlock() is?

Oh, indeed. That would only be an issue if an error arose, so it's
clear why I didn't hit that assertion.

This being the only change (which I've made locally) necessary for
the entire series - shall I re-send, or would you mind giving an ack
for the patch with said adjustment made?

Jan

Re: [PATCH v2 1/5] x86/pass-through: alter locking around pt_irq_{create,destroy}_bind()

[Roger Pau Monné]

On Wed, Oct 28, 2026 at 04:21:15PM +0100, Jan Beulich wrote:
> On 28.10.2026 15:33, Roger Pau Monné wrote:
> > On Wed, Sep 30, 2026 at 01:42:29PM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/hvm/vioapic.c
> >> +++ b/xen/arch/x86/hvm/vioapic.c
> >> @@ -197,8 +197,9 @@ static int vioapic_hwdom_map_gsi(unsigne
> >>          return ret;
> >>      }
> >>  
> >> -    pcidevs_lock();
> >> +    read_lock(&currd->pci_lock);
> >>      ret = pt_irq_create_bind(currd, &pt_irq_bind);
> >> +    read_unlock(&currd->pci_lock);
> > 
> > I think you need to extend the ->pci_lock'ed region so it also covers
> > unmap_domain_pirq(), otherwise the ASSERT_PDEV_LIST_IS_READ_LOCKED()
> > in there will trigger?  IE: the read_unlock() needs to be where the
> > pcidevs_unlock() is?
> 
> Oh, indeed. That would only be an issue if an error arose, so it's
> clear why I didn't hit that assertion.
> 
> This being the only change (which I've made locally) necessary for
> the entire series - shall I re-send, or would you mind giving an ack
> for the patch with said adjustment made?

I'm happy for you do do the change locally and commit:

Reviewed-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.