Xen devel - Dec 2011 - map_domain_emuirq_pirq() imbalance with unmap_domain_pirq_emuirq()?

Hi Stefano,

in your patch to introduce pIRQ interrupt remapping for HVM guests,
you have physdev_map_pirq() call unmap_domain_pirq_emuirq()
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?

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?

Jan

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.

>>> On 13.12.11 at 18:16, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> 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:
> 
> --- 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;
I don''t think this is correct for the pv case, seems to need a
"is_hvm_domain() && " in front of it. If you agree, we can
give this
a try.
>  
>      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.
I''m afraid it''s not just a matter of returning an error here,
but also
needs (correct) undoing of everything that was already done.

Jan

On Tue, 13 Dec 2011, Jan Beulich wrote:
> >>> On 13.12.11 at 18:16, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> > 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:
> > 
> > --- 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;
> 
> I don''t think this is correct for the pv case, seems to need a
> "is_hvm_domain() && " in front of it. If you agree, we
can give this
> a try.
Yes, you are right.

> >      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.
> 
> I''m afraid it''s not just a matter of returning an error
here, but also
> needs (correct) undoing of everything that was already done.
 
I don''t think there is a particular reason why map_domain_emuirq_pirq
should be at the end of the function, after link_pirq_port. It could be
moved close to the previous goto out, we just need to know the pirq
number.

>>> On 13.12.11 at 18:16, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> --- 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) )
I think this is the correct change (untested so far):

@@ -228,7 +228,8 @@ static int physdev_unmap_pirq(struct phy
     if ( is_hvm_domain(d) )
     {
         spin_lock(&d->event_lock);
-        ret = unmap_domain_pirq_emuirq(d, pirq);
+        if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )
+            ret = unmap_domain_pirq_emuirq(d, pirq);
         spin_unlock(&d->event_lock);
         if ( unmap->domid == DOMID_SELF || ret )
             goto free_domain;

i.e. do the lookup with the lock held (and taking advantage of
''ret''
being zero when reaching the enclosing if()).

Jan

On Wed, 14 Dec 2011, Jan Beulich wrote:
> >>> On 13.12.11 at 18:16, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> > --- 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) )
> 
> I think this is the correct change (untested so far):
> 
> @@ -228,7 +228,8 @@ static int physdev_unmap_pirq(struct phy
>      if ( is_hvm_domain(d) )
>      {
>          spin_lock(&d->event_lock);
> -        ret = unmap_domain_pirq_emuirq(d, pirq);
> +        if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )
> +            ret = unmap_domain_pirq_emuirq(d, pirq);
>          spin_unlock(&d->event_lock);
>          if ( unmap->domid == DOMID_SELF || ret )
>              goto free_domain;
> 
> i.e. do the lookup with the lock held (and taking advantage of
''ret''
> being zero when reaching the enclosing if()).
 
ack