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:
> >>> On 13.12.11 at 18:16, Stefano Stabellini 
> >>> <stefano.stabellini@xxxxxxxxxxxxx> 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.

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