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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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


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