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

From: Daniel Thompson
Date: Wed Apr 22 2015 - 11:40:52 EST

On 22/04/15 13:57, Marc Zyngier wrote:
On Wed, 22 Apr 2015 13:45:33 +0100
Daniel Thompson <daniel.thompson@xxxxxxxxxx> wrote:

On 22/04/15 10:15, Marc Zyngier wrote:
On Tue, 21 Apr 2015 22:03:25 +0100
Daniel Thompson <daniel.thompson@xxxxxxxxxx> 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@xxxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
Tested-by: Jon Medhurst <tixy@xxxxxxxxxx>
---
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.

Assuming your mention of handle_arch_irq() is actually
handle_arch_fiq(), I'd expect some interesting problems if you try to
handle a Linux interrupt while already handling one, as the core IRQ
code is not designed to be reentrant... Your code works so far because
you have been careful to keep the IRQ code at bay. Putting it back into
the equation is going to be hairy at best.

I was actually thinking of option (c) but the question would apply in both cases.

To be clear, I agree we cannot call into big piles of irq code from an NMI. We'd have to introduce new NMI-only ways to dispatch FIQs from real hwirqs (SPIs and PPIs).

In fact, at present we can't even call into handle_IPI() at the moment (because it will call irq_enter) although we could try to modify things and make that possible.

These issues apply whether we have conditional code in handle_arch_irq() or if we introduce handle_arch_fiq().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/