Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7522 bytes --]

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 ...


> 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 :)

> +/*
> + * 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.

> +
> +#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?

> +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?

> +	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 require
an hcall. See for example what we do in __switch_to().

> +/*
> + * Check for virtual address in user space.
> + */
> +static int arch_check_va_in_userspace(unsigned long va)
> +{
> +	return (!(is_kernel_addr(va)));
> +}
> +
> +/*
> + * Check for virtual address in kernel space.
> + */
> +static int arch_check_va_in_kernelspace(unsigned long va)
> +{
> +	return is_kernel_addr(va);
> +}

You don't need these two routines. Just use is_kernel_addr() directly,
that way people will know what the code is doing without having to
lookup these new functions.

> +/*
> + * Store a breakpoint's encoded address, length, and type.
> + */

This doesn't "store" in the sense I was thinking, it actually does a
lookup and returns info in the arg.

> +int arch_store_info(struct hw_breakpoint *bp)
> +{
> +	/*
> +	 * User-space requests will always have the address field populated
> +	 * For kernel-addresses, either the address or symbol name can be
> +	 * specified.
> +	 */

Do user-space requests never have the name populated? Otherwise aren't
you overwriting the supplied address with the one from kallsyms?

> +	if (bp->info.name)
> +		bp->info.address = (unsigned long)
> +					kallsyms_lookup_name(bp->info.name);
> +	if (bp->info.address)
> +		return 0;
> +	return -EINVAL;
> +}
> +
> +/*
> + * Validate the arch-specific HW Breakpoint register settings
> + */
> +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> +						struct task_struct *tsk)
> +{
> +	int ret = -EINVAL;
> +
> +	if (!bp)
> +		return ret;
> +
> +	switch (bp->info.type) {
> +	case DABR_DATA_READ:
> +		break;
> +	case DABR_DATA_WRITE:
> +		break;
> +	case DABR_DATA_RW:
> +		break;

You only need the final break here.

> +	default:
> +		return ret;
> +	}
> +
> +	if (bp->triggered)
> +		ret = arch_store_info(bp);

Shouldn't you check ret here, bp->info.address might be bogus.
> +
> +	/* Check for double word alignment - 8 bytes */
> +	if (bp->info.address & HW_BREAKPOINT_ALIGN)
> +		return -EINVAL;
> +
> +	/* Check that the virtual address is in the proper range */
> +	if (tsk) {
> +		if (!arch_check_va_in_userspace(bp->info.address))
> +			return -EFAULT;
> +	} else {
> +		if (!arch_check_va_in_kernelspace(bp->info.address))
> +			return -EFAULT;
> +	}

Which becomes:

is_kernel = is_kernel_addr(bp->info.address);
if (tsk && is_kernel || !tsk && !is_kernel)
	return -EFAULT;

> +void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk)
> +{
> +	struct thread_struct *thread = &(tsk->thread);
> +	struct hw_breakpoint *bp = thread->hbp[0];
> +
> +	if (bp)
> +		thread->dabr = bp->info.address	| bp->info.type |
> +				DABR_TRANSLATION;

2nd place I've seen that pattern.

> +	else
> +		thread->dabr = 0;
> +}
> +
> +void arch_flush_thread_hw_breakpoint(struct task_struct *tsk)
> +{
> +	struct thread_struct *thread = &(tsk->thread);
> +
> +	thread->dabr = 0;
> +}
> +
> +/*
> + * Handle debug exception notifications.
> + */
> +int __kprobes hw_breakpoint_handler(struct die_args *args)
> +{
> +	int rc = NOTIFY_STOP;
> +	struct hw_breakpoint *bp;
> +	struct pt_regs *regs = args->regs;
> +	unsigned long dar;
> +	int cpu, stepped, is_kernel;
> +
> +	/* Disable breakpoints during exception handling */
> +	set_dabr(0);
> +
> +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;

is_kernel_addr() ?

> +	if (is_kernel)
> +		bp = hbp_kernel[0];
> +	else {
> +		bp = current->thread.hbp[0];
> +		/* Lazy debug register switching */
> +		if (!bp)
> +			return rc;

What if we keep hitting this case?

> +		rc = NOTIFY_DONE;
> +	}
> +
> +	(bp->triggered)(bp, regs);
> +
> +	cpu = get_cpu();
> +	if (is_kernel)
> +		per_cpu(dabr_data, cpu) = kdabr;
> +	else
> +		per_cpu(dabr_data, cpu) = current->thread.dabr;
> +
> +	stepped = emulate_step(regs, regs->nip);
> +	/*
> +	 * Single-step the causative instruction manually if
> +	 * emulate_step() could not execute it
> +	 */
> +	if (stepped == 0) {
> +		regs->msr |= MSR_SE;
> +		goto out;
> +	}
> +
> +	set_dabr(per_cpu(dabr_data, cpu));
> +out:
> +	/* Enable pre-emption only if single-stepping is finished */
> +	if (stepped)
> +		put_cpu_no_resched();
> +	return rc;
> +}

Gotta run, laptop battery running out! :)

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]