Re: [Xen-devel] [PATCH 3/5] xen/arm: Don't reinject the IRQ if it's already in LRs

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-06-25 at 14:24 +0100, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Julien Grall wrote:
> > From: Julien Grall <julien.grall@xxxxxxxxxx>
> > 
> > When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
> >   - Disable the IRQ
> >   - Call the interrupt handler
> >   - Conditionnally enable the IRQ
> >   - EOI the IRQ
> > 
> > When Linux will re-enable the IRQ, Xen will inject again the IRQ because 
> > it's
> > still inflight. Therefore, LRs will contains duplicated IRQs and Xen will
> > EOI it twice if it's a physical IRQ.
> > 
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
> >  xen/arch/arm/vgic.c       |    3 ++-
> >  xen/include/asm-arm/gic.h |    3 +++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 21575df..bf05716 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, 
> > struct irqaction *new)
> >      return rc;
> >  }
> >  
> > +/* Check if an IRQ was already injected to the current VCPU */
> > +bool_t gic_irq_injected(unsigned int irq)
> 
> Can you rename it to something more specific, like gic_irq_inlr?
> 
> > +{
> > +    bool_t found = 0;
> > +    int i = 0;
> > +    unsigned int virq;
> > +
> > +    spin_lock_irq(&gic.lock);
> > +
> > +    while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask),
> > +                               nr_lrs, i)) < nr_lrs )
> > +    {
> > +        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
> > +
> > +        if ( virq == irq )
> > +        {
> > +            found = 1;
> > +            break;
> > +        }
> > +        i++;
> > +    }
> 
> Instead of reading back all the GICH_LR registers, can't just just read
> the ones that have a corresponding bit set in lr_mask?
> 
> Also you should be able to avoid having to read the GICH_LR registers by
> simply checking if the irq is in the lr_queue list: if an irq is in
> inflight but not in lr_queue, it means that it is in one of the LRs.

This sounds roughly equivalent to what I was about to suggest which was
a bool in the irq descriptor.

In any case we should certainly try and avoid walking all the LRs via
some means.

Ian.


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