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



> +    spin_unlock_irq(&gic.lock);
> +
> +    return found;
> +}
> +
>  static inline void gic_set_lr(int lr, unsigned int virtual_irq,
>          unsigned int state, unsigned int priority)
>  {
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2d91dce..cea9233 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -369,8 +369,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, 
> int n)
>      while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 
> ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
> -        if ( !list_empty(&p->inflight) )
> +        if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) )
>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> +
>          i++;
>      }
>  }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 513c1fc..f9e9ef1 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -197,6 +197,9 @@ extern unsigned int gic_number_lines(void);
>  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                    unsigned int *out_hwirq, unsigned int *out_type);
>  
> +/* Check if an IRQ was already injected to the current VCPU */
> +bool_t gic_irq_injected(unsigned int irq);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif
>  
> -- 
> 1.7.10.4
> 

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