Re: [Xen-devel] map_domain_emuirq_pirq() imbalance with unmap_domain_pirq_emuirq()?

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 13 Dec 2011, Jan Beulich wrote:
> Hi Stefano,
> 
> in your patch to introduce pIRQ interrupt remapping for HVM guests,
> you have physdev_map_pirq() call unmap_domain_pirq_emuirq()

I take you mean physdev_unmap_pirq here.

> unconditionally in the HVM case, yet physdev_hvm_map_pirq() call
> map_domain_emuirq_pirq() only if no machine_gsi was found. I'm
> suspecting this to be the reason for pass-through device hot unplug
> resource leaks (leading eventually to hot plug failure on repeated
> attempts), as in this case it is my understanding that
> unmap_domain_pirq_emuirq() can only be expected to fail (for there
> not being an established mapping), leading to physdev_unmap_pirq()
> bailing out early.
> As it's not immediately clear whether using the same lookup approach
> that physdev_hvm_map_pirq() uses would be valid in the unmap path,
> I'm hoping for your advice how to address this problem. Is there
> perhaps a map_domain_emuirq_pirq(..., IRQ_PT) call missing?

I think I understand what the problem is: since 'xen: fix
hvm_domain_use_pirq's behavior' the pirq to emuirq mapping remains set
to IRQ_UNBOUND until an event channel has been bound to the pirq.
That means that if the guest is not binding any event channels at all,
the mapping remains IRQ_UNBOUND for the life of the VM.
Eventually when physdev_unmap_pirq gets called, nothing happens because
unmap_domain_pirq_emuirq keeps failing. This wasn't the original
behavior because it used to be the case that the mapping would always be
either IRQ_PT or a positive number.

A simple solution to this issue would be to introduce a check in
physdev_unmap_pirq:


diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index cca56bb..c1a7fcc 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -216,14 +216,16 @@ int physdev_unmap_pirq(domid_t domid, int pirq)
     if ( ret )
         return ret;
 
-    if ( is_hvm_domain(d) )
+    if ( is_hvm_domain(d) && domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND)
     {
         spin_lock(&d->event_lock);
         ret = unmap_domain_pirq_emuirq(d, pirq);
         spin_unlock(&d->event_lock);
-        if ( domid == DOMID_SELF || ret )
+        if ( ret )
             goto free_domain;
     }
+    if ( domid == DOMID_SELF )
+        goto free_domain;
 
     ret = -EPERM;
     if ( !IS_PRIV_FOR(current->domain, d) )


> Besides that, in 23806:4226ea1785b5 you move a call to
> map_domain_emuirq_pirq() from xen/arch/x86/physdev.c to
> xen/common/event_channel.c, but neither before nor after the patch
> the function's return value gets checked, yet the function has various
> ways to fail. Is failure here really benign?
 
No, it is not. We should return an error if map_domain_emuirq_pirq
fails.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel