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

[Jan Beulich <jbeulich@xxxxxxxx>]

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