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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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?

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


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

> > +            /* Assume only one pcpu needs to EOI the irq */
> > +            cpu = p->desc->arch.eoi_cpu;
> > +            eoi = 1;
> > +            pirq = p->desc->irq;
> > +        }
> > +        if ( !atomic_dec_and_test(&p->inflight_cnt) )
> > +        {
> > +            /* Physical IRQ can't be reinject */
> > +            WARN_ON(p->desc != NULL);
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > +            spin_unlock_irq(&gic.lock);
> > +            i++;
> > +            continue;
> > +        }
> > +
> >          GICH[GICH_LR + i] = 0;
> >          clear_bit(i, &this_cpu(lr_mask));
> >  
> >          if ( !list_empty(&v->arch.vgic.lr_pending) ) {
> > -            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), 
> > lr_queue);
> > -            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > -            list_del_init(&p->lr_queue);
> > +            n = list_entry(v->arch.vgic.lr_pending.next, typeof(*n), 
> > lr_queue);
> > +            gic_set_lr(i, n->irq, GICH_LR_PENDING, n->priority);
> > +            list_del_init(&n->lr_queue);
> >              set_bit(i, &this_cpu(lr_mask));
> >          } else {
> >              gic_inject_irq_stop();
> > @@ -840,14 +859,6 @@ static void maintenance_interrupt(int irq, void 
> > *dev_id, struct cpu_user_regs *r
> >          spin_unlock_irq(&gic.lock);
> >  
> >          spin_lock_irq(&v->arch.vgic.lock);
> > -        p = irq_to_pending(v, virq);
> > -        if ( p->desc != NULL ) {
> > -            p->desc->status &= ~IRQ_INPROGRESS;
> > -            /* Assume only one pcpu needs to EOI the irq */
> > -            cpu = p->desc->arch.eoi_cpu;
> > -            eoi = 1;
> > -            pirq = p->desc->irq;
> > -        }
> >          list_del_init(&p->inflight);
> >          spin_unlock_irq(&v->arch.vgic.lock);
> >  
 

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