On Fri, May 15, 2009 at 12:50:11AM +1000, Michael Ellerman wrote:
On Thu, 2009-05-14 at 19:14 +0530, K.Prasad wrote:
plain text document attachment (ppc64_arch_hwbkpt_implementation_02)
Introduce PPC64 implementation for the generic hardware breakpoint
interfaces
defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and
the
Makefile.
Hi, some comments inline ...
Thanks for reviewing the code.
Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/hw_breakpoint.c
===
--- /dev/null
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/hw_breakpoint.c
@@ -0,0 +1,281 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
USA.
+ *
+ * Copyright (C) 2009 IBM Corporation
+ */
Don't use (C), either use a proper ©, or just skip it. I don't know
why :)
Ok.
+/*
+ * HW_breakpoint: a unified kernel/user-space hardware breakpoint facility,
+ * using the CPU's debug registers.
+ */
This comment would normally go at the top of the file.
Ok.
+
+#include linux/notifier.h
+#include linux/kallsyms.h
+#include linux/kprobes.h
+#include linux/percpu.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/sched.h
+#include linux/init.h
+#include linux/smp.h
+
+#include asm/hw_breakpoint.h
+#include asm/processor.h
+#include asm/sstep.h
+
+/* Store the kernel-space breakpoint address value */
+static unsigned long kdabr;
+
+/*
+ * Temporarily stores address for DABR before it is written by the
+ * single-step handler routine
+ */
+static DEFINE_PER_CPU(unsigned long, dabr_data);
How does this relate to the existing current_dabr per-cpu variable?
The present infrastructure assumes that kernel-space (through xmon and
KGDB) and user-space requests happen at mutually exclusive times.
current_dabr in arch/powerpc/kernel/process.c stores the value of DABR
as requested by the current process and helps initiate a set_dabr() call
if the new incoming process's DABR value is different (when neither of
them use DABR, the values are 0, hence no set_dabr() call).
The per-cpu 'dabr_data' seen above is used to store the value of DABR
temporarily between a HW Breakpoint exception handler
hw_breakpoint_handler() (where the breakpoint exception might be
temporarily disabled if emulate_step() failed) and a single-step
exception handler single_step_dabr_instruction() in which the DABR value
is restored from per-cpu 'dabr_data'.
This is required because PowerPC triggers DABR match exception before
execution of the causative instruction (such as load/store) and if the
DABR value remains enabled, the system will enter into an infinite loop.
+void arch_update_kernel_hw_breakpoint(void *unused)
+{
+ struct hw_breakpoint *bp;
+
+ /* Check if there is nothing to update */
+ if (hbp_kernel_pos == HBP_NUM)
+ return;
Should that be hbp_kernel_pos = HBP_NUM, you're checking array bounds
right?
In short, no. If hbp_kernel_pos HBP_NUM it is only indicative of erroneous
code and not a condition that the system could enter.
To explain 'hbp_kernel_pos' further, it is that variable which points to
the last numbered debug register occupied by the kernel (which is more
meaningful on a processor having more than one debug register).
In the above code, in arch_update_kernel_hw_breakpoint()
hbp_kernel_pos == HBP_NUM condition would be satisfied when
invoked from load_debug_registers() at initialisation time (kernel
requests are serviced starting from highest numbered register).
Having said that, I find that I've missed invoking
load_debug_registers() [a part of kernel/hw_breakpoint.c] in
start_secondary(). Thanks for raising this question which helped me
identify the missing invocation, I will add the same.
+ bp = hbp_kernel[hbp_kernel_pos];
+ if (bp == NULL)
+ kdabr = 0;
+ else
+ kdabr = bp-info.address | bp-info.type | DABR_TRANSLATION;
+ set_dabr(kdabr);
+}
+
+/*
+ * Install the thread breakpoints in their debug registers.
+ */
+void arch_install_thread_hw_breakpoint(struct task_struct *tsk)
+{
+ set_dabr(tsk-thread.dabr);
Can we avoid setting this value if it's not necessary? It might
