Re: [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 17/04/2019 18:27, Stefano Stabellini wrote:
> On Wed, 17 Apr 2019, Julien Grall wrote:
>> Hi,
>>
>> On 17/04/2019 18:12, Stefano Stabellini wrote:
>>> On Tue, 16 Apr 2019, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 4/16/19 10:51 PM, Stefano Stabellini wrote:
>>>>> On Mon, 28 Jan 2019, Julien Grall wrote:
>>>>>> While SPIs are shared between CPU, it is not possible to receive the
>>>>>> same interrupts on a different CPU while the interrupt is in active
>>>>>> state. The deactivation of the interrupt is done at the end of the
>>>>>> handling.
>>>>>>
>>>>>> This means the _IRQ_PENDING logic is unecessary on Arm as a same
>>>>>> interrupt can never come up while in the loop. So remove it to
>>>>>> simplify the interrupt handle code.
>>>>>>
>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>> ---
>>>>>>     xen/arch/arm/irq.c | 32 ++++++++++----------------------
>>>>>>     1 file changed, 10 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>>>>> index c51cf333ce..3877657a52 100644
>>>>>> --- a/xen/arch/arm/irq.c
>>>>>> +++ b/xen/arch/arm/irq.c
>>>>>> @@ -199,6 +199,7 @@ int request_irq(unsigned int irq, unsigned int
>>>>>> irqflags,
>>>>>>     void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int
>>>>>> is_fiq)
>>>>>>     {
>>>>>>         struct irq_desc *desc = irq_to_desc(irq);
>>>>>> +    struct irqaction *action;
>>>>>>           perfc_incr(irqs);
>>>>>>     @@ -242,35 +243,22 @@ void do_IRQ(struct cpu_user_regs *regs,
>>>>>> unsigned
>>>>>> int irq, int is_fiq)
>>>>>>             goto out_no_end;
>>>>>>         }
>>>>>>     -    set_bit(_IRQ_PENDING, &desc->status);
>>>>>> -
>>>>>> -    /*
>>>>>> -     * Since we set PENDING, if another processor is handling a
>>>>>> different
>>>>>> -     * instance of this same irq, the other processor will take care
>>>>>> of
>>>>>> it.
>>>>>> -     */
>>>>>> -    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
>>>>>> -         test_bit(_IRQ_INPROGRESS, &desc->status) )
>>>>>> +    if ( test_bit(_IRQ_DISABLED, &desc->status) )
>>>>>>             goto out;
>>>>>
>>>>> It is a good idea to remove the IRQ_PENDING logic, that is OK.
>>>>>
>>>>>
>>>>> However, are we sure that we want to remove the _IRQ_INPROGRESS check
>>>>> too? IRQ handlers shouldn't be called twice in a row. Given that
>>>>> _IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it
>>>>> would be a good idea to keep the check anyway?
>>>>
>>>> set_active_state is only used by the vGIC to replicate state from of the
>>>> virtual interrupt to the physical interrupt. We don't have the virtual
>>>> interrupt in this path (see above).
>>>>
>>>> Any other user (e.g interrupts routed to Xen) would be pretty broken. At
>>>> best
>>>> you would break the interrupt flow. At worst, you may never receive the
>>>> interrupt again.
>>>>
>>>> So I think we can drop _IRQ_PROGRESS here.
>>>
>>> I gave it a close look. You are right, it is safe to remove the
>>> _IRQ_PROGRESS check here.
>>>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>>
>>> The thing that worries me a bit is that technically set_active_state is
>>> part of the gic_hw_operations functions which are not necessarily guest
>>> specific: we haven't written down anywhere that set_active_state cannot
>>> be called passing one of the xen irqs as parameter. I agree it would be
>>> broken to do so, but still... Maybe we should add a comment?
>>
>> How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ?
> 
> even better

Do you want the change to be in this patch or separately?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel