Re: [PATCH] Enable hw watchpoint with longer ranges using DAWR on Power

[Luis Machado <lgustavo@codesourcery.com>]

Hi,

The patch looks sane enough. A few nits.

On 07/02/2013 02:39 PM, Edjunior Barbosa Machado wrote:
> Hi,
>
> The new DAWR interface provided by the next generation of Power processors
> (Power ISA Version 2.07) included in the kernel this year allows gdb to use
> hardware watchpoint with longer ranges (up to 512 bytes wide), which can't cross
> a 512 byte boundary [1]. The patch below enables the usage of this new feature,
> currently exported to the userspace as PPC_DEBUG_FEATURE_DATA_BP_DAWR via struct
> ppc_debug_info [2]. Ok?
>
> Thanks and regards,
> --
> Edjunior
>
> [1] http://patchwork.ozlabs.org/patch/215520/
> [2] http://patchwork.ozlabs.org/patch/229888/
>
> gdb/ChangeLog
> 2013-07-02  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
>
> 	* ppc-linux-nat.c (PPC_DEBUG_FEATURE_DATA_BP_DAWR): New define.
> 	(ppc_linux_region_ok_for_hw_watchpoint): Add checking to use the new
> 	DAWR interface for longer ranges hardware watchpoint (up to 512 bytes).
>
> ---
>   gdb/ppc-linux-nat.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index 65d4f4a..dd35624 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -177,7 +177,11 @@ struct ppc_hw_breakpoint
>           (1<<((n)+PPC_BREAKPOINT_CONDITION_BE_SHIFT))
>   #endif /* PPC_PTRACE_GETHWDBGINFO */
>
> -
> +/* New feature defined on Linux kernel v3.9: DAWR interface, that enables
> +   wider watchpoint (up to 512 bytes).  */

"New feature" will get old in the future. I'd just mention the feature 
itself without assigning any notion of how recent/old it is.

> +#ifndef PPC_DEBUG_FEATURE_DATA_BP_DAWR
> +#define PPC_DEBUG_FEATURE_DATA_BP_DAWR	0x10
> +#endif /* PPC_DEBUG_FEATURE_DATA_BP_DAWR */
>

Are older kernels/libraries that don't define these constants going to 
be used with these POWER processors in the future? If not, maybe they 
can be dropped instead of forcing their definitions here.

>   /* Similarly for the general-purpose (gp0 -- gp31)
>      and floating-point registers (fp0 -- fp31).  */
> @@ -1502,6 +1506,7 @@ ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
>        to determine the hardcoded watchable region for watchpoints.  */
>     if (have_ptrace_booke_interface ())
>       {
> +      int region_size;
>         /* Embedded DAC-based processors, like the PowerPC 440 have ranged
>   	 watchpoints and can watch any access within an arbitrary memory
>   	 region. This is useful to watch arrays and structs, for instance.  It
> @@ -1510,11 +1515,17 @@ ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
>   	  && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE
>   	  && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
>   	return 2;
> +      /* Check if the processor provides DAWR interface.  */
> +      if (booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR)
> +	/* DAWR interface allows to watch up to 512 byte wide ranges which
> + 	   can't cross a 512 byte boundary.  */
> +        region_size = 512;
> +      else
> +	region_size = booke_debug_info.data_bp_alignment;
>         /* Server processors provide one hardware watchpoint and addr+len should
>            fall in the watchable region provided by the ptrace interface.  */
> -      if (booke_debug_info.data_bp_alignment
> -	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
> -	      + booke_debug_info.data_bp_alignment))
> +      if (region_size
> +	  && (addr + len > (addr & ~(region_size - 1)) + region_size))
>   	return 0;
>       }
>     /* addr+len must fall in the 8 byte watchable region for DABR-based
>

We should really really get this booke thing sorted out. :-)

Otherwise we will be calling everything "booke" and that may lead to 
confusion in the future.

Thanks for the patch.

Luis