From patchwork Tue Apr 21 01:13:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: AKASHI Takahiro X-Patchwork-Id: 6245131 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 20C99BF4A6 for ; Tue, 21 Apr 2015 01:16:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D8B1B20454 for ; Tue, 21 Apr 2015 01:16:23 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B752020435 for ; Tue, 21 Apr 2015 01:16:22 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YkMlT-0000sY-R6; Tue, 21 Apr 2015 01:14:15 +0000 Received: from mail-pd0-f171.google.com ([209.85.192.171]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1YkMlP-0000oC-Pt for linux-arm-kernel@lists.infradead.org; Tue, 21 Apr 2015 01:14:12 +0000 Received: by pdbqd1 with SMTP id qd1so225196058pdb.2 for ; Mon, 20 Apr 2015 18:13:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=tlJOwctRqq0UYl8/4E8d2FbbhCchoz1317/79dTcG3k=; b=eRyL/d4qUTpIdDEW1ZzwQnSJvnV7aGVnRjCQOchBZFGBK45Uz+6U4xnOk3/0rG9PKZ AXhbmtUMCNEg7xYm1pSBOKrqra/ysM6B3V9Zbm4vgr2lZSOvfm2lg51jKcop1ANo1T4X jIugdWG5Os0EoxKe1UOkBZ9rUKQcuAFgCyymp5MZIUXq732EsewiRpzLiUHIdUgYCKiv vRqPD1xTP0ag9QpHG2sOkc4YKIux6CrNs1slnOLBYntY3m0llgrDg4R5qUh7uLSYRQ4C KSQN7F9TuKsxPhACSMBRbl7Di9HweK8mX9VVomvHpdOmq2LqvlCjuO5SSNDcJwSD4/eN NFdw== X-Gm-Message-State: ALoCoQlnERX6UCGUZTsD5FlvHC7lKFybsUhW5AScAGurGt7GHVYkGRILQGnO8qggV4C69LIjG9aw X-Received: by 10.66.124.227 with SMTP id ml3mr32993302pab.28.1429578829435; Mon, 20 Apr 2015 18:13:49 -0700 (PDT) Received: from localhost.localdomain (61-205-3-184m5.grp1.mineo.jp. [61.205.3.184]) by mx.google.com with ESMTPSA id pv2sm199181pbb.12.2015.04.20.18.13.44 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 20 Apr 2015 18:13:47 -0700 (PDT) From: AKASHI Takahiro To: jason.wessel@windriver.com, catalin.marinas@arm.com, will.deacon@arm.com Subject: [RESEND PATCH] arm64: kgdb: fix single stepping Date: Tue, 21 Apr 2015 10:13:13 +0900 Message-Id: <1429578793-3971-1-git-send-email-takahiro.akashi@linaro.org> X-Mailer: git-send-email 1.9.1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150420_181411_906828_DB511D4C X-CRM114-Status: GOOD ( 22.85 ) X-Spam-Score: -1.0 (-) Cc: yong.zhao@amd.com, linaro-kernel@lists.linaro.org, kgdb-bugreport@lists.sourceforge.net, Vijaya.Kumar@caviumnetworks.com, linux-kernel@vger.kernel.org, AKASHI Takahiro , broonie@kernel.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Jason, Could you please review my patch below? See also arm64 maintainer's comment: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/313712.html Thanks, -Takahiro AKASHI I tried to verify kgdb in vanilla kernel on fast model, but it seems that the single stepping with kgdb doesn't work correctly since its first appearance at v3.15. On v3.15, 'stepi' command after breaking the kernel at some breakpoint steps forward to the next instruction, but the succeeding 'stepi' never goes beyond that. On v3.16, 'stepi' moves forward and stops at the next instruction just after enable_dbg in el1_dbg, and never goes beyond that. This variance of behavior seems to come in with the following patch in v3.16: commit 2a2830703a23 ("arm64: debug: avoid accessing mdscr_el1 on fault paths where possible") This patch (1) moves kgdb_disable_single_step() from 'c' command handling to single step handler. This makes sure that single stepping gets effective at every 's' command. Please note that, under the current implementation, single step bit in spsr, which is cleared by the first single stepping, will not be set again for the consecutive 's' commands because single step bit in mdscr is still kept on (that is, kernel_active_single_step() in kgdb_arch_handle_exception() is true). (2) re-implements kgdb_roundup_cpus() because the current implementation enabled interrupts naively. See below. (3) removes 'enable_dbg' in el1_dbg. Single step bit in mdscr is turned on in do_handle_exception()-> kgdb_handle_expection() before returning to debugged context, and if debug exception is enabled in el1_dbg, we will see unexpected single- stepping in el1_dbg. Since v3.18, the following patch does the same: commit 1059c6bf8534 ("arm64: debug: don't re-enable debug exceptions on return from el1_dbg) (4) masks interrupts while single-stepping one instruction. If an interrupt is caught during processing a single-stepping, debug exception is unintentionally enabled by el1_irq's 'enable_dbg' before returning to debugged context. Thus, like in (2), we will see unexpected single-stepping in el1_irq. Basically (1) and (2) are for v3.15, (3) and (4) for v3.1[67]. * issue fixed by (2): Without (2), we would see another problem if a breakpoint is set at interrupt-sensible places, like gic_handle_irq(): KGDB: re-enter error: breakpoint removed ffffffc000081258 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 650 at kernel/debug/debug_core.c:435 kgdb_handle_exception+0x1dc/0x1f4() Modules linked in: CPU: 0 PID: 650 Comm: sh Not tainted 3.17.0-rc2+ #177 Call trace: [] dump_backtrace+0x0/0x130 [] show_stack+0x10/0x1c [] dump_stack+0x74/0xb8 [] warn_slowpath_common+0x8c/0xb4 [] warn_slowpath_null+0x14/0x20 [] kgdb_handle_exception+0x1d8/0x1f4 [] kgdb_brk_fn+0x18/0x28 [] brk_handler+0x9c/0xe8 [] do_debug_exception+0x3c/0xac Exception stack(0xffffffc07e027650 to 0xffffffc07e027770) ... [] el1_dbg+0x14/0x68 [] kgdb_cpu_enter+0x464/0x5c0 [] kgdb_handle_exception+0x190/0x1f4 [] kgdb_brk_fn+0x18/0x28 [] brk_handler+0x9c/0xe8 [] do_debug_exception+0x3c/0xac Exception stack(0xffffffc07e027ac0 to 0xffffffc07e027be0) ... [] el1_dbg+0x14/0x68 [] __handle_sysrq+0x11c/0x190 [] write_sysrq_trigger+0x4c/0x60 [] proc_reg_write+0x54/0x84 [] vfs_write+0x98/0x1c8 [] SyS_write+0x40/0xa0 Once some interrupt occurs, a breakpoint at gic_handle_irq() triggers kgdb. Kgdb then calls kgdb_roundup_cpus() to sync with other cpus. Current kgdb_roundup_cpus() unmasks interrupts temporarily to use smp_call_function(). This eventually allows another interrupt to occur and likely results in hitting a breakpoint at gic_handle_irq() again since debug exception is always enabled in el1_irq. We can avoid this issue by specifying "nokgdbroundup" in kernel parameter, but this will also leave other cpus be in unknown state in terms of kgdb, and may result in interfering with kgdb activity. Signed-off-by: AKASHI Takahiro Tested-by: Giuseppe Cavallaro --- arch/arm64/kernel/kgdb.c | 60 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index a0d10c5..81b5910 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -19,9 +19,13 @@ * along with this program. If not, see . */ +#include #include +#include #include #include +#include +#include #include struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { @@ -95,6 +99,9 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = { { "fpcr", 4, -1 }, }; +static DEFINE_PER_CPU(unsigned int, kgdb_pstate); +static DEFINE_PER_CPU(struct irq_work, kgdb_irq_work); + char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs) { if (regno >= DBG_MAX_REG_NUM || regno < 0) @@ -176,18 +183,14 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, * over and over again. */ kgdb_arch_update_addr(linux_regs, remcom_in_buffer); - atomic_set(&kgdb_cpu_doing_single_step, -1); - kgdb_single_step = 0; - - /* - * Received continue command, disable single step - */ - if (kernel_active_single_step()) - kernel_disable_single_step(); err = 0; break; case 's': + /* mask interrupts while single stepping */ + __this_cpu_write(kgdb_pstate, linux_regs->pstate); + linux_regs->pstate |= PSR_I_BIT; + /* * Update step address value with address passed * with step packet. @@ -198,8 +201,6 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, */ kgdb_arch_update_addr(linux_regs, remcom_in_buffer); atomic_set(&kgdb_cpu_doing_single_step, raw_smp_processor_id()); - kgdb_single_step = 1; - /* * Enable single step handling */ @@ -229,6 +230,18 @@ static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr) static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr) { + unsigned int pstate; + + kernel_disable_single_step(); + atomic_set(&kgdb_cpu_doing_single_step, -1); + + /* restore interrupt mask status */ + pstate = __this_cpu_read(kgdb_pstate); + if (pstate & PSR_I_BIT) + regs->pstate |= PSR_I_BIT; + else + regs->pstate &= ~PSR_I_BIT; + kgdb_handle_exception(1, SIGTRAP, 0, regs); return 0; } @@ -249,16 +262,27 @@ static struct step_hook kgdb_step_hook = { .fn = kgdb_step_brk_fn }; -static void kgdb_call_nmi_hook(void *ignored) +static void kgdb_roundup_hook(struct irq_work *work) { kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs()); } void kgdb_roundup_cpus(unsigned long flags) { - local_irq_enable(); - smp_call_function(kgdb_call_nmi_hook, NULL, 0); - local_irq_disable(); + int cpu; + struct cpumask mask; + struct irq_work *work; + + mask = *cpu_online_mask; + cpumask_clear_cpu(smp_processor_id(), &mask); + cpu = cpumask_first(&mask); + if (cpu >= nr_cpu_ids) + return; + + for_each_cpu(cpu, &mask) { + work = per_cpu_ptr(&kgdb_irq_work, cpu); + irq_work_queue_on(work, cpu); + } } static int __kgdb_notify(struct die_args *args, unsigned long cmd) @@ -299,6 +323,8 @@ static struct notifier_block kgdb_notifier = { int kgdb_arch_init(void) { int ret = register_die_notifier(&kgdb_notifier); + int cpu; + struct irq_work *work; if (ret != 0) return ret; @@ -306,6 +332,12 @@ int kgdb_arch_init(void) register_break_hook(&kgdb_brkpt_hook); register_break_hook(&kgdb_compiled_brkpt_hook); register_step_hook(&kgdb_step_hook); + + for_each_possible_cpu(cpu) { + work = per_cpu_ptr(&kgdb_irq_work, cpu); + init_irq_work(work, kgdb_roundup_hook); + } + return 0; }