Re: [Xen-devel] [XenARM] XEN tools for ARM with Virtualization Extensions - Patch #2

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-07-16 at 12:24 +0100, Stefano Stabellini wrote:
> On Tue, 16 Jul 2013, Eric Trudeau wrote:
> > > -----Original Message-----
> > > From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> > > Sent: Monday, July 15, 2013 7:14 PM
> > > To: Eric Trudeau
> > > Cc: xen-devel; Stefano.Stabellini@xxxxxxxxxx; Ian Campbell
> > > Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with Virtualization
> > > Extensions - Patch #2
> > > 
> > > On 12 July 2013 20:27, Eric Trudeau <etrudeau@xxxxxxxxxxxx> wrote:
> > > > Second patch submitted with changes based on comments on first patch.
> > > >
> > > >> > From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon Sep 17 00:00:00
> > > >> 2001
> > > >> > From: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
> > > >> > Date: Thu, 11 Jul 2013 20:03:51 -0400
> > > >> > Subject: [PATCH] Add support for Guest physdev irqs
> > > >> >
> > > >> > ---
> > > >> >  xen/arch/arm/domain.c  | 16 ++++++++++++++++
> > > >> >  xen/arch/arm/gic.c     | 15 ++++++++++-----
> > > >> >  xen/arch/arm/physdev.c | 48
> > > >> ++++++++++++++++++++++++++++++++++++++++++++++--
> > > >> >  xen/arch/arm/vgic.c    |  5 +----
> > > >> >  4 files changed, 73 insertions(+), 11 deletions(-)
> > > >> >
> > > >> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > >> > index 4c434a1..52d3429 100644
> > > >> > --- a/xen/arch/arm/domain.c
> > > >> > +++ b/xen/arch/arm/domain.c
> > > >> > @@ -31,6 +31,8 @@
> > > >> >  #include <asm/gic.h>
> > > >> >  #include "vtimer.h"
> > > >> >  #include "vpl011.h"
> > > >> > +#include <xen/iocap.h>
> > > >> > +#include <xen/irq.h>
> > > >> >
> > > >> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> > > >> >
> > > >> > @@ -513,8 +515,22 @@ fail:
> > > >> >      return rc;
> > > >> >  }
> > > >> >
> > > >> > +static int release_domain_irqs(struct domain *d)
> > > >> > +{
> > > >> > +    int i;
> > > >> > +    for (i = 0; i <= d->nr_pirqs; i++) {
> > > >> > +        if (irq_access_permitted(d, i)) {
> > > >> > +            release_irq(i);
> > > >> > +        }
> > > >> > +    }
> > > >> > +    return 0;
> > > >> > +}
> > > >>
> > > >> As you may know, release_irq will spin until the flags IRQ_INPROGRESS
> > > >> is unset. This is done my the maintenance handler.
> > > >>
> > > >> Now, let say the domain has crashed and the IRQ is not yet EOIed, then 
> > > >> you
> > > >> will spin forever.
> > > >>
> > > >
> > > > I put a timeout in the IRQ_INPROGRESS check in release_irq() of 100 
> > > > attempts
> > > > with a 1 msec delay per loop iteration.
> > > 
> > > If we plan to only use release_irq when a domain is destroyed, this
> > > check is useless,
> > > so it should be removed.
> > > 
> > > An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi it.
> > > If the domain has crashed or received an hard shutdown (ie xl
> > > destroy), the IRQ will
> > > remain "inflight" and can never come up again.
> > > 
> > > You need to check if the IRQ is still inflight and, if yes, eoi it.
> > >
> 
> I think that Julien is right: we need to call something similar to
> xen/arch/arm/gic.c:maintenance_interrupt. I would refactor the code
> there into a separate function that ends up being called by
> maintenance_interrupt and release_irq.
> 
> 
> > I will have to research this implementation as I am not familiar with the 
> > flow
> > and functions in the IRQ handler code.  If you have any suggestions, please
> > let me know.  Is it simply a matter of checking IRQ_INFLIGHT bit in the
> > desc->status word and then calling a function to EOI the interrupt?
> 
> First we need to make sure that the domain is paused and about to be
> destroyed otherwise we risk breaking the gic/vgic interface.  Then we
> need to check whether the irq is currently inflight, looking at the
> inflight queue, see for example
> xen/arch/arm/vgic.c:vgic_vcpu_inject_irq.  If the irq is inflight we
> need to check whether it's actually in one of the LR registers or just
> in the lr_queue. See xen/arch/arm/gic.c:gic_set_guest_irq to see how the
> lr_queue works. If the irq is queued there, just remove it from the
> lr_queue, EOI it and remove it from the inflight queue. If the irq is
> not present in lr_queue, it means it's in one of the LR registers. In
> that case we can call something similar to maintenance_interrupt to
> remove it from the LR, EOI the interrupt and remove it from the inflight
> queue.

This all sounds plausible to me. I'm not sure we need to worry overly
much about the impact on the guest of pulling an interrupt out from
under it -- that's not ever going to be a clever thing to do and it
seems like it should be up to the host and guest admin to arrange that
the guest has quiesced the device. If the guest admin won't co-operate,
well then they get to suffer the consequences. I guess that doesn't mean
we shouldn't try and at least be a bit graceful about it if it's easy
enough to do.

The thing to worry about is that the IRQ remains usable at the host
level and can be assigned to someone else etc.

One thing to bare in mind is that when you EOI the interrupt, unless the
guest has dealt with it you might immediately get another, which you may
not currently be set up to handle.

You likely want to make sure it is at least masked at the physical level
or maybe you want to try and ensure you do things in the right order
such that it is routed to the host before you do the EOI. Safer to mask
I think.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel