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

[Julien Grall <julien.grall@linaro.org>]

On 06/25/2013 02:24 PM, Stefano Stabellini wrote:

> On Tue, 25 Jun 2013, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>>
>> 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@linaro.org>
>> ---
>>  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.

-- 
Julien