Hi all,
I do realize that we're in the middle of the merge window. But maybe some of you will be bored enough to look into this; and no problem if you don't feel like it -- I promise to send a brand new shiny v4 after the merge window, so you won't miss a bit of this new cool stuff. :-)
In v3:
- Per Colin Cross suggestion, added a way to release a debug console for normal use. This is done via 'disable_nmi' command (in the original FIQ debugger it was 'console' command). For this I added a new callback in the tty ops, and serial drivers have to provide a way to clear its interrupts. The patch 'tty/serial/kgdboc: Add and wire up clear_irqs callback' explains the concept in details. - Made the debug entry prompt more shell-like; - A new knocking mode '-1'. It disables the feature altogether, and thus makes it possible to hook KDB entry to a dedicated button. - The code was rebased on 'v3.5 + kdb kiosk'[1] patches; and for convenience it is now available in the following repo:
git://git.infradead.org/users/cbou/linux-nmi-kdb.git master
Rationale for this patch set:
These patches introduce KGDB FIQ debugger support. The idea (and some code, of course) comes from Google's FIQ debugger[2]. There are some differences (mostly implementation details, feature-wise they're almost equivalent, or can be made equivalent, if desired).
The FIQ debugger is a facility that can be used to debug situations when the kernel stuck in uninterruptable sections, e.g. the kernel infinitely loops or deadlocked in an interrupt or with interrupts disabled. On some development boards there is even a special NMI button, which is very useful for debugging weird kernel hangs.
And FIQ is basically an NMI, it has a higher priority than IRQs, and upon IRQ exception FIQs are not disabled. It is still possible to disable FIQs (as well as some "NMIs" on other architectures), but via special means.
So, here FIQs and NMIs are synonyms, but in the code I use NMI term for arch-independent code, and FIQs for ARM code.
A few years ago KDB wasn't yet ready for production, or even not well-known, so originally Google implemented its own FIQ debugger that included its own shell, ring-buffer, commands, dumping, backtracing logic and whatnot. This is very much like PowerPC's xmon (arch/powerpc/xmon), except that xmon was there for a decade, so it even predates KDB.
Anyway, nowadays KGDB/KDB is the cross-platform debugger, and the only feature that was missing is NMI handling. This is now fixed for ARM.
There are a few differences comparing to the original (Google's) FIQ debugger:
- Doing stuff in FIQ context is dangerous, as there we are not allowed to cause aborts or faults. In the original FIQ debugger there was a "signal" software-induced interrupt, upon exit from FIQ it would fire, and we would continue to execute "dangerous" commands from there.
In KGDB/KDB we don't use signal interrupts. We can do easier: set up a breakpoint, continue, and you'll trap into KGDB again in a safe context.
It works for most cases, but I can imagine cases when you can't set up a breakpoint. For these cases we'd better introduce a KDB command "exit_nmi", that will rise the SW IRQ, after which we're allowed to do anything.
- KGDB/KDB FIQ debugger shell is synchronous. In Google's version you could have a dedicated shell always running in the FIQ context, so when you type something on a serial line, you won't actually cause any debugging actions, FIQ would save the characters in its own buffer and continue execution normally. But when you hit return key after the command, then the command is executed.
In KGDB/KDB FIQ debugger it is different. Once you enter KGDB, the kernel will stop until you instruct it to continue.
This might look as a drastic change, but it is not. There is actually no difference whether you have sync or async shell, or at least I couldn't find any use-case where this would matter at all. Anyways, it is still possible to do async shell in KDB, just don't see any need for this.
- Original FIQ debugger used a custom FIQ vector handling code, w/ a lot of logic in it. In this approach I'm using the fact that FIQs are basically IRQs, except that we there are a bit more registers banked, and we can actually trap from the IRQ context.
But this all does not prevent us from using a simple jump-table based approach as used in the generic ARM entry code. So, here I just reuse the generic approach.
Note that I test the code on a modelled ARM machine (QEMU Versatile), so there might be some issues on a real HW, but it works in QEMU tho. :-)
Assuming you have QEMU >= 1.1.0, you can easily play with the code using ARM/versatile defconfig and command like this:
qemu-system-arm -nographic -machine versatilepb \ -kernel linux/arch/arm/boot/zImage \ -append "console=ttyAMA0 kgdboc=ttyAMA0 kgdb_fiq.enable=1"
Thanks,
-- arch/arm/Kconfig | 19 +++ arch/arm/common/vic.c | 28 +++++ arch/arm/include/asm/hardware/vic.h | 2 + arch/arm/include/asm/kgdb.h | 8 ++ arch/arm/kernel/Makefile | 1 + arch/arm/kernel/entry-armv.S | 169 +------------------------ arch/arm/kernel/entry-header.S | 176 ++++++++++++++++++++++++++- arch/arm/kernel/kgdb_fiq.c | 159 ++++++++++++++++++++++++ arch/arm/kernel/kgdb_fiq_entry.S | 76 ++++++++++++ arch/arm/mach-versatile/Makefile | 1 + arch/arm/mach-versatile/include/mach/irqs.h | 1 + arch/arm/mach-versatile/kgdb_fiq.c | 31 +++++ drivers/tty/serial/amba-pl011.c | 13 ++ drivers/tty/serial/kgdboc.c | 9 ++ drivers/tty/serial/serial_core.c | 15 +++ include/linux/kgdb.h | 14 +++ include/linux/serial_core.h | 1 + include/linux/tty_driver.h | 1 + kernel/debug/debug_core.c | 13 +- kernel/debug/kdb/kdb_debugger.c | 4 + kernel/debug/kdb/kdb_main.c | 20 +++ 21 files changed, 591 insertions(+), 170 deletions(-)
In v2:
- Per Colin Cross' suggestion, we should not enter the debugger on any received byte (this might be a problem when there's a noise on the serial line). So there is now an additional patch that implements "knocking" to the KDB (either via $3#33 command or return key, this is configurable); - Reworked {enable,select}_fiq/is_fiq callbacks, now multi-mach kernels should not be a problem; - For versatile machines there are run-time checks for proper UART port (kernel will scream aloud if out of range port is specified); - Added some __init annotations; - Since not every architecture defines FIQ_START, we can't just blindly select CONFIG_FIQ symbol. So ARCH_MIGHT_HAVE_FIQ introduced; - Add !THUMB2_KERNEL dependency for KGDB_FIQ, we don't support Thumb2 kernels; - New patch that is used to get rid of LCcralign label in alignment_trap macro.
[1] https://lkml.org/lkml/2012/7/26/260 [2] Original Google's FIQ debugger, fiq_* files: http://android.git.linaro.org/gitweb?p=kernel/common.git%3Ba=tree%3Bf=arch/a... And board support as an example of using it: http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git%3Ba=commitdiff%3Bh=461cb8...
Currently kernel never set KGDB_REASON_NMI. We do now, when we enter KGDB/KDB from an NMI.
This is not to be confused with kgdb_nmicallback(), NMI callback is an entry for the slave CPUs during CPUs roundup, but REASON_NMI is the entry for the master CPU.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- kernel/debug/kdb/kdb_debugger.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/debug/kdb/kdb_debugger.c b/kernel/debug/kdb/kdb_debugger.c index 8b68ce7..be7b33b 100644 --- a/kernel/debug/kdb/kdb_debugger.c +++ b/kernel/debug/kdb/kdb_debugger.c @@ -12,6 +12,7 @@ #include <linux/kdb.h> #include <linux/kdebug.h> #include <linux/export.h> +#include <linux/hardirq.h> #include "kdb_private.h" #include "../debug_core.h"
@@ -52,6 +53,9 @@ int kdb_stub(struct kgdb_state *ks) if (atomic_read(&kgdb_setting_breakpoint)) reason = KDB_REASON_KEYBOARD;
+ if (in_nmi()) + reason = KDB_REASON_NMI; + for (i = 0, bp = kdb_breakpoints; i < KDB_MAXBPT; i++, bp++) { if ((bp->bp_enabled) && (bp->bp_addr == addr)) { reason = KDB_REASON_BREAK;
On 07/30/2012 06:58 AM, Anton Vorontsov wrote:
Currently kernel never set KGDB_REASON_NMI. We do now, when we enter KGDB/KDB from an NMI.
This is not to be confused with kgdb_nmicallback(), NMI callback is an entry for the slave CPUs during CPUs roundup, but REASON_NMI is the entry for the master CPU.
No need for confusion here :-)
I'll take this one for the kernel merge window if it passes regression tests, no reason not to be setting the stop codes properly.
Thanks, Jason.
The new arch callback should manage NMIs that usually cause KGDB to enter. That is, not all NMIs should be enabled/disabled, but only those that issue kgdb_handle_exception().
We must mask it as serial-line interrupt can be used as an NMI, so if the original KGDB-entry cause was say a breakpoint, then every input to KDB console will cause KGDB to reenter, which we don't want.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- include/linux/kgdb.h | 13 +++++++++++++ kernel/debug/debug_core.c | 13 ++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h index c4d2fc1..e0c0a2e 100644 --- a/include/linux/kgdb.h +++ b/include/linux/kgdb.h @@ -221,6 +221,19 @@ extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt); */ extern void kgdb_arch_late(void);
+/** + * kgdb_arch_enable_nmi - Enable or disable KGDB-entry NMI + * @on: Flag to either enable or disable an NMI + * + * This function manages NMIs that usually cause KGDB to enter. That is, + * not all NMIs should be enabled or disabled, but only those that issue + * kgdb_handle_exception(). + * + * The call counts disable/enable requests, it returns 1 if NMI has been + * actually enabled after the call, and a value <= 0 if it is still + * disabled. + */ +extern int kgdb_arch_enable_nmi(bool on);
/** * struct kgdb_arch - Describe architecture specific values. diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c index 0557f24..38b0ab2 100644 --- a/kernel/debug/debug_core.c +++ b/kernel/debug/debug_core.c @@ -214,6 +214,11 @@ int __weak kgdb_skipexception(int exception, struct pt_regs *regs) return 0; }
+int __weak kgdb_arch_enable_nmi(bool on) +{ + return 0; +} + /* * Some architectures need cache flushes when we set/clear a * breakpoint: @@ -672,6 +677,9 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs) { struct kgdb_state kgdb_var; struct kgdb_state *ks = &kgdb_var; + int ret; + + kgdb_arch_enable_nmi(0);
ks->cpu = raw_smp_processor_id(); ks->ex_vector = evector; @@ -685,7 +693,10 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs) if (kgdb_info[ks->cpu].enter_kgdb != 0) return 0;
- return kgdb_cpu_enter(ks, regs, DCPU_WANT_MASTER); + ret = kgdb_cpu_enter(ks, regs, DCPU_WANT_MASTER); + + kgdb_arch_enable_nmi(1); + return ret; }
int kgdb_nmicallback(int cpu, void *regs)
This command disables NMI-entry. If NMI source was previously shared with a serial console ("debug port"), this effectively releases the port from KDB exclusive use, and makes the console available for normal use.
Of course, NMI can be reenabled, enable_nmi modparam is used for that:
echo 1 > /sys/module/kdb/parameters/enable_nmi
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- kernel/debug/kdb/kdb_main.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 3ee92a1..1e9f20e 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -2131,6 +2131,24 @@ static int kdb_dmesg(int argc, const char **argv) return 0; } #endif /* CONFIG_PRINTK */ + +static int kdb_disable_nmi(int argc, const char *argv[]) +{ + kgdb_arch_enable_nmi(0); + return 0; +} + +static int kdb_param_enable_nmi(const char *val, const struct kernel_param *kp) +{ + kgdb_arch_enable_nmi(1); + return 0; +} + +static const struct kernel_param_ops kdb_param_ops_enable_nmi = { + .set = kdb_param_enable_nmi, +}; +module_param_cb(enable_nmi, &kdb_param_ops_enable_nmi, NULL, 0600); + /* * kdb_cpu - This function implements the 'cpu' command. * cpu [<cpunum>] @@ -2873,6 +2891,8 @@ static void __init kdb_inittab(void) kdb_register_flags("dmesg", kdb_dmesg, "[lines]", "Display syslog buffer", 0, KDB_SAFE); #endif + kdb_register_flags("disable_nmi", kdb_disable_nmi, "", + "Disable NMI entry to KDB", 0, KDB_SAFE); kdb_register_flags("defcmd", kdb_defcmd, "name "usage" "help"", "Define a set of commands, down to endefcmd", 0, KDB_SAFE); kdb_register_flags("kill", kdb_kill, "<-signal> <pid>",
On Mon, Jul 30, 2012 at 4:58 AM, Anton Vorontsov anton.vorontsov@linaro.org wrote:
This command disables NMI-entry. If NMI source was previously shared with a serial console ("debug port"), this effectively releases the port from KDB exclusive use, and makes the console available for normal use.
Of course, NMI can be reenabled, enable_nmi modparam is used for that:
echo 1 > /sys/module/kdb/parameters/enable_nmi
This is very different behavior from the FIQ debugger "console" command you are trying to replace. In the FIQ debugger, everything goes through the FIQ/NMI, even when in console mode. That means that the user can always get back to FIQ debugger/KDB mode using a special sequence (we use a break character). With your implementation, if you switch from KDB to console to see if the console is working, and find that it is not working, you can never get back into KDB.
On Mon, Jul 30, 2012 at 10:33:34AM -0700, Colin Cross wrote:
On Mon, Jul 30, 2012 at 4:58 AM, Anton Vorontsov anton.vorontsov@linaro.org wrote:
This command disables NMI-entry. If NMI source was previously shared with a serial console ("debug port"), this effectively releases the port from KDB exclusive use, and makes the console available for normal use.
Of course, NMI can be reenabled, enable_nmi modparam is used for that:
echo 1 > /sys/module/kdb/parameters/enable_nmiThis is very different behavior from the FIQ debugger "console" command you are trying to replace. In the FIQ debugger, everything goes through the FIQ/NMI, even when in console mode. That means that the user can always get back to FIQ debugger/KDB mode using a special sequence (we use a break character). With your implementation, if you switch from KDB to console to see if the console is working, and find that it is not working, you can never get back into KDB.
Ah, I see. But with disable_nmi, in addition to kernel console, applications can use /dev/ttyXX as normal, and with 'console' command that is not possible (at least w/o modifying applications to escape magic sequence). So, I think we should have both commands, each would handle its own use case.
(Initially I just tried to avoid adding another console driver, but it seems there is no other way, heh.)
Thanks!
This patch implements a new callback: clear_irqs. It is used for the cases when KDB-entry (e.g. NMI) and KDB IO (e.g. serial port) shares the same interrupt. To get the idea, let's take some real example (ARM machine): we have a serial port which interrupt is routed to an NMI, and the interrupt is used to enter KDB. Once there is some activity on the serial port, the CPU receives NMI exception, and we fall into KDB shell. So, it is our "debug console", and it is able to interrupt (and thus debug) even IRQ handlers themselves.
When used that way, the interrupt never reaches serial driver's IRQ handler routine, which means that serial driver will not silence the interrupt. NMIs behaviour are quite arch-specific, and we can't assume that we can use them as ordinary IRQs, e.g. on some arches (like ARM) we can't handle data aborts, the behaviour is undefined then. So we can't just handle execution to serial driver's IRQ handler from the NMI context once we're done with KDB (plus this would defeat the debugger's purpose: we want the NMI handler be as simple as possible, so it will have less chances to hang).
So, given that have to deal with it somehow, we have two options:
1. Implement something that clears the interrupt; 2. Implement a whole new concept of grabbing tty for exclusive KDB use, plus implement mask/unmask callbacks, i.e.: - Since consoles might use ttys w/o opending them, we would have to make kdb respect CON_ENABLED flag (maybe a good idea to do it anyway); - Add 'bool exclusive' argument to tty_find_polling_driver(), if set to 1, the function will refuse to return an already tty; and will use the flag in tty_reopen() to not allow multiple users (there are already checks for pty masters, which are "open once" ttys); - Once we got the tty exclusively, we would need to call some new uart->mask_all_but_rx_interrupts call before we want to use the port for NMI/KDB, and unmask_all_but_rx_interrupts after we're done with it.
The second option is obviously more complex, needlessly so, and less generic. So I went with the first one: we just consume all the interrupts. The tty becomes silently unusable for the rest of the world when we use it with KDB; but once we reroute the serial IRQ source back from NMI to an ordinary IRQ (in KDB this can be done with 'disable_nmi' command), it will behave as normal.
p.s. Since the callback is so far used only by polling user, we place it under the appropriate #ifdef.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- drivers/tty/serial/kgdboc.c | 9 +++++++++ drivers/tty/serial/serial_core.c | 15 +++++++++++++++ include/linux/kgdb.h | 1 + include/linux/serial_core.h | 1 + include/linux/tty_driver.h | 1 + 5 files changed, 27 insertions(+)
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c index 2b42a01..178e62c 100644 --- a/drivers/tty/serial/kgdboc.c +++ b/drivers/tty/serial/kgdboc.c @@ -227,6 +227,14 @@ static int kgdboc_get_char(void) kgdb_tty_line); }
+static void kgdboc_clear_irqs(void) +{ + if (!kgdb_tty_driver) + return; + if (kgdb_tty_driver->ops->clear_irqs) + kgdb_tty_driver->ops->clear_irqs(kgdb_tty_driver, kgdb_tty_line); +} + static void kgdboc_put_char(u8 chr) { if (!kgdb_tty_driver) @@ -298,6 +306,7 @@ static struct kgdb_io kgdboc_io_ops = { .name = "kgdboc", .read_char = kgdboc_get_char, .write_char = kgdboc_put_char, + .clear_irqs = kgdboc_clear_irqs, .pre_exception = kgdboc_pre_exp_handler, .post_exception = kgdboc_post_exp_handler, }; diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 246b823..89c4093 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2154,6 +2154,20 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch) port = state->uart_port; port->ops->poll_put_char(port, ch); } + +static void uart_clear_irqs(struct tty_driver *driver, int line) +{ + struct uart_driver *drv = driver->driver_state; + struct uart_state *state = drv->state + line; + struct uart_port *port; + + if (!state || !state->uart_port) + return; + + port = state->uart_port; + if (port->ops->clear_irqs) + port->ops->clear_irqs(port); +} #endif
static const struct tty_operations uart_ops = { @@ -2186,6 +2200,7 @@ static const struct tty_operations uart_ops = { .poll_init = uart_poll_init, .poll_get_char = uart_poll_get_char, .poll_put_char = uart_poll_put_char, + .clear_irqs = uart_clear_irqs, #endif };
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h index e0c0a2e..f695b26 100644 --- a/include/linux/kgdb.h +++ b/include/linux/kgdb.h @@ -285,6 +285,7 @@ struct kgdb_io { const char *name; int (*read_char) (void); void (*write_char) (u8); + void (*clear_irqs) (void); void (*flush) (void); int (*init) (void); void (*pre_exception) (void); diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index 65db992..081d300 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -275,6 +275,7 @@ struct uart_ops { #ifdef CONFIG_CONSOLE_POLL void (*poll_put_char)(struct uart_port *, unsigned char); int (*poll_get_char)(struct uart_port *); + void (*clear_irqs)(struct uart_port *); #endif };
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h index 6e6dbb7..94b14cd 100644 --- a/include/linux/tty_driver.h +++ b/include/linux/tty_driver.h @@ -287,6 +287,7 @@ struct tty_operations { int (*poll_init)(struct tty_driver *driver, int line, char *options); int (*poll_get_char)(struct tty_driver *driver, int line); void (*poll_put_char)(struct tty_driver *driver, int line, char ch); + void (*clear_irqs)(struct tty_driver *driver, int line); #endif const struct file_operations *proc_fops; };
It's all pretty straightforward, except for TXIM interrupt. The interrupt has meaning "ready to transmit", so it's almost always raised, and the only way to silence it is to mask it. But that's OK, ops->start_tx will unmask it.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- drivers/tty/serial/amba-pl011.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index c17923e..378d26b 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1308,6 +1308,18 @@ static void pl010_put_poll_char(struct uart_port *port, writew(ch, uap->port.membase + UART01x_DR); }
+static void pl010_clear_irqs(struct uart_port *port) +{ + struct uart_amba_port *uap = (struct uart_amba_port *)port; + unsigned char __iomem *regs = uap->port.membase; + + writew(readw(regs + UART011_MIS), regs + UART011_ICR); + /* + * There is no way to clear TXIM, this is "ready to transmit IRQ", so + * we simply mask it. ops->start_tx will unmask it. + */ + writew(readw(regs + UART011_IMSC) & ~UART011_TXIM, regs + UART011_IMSC); +} #endif /* CONFIG_CONSOLE_POLL */
static int pl011_startup(struct uart_port *port) @@ -1698,6 +1710,7 @@ static struct uart_ops amba_pl011_pops = { #ifdef CONFIG_CONSOLE_POLL .poll_get_char = pl010_get_poll_char, .poll_put_char = pl010_put_poll_char, + .clear_irqs = pl010_clear_irqs, #endif };
Just move the macros into header file as we would want to use them for KGDB FIQ entry code.
The following macros were moved:
- svc_entry - usr_entry - kuser_cmpxchg_check - vector_stub
To make kuser_cmpxchg_check actually work across different files, we also have to make kuser_cmpxchg64_fixup global.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/kernel/entry-armv.S | 167 +-------------------------------------- arch/arm/kernel/entry-header.S | 170 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 166 deletions(-)
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S index 0d1851c..6aeb9b8 100644 --- a/arch/arm/kernel/entry-armv.S +++ b/arch/arm/kernel/entry-armv.S @@ -136,57 +136,6 @@ common_invalid: b bad_mode ENDPROC(__und_invalid)
-/* - * SVC mode handlers - */ - -#if defined(CONFIG_AEABI) && (__LINUX_ARM_ARCH__ >= 5) -#define SPFIX(code...) code -#else -#define SPFIX(code...) -#endif - - .macro svc_entry, stack_hole=0 - UNWIND(.fnstart ) - UNWIND(.save {r0 - pc} ) - sub sp, sp, #(S_FRAME_SIZE + \stack_hole - 4) -#ifdef CONFIG_THUMB2_KERNEL - SPFIX( str r0, [sp] ) @ temporarily saved - SPFIX( mov r0, sp ) - SPFIX( tst r0, #4 ) @ test original stack alignment - SPFIX( ldr r0, [sp] ) @ restored -#else - SPFIX( tst sp, #4 ) -#endif - SPFIX( subeq sp, sp, #4 ) - stmia sp, {r1 - r12} - - ldmia r0, {r3 - r5} - add r7, sp, #S_SP - 4 @ here for interlock avoidance - mov r6, #-1 @ "" "" "" "" - add r2, sp, #(S_FRAME_SIZE + \stack_hole - 4) - SPFIX( addeq r2, r2, #4 ) - str r3, [sp, #-4]! @ save the "real" r0 copied - @ from the exception stack - - mov r3, lr - - @ - @ We are now ready to fill in the remaining blanks on the stack: - @ - @ r2 - sp_svc - @ r3 - lr_svc - @ r4 - lr_<exception>, already fixed up for correct return/restart - @ r5 - spsr_<exception> - @ r6 - orig_r0 (see pt_regs definition in ptrace.h) - @ - stmia r7, {r2 - r6} - -#ifdef CONFIG_TRACE_IRQFLAGS - bl trace_hardirqs_off -#endif - .endm - .align 5 __dabt_svc: svc_entry @@ -328,71 +277,8 @@ ENDPROC(__pabt_svc)
/* * User mode handlers - * - * EABI note: sp_svc is always 64-bit aligned here, so should S_FRAME_SIZE */
-#if defined(CONFIG_AEABI) && (__LINUX_ARM_ARCH__ >= 5) && (S_FRAME_SIZE & 7) -#error "sizeof(struct pt_regs) must be a multiple of 8" -#endif - - .macro usr_entry - UNWIND(.fnstart ) - UNWIND(.cantunwind ) @ don't unwind the user space - sub sp, sp, #S_FRAME_SIZE - ARM( stmib sp, {r1 - r12} ) - THUMB( stmia sp, {r0 - r12} ) - - ldmia r0, {r3 - r5} - add r0, sp, #S_PC @ here for interlock avoidance - mov r6, #-1 @ "" "" "" "" - - str r3, [sp] @ save the "real" r0 copied - @ from the exception stack - - @ - @ We are now ready to fill in the remaining blanks on the stack: - @ - @ r4 - lr_<exception>, already fixed up for correct return/restart - @ r5 - spsr_<exception> - @ r6 - orig_r0 (see pt_regs definition in ptrace.h) - @ - @ Also, separately save sp_usr and lr_usr - @ - stmia r0, {r4 - r6} - ARM( stmdb r0, {sp, lr}^ ) - THUMB( store_user_sp_lr r0, r1, S_SP - S_PC ) - - @ - @ Enable the alignment trap while in kernel mode - @ - alignment_trap r0 - - @ - @ Clear FP to mark the first stack frame - @ - zero_fp - -#ifdef CONFIG_IRQSOFF_TRACER - bl trace_hardirqs_off -#endif - .endm - - .macro kuser_cmpxchg_check -#if !defined(CONFIG_CPU_32v6K) && !defined(CONFIG_NEEDS_SYSCALL_FOR_CMPXCHG) -#ifndef CONFIG_MMU -#warning "NPTL on non MMU needs fixing" -#else - @ Make sure our user space atomic helper is restarted - @ if it was interrupted in a critical region. Here we - @ perform a quick test inline since it should be false - @ 99.9999% of the time. The rest is done out of line. - cmp r4, #TASK_SIZE - blhs kuser_cmpxchg64_fixup -#endif -#endif - .endm - .align 5 __dabt_usr: usr_entry @@ -813,6 +699,7 @@ __kuser_cmpxchg64: @ 0xffff0f60 ldmfd sp!, {r4, r5, r6, pc}
.text + .global kuser_cmpxchg64_fixup kuser_cmpxchg64_fixup: @ Called from kuser_cmpxchg_fixup. @ r4 = address of interrupted insn (must be preserved). @@ -943,58 +830,6 @@ __kuser_helper_end:
THUMB( .thumb )
-/* - * Vector stubs. - * - * This code is copied to 0xffff0200 so we can use branches in the - * vectors, rather than ldr's. Note that this code must not - * exceed 0x300 bytes. - * - * Common stub entry macro: - * Enter in IRQ mode, spsr = SVC/USR CPSR, lr = SVC/USR PC - * - * SP points to a minimal amount of processor-private memory, the address - * of which is copied into r0 for the mode specific abort handler. - */ - .macro vector_stub, name, mode, correction=0 - .align 5 - -vector_\name: - .if \correction - sub lr, lr, #\correction - .endif - - @ - @ Save r0, lr_<exception> (parent PC) and spsr_<exception> - @ (parent CPSR) - @ - stmia sp, {r0, lr} @ save r0, lr - mrs lr, spsr - str lr, [sp, #8] @ save spsr - - @ - @ Prepare for SVC32 mode. IRQs remain disabled. - @ - mrs r0, cpsr - eor r0, r0, #(\mode ^ SVC_MODE | PSR_ISETSTATE) - msr spsr_cxsf, r0 - - @ - @ the branch table must immediately follow this code - @ - and lr, lr, #0x0f - THUMB( adr r0, 1f ) - THUMB( ldr lr, [r0, lr, lsl #2] ) - mov r0, sp - ARM( ldr lr, [pc, lr, lsl #2] ) - movs pc, lr @ branch to handler in SVC mode -ENDPROC(vector_\name) - - .align 2 - @ handler addresses follow this label -1: - .endm - .globl __stubs_start __stubs_start: /* diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S index 9a8531e..c3c09ac 100644 --- a/arch/arm/kernel/entry-header.S +++ b/arch/arm/kernel/entry-header.S @@ -73,6 +73,109 @@ msr cpsr_c, \rtemp @ switch back to the SVC mode .endm
+/* + * Vector stubs. + * + * This code is copied to 0xffff0200 so we can use branches in the + * vectors, rather than ldr's. Note that this code must not + * exceed 0x300 bytes. + * + * Common stub entry macro: + * Enter in IRQ mode, spsr = SVC/USR CPSR, lr = SVC/USR PC + * + * SP points to a minimal amount of processor-private memory, the address + * of which is copied into r0 for the mode specific abort handler. + */ + .macro vector_stub, name, mode, correction=0 + .align 5 + +vector_\name: + .if \correction + sub lr, lr, #\correction + .endif + + @ + @ Save r0, lr_<exception> (parent PC) and spsr_<exception> + @ (parent CPSR) + @ + stmia sp, {r0, lr} @ save r0, lr + mrs lr, spsr + str lr, [sp, #8] @ save spsr + + @ + @ Prepare for SVC32 mode. IRQs remain disabled. + @ + mrs r0, cpsr + eor r0, r0, #(\mode ^ SVC_MODE | PSR_ISETSTATE) + msr spsr_cxsf, r0 + + @ + @ the branch table must immediately follow this code + @ + and lr, lr, #0x0f + THUMB( adr r0, 1f ) + THUMB( ldr lr, [r0, lr, lsl #2] ) + mov r0, sp + ARM( ldr lr, [pc, lr, lsl #2] ) + movs pc, lr @ branch to handler in SVC mode +ENDPROC(vector_\name) + + .align 2 + @ handler addresses follow this label +1: + .endm + +/* + * SVC mode handlers + */ + +#if defined(CONFIG_AEABI) && (__LINUX_ARM_ARCH__ >= 5) +#define SPFIX(code...) code +#else +#define SPFIX(code...) +#endif + + .macro svc_entry, stack_hole=0 + UNWIND(.fnstart ) + UNWIND(.save {r0 - pc} ) + sub sp, sp, #(S_FRAME_SIZE + \stack_hole - 4) +#ifdef CONFIG_THUMB2_KERNEL + SPFIX( str r0, [sp] ) @ temporarily saved + SPFIX( mov r0, sp ) + SPFIX( tst r0, #4 ) @ test original stack alignment + SPFIX( ldr r0, [sp] ) @ restored +#else + SPFIX( tst sp, #4 ) +#endif + SPFIX( subeq sp, sp, #4 ) + stmia sp, {r1 - r12} + + ldmia r0, {r3 - r5} + add r7, sp, #S_SP - 4 @ here for interlock avoidance + mov r6, #-1 @ "" "" "" "" + add r2, sp, #(S_FRAME_SIZE + \stack_hole - 4) + SPFIX( addeq r2, r2, #4 ) + str r3, [sp, #-4]! @ save the "real" r0 copied + @ from the exception stack + + mov r3, lr + + @ + @ We are now ready to fill in the remaining blanks on the stack: + @ + @ r2 - sp_svc + @ r3 - lr_svc + @ r4 - lr_<exception>, already fixed up for correct return/restart + @ r5 - spsr_<exception> + @ r6 - orig_r0 (see pt_regs definition in ptrace.h) + @ + stmia r7, {r2 - r6} + +#ifdef CONFIG_TRACE_IRQFLAGS + bl trace_hardirqs_off +#endif + .endm + #ifndef CONFIG_THUMB2_KERNEL .macro svc_exit, rpsr msr spsr_cxsf, \rpsr @@ -164,6 +267,73 @@ #endif /* !CONFIG_THUMB2_KERNEL */
/* + * User mode handlers + * + * EABI note: sp_svc is always 64-bit aligned here, so should S_FRAME_SIZE + */ + +#if defined(CONFIG_AEABI) && (__LINUX_ARM_ARCH__ >= 5) && (S_FRAME_SIZE & 7) +#error "sizeof(struct pt_regs) must be a multiple of 8" +#endif + + .macro usr_entry + UNWIND(.fnstart ) + UNWIND(.cantunwind ) @ don't unwind the user space + sub sp, sp, #S_FRAME_SIZE + ARM( stmib sp, {r1 - r12} ) + THUMB( stmia sp, {r0 - r12} ) + + ldmia r0, {r3 - r5} + add r0, sp, #S_PC @ here for interlock avoidance + mov r6, #-1 @ "" "" "" "" + + str r3, [sp] @ save the "real" r0 copied + @ from the exception stack + + @ + @ We are now ready to fill in the remaining blanks on the stack: + @ + @ r4 - lr_<exception>, already fixed up for correct return/restart + @ r5 - spsr_<exception> + @ r6 - orig_r0 (see pt_regs definition in ptrace.h) + @ + @ Also, separately save sp_usr and lr_usr + @ + stmia r0, {r4 - r6} + ARM( stmdb r0, {sp, lr}^ ) + THUMB( store_user_sp_lr r0, r1, S_SP - S_PC ) + + @ + @ Enable the alignment trap while in kernel mode + @ + alignment_trap r0 + + @ + @ Clear FP to mark the first stack frame + @ + zero_fp + +#ifdef CONFIG_IRQSOFF_TRACER + bl trace_hardirqs_off +#endif + .endm + + .macro kuser_cmpxchg_check +#if !defined(CONFIG_CPU_32v6K) && !defined(CONFIG_NEEDS_SYSCALL_FOR_CMPXCHG) +#ifndef CONFIG_MMU +#warning "NPTL on non MMU needs fixing" +#else + @ Make sure our user space atomic helper is restarted + @ if it was interrupted in a critical region. Here we + @ perform a quick test inline since it should be false + @ 99.9999% of the time. The rest is done out of line. + cmp r4, #TASK_SIZE + blhs kuser_cmpxchg64_fixup +#endif +#endif + .endm + +/* * These are the registers used in the syscall handler, and allow us to * have in theory up to 7 arguments to a function - r0 to r6. *
The FIQ debugger may be used to debug situations when the kernel stuck in uninterruptable sections, e.g. the kernel infinitely loops or deadlocked in an interrupt or with interrupts disabled.
By default KGDB FIQ is disabled in runtime, but can be enabled with kgdb_fiq.enable=1 kernel command line option.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/Kconfig | 18 +++++++ arch/arm/include/asm/kgdb.h | 8 +++ arch/arm/kernel/Makefile | 1 + arch/arm/kernel/kgdb_fiq.c | 101 ++++++++++++++++++++++++++++++++++++++ arch/arm/kernel/kgdb_fiq_entry.S | 79 +++++++++++++++++++++++++++++ 5 files changed, 207 insertions(+) create mode 100644 arch/arm/kernel/kgdb_fiq.c create mode 100644 arch/arm/kernel/kgdb_fiq_entry.S
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index a91009c..ae6ddf1 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -172,6 +172,24 @@ config GENERIC_ISA_DMA config FIQ bool
+config ARCH_MIGHT_HAVE_FIQ + bool + +config KGDB_FIQ + bool "KGDB/KDB FIQ debugger" + depends on KGDB_KDB && ARCH_MIGHT_HAVE_FIQ && !THUMB2_KERNEL + select FIQ + help + The FIQ debugger may be used to debug situations when the + kernel stuck in uninterruptable sections, e.g. the kernel + infinitely loops or deadlocked in an interrupt or with + interrupts disabled. + + By default KGDB FIQ is disabled in runtime, but can be + enabled with kgdb_fiq.enable=1 kernel command line option. + + If unsure, say N. + config NEED_RET_TO_USER bool
diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h index 48066ce..807e547 100644 --- a/arch/arm/include/asm/kgdb.h +++ b/arch/arm/include/asm/kgdb.h @@ -11,6 +11,8 @@ #define __ARM_KGDB_H__
#include <linux/ptrace.h> +#include <linux/linkage.h> +#include <asm/exception.h>
/* * GDB assumes that we're a user process being debugged, so @@ -47,6 +49,12 @@ static inline void arch_kgdb_breakpoint(void) extern void kgdb_handle_bus_error(void); extern int kgdb_fault_expected;
+extern char kgdb_fiq_handler; +extern char kgdb_fiq_handler_end; +asmlinkage void __exception_irq_entry kgdb_fiq_do_handle(struct pt_regs *regs); +extern int __init kgdb_register_fiq(unsigned int mach_kgdb_fiq, + void (*mach_kgdb_enable_fiq)(unsigned int irq, bool on), + bool (*mach_is_kgdb_fiq)(unsigned int irq)); #endif /* !__ASSEMBLY__ */
/* diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index 7ad2d5c..5aa079b 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -56,6 +56,7 @@ obj-$(CONFIG_ATAGS_PROC) += atags.o obj-$(CONFIG_OABI_COMPAT) += sys_oabi-compat.o obj-$(CONFIG_ARM_THUMBEE) += thumbee.o obj-$(CONFIG_KGDB) += kgdb.o +obj-$(CONFIG_KGDB_FIQ) += kgdb_fiq_entry.o kgdb_fiq.o obj-$(CONFIG_ARM_UNWIND) += unwind.o obj-$(CONFIG_HAVE_TCM) += tcm.o obj-$(CONFIG_OF) += devtree.o diff --git a/arch/arm/kernel/kgdb_fiq.c b/arch/arm/kernel/kgdb_fiq.c new file mode 100644 index 0000000..72a62c7 --- /dev/null +++ b/arch/arm/kernel/kgdb_fiq.c @@ -0,0 +1,101 @@ +/* + * KGDB FIQ entry + * + * Copyright 2010 Google, Inc. + * Arve Hjønnevåg arve@android.com + * Colin Cross ccross@android.com + * Copyright 2012 Linaro Ltd. + * Anton Vorontsov anton.vorontsov@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/errno.h> +#include <linux/hardirq.h> +#include <linux/kgdb.h> +#include <asm/fiq.h> +#include <asm/exception.h> + +static int kgdb_fiq_enabled; +module_param_named(enable, kgdb_fiq_enabled, int, 0600); +MODULE_PARM_DESC(enable, "set to 1 to enable FIQ KGDB"); + +static unsigned int kgdb_fiq; +static void (*kgdb_enable_fiq)(unsigned int irq, bool on); +static bool (*is_kgdb_fiq)(unsigned int irq); + +asmlinkage void __exception_irq_entry kgdb_fiq_do_handle(struct pt_regs *regs) +{ + if (!is_kgdb_fiq(kgdb_fiq)) + return; + dbg_io_ops->clear_irqs(); + + nmi_enter(); + kgdb_handle_exception(1, 0, 0, regs); + nmi_exit(); +} + +static struct fiq_handler kgdb_fiq_desc = { + .name = "kgdb", +}; + +static long kgdb_fiq_setup_stack(void *info) +{ + struct pt_regs regs; + + regs.ARM_sp = __get_free_pages(GFP_KERNEL, THREAD_SIZE_ORDER) + + THREAD_START_SP; + WARN_ON(!regs.ARM_sp); + + set_fiq_regs(®s); + return 0; +} + +int kgdb_arch_enable_nmi(bool on) +{ + static int cnt; + + if (cnt > 0 && on) + return cnt; + cnt += on ? 1 : -1; + kgdb_enable_fiq(kgdb_fiq, cnt > 0); + return cnt; +} + +int __init kgdb_register_fiq(unsigned int mach_kgdb_fiq, + void (*mach_kgdb_enable_fiq)(unsigned int irq, bool on), + bool (*mach_is_kgdb_fiq)(unsigned int irq)) +{ + int err; + int cpu; + + if (!kgdb_fiq_enabled) + return -ENODEV; + if (kgdb_fiq) + return -EBUSY; + + kgdb_fiq = mach_kgdb_fiq; + kgdb_enable_fiq = mach_kgdb_enable_fiq; + is_kgdb_fiq = mach_is_kgdb_fiq; + + err = claim_fiq(&kgdb_fiq_desc); + if (err) { + pr_warn("%s: unable to claim fiq", __func__); + return err; + } + + for_each_possible_cpu(cpu) + work_on_cpu(cpu, kgdb_fiq_setup_stack, NULL); + + set_fiq_handler(&kgdb_fiq_handler, + &kgdb_fiq_handler_end - &kgdb_fiq_handler); + + kgdb_arch_enable_nmi(1); + return 0; +} diff --git a/arch/arm/kernel/kgdb_fiq_entry.S b/arch/arm/kernel/kgdb_fiq_entry.S new file mode 100644 index 0000000..7be3726 --- /dev/null +++ b/arch/arm/kernel/kgdb_fiq_entry.S @@ -0,0 +1,79 @@ +/* + * KGDB FIQ entry + * + * Copyright 1996,1997,1998 Russell King. + * Copyright 2012 Linaro Ltd. + * Anton Vorontsov anton.vorontsov@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + */ + +#include <linux/linkage.h> +#include <asm/assembler.h> +#include <asm/memory.h> +#include <asm/unwind.h> +#include "entry-header.S" + + .text + +@ This is needed for usr_entry/alignment_trap +.LCcralign: + .long cr_alignment +.LCdohandle: + .long kgdb_fiq_do_handle + + .macro fiq_handler + ldr r1, =.LCdohandle + mov r0, sp + adr lr, BSYM(9997f) + ldr pc, [r1] +9997: + .endm + + .align 5 +__fiq_svc: + svc_entry + fiq_handler + svc_exit r5 @ return from exception + UNWIND(.fnend ) +ENDPROC(__fiq_svc) + .ltorg + + .align 5 +__fiq_usr: + usr_entry + kuser_cmpxchg_check + fiq_handler + get_thread_info tsk + mov why, #0 + b ret_to_user_from_irq + UNWIND(.fnend ) +ENDPROC(__fiq_usr) + .ltorg + + .global kgdb_fiq_handler +kgdb_fiq_handler: + + vector_stub fiq, FIQ_MODE, 4 + + .long __fiq_usr @ 0 (USR_26 / USR_32) + .long __fiq_svc @ 1 (FIQ_26 / FIQ_32) + .long __fiq_svc @ 2 (IRQ_26 / IRQ_32) + .long __fiq_svc @ 3 (SVC_26 / SVC_32) + .long __fiq_svc @ 4 + .long __fiq_svc @ 5 + .long __fiq_svc @ 6 + .long __fiq_svc @ 7 + .long __fiq_svc @ 8 + .long __fiq_svc @ 9 + .long __fiq_svc @ a + .long __fiq_svc @ b + .long __fiq_svc @ c + .long __fiq_svc @ d + .long __fiq_svc @ e + .long __fiq_svc @ f + + .global kgdb_fiq_handler_end +kgdb_fiq_handler_end:
On Mon, Jul 30, 2012 at 04:58:16AM -0700, Anton Vorontsov wrote:
- .align 5
+__fiq_svc:
- svc_entry
- fiq_handler
- svc_exit r5 @ return from exception
- UNWIND(.fnend )
+ENDPROC(__fiq_svc)
- .ltorg
- .align 5
+__fiq_usr:
- usr_entry
- kuser_cmpxchg_check
- fiq_handler
- get_thread_info tsk
- mov why, #0
- b ret_to_user_from_irq
- UNWIND(.fnend )
+ENDPROC(__fiq_usr)
- .ltorg
- .global kgdb_fiq_handler
+kgdb_fiq_handler:
- vector_stub fiq, FIQ_MODE, 4
- .long __fiq_usr @ 0 (USR_26 / USR_32)
- .long __fiq_svc @ 1 (FIQ_26 / FIQ_32)
- .long __fiq_svc @ 2 (IRQ_26 / IRQ_32)
- .long __fiq_svc @ 3 (SVC_26 / SVC_32)
- .long __fiq_svc @ 4
- .long __fiq_svc @ 5
- .long __fiq_svc @ 6
- .long __fiq_svc @ 7
- .long __fiq_svc @ 8
- .long __fiq_svc @ 9
- .long __fiq_svc @ a
- .long __fiq_svc @ b
- .long __fiq_svc @ c
- .long __fiq_svc @ d
- .long __fiq_svc @ e
- .long __fiq_svc @ f
I am not convinced that this does not cause loss of state from the parent context. Let's review what happens when a FIQ is received from SVC mode with the above code.
- The CPU will be in SVC mode. - FIQ received. - CPU saves CPSR into SPSR_fiq and PC into LR_fiq, and jumps to the FIQ vector. - We apply the 4 byte correction to LR_fiq, and store r0, LR_fiq and SPSR_fiq to the FIQ 'stack' - We switch to SVC mode and jump to __fiq_svc - svc_entry: - adjusts the SVC stack pointer down, and saves r1 - r12 - loads r0, LR_fiq and SPSR_fiq and saves them as ARM_r0, ARM_pc, ARM_cpsr into the pt_regs - the original value of the SVC stack pointer is saved as ARM_r13 - LR_svc is saved as ARM_r14
At this point, we have saved everything *except* for the SPSR_svc register.
Now, when we return from the above, we use svc_exit: - write SPSR_svc with ARM_cpsr (from SPSR_fiq) - load r0-pc from the pt_regs and load CPSR from SPSR_svc
Now the thing here is that even if we did preserve SPSR_svc, with the above exit sequence, there is _no_ way to preserve the value of SPSR_svc. Normally, this doesn't matter because we know that the regions we care about this have IRQs disabled.
However, what this means, if we receive an FIQ and use this path from any part of the kernel which expects SPSR_svc to be preserved (eg, the exit path from any exception) the kernel will blow up.
I guess you could do something like this instead: - disable FIQs - load SPSR_svc with a saved value of it from entry. - load r1-r14 from ARM_r1..ARM_lr - switch to FIQ mode - load SPSR_fiq from saved ARM_cpsr - load r0 from ARM_r0 - load pc from ARM_pc
So, maybe something like this for the svc return path:
cpsid f ldr r1, [saved_spsr_svc] mov r0, sp mrs spsr_cxsf, r1 ldmib r0, {r1 - r14} msr cpsr_c, #FIQ_MODE | PSR_I_BIT | PSR_F_BIT add r7, r0, #S_PC ldr r8, [r0, #S_CPSR] mrs spsr_cxsf, r8 ldr r0, [r0, #S_R0] ldmia r7, {pc}^
On Mon, Jul 30, 2012 at 03:07:24PM +0100, Russell King - ARM Linux wrote: [....]
Now the thing here is that even if we did preserve SPSR_svc, with the above exit sequence, there is _no_ way to preserve the value of SPSR_svc. Normally, this doesn't matter because we know that the regions we care about this have IRQs disabled.
Wow, that is quite subtle. Indeed, now I clearly see that we do mangle SVC's SPSR.
Thanks!
[...]
So, maybe something like this for the svc return path:
cpsid f ldr r1, [saved_spsr_svc] mov r0, sp mrs spsr_cxsf, r1 ldmib r0, {r1 - r14} msr cpsr_c, #FIQ_MODE | PSR_I_BIT | PSR_F_BIT
...
ldmia r7, {pc}^
Yup, I got the idea: we have to restore the SPSR in SVC mode, but then we switch to FIQ mode and exit the exception while being in the FIQ mode, that way CPU won't change our restored SPSR_svc.
As for "saved_spsr_svc", I think the easiest way to get it, is to switch to FIQ mode temporary, and read it from the stack.
(Plus, cpsid is not available for older CPUs, but the thing is that we don't touch global enable/disable FIQs/IRQs flags, so I guess I don't actually need it.)
So, that's what I've applied on top:
@@ -33,7 +33,20 @@ __fiq_svc: svc_entry fiq_handler - svc_exit r5 @ return from exception + mrs r4, cpsr + msr cpsr_c, #FIQ_MODE | PSR_I_BIT | PSR_F_BIT + ldr r1, [sp, #8] + msr cpsr_c, r4 + msr spsr_cxsf, r1 + mov r0, sp + ldmib r0, {r1 - r14} + msr cpsr_c, #FIQ_MODE | PSR_I_BIT | PSR_F_BIT + add r8, r0, #S_PC + ldr r9, [r0, #S_PSR] + msr spsr_cxsf, r9 + ldr r0, [r0, #S_R0] + ldmia r8, {pc}^
And the whole fixed up patch is down below:
- - - - From: Anton Vorontsov anton.vorontsov@linaro.org Subject: ARM: Add KGDB/KDB FIQ debugger generic code
The FIQ debugger may be used to debug situations when the kernel stuck in uninterruptable sections, e.g. the kernel infinitely loops or deadlocked in an interrupt or with interrupts disabled.
By default KGDB FIQ is disabled in runtime, but can be enabled with kgdb_fiq.enable=1 kernel command line option.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/Kconfig | 18 +++++++ arch/arm/include/asm/kgdb.h | 8 +++ arch/arm/kernel/Makefile | 1 + arch/arm/kernel/kgdb_fiq.c | 101 ++++++++++++++++++++++++++++++++++++++ arch/arm/kernel/kgdb_fiq_entry.S | 92 ++++++++++++++++++++++++++++++++++ 5 files changed, 220 insertions(+) create mode 100644 arch/arm/kernel/kgdb_fiq.c create mode 100644 arch/arm/kernel/kgdb_fiq_entry.S
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index a91009c..ae6ddf1 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -172,6 +172,24 @@ config GENERIC_ISA_DMA config FIQ bool
+config ARCH_MIGHT_HAVE_FIQ + bool + +config KGDB_FIQ + bool "KGDB/KDB FIQ debugger" + depends on KGDB_KDB && ARCH_MIGHT_HAVE_FIQ && !THUMB2_KERNEL + select FIQ + help + The FIQ debugger may be used to debug situations when the + kernel stuck in uninterruptable sections, e.g. the kernel + infinitely loops or deadlocked in an interrupt or with + interrupts disabled. + + By default KGDB FIQ is disabled in runtime, but can be + enabled with kgdb_fiq.enable=1 kernel command line option. + + If unsure, say N. + config NEED_RET_TO_USER bool
diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h index 48066ce..807e547 100644 --- a/arch/arm/include/asm/kgdb.h +++ b/arch/arm/include/asm/kgdb.h @@ -11,6 +11,8 @@ #define __ARM_KGDB_H__
#include <linux/ptrace.h> +#include <linux/linkage.h> +#include <asm/exception.h>
/* * GDB assumes that we're a user process being debugged, so @@ -47,6 +49,12 @@ static inline void arch_kgdb_breakpoint(void) extern void kgdb_handle_bus_error(void); extern int kgdb_fault_expected;
+extern char kgdb_fiq_handler; +extern char kgdb_fiq_handler_end; +asmlinkage void __exception_irq_entry kgdb_fiq_do_handle(struct pt_regs *regs); +extern int __init kgdb_register_fiq(unsigned int mach_kgdb_fiq, + void (*mach_kgdb_enable_fiq)(unsigned int irq, bool on), + bool (*mach_is_kgdb_fiq)(unsigned int irq)); #endif /* !__ASSEMBLY__ */
/* diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index 7ad2d5c..5aa079b 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -56,6 +56,7 @@ obj-$(CONFIG_ATAGS_PROC) += atags.o obj-$(CONFIG_OABI_COMPAT) += sys_oabi-compat.o obj-$(CONFIG_ARM_THUMBEE) += thumbee.o obj-$(CONFIG_KGDB) += kgdb.o +obj-$(CONFIG_KGDB_FIQ) += kgdb_fiq_entry.o kgdb_fiq.o obj-$(CONFIG_ARM_UNWIND) += unwind.o obj-$(CONFIG_HAVE_TCM) += tcm.o obj-$(CONFIG_OF) += devtree.o diff --git a/arch/arm/kernel/kgdb_fiq.c b/arch/arm/kernel/kgdb_fiq.c new file mode 100644 index 0000000..72a62c7 --- /dev/null +++ b/arch/arm/kernel/kgdb_fiq.c @@ -0,0 +1,101 @@ +/* + * KGDB FIQ entry + * + * Copyright 2010 Google, Inc. + * Arve Hjønnevåg arve@android.com + * Colin Cross ccross@android.com + * Copyright 2012 Linaro Ltd. + * Anton Vorontsov anton.vorontsov@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/errno.h> +#include <linux/hardirq.h> +#include <linux/kgdb.h> +#include <asm/fiq.h> +#include <asm/exception.h> + +static int kgdb_fiq_enabled; +module_param_named(enable, kgdb_fiq_enabled, int, 0600); +MODULE_PARM_DESC(enable, "set to 1 to enable FIQ KGDB"); + +static unsigned int kgdb_fiq; +static void (*kgdb_enable_fiq)(unsigned int irq, bool on); +static bool (*is_kgdb_fiq)(unsigned int irq); + +asmlinkage void __exception_irq_entry kgdb_fiq_do_handle(struct pt_regs *regs) +{ + if (!is_kgdb_fiq(kgdb_fiq)) + return; + dbg_io_ops->clear_irqs(); + + nmi_enter(); + kgdb_handle_exception(1, 0, 0, regs); + nmi_exit(); +} + +static struct fiq_handler kgdb_fiq_desc = { + .name = "kgdb", +}; + +static long kgdb_fiq_setup_stack(void *info) +{ + struct pt_regs regs; + + regs.ARM_sp = __get_free_pages(GFP_KERNEL, THREAD_SIZE_ORDER) + + THREAD_START_SP; + WARN_ON(!regs.ARM_sp); + + set_fiq_regs(®s); + return 0; +} + +int kgdb_arch_enable_nmi(bool on) +{ + static int cnt; + + if (cnt > 0 && on) + return cnt; + cnt += on ? 1 : -1; + kgdb_enable_fiq(kgdb_fiq, cnt > 0); + return cnt; +} + +int __init kgdb_register_fiq(unsigned int mach_kgdb_fiq, + void (*mach_kgdb_enable_fiq)(unsigned int irq, bool on), + bool (*mach_is_kgdb_fiq)(unsigned int irq)) +{ + int err; + int cpu; + + if (!kgdb_fiq_enabled) + return -ENODEV; + if (kgdb_fiq) + return -EBUSY; + + kgdb_fiq = mach_kgdb_fiq; + kgdb_enable_fiq = mach_kgdb_enable_fiq; + is_kgdb_fiq = mach_is_kgdb_fiq; + + err = claim_fiq(&kgdb_fiq_desc); + if (err) { + pr_warn("%s: unable to claim fiq", __func__); + return err; + } + + for_each_possible_cpu(cpu) + work_on_cpu(cpu, kgdb_fiq_setup_stack, NULL); + + set_fiq_handler(&kgdb_fiq_handler, + &kgdb_fiq_handler_end - &kgdb_fiq_handler); + + kgdb_arch_enable_nmi(1); + return 0; +} diff --git a/arch/arm/kernel/kgdb_fiq_entry.S b/arch/arm/kernel/kgdb_fiq_entry.S new file mode 100644 index 0000000..a6a1ad1 --- /dev/null +++ b/arch/arm/kernel/kgdb_fiq_entry.S @@ -0,0 +1,92 @@ +/* + * KGDB FIQ entry + * + * Copyright 1996,1997,1998 Russell King. + * Copyright 2012 Linaro Ltd. + * Anton Vorontsov anton.vorontsov@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + */ + +#include <linux/linkage.h> +#include <asm/assembler.h> +#include <asm/memory.h> +#include <asm/unwind.h> +#include "entry-header.S" + + .text + +@ This is needed for usr_entry/alignment_trap +.LCcralign: + .long cr_alignment +.LCdohandle: + .long kgdb_fiq_do_handle + + .macro fiq_handler + ldr r1, =.LCdohandle + mov r0, sp + adr lr, BSYM(9997f) + ldr pc, [r1] +9997: + .endm + + .align 5 +__fiq_svc: + svc_entry + fiq_handler + mrs r4, cpsr + msr cpsr_c, #FIQ_MODE | PSR_I_BIT | PSR_F_BIT + ldr r1, [sp, #8] + msr cpsr_c, r4 + msr spsr_cxsf, r1 + mov r0, sp + ldmib r0, {r1 - r14} + msr cpsr_c, #FIQ_MODE | PSR_I_BIT | PSR_F_BIT + add r8, r0, #S_PC + ldr r9, [r0, #S_PSR] + msr spsr_cxsf, r9 + ldr r0, [r0, #S_R0] + ldmia r8, {pc}^ + + UNWIND(.fnend ) +ENDPROC(__fiq_svc) + .ltorg + + .align 5 +__fiq_usr: + usr_entry + kuser_cmpxchg_check + fiq_handler + get_thread_info tsk + mov why, #0 + b ret_to_user_from_irq + UNWIND(.fnend ) +ENDPROC(__fiq_usr) + .ltorg + + .global kgdb_fiq_handler +kgdb_fiq_handler: + + vector_stub fiq, FIQ_MODE, 4 + + .long __fiq_usr @ 0 (USR_26 / USR_32) + .long __fiq_svc @ 1 (FIQ_26 / FIQ_32) + .long __fiq_svc @ 2 (IRQ_26 / IRQ_32) + .long __fiq_svc @ 3 (SVC_26 / SVC_32) + .long __fiq_svc @ 4 + .long __fiq_svc @ 5 + .long __fiq_svc @ 6 + .long __fiq_svc @ 7 + .long __fiq_svc @ 8 + .long __fiq_svc @ 9 + .long __fiq_svc @ a + .long __fiq_svc @ b + .long __fiq_svc @ c + .long __fiq_svc @ d + .long __fiq_svc @ e + .long __fiq_svc @ f + + .global kgdb_fiq_handler_end +kgdb_fiq_handler_end:
As Colin Cross noticed, serial ports could be noisy, so occasional characters once in a while are possible. So, considering the noise possibility, entering the debugger on any received byte is unacceptable for production devices.
This changes KGDB FIQ behaviour in a such way so that we have to type the GDB-protocol "$3#33" command to actually enter the debugger, the kernel will print the following prompt:
Type $3#33 to enter the debugger>
This is the exactly the same command we use to escape from KGDB to KDB, so it should be all pretty familiar.
For convenience, there is a kgdb_fiq.knock kernel command line option, when set to 0, this turns the special command to just a return key press, so the kernel will be printing this:
Hit <return> to enter the debugger>
And for the cases when NMI connected to a dedicated button, the knocking can be disabled altogether by setting kgdb_fiq.knock to -1.
Suggested-by: Colin Cross ccross@android.com Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/kernel/kgdb_fiq.c | 58 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/arch/arm/kernel/kgdb_fiq.c b/arch/arm/kernel/kgdb_fiq.c index 72a62c7..2bf4467 100644 --- a/arch/arm/kernel/kgdb_fiq.c +++ b/arch/arm/kernel/kgdb_fiq.c @@ -18,6 +18,7 @@ #include <linux/slab.h> #include <linux/errno.h> #include <linux/hardirq.h> +#include <linux/kdb.h> #include <linux/kgdb.h> #include <asm/fiq.h> #include <asm/exception.h> @@ -26,6 +27,60 @@ static int kgdb_fiq_enabled; module_param_named(enable, kgdb_fiq_enabled, int, 0600); MODULE_PARM_DESC(enable, "set to 1 to enable FIQ KGDB");
+static int kgdb_fiq_knock = 1; +module_param_named(knock, kgdb_fiq_knock, int, 0600); +MODULE_PARM_DESC(knock, "if set to 1 (default), the special '$3#33' command " + "must be used to enter the debugger; when set to 0, " + "hitting return key is enough to enter the debugger; " + "when set to -1, the debugger is entered immediately " + "upon NMI"); + +/* + * "Serial ports are often noisy, especially when muxed over another port (we + * often use serial over the headset connector). Noise on the async command + * line just causes characters that are ignored, on a command line that blocked + * execution noise would be catastrophic." -- Colin Cross + * + * So, this small function implements KGDB/KDB knocking on the serial line: we + * won't enter the debugger until we receive a known magic phrase (which is + * actually "$3#33", known as "escape to KDB" command. If knocking is disabled, + * just pressing the return key is enough to enter the debugger. + */ +static bool kgdb_fiq_poll_knock(void) +{ + static int n; + int c = -1; + get_char_func *getc; + char magic[] = "$3#33"; + size_t m = strlen(magic); + + if (kgdb_fiq_knock < 0) + return 1; + + for (getc = &kdb_poll_funcs[0]; *getc; ++getc) { + c = (*getc)(); + if (c >= 0) + break; + } + + if (!kgdb_fiq_knock && (c == '\r' || c == '\n')) { + return 1; + } else if (c == magic[n]) { + kdb_printf("%c", c); + n = (n + 1) % m; + if (!n) + return 1; + } else { + n = 0; + kdb_printf("\r%s %s to enter the debugger> %*s", + kgdb_fiq_knock ? "Type" : "Hit", + kgdb_fiq_knock ? magic : "<return>", m, ""); + memset(magic, '\b', m); + kdb_printf("%s", magic); + } + return 0; +} + static unsigned int kgdb_fiq; static void (*kgdb_enable_fiq)(unsigned int irq, bool on); static bool (*is_kgdb_fiq)(unsigned int irq); @@ -36,6 +91,9 @@ asmlinkage void __exception_irq_entry kgdb_fiq_do_handle(struct pt_regs *regs) return; dbg_io_ops->clear_irqs();
+ if (!kgdb_fiq_poll_knock()) + return; + nmi_enter(); kgdb_handle_exception(1, 0, 0, regs); nmi_exit();
Just a couple of calls to manage VIC FIQ routing. We'll use them for KGDB FIQ support on ARM Versatile machines.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/common/vic.c | 28 ++++++++++++++++++++++++++++ arch/arm/include/asm/hardware/vic.h | 2 ++ 2 files changed, 30 insertions(+)
diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c index e0d5388..df2fc82 100644 --- a/arch/arm/common/vic.c +++ b/arch/arm/common/vic.c @@ -66,6 +66,34 @@ static struct vic_device vic_devices[CONFIG_ARM_VIC_NR];
static int vic_id;
+static void __iomem *vic_base(struct irq_data *d) +{ + return (void __iomem *)irq_data_get_irq_chip_data(d); +} + +void vic_fiq_select(unsigned int irq, bool on) +{ + void __iomem *base = vic_base(&irq_to_desc(irq)->irq_data); + void __iomem *sel = base + VIC_INT_SELECT; + u32 msk = 1 << irq; + u32 val; + + pr_debug("rerouting VIC vector %d to %s\n", irq, on ? "FIQ" : "IRQ"); + + val = readl(sel); + val &= ~msk; + if (on) + val |= msk; + writel(val, sel); +} + +bool vic_is_fiq_rised(unsigned int irq) +{ + void __iomem *base = vic_base(&irq_to_desc(irq)->irq_data); + + return readl(base + VIC_FIQ_STATUS) & (1 << irq); +} + /** * vic_init2 - common initialisation code * @base: Base of the VIC. diff --git a/arch/arm/include/asm/hardware/vic.h b/arch/arm/include/asm/hardware/vic.h index e14af1a..2728975 100644 --- a/arch/arm/include/asm/hardware/vic.h +++ b/arch/arm/include/asm/hardware/vic.h @@ -52,6 +52,8 @@ void __vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources); int vic_of_init(struct device_node *node, struct device_node *parent); void vic_handle_irq(struct pt_regs *regs); +void vic_fiq_select(unsigned int irq, bool on); +bool vic_is_fiq_rised(unsigned int irq);
#endif /* __ASSEMBLY__ */ #endif
If enabled, kernel will able to enter KGDB upon serial line activity on UART ports.
Note that even with this patch and CONFIG_KGDB_FIQ is enabled, you still need to pass kgdb_fiq.enable=1 kernel command line option, otherwise UART will behave in a normal way.
By default UART0 is used, but this can be changed via kgdb_fiq.uart_num kernel command line option.
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/Kconfig | 1 + arch/arm/mach-versatile/Makefile | 1 + arch/arm/mach-versatile/include/mach/irqs.h | 1 + arch/arm/mach-versatile/kgdb_fiq.c | 31 +++++++++++++++++++++++++++ 4 files changed, 34 insertions(+) create mode 100644 arch/arm/mach-versatile/kgdb_fiq.c
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index ae6ddf1..c0df1ba 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -311,6 +311,7 @@ config ARCH_VERSATILE select ICST select GENERIC_CLOCKEVENTS select ARCH_WANT_OPTIONAL_GPIOLIB + select ARCH_MIGHT_HAVE_FIQ select NEED_MACH_IO_H if PCI select PLAT_VERSATILE select PLAT_VERSATILE_CLCD diff --git a/arch/arm/mach-versatile/Makefile b/arch/arm/mach-versatile/Makefile index 81fa3fe..bfd761f 100644 --- a/arch/arm/mach-versatile/Makefile +++ b/arch/arm/mach-versatile/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_ARCH_VERSATILE_PB) += versatile_pb.o obj-$(CONFIG_MACH_VERSATILE_AB) += versatile_ab.o obj-$(CONFIG_MACH_VERSATILE_DT) += versatile_dt.o obj-$(CONFIG_PCI) += pci.o +obj-$(CONFIG_KGDB_FIQ) += kgdb_fiq.o diff --git a/arch/arm/mach-versatile/include/mach/irqs.h b/arch/arm/mach-versatile/include/mach/irqs.h index bf44c61..fcd2a95 100644 --- a/arch/arm/mach-versatile/include/mach/irqs.h +++ b/arch/arm/mach-versatile/include/mach/irqs.h @@ -26,6 +26,7 @@ * held within platform.h */ #define IRQ_VIC_START 0 +#define FIQ_START IRQ_VIC_START #define IRQ_WDOGINT (IRQ_VIC_START + INT_WDOGINT) #define IRQ_SOFTINT (IRQ_VIC_START + INT_SOFTINT) #define IRQ_COMMRx (IRQ_VIC_START + INT_COMMRx) diff --git a/arch/arm/mach-versatile/kgdb_fiq.c b/arch/arm/mach-versatile/kgdb_fiq.c new file mode 100644 index 0000000..3cdf71d --- /dev/null +++ b/arch/arm/mach-versatile/kgdb_fiq.c @@ -0,0 +1,31 @@ +/* + * KGDB FIQ board support + * + * Copyright 2012 Linaro Ltd. + * Anton Vorontsov anton.vorontsov@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/kgdb.h> +#include <mach/hardware.h> +#include <mach/platform.h> +#include <asm/hardware/vic.h> + +static int kgdb_fiq; +module_param_named(uart_num, kgdb_fiq, int, 0600); +MODULE_PARM_DESC(uart_num, "UART<number> port to use for KGDB FIQ"); + +static int __init kgdb_fiq_init(void) +{ + WARN_ON(kgdb_fiq > INT_UARTINT2 - INT_UARTINT0); + + return kgdb_register_fiq(INT_UARTINT0 + kgdb_fiq, + vic_fiq_select, + vic_is_fiq_rised); +} +console_initcall(kgdb_fiq_init);
On Mon, Jul 30, 2012 at 04:58:19AM -0700, Anton Vorontsov wrote:
diff --git a/arch/arm/mach-versatile/include/mach/irqs.h b/arch/arm/mach-versatile/include/mach/irqs.h index bf44c61..fcd2a95 100644 --- a/arch/arm/mach-versatile/include/mach/irqs.h +++ b/arch/arm/mach-versatile/include/mach/irqs.h @@ -26,6 +26,7 @@
- held within platform.h
*/ #define IRQ_VIC_START 0 +#define FIQ_START IRQ_VIC_START
No. We want to ultimately get rid of FIQ_START.
This makes the code more izolated.
The downside of this is that we now have an additional branch and the code itself is 8 bytes longer. But on the bright side, this new layout can be more cache friendly since cr_alignment address might be already in the cache line (not that I measured anything, it's just fun to think about it).
Signed-off-by: Anton Vorontsov anton.vorontsov@linaro.org --- arch/arm/kernel/entry-armv.S | 2 -- arch/arm/kernel/entry-header.S | 6 +++++- arch/arm/kernel/kgdb_fiq_entry.S | 3 --- 3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S index 6aeb9b8..6b04ab5 100644 --- a/arch/arm/kernel/entry-armv.S +++ b/arch/arm/kernel/entry-armv.S @@ -266,8 +266,6 @@ __pabt_svc: ENDPROC(__pabt_svc)
.align 5 -.LCcralign: - .word cr_alignment #ifdef MULTI_DABORT .LCprocfns: .word processor diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S index c3c09ac..5a05e7f 100644 --- a/arch/arm/kernel/entry-header.S +++ b/arch/arm/kernel/entry-header.S @@ -38,9 +38,13 @@
.macro alignment_trap, rtemp #ifdef CONFIG_ALIGNMENT_TRAP - ldr \rtemp, .LCcralign + ldr \rtemp, 1f ldr \rtemp, [\rtemp] mcr p15, 0, \rtemp, c1, c0 + b 2f +1: + .word cr_alignment +2: #endif .endm
diff --git a/arch/arm/kernel/kgdb_fiq_entry.S b/arch/arm/kernel/kgdb_fiq_entry.S index 7be3726..e7c05fc 100644 --- a/arch/arm/kernel/kgdb_fiq_entry.S +++ b/arch/arm/kernel/kgdb_fiq_entry.S @@ -18,9 +18,6 @@
.text
-@ This is needed for usr_entry/alignment_trap -.LCcralign: - .long cr_alignment .LCdohandle: .long kgdb_fiq_do_handle
On Mon, Jul 30, 2012 at 04:58:20AM -0700, Anton Vorontsov wrote:
This makes the code more izolated.
The downside of this is that we now have an additional branch and the code itself is 8 bytes longer. But on the bright side, this new layout can be more cache friendly since cr_alignment address might be already in the cache line (not that I measured anything, it's just fun to think about it).
The caches are harvard, so mixing data and code together does not increase performance. Having data which is used by the same code in the same cache line results in better performance.
The additional branch will also cause a pipeline stall on older CPUs.
So no, I don't see any way that this is a performance improvement. Please leave this as is.
On Mon, Jul 30, 2012 at 03:15:44PM +0100, Russell King - ARM Linux wrote:
On Mon, Jul 30, 2012 at 04:58:20AM -0700, Anton Vorontsov wrote:
This makes the code more izolated.
The downside of this is that we now have an additional branch and the code itself is 8 bytes longer. But on the bright side, this new layout can be more cache friendly since cr_alignment address might be already in the cache line (not that I measured anything, it's just fun to think about it).
The caches are harvard, so mixing data and code together does not increase performance. Having data which is used by the same code in the same cache line results in better performance.
The additional branch will also cause a pipeline stall on older CPUs.
So no, I don't see any way that this is a performance improvement. Please leave this as is.
Sure, will drop it.
Thanks!
linaro-kernel@lists.linaro.org
-
Anton Vorontsov -
Anton Vorontsov -
Colin Cross -
Jason Wessel -
Russell King - ARM Linux