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

[PATCH 0/6] 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: defer event unlock in pt_irq_create_bind()
2: x86/pass-through: no locking around pt_irq_{create,destroy}_bind()
3: x86/vPCI: tighten locking assertions
4: vPCI: drop bogus locking assertion
5: x86/pass-through: use simpler locking primitives in pt_irq_{create,destroy}_bind()
6: x86/HVM: drop vector parameter from .pi_update_irte() hook

Jan

[PATCH 1/6] x86/pass-through: defer event unlock in pt_irq_create_bind()

[Jan Beulich]

The radix tree holding struct pirq * as obtained by pirq_get_info() is
protected by the domain's event lock. The result ("info") and the derived
"pirq_dpci" therefore may not be de-referenced past the dropping of that
lock. Moving the unlock down is safe, but perhaps not obviously so:
- vector_hashing_dest() does a memory allocation, but core event channel
  code does so too while holding the lock; the call to
  vlapic_match_dest() doesn't involve any further locking,
- hvm_migrate_pirq() operates on the corresponding IRQ descriptor, where
  obtaining of its lock is of course fine (those locks always nest inside
  the event lock),
- {hvm,vmx}_pi_update_irte() are very similar to hvm_migrate_pirq()
  locking-wise,
- the locking around guest_mask_msi_irq() is the same as in the earlier
  two bullet points.

As long as the PCI-devs lock is held around both
pt_irq_{create,destroy}_bind(), this is only a latent issue.

Fixes: 35a1caf8b6b5 ("pass-through: update IRTE according to guest interrupt config changes")
Fixes: 1066331913c9 ("passthrough: don't migrate pirq when it is delivered through VT-d PI")
Fixes: 782cf8ba4678 ("pass-through: adjust pIRQ migration")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The last two unlocks could be done a little more efficiently, but then
also in a little less straightforward a way:

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

            write_unlock(&d->event_lock);

            if ( !desc )
            {
                pt_irq_destroy_bind(d, pt_irq_bind);
                return -EINVAL;
            }

            guest_mask_msi_irq(desc, false);
            spin_unlock_irqrestore(&desc->lock, flags);
        }
        else
            write_unlock(&d->event_lock);

        break;

Seeing that the IRQ descriptor lock is taken up to three times in a row,
I wonder whether we shouldn't consolidate this (by obtaining desc once and
then passing it into hvm_migrate_pirq() (or a suitable new sibling
thereof) and hvm_pi_update_irte()).

--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -371,7 +371,6 @@ int pt_irq_create_bind(
 
         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
         pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
-        write_unlock(&d->event_lock);
 
         pirq_dpci->gmsi.posted = false;
         vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
@@ -393,6 +392,7 @@ int pt_irq_create_bind(
 
             if ( rc )
             {
+                write_unlock(&d->event_lock);
                 pt_irq_destroy_bind(d, pt_irq_bind);
                 return rc;
             }
@@ -405,6 +405,7 @@ int pt_irq_create_bind(
 
             if ( !desc )
             {
+                write_unlock(&d->event_lock);
                 pt_irq_destroy_bind(d, pt_irq_bind);
                 return -EINVAL;
             }
@@ -413,6 +414,7 @@ int pt_irq_create_bind(
             spin_unlock_irqrestore(&desc->lock, flags);
         }
 
+        write_unlock(&d->event_lock);
         break;
     }
 

[PATCH 2/6] x86/pass-through: no 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().

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

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

[PATCH 3/6] 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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
With this perhaps the comment near the top of vpci_msix_arch_print() might
better go away. Thoughts?

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -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 )

[PATCH 4/6] vPCI: drop bogus locking assertion

[Jan Beulich]

msix_find() is a local helper, with all callers explicitly acquiring the
per-domain PCI r/w lock. The checking, which should never have included
the alternative of holding the global PCI devices lock, therefore is
pretty much pointless.

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

--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -158,8 +158,6 @@ static struct vpci_msix *msix_find(const
 {
     struct vpci_msix *msix;
 
-    ASSERT_PDEV_LIST_IS_READ_LOCKED(d);
-
     list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
     {
         const struct vpci_bar *bars = msix->pdev->vpci->header.bars;

[PATCH 5/6] 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>

--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -400,8 +400,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 +410,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);
@@ -606,9 +605,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 +614,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 6/6] x86/HVM: drop vector parameter from .pi_update_irte() hook

[Jan Beulich]

It's redundant with the struct pirq * being passed, and the vector field
in struct msi_desc wanting to be cleared can be derived from v (and hence
pi_desc) being NULL.

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

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -395,7 +395,7 @@ void vmx_pi_hooks_deassign(struct domain
  * when guest changes MSI/MSI-X information.
  */
 static int cf_check vmx_pi_update_irte(const struct vcpu *v,
-                                       const struct pirq *pirq, uint8_t gvec)
+                                       const struct pirq *pirq)
 {
     const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
     struct irq_desc *desc;
@@ -414,7 +414,7 @@ static int cf_check vmx_pi_update_irte(c
         goto unlock_out;
     }
     msi_desc->pi_desc = pi_desc;
-    msi_desc->gvec = gvec;
+    msi_desc->gvec = pi_desc ? pirq_dpci(pirq)->gmsi.gvec : 0;
     msg = msi_desc->msg;
 
     spin_unlock_irq(&desc->lock);
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -220,8 +220,7 @@ struct hvm_function_table {
     void (*sync_pir_to_irr)(struct vcpu *v);
     bool (*test_pir)(const struct vcpu *v, uint8_t vector);
     void (*handle_eoi)(uint8_t vector, int isr);
-    int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq,
-                          uint8_t gvec);
+    int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq);
     void (*update_vlapic_mode)(struct vcpu *v);
 
     /*Walk nested p2m  */
@@ -835,9 +834,9 @@ static inline void hvm_set_nonreg_state(
 }
 
 static inline int hvm_pi_update_irte(const struct vcpu *v,
-                                     const struct pirq *pirq, uint8_t gvec)
+                                     const struct pirq *pirq)
 {
-    return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
+    return alternative_call(hvm_funcs.pi_update_irte, v, pirq);
 }
 
 static inline void hvm_update_vlapic_mode(struct vcpu *v)
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -388,7 +388,7 @@ int pt_irq_create_bind(
         /* Use interrupt posting if it is supported. */
         if ( iommu_intpost )
         {
-            rc = hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
+            rc = hvm_pi_update_irte(vcpu, info);
 
             if ( rc )
             {
@@ -686,7 +686,7 @@ int pt_irq_destroy_bind(
             what = "bogus";
     }
     else if ( pirq_dpci && pirq_dpci->gmsi.posted )
-        hvm_pi_update_irte(NULL, pirq, 0);
+        hvm_pi_update_irte(NULL, pirq);
 
     if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
          list_empty(&pirq_dpci->digl_list) )

Re: [PATCH 1/6] x86/pass-through: defer event unlock in pt_irq_create_bind()

[Roger Pau Monné]

On Tue, Sep 08, 2026 at 03:01:27PM +0200, Jan Beulich wrote:
> The radix tree holding struct pirq * as obtained by pirq_get_info() is
> protected by the domain's event lock. The result ("info") and the derived
> "pirq_dpci" therefore may not be de-referenced past the dropping of that
> lock. Moving the unlock down is safe, but perhaps not obviously so:
> - vector_hashing_dest() does a memory allocation, but core event channel
>   code does so too while holding the lock; the call to
>   vlapic_match_dest() doesn't involve any further locking,
> - hvm_migrate_pirq() operates on the corresponding IRQ descriptor, where
>   obtaining of its lock is of course fine (those locks always nest inside
>   the event lock),
> - {hvm,vmx}_pi_update_irte() are very similar to hvm_migrate_pirq()
>   locking-wise,
> - the locking around guest_mask_msi_irq() is the same as in the earlier
>   two bullet points.
> 
> As long as the PCI-devs lock is held around both
> pt_irq_{create,destroy}_bind(), this is only a latent issue.
> 
> Fixes: 35a1caf8b6b5 ("pass-through: update IRTE according to guest interrupt config changes")
> Fixes: 1066331913c9 ("passthrough: don't migrate pirq when it is delivered through VT-d PI")
> Fixes: 782cf8ba4678 ("pass-through: adjust pIRQ migration")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> ---
> The last two unlocks could be done a little more efficiently, but then
> also in a little less straightforward a way:
> 
>         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);
> 
>             write_unlock(&d->event_lock);
> 
>             if ( !desc )
>             {
>                 pt_irq_destroy_bind(d, pt_irq_bind);
>                 return -EINVAL;
>             }
> 
>             guest_mask_msi_irq(desc, false);
>             spin_unlock_irqrestore(&desc->lock, flags);
>         }
>         else
>             write_unlock(&d->event_lock);
> 
>         break;

I think your proposed approach is easier to follow, I'm happy with it.

> 
> Seeing that the IRQ descriptor lock is taken up to three times in a row,
> I wonder whether we shouldn't consolidate this (by obtaining desc once and
> then passing it into hvm_migrate_pirq() (or a suitable new sibling
> thereof) and hvm_pi_update_irte()).

Possibly?  I haven't looked what would be the adjustment needed on
other callers.  Just passing the locked irq_desc?

Thanks, Roger.

Re: [PATCH 1/6] x86/pass-through: defer event unlock in pt_irq_create_bind()

[Jan Beulich]

On 22.09.2026 15:37, Roger Pau Monné wrote:
> On Tue, Sep 08, 2026 at 03:01:27PM +0200, Jan Beulich wrote:
>> The radix tree holding struct pirq * as obtained by pirq_get_info() is
>> protected by the domain's event lock. The result ("info") and the derived
>> "pirq_dpci" therefore may not be de-referenced past the dropping of that
>> lock. Moving the unlock down is safe, but perhaps not obviously so:
>> - vector_hashing_dest() does a memory allocation, but core event channel
>>   code does so too while holding the lock; the call to
>>   vlapic_match_dest() doesn't involve any further locking,
>> - hvm_migrate_pirq() operates on the corresponding IRQ descriptor, where
>>   obtaining of its lock is of course fine (those locks always nest inside
>>   the event lock),
>> - {hvm,vmx}_pi_update_irte() are very similar to hvm_migrate_pirq()
>>   locking-wise,
>> - the locking around guest_mask_msi_irq() is the same as in the earlier
>>   two bullet points.
>>
>> As long as the PCI-devs lock is held around both
>> pt_irq_{create,destroy}_bind(), this is only a latent issue.
>>
>> Fixes: 35a1caf8b6b5 ("pass-through: update IRTE according to guest interrupt config changes")
>> Fixes: 1066331913c9 ("passthrough: don't migrate pirq when it is delivered through VT-d PI")
>> Fixes: 782cf8ba4678 ("pass-through: adjust pIRQ migration")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks.

>> Seeing that the IRQ descriptor lock is taken up to three times in a row,
>> I wonder whether we shouldn't consolidate this (by obtaining desc once and
>> then passing it into hvm_migrate_pirq() (or a suitable new sibling
>> thereof) and hvm_pi_update_irte()).
> 
> Possibly?  I haven't looked what would be the adjustment needed on
> other callers.  Just passing the locked irq_desc?

Yes. Yet in carrying that out I realize that calling iommu_update_ire_from_msi()
(out of vmx_pi_update_irte()) with the lock held is perhaps more risky than I'd
like it to be. With limited effort I couldn't convince myself that there might
not be a lock order inversion issue somewhere.

Jan

Re: [PATCH 2/6] x86/pass-through: no locking around pt_irq_{create,destroy}_bind()

[Roger Pau Monné]

On Tue, Sep 08, 2026 at 03:01:51PM +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().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -636,10 +636,7 @@ long arch_do_domctl(
>              ret = -EPERM;
>          else if ( is_iommu_enabled(d) )
>          {
> -            pcidevs_lock();
>              ret = pt_irq_create_bind(d, bind);
> -            pcidevs_unlock();

pt_irq_create_bind() might call into msixtbl_pt_register() which
requires either the pcidevs_lock() or the per-domain d->pci_lock lock
to be taken, which I think is not the case in the context here?

Adding such locking aroiund the calls would mimic the vPCI context,
where d->pci_lock is also taken while executing the vPCI handlers.

Thanks, Roger.

Re: [PATCH 3/6] x86/vPCI: tighten locking assertions

[Roger Pau Monné]

On Tue, Sep 08, 2026 at 03:02:19PM +0200, Jan Beulich wrote:
> 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.

I think this was (mostly?) done so that the macro could beused
generically without having to think whether the context is locked by
the pcidev_lock or the domain lock (or possibly both).

> 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.

I was under the impression that the long term plan was to drop the
pcidevs_lock side of ASSERT_PDEV_LIST_IS_READ_LOCKED(), and convert
that assert to check exclusively for the per-domain pci_lock.

However doing it would require assessing (and possibly adjusting) of
all users, which is unlikely to happen.

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

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

> ---
> With this perhaps the comment near the top of vpci_msix_arch_print() might
> better go away. Thoughts?

I would remove it now - previously it was the outlier and hence
deserved a comment, that's not the case after your change.

Thanks, Roger.

Re: [PATCH 4/6] vPCI: drop bogus locking assertion

[Roger Pau Monné]

On Tue, Sep 08, 2026 at 03:03:17PM +0200, Jan Beulich wrote:
> msix_find() is a local helper, with all callers explicitly acquiring the
> per-domain PCI r/w lock. The checking, which should never have included
> the alternative of holding the global PCI devices lock, therefore is
> pretty much pointless.

Hm, maybe.  I guess we need to put some limits on how much we assert
for correct locking if the callers are all in the same translation
unit.

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

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

I wouldn't have removed it myself, but I'm also not going to oppose.

Thanks, Roger.

Re: [PATCH 2/6] x86/pass-through: no locking around pt_irq_{create,destroy}_bind()

[Jan Beulich]

On 23.09.2026 12:37, Roger Pau Monné wrote:
> On Tue, Sep 08, 2026 at 03:01:51PM +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().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -636,10 +636,7 @@ long arch_do_domctl(
>>              ret = -EPERM;
>>          else if ( is_iommu_enabled(d) )
>>          {
>> -            pcidevs_lock();
>>              ret = pt_irq_create_bind(d, bind);
>> -            pcidevs_unlock();
> 
> pt_irq_create_bind() might call into msixtbl_pt_register() which
> requires either the pcidevs_lock() or the per-domain d->pci_lock lock
> to be taken, which I think is not the case in the context here?

Hmm, indeed. Not having seen the assertion there trigger kind of worries
me a little. Do you agree that the change to vioapic_hwdom_map_gsi() can,
otoh, be left as is?

In turn I will then extend patch 3 to also tighten the assertions in
msixtbl_pt_{,un}register(), as each of them has only this one call site.
Would you mind indicating whether in doing so I may retain you A-b there?

Jan

Re: [PATCH 2/6] x86/pass-through: no locking around pt_irq_{create,destroy}_bind()

[Roger Pau Monné]

On Thu, Sep 24, 2026 at 11:57:19AM +0200, Jan Beulich wrote:
> On 23.09.2026 12:37, Roger Pau Monné wrote:
> > On Tue, Sep 08, 2026 at 03:01:51PM +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().
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -636,10 +636,7 @@ long arch_do_domctl(
> >>              ret = -EPERM;
> >>          else if ( is_iommu_enabled(d) )
> >>          {
> >> -            pcidevs_lock();
> >>              ret = pt_irq_create_bind(d, bind);
> >> -            pcidevs_unlock();
> > 
> > pt_irq_create_bind() might call into msixtbl_pt_register() which
> > requires either the pcidevs_lock() or the per-domain d->pci_lock lock
> > to be taken, which I think is not the case in the context here?
> 
> Hmm, indeed. Not having seen the assertion there trigger kind of worries
> me a little. Do you agree that the change to vioapic_hwdom_map_gsi() can,
> otoh, be left as is?

Hm, I'm borderline on that one - I can't find a path where d->pci_lock
will be needed for legacy PCI interrupt binding, yet at the same time
I feel it would be better if the locking context is uniform across
call sites.  I guess I'm fine with the asymmetric locking context if
that's your preference.  Maybe worth a mention in a comment somewhere.

> In turn I will then extend patch 3 to also tighten the assertions in
> msixtbl_pt_{,un}register(), as each of them has only this one call site.
> Would you mind indicating whether in doing so I may retain you A-b there?

Please keep the A-b there.

Thanks, Roger.

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

[Roger Pau Monné]

On Tue, Sep 08, 2026 at 03:03:45PM +0200, Jan Beulich wrote:
> 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).

To clarify this expectation (as code moves around and changes), I
would introduce an explicit ASSERT() that interrupts are enabled.

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

Preferably with the above:

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

Thanks, Roger.

Re: [PATCH 6/6] x86/HVM: drop vector parameter from .pi_update_irte() hook

[Roger Pau Monné]

On Tue, Sep 08, 2026 at 03:04:05PM +0200, Jan Beulich wrote:
> It's redundant with the struct pirq * being passed, and the vector field
> in struct msi_desc wanting to be cleared can be derived from v (and hence
> pi_desc) being NULL.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

Re: [PATCH 2/6] x86/pass-through: no locking around pt_irq_{create,destroy}_bind()

[Jan Beulich]

On 25.09.2026 16:53, Roger Pau Monné wrote:
> On Thu, Sep 24, 2026 at 11:57:19AM +0200, Jan Beulich wrote:
>> On 23.09.2026 12:37, Roger Pau Monné wrote:
>>> On Tue, Sep 08, 2026 at 03:01:51PM +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().
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/domctl.c
>>>> +++ b/xen/arch/x86/domctl.c
>>>> @@ -636,10 +636,7 @@ long arch_do_domctl(
>>>>              ret = -EPERM;
>>>>          else if ( is_iommu_enabled(d) )
>>>>          {
>>>> -            pcidevs_lock();
>>>>              ret = pt_irq_create_bind(d, bind);
>>>> -            pcidevs_unlock();
>>>
>>> pt_irq_create_bind() might call into msixtbl_pt_register() which
>>> requires either the pcidevs_lock() or the per-domain d->pci_lock lock
>>> to be taken, which I think is not the case in the context here?
>>
>> Hmm, indeed. Not having seen the assertion there trigger kind of worries
>> me a little. Do you agree that the change to vioapic_hwdom_map_gsi() can,
>> otoh, be left as is?
> 
> Hm, I'm borderline on that one - I can't find a path where d->pci_lock
> will be needed for legacy PCI interrupt binding, yet at the same time
> I feel it would be better if the locking context is uniform across
> call sites.  I guess I'm fine with the asymmetric locking context if
> that's your preference.  Maybe worth a mention in a comment somewhere.

Maybe it's best if I get v2 out before we settle on this. The need for a
comment may, with how v2 is done, go away. E.g. the first of the hunks
now is

@@ -637,9 +637,13 @@ long arch_do_domctl(
             ret = -EPERM;
         else if ( is_iommu_enabled(d) )
         {
-            pcidevs_lock();
+            if ( bind->irq_type == PT_IRQ_TYPE_MSI )
+                read_lock(&d->pci_lock);
+
             ret = pt_irq_create_bind(d, bind);
-            pcidevs_unlock();
+
+            if ( bind->irq_type == PT_IRQ_TYPE_MSI )
+                read_unlock(&d->pci_lock);
 
             if ( ret < 0 )
                 printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for %pd\n",

While it's only a read-lock now, effects on parallelism aren't as bad
anymore. Yet still I'm rather hesitant to acquire a lock when there's no
need for doing so. In the case here we'd still impact any write-lock
paths, i.e. first and foremost vpci_write().

There's possibly another somewhat related issue: vpci_read() only uses
read_lock(), yet reads can in principle have side effects. Are we (once
again) building upon Dom0 knowing what it's doing, and this - like many
other aspect - being in need of auditing before DomU supported can be
declared complete?

Jan

Re: [PATCH 2/6] x86/pass-through: no locking around pt_irq_{create,destroy}_bind()

[Roger Pau Monné]

On Mon, Sep 28, 2026 at 02:50:41PM +0200, Jan Beulich wrote:
> On 25.09.2026 16:53, Roger Pau Monné wrote:
> > On Thu, Sep 24, 2026 at 11:57:19AM +0200, Jan Beulich wrote:
> >> On 23.09.2026 12:37, Roger Pau Monné wrote:
> >>> On Tue, Sep 08, 2026 at 03:01:51PM +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().
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> --- a/xen/arch/x86/domctl.c
> >>>> +++ b/xen/arch/x86/domctl.c
> >>>> @@ -636,10 +636,7 @@ long arch_do_domctl(
> >>>>              ret = -EPERM;
> >>>>          else if ( is_iommu_enabled(d) )
> >>>>          {
> >>>> -            pcidevs_lock();
> >>>>              ret = pt_irq_create_bind(d, bind);
> >>>> -            pcidevs_unlock();
> >>>
> >>> pt_irq_create_bind() might call into msixtbl_pt_register() which
> >>> requires either the pcidevs_lock() or the per-domain d->pci_lock lock
> >>> to be taken, which I think is not the case in the context here?
> >>
> >> Hmm, indeed. Not having seen the assertion there trigger kind of worries
> >> me a little. Do you agree that the change to vioapic_hwdom_map_gsi() can,
> >> otoh, be left as is?
> > 
> > Hm, I'm borderline on that one - I can't find a path where d->pci_lock
> > will be needed for legacy PCI interrupt binding, yet at the same time
> > I feel it would be better if the locking context is uniform across
> > call sites.  I guess I'm fine with the asymmetric locking context if
> > that's your preference.  Maybe worth a mention in a comment somewhere.
> 
> Maybe it's best if I get v2 out before we settle on this. The need for a
> comment may, with how v2 is done, go away. E.g. the first of the hunks
> now is
> 
> @@ -637,9 +637,13 @@ long arch_do_domctl(
>              ret = -EPERM;
>          else if ( is_iommu_enabled(d) )
>          {
> -            pcidevs_lock();
> +            if ( bind->irq_type == PT_IRQ_TYPE_MSI )
> +                read_lock(&d->pci_lock);
> +
>              ret = pt_irq_create_bind(d, bind);
> -            pcidevs_unlock();
> +
> +            if ( bind->irq_type == PT_IRQ_TYPE_MSI )
> +                read_unlock(&d->pci_lock);
>  
>              if ( ret < 0 )
>                  printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for %pd\n",

Binding an unbinding is an expensive operation.  The legacy PCI
interrupt bindings are done only once when the device is assigned to a
domain, and afterwards all calls to XEN_DOMCTL_bind_pt_irq should be
for MSI interrupts.  I don't think taking the lock unconditionally
would be that bad, the extra penalty for the one-shot PCI legacy
binding is possibly likely fine if we can remove one conditional?

> While it's only a read-lock now, effects on parallelism aren't as bad
> anymore. Yet still I'm rather hesitant to acquire a lock when there's no
> need for doing so. In the case here we'd still impact any write-lock
> paths, i.e. first and foremost vpci_write().

It's unlikely (albeit not impossible) to have both
XEN_DOMCTL_bind_pt_irq hypercalls and vPCI against the same domain.
Either the domain uses vPCI for passthrough or it uses an external
device model IMO.

> There's possibly another somewhat related issue: vpci_read() only uses
> read_lock(), yet reads can in principle have side effects. Are we (once
> again) building upon Dom0 knowing what it's doing, and this - like many
> other aspect - being in need of auditing before DomU supported can be
> declared complete?

The point of taking d->pci_lock in write mode is to prevent accesses
to any pdevs assigned to the domain, so that the position of BARs
across any devices assigned to a domain cannot change as we have to
check for overlaps.  However for other accesses we so far have no need
to cross check like this against all devices assigned to a domain, and
hence just taking the pdev->vpci->lock (so a per-device lock) is
possibly enough, as per-device accesses are still serialized?

Thanks, Roger.