Re: [Xen-devel] [PATCH 2/5] xen/arm: Keep count of inflight interrupts

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 05:58 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Ian Campbell wrote:
> >> On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> >>> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >>>
> >>> For guest's timers (both virtual and physical), Xen will inject virtual
> >>> interrupt. Linux handles timer interrupt as:
> >>
> >> We should be wary of developing things based on the way which Linux
> >> happens to do things. On x86 we have several time modes, which can be
> >> selected based upon guest behaviour (described in
> >> docs/man/xl.cfg.pod.5). Do we need to do something similar here?
> >>
> >>>   1) Receive the interrupt and ack it
> >>>   2) Handle the current event timer
> >>>   3) Set the next event timer
> >>>   4) EOI the interrupt
> >>>
> >>> It's unlikely possible to reinject another interrupt before
> >>
> >> I can't parse this sentence. "unlikely to be possible" perhaps? but I'm
> >> not sure if that is what you meant.
> >>
> >>> the previous one is EOIed because the next deadline is shorter than the 
> >>> time
> >>> to execute code until EOI it.
> >>
> >> If we get into this situation once is there any danger that we will get
> >> into it repeatedly and overflow the count?
> >>
> >> Overall I'm not convinced this is the right approach to get the
> >> behaviour we want. Isn't this interrupt level triggered, with the level
> >> being determined by a comparison of two registers? IOW can't we
> >> determine whether to retrigger the interrupt or not by examining the
> >> state of our emulated versions of those registers? A generic mechanism
> >> to callback into the appropriate emulator on EOI plus a little bit of
> >> logic in the vtimer code is all it ought to take.
> > 
> > AFAICT this what could happen:
> > 
> > - vtimer fires
> > - xen mask the vtimer
> > - xen adds the vtimer to the LR for the guest
> > - xen EOIs the vtimer
> > - the guest receive the vtimer interrupt
> > - the guest set the next deadline in the past
> > - the guest enables the vtimer
> > ## an unexpected vtimer interrupt is received by Xen but the current
> > ## one is still being serviced 
> > - the guest eoi the vtimer
> > 
> > as a result the guest looses an interrupt.
> > Julien, is that correct?
> 
> Yes.

Could you please add something like that to the commit message?


> > If that is the case, this can only happen once, right?  In that case
> > rather than an atomic_t we could just have a bit in the status field I
> > proposed before. It should be enough for us to keep track of the case
> > when the irq is supposed to stay high even after the guest EOIs it. (Of
> > course that means that we need to re-inject it into the guest).
> 
> 
> For the timer yes. I wonder, what happen if Xen receive an SGI (for
> instance SCHEDULE ipi) twice, does Xen must re-inject it multiple times?

I don't think it can happen with anything but the vtimer: in order for
the scenario above to happen it takes an irq that is EOId in the
hardware before the guest EOIs it.

SGIs are completely "emulated", there is not an hardware irq
corresponding to them. From the Xen point of view the SGI is inflight
exactly and only from the moment the first vcpu sends it, to the point
when the receiving vcpu EOIs it.


> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >>> ---
> >>>  xen/arch/arm/gic.c           |   35 +++++++++++++++++++++++------------
> >>>  xen/arch/arm/vgic.c          |    1 +
> >>>  xen/include/asm-arm/domain.h |    2 ++
> >>>  3 files changed, 26 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>> index 0fee3f2..21575df 100644
> >>> --- a/xen/arch/arm/gic.c
> >>> +++ b/xen/arch/arm/gic.c
> >>> @@ -817,7 +817,7 @@ static void maintenance_interrupt(int irq, void 
> >>> *dev_id, struct cpu_user_regs *r
> >>>  
> >>>      while ((i = find_next_bit((const long unsigned int *) &eisr,
> >>>                                64, i)) < 64) {
> >>> -        struct pending_irq *p;
> >>> +        struct pending_irq *p, *n;
> >>>          int cpu, eoi;
> >>>  
> >>>          cpu = -1;
> >>> @@ -826,13 +826,32 @@ static void maintenance_interrupt(int irq, void 
> >>> *dev_id, struct cpu_user_regs *r
> >>>          spin_lock_irq(&gic.lock);
> >>>          lr = GICH[GICH_LR + i];
> >>>          virq = lr & GICH_LR_VIRTUAL_MASK;
> >>> +
> >>> +        p = irq_to_pending(v, virq);
> >>> +        if ( p->desc != NULL ) {
> >>> +            p->desc->status &= ~IRQ_INPROGRESS;
> > 
> > Now that I am thinking about this, shouldn't this be protected by taking
> > the desc->lock?
> 
> Right. I don't think desc->lock is enough if we want to protect from
> release_irq.
> If this function is called, it will wait until IRQ_INPROGRESS is
> removed. How about moving ~IRQ_INPROGRESS at then end of the block and
> add a dmb() before?

that should work

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