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 05:36 PM, Stefano Stabellini wrote:

> 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@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.
> 
> I think it's time we introduce a "status" member in struct irq_desc, so

Do you mean struct irq_pending?

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


Sounds good to me. I will try to implement this approach.

Moreover, it will fix another issue with this patch. I have just noticed
that it's possible to reinject an IRQ on different vCPU even if it's
injected on another vCPU.

-- 
Julien