On Wed, Jul 28, 2010 at 10:56:05AM +0200, Heiko Carstens wrote:
> 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.
Exactly.
> 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).
(I will answer below)
> > + }
> > +
> > + /*
> > + * 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.
Ah right I missed that. That doesn't look right.
Beeing bound to a task doesn't mean beeing bound in userspace.
We may want to trigger in a kernel variable access only in a given
task context for example.
> > + }
> > + 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).
So it's something I was really not aware of. It basically means one
address can have two different meanings, one in userspace and one
in kernelspace?
Ok, I'm trying to understand how your copy_from_user() works, but
I gave up on the asm in copy_from_user_std() :)
Then yeah, s390 breakpoints must always be either exclude_kernel or
exclude_user for the necessary disambiguation.
Having 0 on both must end in -EINVAL.
--
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
