Re: [RESEND PATCH 4.0-rc7 v20 3/6] irqchip: gic: Introduce plumbing for IPI FIQ

[Daniel Thompson <daniel.thompson@linaro.org>]

On 22/04/15 10:15, Marc Zyngier wrote:
> On Tue, 21 Apr 2015 22:03:25 +0100
> Daniel Thompson <daniel.thompson@linaro.org> wrote:
>
> Hi Daniel,
>
>> On 21/04/15 14:45, Marc Zyngier wrote:
>>> On 10/04/15 10:51, Daniel Thompson wrote:
>>>> Currently it is not possible to exploit FIQ for systems with a GIC, even if
>>>> the systems are otherwise capable of it. This patch makes it possible
>>>> for IPIs to be delivered using FIQ.
>>>>
>>>> To do so it modifies the register state so that normal interrupts are
>>>> placed in group 1 and specific IPIs are placed into group 0. It also
>>>> configures the controller to raise group 0 interrupts using the FIQ
>>>> signal. It provides a means for architecture code to define which IPIs
>>>> shall use FIQ and to acknowledge any IPIs that are raised.
>>>>
>>>> All GIC hardware except GICv1-without-TrustZone support provides a means
>>>> to group exceptions into group 0 and group 1 but the hardware
>>>> functionality is unavailable to the kernel when a secure monitor is
>>>> present because access to the grouping registers are prohibited outside
>>>> "secure world". However when grouping is not available (or in the case
>>>> of early GICv1 implementations is very hard to configure) the code to
>>>> change groups does not deploy and all IPIs will be raised via IRQ.
>>>>
>>>> It has been tested and shown working on two systems capable of
>>>> supporting grouping (Freescale i.MX6 and STiH416). It has also been
>>>> tested for boot regressions on two systems that do not support grouping
>>>> (vexpress-a9 and Qualcomm Snapdragon 600).
>>>>
>>>> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Jason Cooper <jason@lakedaemon.net>
>>>> Cc: Russell King <linux@arm.linux.org.uk>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> Tested-by: Jon Medhurst <tixy@linaro.org>
>>>> ---
>>>>    arch/arm/kernel/traps.c         |   5 +-
>>>>    drivers/irqchip/irq-gic.c       | 151 +++++++++++++++++++++++++++++++++++++---
>>>>    include/linux/irqchip/arm-gic.h |   8 +++
>>>>    3 files changed, 153 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>>> index 788e23fe64d8..b35e220ae1b1 100644
>>>> --- a/arch/arm/kernel/traps.c
>>>> +++ b/arch/arm/kernel/traps.c
>>>> @@ -26,6 +26,7 @@
>>>>    #include <linux/init.h>
>>>>    #include <linux/sched.h>
>>>>    #include <linux/irq.h>
>>>> +#include <linux/irqchip/arm-gic.h>
>>>>
>>>>    #include <linux/atomic.h>
>>>>    #include <asm/cacheflush.h>
>>>> @@ -479,7 +480,9 @@ asmlinkage void __exception_irq_entry handle_fiq_as_nmi(struct pt_regs *regs)
>>>>
>>>>           nmi_enter();
>>>>
>>>> -       /* nop. FIQ handlers for special arch/arm features can be added here. */
>>>> +#ifdef CONFIG_ARM_GIC
>>>> +       gic_handle_fiq_ipi();
>>>> +#endif
>>>
>>> This hunk is what irritates me. It creates a hard dependency between
>>> core ARM code and the GIC, and I don't really see how this works with
>>> multiplatform, where the interrupt controller is not necessarily a GIC.
>>> In that case, you will die a horrible death.
>>
>> I was just about to reassure you that there is no bug here... but then I
>> read the code.
>>
>> gic_handle_fiq_ipi() was *supposed* to do a check to make it safe to
>> call when there is no gic meaning multi-platform support could be
>> achieved by calling into multiple handlers.
>>
>> It looks like I forgot to write the code that would make this possible.
>> Maybe I was too disgusted with the approach to implement it correctly.
>> Looking at this with fresher eyes (I've been having a bit of a break
>> from FIQ recently) I can see how bad the current approach is.
>>
>>
>>> Why can't we just call handle_arch_irq(), and let the normal handler do
>>> its thing? You can have a "if (in_nmi())" in there, and call your FIQ
>>> function. It would at least save us the above problem.
>>
>> It should certainly work although it feels odd to reuse the IRQ handler
>> for FIQ.
>
> I can see three options:
>
> - (a) Either we have an interrupt controller specific, FIQ only entry
>    point, and we add calls in traps.c: this implies that each driver has
>    to defend itself against spurious calls.
>
> - (b) We add a separate handle_arch_fiq() indirection that only deals
>    with FIQ. Much better, but it also means that we have to keep this in
>    sync with arm64, for which the interest is relatively limited (FIQ
>    only works if you have a single security domain like XGene, or for a
>    VM).
>
> - (c) We call handle_arch_irq(), and let the interrupt controller code
>    sort the mess.
>
> I really hate (a) with a passion, because it litters both the ARM core
> code with IC specific code *and* introduce some defensive programming
> in the IC code, which is a waste...
>
> Option (b) is nicer, but requires additional work and buy-in from the
> arm64 maintainers, for a non obvious gain (I quite like the idea of
> injecting FIQs in a VM though, just for fun...).
>
> Option (c) is the simplest, if a little ugly on the side.
>
> Thoughts?

For FIQs, do you anticipate handle_arch_irq() having a role like the 
current gic_handle_fiq_ipi(), which is acknowledge an IPI and get out? 
Alternatively it could behave more like its current role for IRQ and 
call into the handlers itself.

The later seems more likely to work out well when I take another look at 
hooking up the perf interrupt.


Daniel.