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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 02:24 PM, 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?
> 
> It's already the case, I use find_next_bit to find the next used LRs.
> 
> > 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.
> 
> 
> No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight
> but not in lr_queue. We don't have a way to know whether the IRQ is
> really in LRs or not.

I think it's time we introduce a "status" member in struct irq_desc, so
that we are not dependent on the information in the GICH_LR registers or
the queue a pending_irq has been added to.
I would implement it as a bitfield:

int status;
#define GIC_IRQ_ENABLED  (1<<0)
#define GIC_IRQ_INFLIGHT (1<<1)
#define GIC_IRQ_INLR     (1<<2)

This way you should just go through the inflight queue and check whether
status & GIC_IRQ_INLR.

At the moment we just want to represent this basic state machine:

irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance 
interrupt)

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