> On Mon, Dec 14, 2009 at 07:27:41PM +0530, Mahesh Salgaonkar wrote:
> Introduce s390 specific implementation for the perf-events based hardware
> breakpoint interfaces defined in kernel/hw_breakpoint.c. Enable the
> HAVE_HW_BREAKPOINT flag and the Makefile.
>
> Signed-off-by: Mahesh Salgaonkar <mahesh@xxxxxxxxxxxxxxxxxx>
> ---
> arch/s389/Kconfig | 1
> arch/s390/include/asm/Kbuild | 1
> arch/s390/kernel/Makefile | 2
> arch/s390/kernel/hw_breakpoint.c | 340 ++++++++++++++++++++++++++++++++
> kernel/hw_breakpoint.c | 13 +
> samples/hw_breakpoint/data_breakpoint.c | 4
> 6 files changed, 360 insertions(+), 1 deletion(-)
>
> Index: linux-2.6-tip/arch/s390/kernel/hw_breakpoint.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6-tip/arch/s390/kernel/hw_breakpoint.c
> @@ -0,0 +1,340 @@
> +#include <asm/bitsperlong.h>
> +#include <linux/hw_breakpoint.h>
> +#include <linux/irqflags.h>
> +#include <linux/notifier.h>
> +#include <linux/kallsyms.h>
> +#include <linux/kprobes.h>
> +#include <linux/percpu.h>
> +#include <linux/kdebug.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/sections.h>
> +#include <asm/uaccess.h>
> +
> +/* Per cpu PER control register values */
> +DEFINE_PER_CPU(per_cr_bits, cpu_per_regs[1]);
> +
> +/*
> + * Stores the breakpoints currently in use on each breakpoint address
> + * register for each cpus
> + */
> +static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM]);
> +
> +/*
> + * Install a perf counter breakpoint.
> + *
> + * S390 provides only one hardware breakpoint register. Use it for this
> + * breakpoint if free.
> + *
> + * Atomic: we hold the counter->ctx->lock and we only handle variables,
> + * lowcore area and control registers local to this cpu.
> + */
> +int arch_install_hw_breakpoint(struct perf_event *bp)
> +{
> + struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> + struct perf_event **slot;
> + struct task_struct *tsk = bp->ctx->task;
> + per_cr_bits per_regs[1];
> +
> + memset(per_regs, 0, sizeof(per_cr_bits));
> +
> + slot = &__get_cpu_var(bp_per_reg[0]);
> + if (!*slot) {
> + *slot = bp;
> + } else {
> + WARN_ONCE(1 , "Can't find any breakpoint slot");
> + return -EBUSY;
> + }
> +
> + per_regs[0].em_storage_alteration =
> + (info->type == S390_BREAKPOINT_WRITE) ? 1 : 0;
> + per_regs[0].em_instruction_fetch =
> + (info->type == S390_BREAKPOINT_EXECUTE) ? 1 : 0;
> + per_regs[0].starting_addr = (unsigned long) info->address;
> + per_regs[0].ending_addr =
> + (unsigned long) info->address + info->len - 1;
> +
> + /* Load the control register 9, 10 and 11 with per info */
> + __ctl_load(per_regs, 9, 11);
> + __get_cpu_var(cpu_per_regs[0]) = per_regs[0];
> +
> + if (((per_cr_words *)per_regs)->cr[0] & PER_EM_MASK) {
> + if (!tsk) {
> + /* wide breakpoint in the kernel */
> + /* Disable lowcore protection */
> + __ctl_clear_bit(0, 28);
> +
> + S390_lowcore.svc_new_psw.mask |= PSW_MASK_PER;
> +
> + /* Enable lowcore protection */
> + __ctl_set_bit(0, 28);
> + } else {
> + /* user-space hardware breakpoint */
> + struct pt_regs *regs = task_pt_regs(tsk);
> +
> + regs->psw.mask |= PSW_MASK_PER;
> + }
> + }
> + return 0;
> +}
Ok, so this would allow us to have exactly one PER set active on a cpu. We
should allow two distinct sets: one for userspace and one for kernel space.
Otherwise this would be a userspace visible change.
Besides that it isn't a good idea to enable the PSW_MASK_PER bit in the
lowcore:
- we would see events only if kernel space is entered via a system call.
What about interrupts and exceptions?
- on cpu hotplug (online) the current lowcore contents are copied to the
to be onlined cpu. If the PER bit happens to be switched on the new
cpu will have it set as well, even if PER should be disabled.
- when setting the bit in the lowcore it might get lost later on since
some places in our arch code modify the PSW directly (grab for
__load_psw_mask).
To me it looks like that we want to have two PER register sets for each
cpu: one for user space and one for kernel space.
Switching should happen on kernel space entry/exit. In order to avoid
losing the PER bit out of the PSW by accident it should be set in the
psw_kernel_bit variable which always should be used if somebody wants
to modify the PSW bits.
I assume that kernel space PER tracing happens on all cpus. Otherwise
we would need per a cpu psw_kernel_bits variable.
> Index: linux-2.6-tip/arch/s390/Kconfig
> ===================================================================
> --- linux-2.6-tip.orig/arch/s390/Kconfig
> +++ linux-2.6-tip/arch/s390/Kconfig
> @@ -123,6 +123,7 @@ config S390
> select ARCH_INLINE_WRITE_UNLOCK_BH
> select ARCH_INLINE_WRITE_UNLOCK_IRQ
> select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
> + select HAVE_HW_BREAKPOINT
This selects PERF_EVENTS unconditionally. Just be saying "we support
HW_BREAKPOINT" we will get additional cpu overhead by perf events?
Not good.
The big question is: what will this feature buy us? I currently can't
see a strong reason why we want to have HW breakpoint support on s390.
> Index: linux-2.6-tip/arch/s390/include/asm/Kbuild
> ===================================================================
> --- linux-2.6-tip.orig/arch/s390/include/asm/Kbuild
> +++ linux-2.6-tip/arch/s390/include/asm/Kbuild
> @@ -8,6 +8,7 @@ header-y += ucontext.h
> header-y += vtoc.h
> header-y += zcrypt.h
> header-y += chsc.h
> +header-y += hw_breakpoint.h
Oh, so it does get exported and the ifdef __KERNEL__ I objected to
is indeed needed. Really the two patches should be merged.
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
