Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

[David Gibson <dwg@au1.ibm.com>]

On Fri, Sep 16, 2011 at 12:57:10PM +0530, K.Prasad wrote:
> On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
> > On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
> > > On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
> > > > On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
> > > > > On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
> > > > > > PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
> > > > > > PowerPC specific ptrace flags that use the watchpoint register. While they are
> > > > > > targeted primarily towards BookE users, user-space applications such as GDB
> > > > > > have started using them for BookS too.
> > > > > > 
> > > > > > This patch enables the use of generic hardware breakpoint interfaces for these
> > > > > > new flags. The version number of the associated data structures
> > > > > > "ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
> > > > > 
> > > > > So, the structure itself doesn't seem to have been extended.  I don't
> > > > > understand what the semantic difference is - your patch comment needs
> > > > > to explain this clearly.
> > > > >
> > > > 
> > > > We had a request to extend the structure but thought it was dangerous to
> > > > do so. For instance if the user-space used version1 of the structure,
> > > > while kernel did a copy_to_user() pertaining to version2, then we'd run
> > > > into problems. Unfortunately the ptrace flags weren't designed to accept
> > > > a version number as input from the user through the
> > > > PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
> > > 
> > > I still don't follow you.
> > > 
> > 
> > Two things here.
> > 
> > One, the change of semantics warranted an increment of the version
> > number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
> > BookS, while the old version number did not. I've added a small comment
> > in the code to this effect.
> > 
> > Two, regarding changes in the "ppc_hw_breakpoint" and "ppc_debug_info"
> > structures - we would like to add more members to it if we can (GDB has a
> > pending request to add more members to it). However the problem foreseen
> > is that there could be a mismatch between the versions of the structure
> > used by the user vs kernel-space i.e. if a new version of the structure,
> > known to the kernel, had an extra member while the user-space still had
> > the old version, then it becomes dangerous because the __copy_to_user
> > function would overflow the buffer size in user-space.
> > 
> > This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
> > designed to accept a version number (and provide corresponding
> > "struct ppc_debug_info") rather than send a populated "ppc_debug_info"
> > structure along with the version number.
> >
> 
> Based on further discussions with the code-reviewer (David Gibson
> <dwg@au1.ibm.com>), it was decided that incrementing the version number
> for the proposed changes is unnecessary as the patch only introduces new
> features but not a change in semantics.
> 
> Please find a new version of the patch where the version number is
> retained as 1, along with the other planned changes.
> 
> Thanks,
> K.Prasad
> 
>  
>     [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
>     
>     PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
>     PowerPC specific ptrace flags that use the watchpoint register. While they are
>     targeted primarily towards BookE users, user-space applications such as GDB
>     have started using them for BookS too.
>     
>     This patch enables the use of generic hardware breakpoint interfaces for these
>     new flags. The version number of the associated data structures
>     "ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.

Above pargraph needs revision.


>     Apart from the usual benefits of using generic hw-breakpoint interfaces, these
>     changes allow debuggers (such as GDB) to use a common set of ptrace flags for
>     their watchpoint needs and allow more precise breakpoint specification (length
>     of the variable can be specified).
>     
>     [Edjunior: Identified an issue in the patch with the sanity check for version
>     numbers]
>     
>     Tested-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
>     Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> 
> diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt
> index f4a5499..04656ec 100644
> --- a/Documentation/powerpc/ptrace.txt
> +++ b/Documentation/powerpc/ptrace.txt
> @@ -127,6 +127,22 @@ Some examples of using the structure to:
>    p.addr2           = (uint64_t) end_range;
>    p.condition_value = 0;
>  
> +- set a watchpoint in server processors (BookS)
> +
> +  p.version         = 1;
> +  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_RW;
> +  p.addr_mode       = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
> +  or
> +  p.addr_mode       = PPC_BREAKPOINT_MODE_RANGE_EXACT;

MODE_RANGE_EXACT?  Shouldn't that just be MODE_EXACT?

> +
> +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> +  p.addr            = (uint64_t) begin_range;
> +  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
> +   * addr2 - addr <= 8 Bytes.
> +   */
> +  p.addr2           = (uint64_t) end_range;
> +  p.condition_value = 0;
> +
>  3. PTRACE_DELHWDEBUG
>  
>  Takes an integer which identifies an existing breakpoint or watchpoint
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 05b7dd2..2449495 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -1339,6 +1339,12 @@ static int set_dac_range(struct task_struct *child,
>  static long ppc_set_hwdebug(struct task_struct *child,
>  		     struct ppc_hw_breakpoint *bp_info)
>  {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	int ret, len = 0;
> +	struct thread_struct *thread = &(child->thread);
> +	struct perf_event *bp;
> +	struct perf_event_attr attr;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  #ifndef CONFIG_PPC_ADV_DEBUG_REGS
>  	unsigned long dabr;
>  #endif
> @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
>  	 */
>  	if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
>  	    (bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
> -	    bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
>  	    bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
>  		return -EINVAL;
>  
> -	if (child->thread.dabr)
> -		return -ENOSPC;
> -
>  	if ((unsigned long)bp_info->addr >= TASK_SIZE)
>  		return -EIO;
>  
> @@ -1398,15 +1400,83 @@ static long ppc_set_hwdebug(struct task_struct *child,
>  		dabr |= DABR_DATA_READ;
>  	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
>  		dabr |= DABR_DATA_WRITE;
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	if (ptrace_get_breakpoints(child) < 0)
> +		return -ESRCH;
>  
> -	child->thread.dabr = dabr;
> +	bp = thread->ptrace_bps[0];
> +	if (!bp_info->addr) {

I think this is treating a hardware breakpoint at address 0 as if it
didn't exist.  That seems wrong.

> +		if (bp) {
> +			unregister_hw_breakpoint(bp);
> +			thread->ptrace_bps[0] = NULL;
> +		}
> +		ptrace_put_breakpoints(child);
> +		return 0;
> +	}
> +	/*
> +	 * Check if the request is for 'range' breakpoints. We can
> +	 * support it if range < 8 bytes.
> +	 */
> +	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
> +		len = bp_info->addr2 - bp_info->addr;
> +	else if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
> +			ptrace_put_breakpoints(child);
> +			return -EINVAL;
> +		}

Misindent here.  This statement would be clearer if you had {} on all
of the if branches.

> +	if (bp) {
> +		attr = bp->attr;
> +		attr.bp_addr = (unsigned long)bp_info->addr & ~HW_BREAKPOINT_ALIGN;
> +		arch_bp_generic_fields(dabr &
> +					(DABR_DATA_WRITE | DABR_DATA_READ),
> +							&attr.bp_type);
> +		attr.bp_len = len;

If gdb is using the new breakpoint interface, surely it should just
use it, rather than doing this bit frobbing as in the old SET_DABR
call.

> +		ret =  modify_user_hw_breakpoint(bp, &attr);
> +		if (ret) {
> +			ptrace_put_breakpoints(child);
> +			return ret;
> +		}
> +		thread->ptrace_bps[0] = bp;
> +		ptrace_put_breakpoints(child);
> +		thread->dabr = dabr;
> +		return 0;
> +	}
>  
> +	/* Create a new breakpoint request if one doesn't exist already */
> +	hw_breakpoint_init(&attr);
> +	attr.bp_addr = (unsigned long)bp_info->addr & ~HW_BREAKPOINT_ALIGN;
> +	attr.bp_len = len;
> +	arch_bp_generic_fields(dabr & (DABR_DATA_WRITE | DABR_DATA_READ),
> +								&attr.bp_type);
> +
> +	thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr,
> +					       ptrace_triggered, NULL, child);
> +	if (IS_ERR(bp)) {
> +		thread->ptrace_bps[0] = NULL;
> +		ptrace_put_breakpoints(child);
> +		return PTR_ERR(bp);
> +	}
> +
> +	ptrace_put_breakpoints(child);
> +	return 1;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> +
> +	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
> +		return -EINVAL;
> +
> +	if (child->thread.dabr)
> +		return -ENOSPC;
> +
> +	child->thread.dabr = dabr;
>  	return 1;
>  #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
>  }
>  
>  static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
>  {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	struct thread_struct *thread = &(child->thread);
> +	struct perf_event *bp;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>  	int rc;
>  
> @@ -1426,10 +1496,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
>  #else
>  	if (data != 1)
>  		return -EINVAL;
> +
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	if (ptrace_get_breakpoints(child) < 0)
> +		return -ESRCH;
> +
> +	bp = thread->ptrace_bps[0];
> +	if (bp) {
> +		unregister_hw_breakpoint(bp);
> +		thread->ptrace_bps[0] = NULL;
> +	}
> +	ptrace_put_breakpoints(child);
> +	return 0;
> +#else /* CONFIG_HAVE_HW_BREAKPOINT */
>  	if (child->thread.dabr == 0)
>  		return -ENOENT;
>  
>  	child->thread.dabr = 0;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  
>  	return 0;
>  #endif
> @@ -1560,7 +1644,7 @@ long arch_ptrace(struct task_struct *child, long request,
>  		dbginfo.data_bp_alignment = 4;
>  #endif
>  		dbginfo.sizeof_condition = 0;
> -		dbginfo.features = 0;
> +		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
>  #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
>  
>  		if (!access_ok(VERIFY_WRITE, datavp,
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson