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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
