Re: [RFC] 4xx hardware watchpoint support

[Josh Boyer <jwboyer@linux.vnet.ibm.com>]

On Fri, 20 Jun 2008 17:14:53 -0300
Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:
> 
> Follows a re-worked patch that unifies the handling of hardware
> watchpoint structures for DABR-based and DAC-based processors.
> 
> The flow is exactly the same for DABR-based processors.
> 
> As for the DAC-based code, i've eliminated the "dac" field from the
> thread structure, since now we use the "dabr" field to represent DAC1 of
> the DAC-based processors. As a consequence, i removed all the
> occurrences of "dac" throughout the code, using "dabr" in those cases.
> 
> The function "set_dabr" sets the correct register (DABR OR DAC) for each
> specific processor now, instead of only setting the value for DABR-based
> processors.
> 
> "do_dac" was merged with "do_dabr" as they were mostly equivalent pieces
> of code. I've moved "do_dabr" to "arch/powerpc/kernel/process.c" so it
> is visible for DAC-based code as well.
> 
> Tested on a Taishan 440GX with success.
> 
> Is it OK to leave it as "dabr" for both DABR and DAC? What do you think
> of the patch overall?

Aside from the comment I mention below (which really does need fixing),
the overall look of the patch seems good.

> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/process.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c	2008-06-20 02:51:16.000000000 -0700
> @@ -241,6 +241,35 @@
>  }
>  #endif /* CONFIG_SMP */
> 
> +void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code)
> +{
> +	siginfo_t info;
> +
> +#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) == NOTIFY_STOP)
> +		return;
> +
> +	if (debugger_dabr_match(regs))
> +		return;
> +#else
> +        /* Clear the DAC and struct entries.  One shot trigger.  */
> +        mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> +                                                        | DBCR0_IDM));

This doesn't look right for how it's coded.  This would be the
CONFIG_4xx || CONFIG_BOOKE case, but CONFIG_4xx includes PowerPC 405.
That has a different bit layout among the DBCR registers.  Namely, on
405 you would be clearing the TDE and IAC1 events because the DAC
events are in DBCR1, not DBCR0.

<snip>

> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/signal.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c	2008-06-20 02:51:16.000000000 -0700
> @@ -144,8 +144,12 @@
>  	 * user space. The DABR will have been cleared if it
>  	 * triggered inside the kernel.
>  	 */
> -	if (current->thread.dabr)
> +	if (current->thread.dabr) {
>  		set_dabr(current->thread.dabr);
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> +#endif

This has the same problem I mention above, as the 44x bit layout is
stored in dbcr0 throughout the code.

> +	}
> 
>  	if (is32) {
>          	if (ka.sa.sa_flags & SA_SIGINFO)
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/traps.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c	2008-06-20 02:54:37.000000000 -0700
> @@ -1045,6 +1045,21 @@
>  				return;
>  		}
>  		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
> +	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
> +		regs->msr &= ~MSR_DE;
> +	        printk("\nWatchpoint Triggered\n");
> +		if (user_mode(regs)) {
> +			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
> +								DBCR0_IDM);
> +		} else {
> +			/* Disable DAC interupts.  */
> +			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
> +							DBSR_DAC1W | DBCR0_IDM));
> +			/* Clear the DAC event.  */
> +			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));

And again here.

josh