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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

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@xxxxxxxx>

Acked-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>

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