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

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

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

Cheers,

-- 
Julien Grall

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