Mailing List Archive

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@lists.xensource.com
http://lists.xensource.com/xen-devel

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


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

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.

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

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


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

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

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