Hi Mahesh,
sorry for the (very) reply to your patches:
> 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.
>
> Change:
> - Use exclude_kernel attribute set to decide whether to install wide
> breakpoint in kernel or not. This flag will be used by ptrace
> breakpoint.
> - Removed 'info->name' field as Name to address resolving is done in
> generic code.
> - Select CONFIG_HAVE_MIXED_BREAKPOINTS_REGS as s390 uses same register
> for instruction as well as data breakpoint.
>
[...]
> --- /dev/null
> +++ b/arch/s390/kernel/hw_breakpoint.c
[...]
> +/*
> + * 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;
> + }
> +
> + if (info->type == S390_BREAKPOINT_WRITE)
> + per_regs[0].em_storage_alteration = 1;
> + if (info->type == S390_BREAKPOINT_EXECUTE)
> + per_regs[0].em_instruction_fetch = 1;
> + per_regs[0].starting_addr = info->address;
> + per_regs[0].ending_addr = 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) {
> + /*
> + * Do not enable kernel wide breakpoint if exclude_kernel is
> + * set to 1.
> + */
> + if (!bp->attr.exclude_kernel) {
Question, since it is not entirely clear from reading the perf/hw_breakpoint
code: what exactly are the semantics of the exclude_kernel flag?
It _seems_ to be very x86 centric in that it assumes a split address space
where part of the address space is user space and part is kernel space. At
least I come to this conclusion when I look at x86 implementation of this
function.
My hope was that it would mean either exclusively add breakpoints to user
space code (or accesses) or kernel space code/accesses. Otherwise this
could cause problems (see below).
> + /* Enable wide breakpoint in the kernel */
> + /* FIXME:
> + * It's not good idea to use existing flag in lowcore
> + * for turning on/off PER tracing in kernel. instead
> + * define a new flag and handle PER tracing checks in
> + * entry*.S
> + */
> +
> +
> + /* set PER bit int psw_kernel_bits to avoid loosing it
> + * accidently if someone modifies PSW bit directly.
> + */
> + psw_kernel_bits |= PSW_MASK_PER;
Since breakpoints (even kernel wide ones) are enabled/disabled per cpu we
probably need per cpu psw_kernel_bits.
> + }
> +
> + /*
> + * If the breakpoint is boound to a task context then enable
> + * it in userspace also.
> + */
> + if (tsk) {
> + struct pt_regs *regs = task_pt_regs(tsk);
> +
> + regs->psw.mask |= PSW_MASK_PER;
> + }
Why this? This is not part of the x86 implementation.
> + }
> + return 0;
> +}
> +
[...]
> +/*
> + * Check for virtual address in kernel space.
> + */
> +int arch_check_bp_in_kernelspace(struct perf_event *bp)
> +{
> + unsigned long hbp_len;
> + unsigned long va;
> +
> + struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +
> + va = info->address;
> + hbp_len = info->len;
> +
> + if (kernel_text_address(va))
> + return kernel_text_address(va + hbp_len - 1);
> +
> + return 0;
> +}
How is this supposed to work on s390? By just passing an address it is
impossible to tell if it is user space or kernel space. s390 has distinct
address spaces for kernel/user space where in each space the accessible
addresses are in the range from 0x-0xffff...... The same is true for
other architectures (e.g. powerpc and sparc, if I remember correctly).
--
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
