| Message ID | 1434036566-9848-10-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dave, Just a few comments, inline. On Thu, Jun 11, 2015 at 04:29:23PM +0100, Dave P Martin wrote: > Currently, the minimal default BUG() implementation from asm- > generic is used for arm64. > > This patch uses the BRK software breakpoint instruction to generate > a trap instead, similarly to most other arches, with the generic > BUG code generating the dmesg boilerplate. > > This allows bug metadata to be moved to a separate table and > reduces the amount of inline code at BUG and WARN sites. This also > avoids clobbering any registers before they can be dumped. > > To mitigate the size of the bug table further, this patch makes > use of the existing infrastructure for encoding addresses within > the bug table as 32-bit offsets instead of absolute pointers. > (Note that this limits the kernel size to 2GB.) > > Traps are registered at arch_initcall time for aarch64, but BUG > has minimal real dependencies and it is desirable to be able to > generate bug splats as early as possible. This patch redirects > all debug exceptions caused by BRK directly to bug_handler() until > the full debug exception support has been initialised. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > arch/arm64/Kconfig | 8 ++++++ > arch/arm64/include/asm/brk.h | 1 + > arch/arm64/include/asm/bug.h | 64 ++++++++++++++++++++++++++++++++++++++++++ > arch/arm64/kernel/traps.c | 57 ++++++++++++++++++++++++++++++++++++- > arch/arm64/mm/fault.c | 12 ++++++-- > 5 files changed, 139 insertions(+), 3 deletions(-) > create mode 100644 arch/arm64/include/asm/bug.h > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 7796af4..aedda42 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -110,6 +110,14 @@ config TRACE_IRQFLAGS_SUPPORT > config RWSEM_XCHGADD_ALGORITHM > def_bool y > > +config GENERIC_BUG > + def_bool y > + depends on BUG > + > +config GENERIC_BUG_RELATIVE_POINTERS > + def_bool y > + depends on GENERIC_BUG > + > config GENERIC_HWEIGHT > def_bool y > > diff --git a/arch/arm64/include/asm/brk.h b/arch/arm64/include/asm/brk.h > index 99b8dfb..f4d5894 100644 > --- a/arch/arm64/include/asm/brk.h > +++ b/arch/arm64/include/asm/brk.h > @@ -27,5 +27,6 @@ > #define FAULT_BRK_IMM 0x100 > #define KGDB_DYN_DBG_BRK_IMM 0x400 > #define KGDB_COMPILED_DBG_BRK_IMM 0x401 > +#define BUG_BRK_IMM 0x7ff Just curious, but how did you come up with this number? > #endif /* ! _ARCH_ARM64_ASM_BRK_H */ > diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h > new file mode 100644 > index 0000000..0429c7b > --- /dev/null > +++ b/arch/arm64/include/asm/bug.h > @@ -0,0 +1,64 @@ > +/* > + * Copyright (C) 2015 ARM Limited > + * Author: Dave Martin <Dave.Martin@arm.com> > + * > + * 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef _ARCH_ARM64_ASM_BUG_H > +#define _ARCH_ARM64_ASM_BUG_H Please follow the standard header guard pattern we use for arm64 (__ASM_). > + > +#include <asm/brk.h> > + > +#ifdef CONFIG_GENERIC_BUG > +#define HAVE_ARCH_BUG > + > +#ifdef CONFIG_DEBUG_BUGVERBOSE > +#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) > +#define __BUGVERBOSE_LOCATION(file, line) \ > + ".pushsection .rodata.str,\"aMS\",@progbits,1\n" \ > + "2: .string \"" file "\"\n\t" \ > + ".popsection\n\t" \ > + \ > + ".long 2b - 0b\n\t" \ > + ".short " #line "\n\t" > +#else > +#define _BUGVERBOSE_LOCATION(file, line) > +#endif > + > +#define _BUG_FLAGS(flags) __BUG_FLAGS(flags) > + > +#define __BUG_FLAGS(flags) asm volatile ( \ > + ".pushsection __bug_table,\"a\"\n\t" \ > + ".align 2\n\t" \ > + "0: .long 1f - 0b\n\t" \ > +_BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ > + ".short " #flags "\n\t" \ > + ".popsection\n" \ > + \ > + "1: brk %[imm]" \ > + :: [imm] "i" (BUG_BRK_IMM) \ > +) > + > +#define BUG() do { \ > + _BUG_FLAGS(0); \ > + unreachable(); \ > +} while (0) > + > +#define __WARN_TAINT(taint) _BUG_FLAGS(BUGFLAG_TAINT(taint)) > + > +#endif /* ! CONFIG_GENERIC_BUG */ > + > +#include <asm-generic/bug.h> > + > +#endif /* ! _ARCH_ARM64_ASM_BUG_H */ > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index 1ef2940..5fdf776 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -17,6 +17,7 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/bug.h> > #include <linux/signal.h> > #include <linux/personality.h> > #include <linux/kallsyms.h> > @@ -32,8 +33,11 @@ > #include <linux/syscalls.h> > > #include <asm/atomic.h> > +#include <asm/brk.h> > +#include <asm/bug.h> > #include <asm/debug-monitors.h> > #include <asm/esr.h> > +#include <asm/insn.h> > #include <asm/traps.h> > #include <asm/stacktrace.h> > #include <asm/exception.h> > @@ -460,7 +464,58 @@ void __pgd_error(const char *file, int line, unsigned long val) > pr_crit("%s:%d: bad pgd %016lx.\n", file, line, val); > } > > +/* GENERIC_BUG traps */ > + > +int is_valid_bugaddr(unsigned long addr) > +{ > + /* > + * bug_handler() only called for BUG #BUG_BRK_IMM. s/BUG/BRK/ ? > + * So the answer is trivial -- any spurious instances with no > + * bug table entry will be rejected by report_bug() and passed > + * back to the debug-monitors code and handled as a fatal > + * unexpected debug exception. > + */ > + return 1; > +} Could we define is_valid_bugaddr as a macro in the header file and avoid the potential out-of-line call? > + > +static int bug_handler(struct pt_regs *regs, unsigned int esr) > +{ > + if (user_mode(regs)) > + return DBG_HOOK_ERROR; > + > + switch (report_bug(regs->pc, regs)) { > + case BUG_TRAP_TYPE_BUG: > + die("Oops - BUG", regs, 0); > + /* die() does not return */ Are you sure about that? A quick glance at the code didn't convince me... Will
On Tue, Jun 16, 2015 at 03:48:10PM +0100, Will Deacon wrote: > Hi Dave, > > Just a few comments, inline. > > On Thu, Jun 11, 2015 at 04:29:23PM +0100, Dave P Martin wrote: > > Currently, the minimal default BUG() implementation from asm- > > generic is used for arm64. > > > > This patch uses the BRK software breakpoint instruction to generate > > a trap instead, similarly to most other arches, with the generic > > BUG code generating the dmesg boilerplate. > > > > This allows bug metadata to be moved to a separate table and > > reduces the amount of inline code at BUG and WARN sites. This also > > avoids clobbering any registers before they can be dumped. > > > > To mitigate the size of the bug table further, this patch makes > > use of the existing infrastructure for encoding addresses within > > the bug table as 32-bit offsets instead of absolute pointers. > > (Note that this limits the kernel size to 2GB.) > > > > Traps are registered at arch_initcall time for aarch64, but BUG > > has minimal real dependencies and it is desirable to be able to > > generate bug splats as early as possible. This patch redirects > > all debug exceptions caused by BRK directly to bug_handler() until > > the full debug exception support has been initialised. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > --- > > arch/arm64/Kconfig | 8 ++++++ > > arch/arm64/include/asm/brk.h | 1 + > > arch/arm64/include/asm/bug.h | 64 ++++++++++++++++++++++++++++++++++++++++++ > > arch/arm64/kernel/traps.c | 57 ++++++++++++++++++++++++++++++++++++- > > arch/arm64/mm/fault.c | 12 ++++++-- > > 5 files changed, 139 insertions(+), 3 deletions(-) > > create mode 100644 arch/arm64/include/asm/bug.h [...] > > diff --git a/arch/arm64/include/asm/brk.h b/arch/arm64/include/asm/brk.h > > index 99b8dfb..f4d5894 100644 > > --- a/arch/arm64/include/asm/brk.h > > +++ b/arch/arm64/include/asm/brk.h > > @@ -27,5 +27,6 @@ > > #define FAULT_BRK_IMM 0x100 > > #define KGDB_DYN_DBG_BRK_IMM 0x400 > > #define KGDB_COMPILED_DBG_BRK_IMM 0x401 > > +#define BUG_BRK_IMM 0x7ff > > Just curious, but how did you come up with this number? There's a comment in debug-monitors.h that the "allowed values for kgbd" (sic) are 0x400..0x7ff. So 0x7ff seemed unlikely to clash with any other use, and well out of the way of the values that kgbd currently uses. It's debatable that the BUG value shouldn't be in the range at all. However, BUG_BRK_IMM is a contract between the kernel and itself for EL1 only, so it can be changed at any time in the future with minimal impact. What's your view? [...] > > diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h > > new file mode 100644 > > index 0000000..0429c7b > > --- /dev/null > > +++ b/arch/arm64/include/asm/bug.h > > @@ -0,0 +1,64 @@ [...] > > +#ifndef _ARCH_ARM64_ASM_BUG_H > > +#define _ARCH_ARM64_ASM_BUG_H > > Please follow the standard header guard pattern we use for arm64 (__ASM_). Hmmm, I would say I picked a bad example to copy from, but I can't see another header matching what I have. __ASM_ is certainly the most common. There are also a few _ASM_ (mostly ACPI/EFI) and a few _ASM_ARM64. I'll follow the crowd and change to __ASM_. [...] > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > index 1ef2940..5fdf776 100644 > > --- a/arch/arm64/kernel/traps.c > > +++ b/arch/arm64/kernel/traps.c [...] > > @@ -460,7 +464,58 @@ void __pgd_error(const char *file, int line, unsigned long val) > > pr_crit("%s:%d: bad pgd %016lx.\n", file, line, val); > > } > > > > +/* GENERIC_BUG traps */ > > + > > +int is_valid_bugaddr(unsigned long addr) > > +{ > > + /* > > + * bug_handler() only called for BUG #BUG_BRK_IMM. > > s/BUG/BRK/ ? Hmmm, yes. > > > + * So the answer is trivial -- any spurious instances with no > > + * bug table entry will be rejected by report_bug() and passed > > + * back to the debug-monitors code and handled as a fatal > > + * unexpected debug exception. > > + */ > > + return 1; > > +} > > Could we define is_valid_bugaddr as a macro in the header file and avoid > the potential out-of-line call? We could, but it would require a change to <linux/bug.h>. Since this is slowpath anyway, I wasn't sure it was worth it. Happy to add that and try to push it as part of this series if you like -- let me know. > > > + > > +static int bug_handler(struct pt_regs *regs, unsigned int esr) > > +{ > > + if (user_mode(regs)) > > + return DBG_HOOK_ERROR; > > + > > + switch (report_bug(regs->pc, regs)) { > > + case BUG_TRAP_TYPE_BUG: > > + die("Oops - BUG", regs, 0); > > + /* die() does not return */ > > Are you sure about that? A quick glance at the code didn't convince me... Ouch. That explains why attempting to continue a BUGged thread using kgdb wasn't working too well... I'll fix that the re-test this behaviour. Thanks for the review. Cheers ---Dave
Hi Dave, On Wed, Jun 17, 2015 at 12:35:18PM +0100, Dave P Martin wrote: > On Tue, Jun 16, 2015 at 03:48:10PM +0100, Will Deacon wrote: > > On Thu, Jun 11, 2015 at 04:29:23PM +0100, Dave P Martin wrote: > > > diff --git a/arch/arm64/include/asm/brk.h b/arch/arm64/include/asm/brk.h > > > index 99b8dfb..f4d5894 100644 > > > --- a/arch/arm64/include/asm/brk.h > > > +++ b/arch/arm64/include/asm/brk.h > > > @@ -27,5 +27,6 @@ > > > #define FAULT_BRK_IMM 0x100 > > > #define KGDB_DYN_DBG_BRK_IMM 0x400 > > > #define KGDB_COMPILED_DBG_BRK_IMM 0x401 > > > +#define BUG_BRK_IMM 0x7ff > > > > Just curious, but how did you come up with this number? > > There's a comment in debug-monitors.h that the "allowed values for > kgbd" (sic) are 0x400..0x7ff. > > So 0x7ff seemed unlikely to clash with any other use, and well > out of the way of the values that kgbd currently uses. > > It's debatable that the BUG value shouldn't be in the range at all. > However, BUG_BRK_IMM is a contract between the kernel and itself for > EL1 only, so it can be changed at any time in the future with minimal > impact. > > What's your view? I wonder if that kgdb range is inclusive? Maybe best to use 0x800 to be sure. > > Could we define is_valid_bugaddr as a macro in the header file and avoid > > the potential out-of-line call? > > We could, but it would require a change to <linux/bug.h>. Since > this is slowpath anyway, I wasn't sure it was worth it. > > Happy to add that and try to push it as part of this series if > you like -- let me know. Nah, it's fine. I hadn't spotted the declaration. Will
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 7796af4..aedda42 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -110,6 +110,14 @@ config TRACE_IRQFLAGS_SUPPORT config RWSEM_XCHGADD_ALGORITHM def_bool y +config GENERIC_BUG + def_bool y + depends on BUG + +config GENERIC_BUG_RELATIVE_POINTERS + def_bool y + depends on GENERIC_BUG + config GENERIC_HWEIGHT def_bool y diff --git a/arch/arm64/include/asm/brk.h b/arch/arm64/include/asm/brk.h index 99b8dfb..f4d5894 100644 --- a/arch/arm64/include/asm/brk.h +++ b/arch/arm64/include/asm/brk.h @@ -27,5 +27,6 @@ #define FAULT_BRK_IMM 0x100 #define KGDB_DYN_DBG_BRK_IMM 0x400 #define KGDB_COMPILED_DBG_BRK_IMM 0x401 +#define BUG_BRK_IMM 0x7ff #endif /* ! _ARCH_ARM64_ASM_BRK_H */ diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h new file mode 100644 index 0000000..0429c7b --- /dev/null +++ b/arch/arm64/include/asm/bug.h @@ -0,0 +1,64 @@ +/* + * Copyright (C) 2015 ARM Limited + * Author: Dave Martin <Dave.Martin@arm.com> + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef _ARCH_ARM64_ASM_BUG_H +#define _ARCH_ARM64_ASM_BUG_H + +#include <asm/brk.h> + +#ifdef CONFIG_GENERIC_BUG +#define HAVE_ARCH_BUG + +#ifdef CONFIG_DEBUG_BUGVERBOSE +#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) +#define __BUGVERBOSE_LOCATION(file, line) \ + ".pushsection .rodata.str,\"aMS\",@progbits,1\n" \ + "2: .string \"" file "\"\n\t" \ + ".popsection\n\t" \ + \ + ".long 2b - 0b\n\t" \ + ".short " #line "\n\t" +#else +#define _BUGVERBOSE_LOCATION(file, line) +#endif + +#define _BUG_FLAGS(flags) __BUG_FLAGS(flags) + +#define __BUG_FLAGS(flags) asm volatile ( \ + ".pushsection __bug_table,\"a\"\n\t" \ + ".align 2\n\t" \ + "0: .long 1f - 0b\n\t" \ +_BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ + ".short " #flags "\n\t" \ + ".popsection\n" \ + \ + "1: brk %[imm]" \ + :: [imm] "i" (BUG_BRK_IMM) \ +) + +#define BUG() do { \ + _BUG_FLAGS(0); \ + unreachable(); \ +} while (0) + +#define __WARN_TAINT(taint) _BUG_FLAGS(BUGFLAG_TAINT(taint)) + +#endif /* ! CONFIG_GENERIC_BUG */ + +#include <asm-generic/bug.h> + +#endif /* ! _ARCH_ARM64_ASM_BUG_H */ diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 1ef2940..5fdf776 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -17,6 +17,7 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <linux/bug.h> #include <linux/signal.h> #include <linux/personality.h> #include <linux/kallsyms.h> @@ -32,8 +33,11 @@ #include <linux/syscalls.h> #include <asm/atomic.h> +#include <asm/brk.h> +#include <asm/bug.h> #include <asm/debug-monitors.h> #include <asm/esr.h> +#include <asm/insn.h> #include <asm/traps.h> #include <asm/stacktrace.h> #include <asm/exception.h> @@ -460,7 +464,58 @@ void __pgd_error(const char *file, int line, unsigned long val) pr_crit("%s:%d: bad pgd %016lx.\n", file, line, val); } +/* GENERIC_BUG traps */ + +int is_valid_bugaddr(unsigned long addr) +{ + /* + * bug_handler() only called for BUG #BUG_BRK_IMM. + * So the answer is trivial -- any spurious instances with no + * bug table entry will be rejected by report_bug() and passed + * back to the debug-monitors code and handled as a fatal + * unexpected debug exception. + */ + return 1; +} + +static int bug_handler(struct pt_regs *regs, unsigned int esr) +{ + if (user_mode(regs)) + return DBG_HOOK_ERROR; + + switch (report_bug(regs->pc, regs)) { + case BUG_TRAP_TYPE_BUG: + die("Oops - BUG", regs, 0); + /* die() does not return */ + + case BUG_TRAP_TYPE_WARN: + regs->pc += AARCH64_INSN_SIZE; /* skip BRK and resume */ + return DBG_HOOK_HANDLED; + + default: + /* unknown/unrecognised bug trap type */ + return DBG_HOOK_ERROR; + } +} + +static struct break_hook bug_break_hook = { + .esr_val = 0xf2000000 | BUG_BRK_IMM, + .esr_mask = 0xffffffff, + .fn = bug_handler, +}; + +/* + * Initial handler for AArch64 BRK exceptions + * This handler only used until debug_traps_init(). + */ +int __init early_brk64(unsigned long addr, unsigned int esr, + struct pt_regs *regs) +{ + return bug_handler(regs, esr) != DBG_HOOK_HANDLED; +} + +/* This registration must happen early, before debug_traps_init(). */ void __init trap_init(void) { - return; + register_break_hook(&bug_break_hook); } diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 96da131..c7c5eeb 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -486,14 +486,22 @@ asmlinkage void __exception do_sp_pc_abort(unsigned long addr, arm64_notify_die("", regs, &info, esr); } -static struct fault_info debug_fault_info[] = { +int __init early_brk64(unsigned long addr, unsigned int esr, + struct pt_regs *regs); + +/* + * __refdata because early_brk64 is __init, but the reference to it is + * clobbered at arch_initcall time. + * See traps.c and debug-monitors.c:debug_traps_init(). + */ +static struct fault_info __refdata debug_fault_info[] = { { do_bad, SIGTRAP, TRAP_HWBKPT, "hardware breakpoint" }, { do_bad, SIGTRAP, TRAP_HWBKPT, "hardware single-step" }, { do_bad, SIGTRAP, TRAP_HWBKPT, "hardware watchpoint" }, { do_bad, SIGBUS, 0, "unknown 3" }, { do_bad, SIGTRAP, TRAP_BRKPT, "aarch32 BKPT" }, { do_bad, SIGTRAP, 0, "aarch32 vector catch" }, - { do_bad, SIGTRAP, TRAP_BRKPT, "aarch64 BRK" }, + { early_brk64, SIGTRAP, TRAP_BRKPT, "aarch64 BRK" }, { do_bad, SIGBUS, 0, "unknown 7" }, };
Currently, the minimal default BUG() implementation from asm- generic is used for arm64. This patch uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate. This allows bug metadata to be moved to a separate table and reduces the amount of inline code at BUG and WARN sites. This also avoids clobbering any registers before they can be dumped. To mitigate the size of the bug table further, this patch makes use of the existing infrastructure for encoding addresses within the bug table as 32-bit offsets instead of absolute pointers. (Note that this limits the kernel size to 2GB.) Traps are registered at arch_initcall time for aarch64, but BUG has minimal real dependencies and it is desirable to be able to generate bug splats as early as possible. This patch redirects all debug exceptions caused by BRK directly to bug_handler() until the full debug exception support has been initialised. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/Kconfig | 8 ++++++ arch/arm64/include/asm/brk.h | 1 + arch/arm64/include/asm/bug.h | 64 ++++++++++++++++++++++++++++++++++++++++++ arch/arm64/kernel/traps.c | 57 ++++++++++++++++++++++++++++++++++++- arch/arm64/mm/fault.c | 12 ++++++-- 5 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 arch/arm64/include/asm/bug.h