[patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Luis Machado]

Hi,

Currently GDB only supports DABR-based processors (POWER5/5+/6,
PowerPC970 etc). DABR-based processors store the address to be watched
together with 3 other bits, one for the Breakpoint Translation and two
others to control read/write modes. Thus we have a 8-byte alignment
restriction.

Embedded processors, like the PowerPC 440/BookE, use a different
mechanism, the DAC register, which has the sole purpose of storing a
whole 32-bit data address. But GDB's implementation and also the kernel
code didn't think about these when the support was first implemented.

Recently, the kernel has been patched
( http://patchwork.ozlabs.org/linuxppc/patch?id=19790 ) to enable 44X
hardware watchpoint support, and it is a good idea to have the same
support in GDB.

In order to properly identify the 44x/BookE processors, the AT_HWCAP
entry from the AUXV has been used. Also, alignment restrictions were set
to cope with the requirements for these processors. Note also that only
bits for read/write modes are set so that we preserve at least 4-bytes
alignment. Passing the Breakpoint Translation bit would imply in an
8-bytes alignment for a 32-bit processor.

So, in the end, addresses will be aligned to 4-bytes for the 44x/BookE's
and to 8-bytes in the other DABR-based processors.

Tested on both DABR-based and DAC-based processors, no regressions
found. 

Luis

---

2008-08-05  Luis Machado  <luisgpm@br.ibm.com>

	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
	Define PPC_FEATURE_BOOKE.	
	(ppc_linux_get_hwcap): New function.
	(ppc_linux_region_ok_for_hw_watchpoint): Handle PowerPC 440
	4-bytes alignment restrictions.
	(ppc_linux_insert_watchpoint): Handle PowerPC 440-specific
	positioning of the read/write flags.
	(ppc_linux_watchpoint_addr_within_range): Handle PowerPC 440
	4-bytes alignment.

Index: gdb/ppc-linux-nat.c
===================================================================
--- gdb.orig/ppc-linux-nat.c	2008-08-04 06:10:00.000000000 -0700
+++ gdb/ppc-linux-nat.c	2008-08-05 09:36:13.000000000 -0700
@@ -44,6 +44,10 @@
 #include "ppc-tdep.h"
 #include "ppc-linux-tdep.h"
 
+/* Required when using the AUXV.  */
+#include "elf/common.h"
+#include "auxv.h"
+
 /* This sometimes isn't defined.  */
 #ifndef PT_ORIG_R3
 #define PT_ORIG_R3 34
@@ -52,6 +56,10 @@
 #define PT_TRAP 40
 #endif
 
+#ifndef PPC_FEATURE_BOOKE
+#define PPC_FEATURE_BOOKE 0x00008000
+#endif
+
 /* Glibc's headers don't define PTRACE_GETVRREGS so we cannot use a
    configure time check.  Some older glibc's (for instance 2.2.1)
    don't have a specific powerpc version of ptrace.h, and fall back on
@@ -1014,6 +1022,17 @@
   return 1;
 }
 
+/* Fetch the AT_HWCAP entry from the aux vector.  */
+unsigned long ppc_linux_get_hwcap (void)
+{
+  CORE_ADDR field;
+
+  if (target_auxv_search (&current_target, AT_PLATFORM, &field))
+    return (unsigned long) field;
+
+  return 0;
+}
+
 static int
 ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
 {
@@ -1021,8 +1040,13 @@
   if (len <= 0)
     return 0;
 
-  /* addr+len must fall in the 8 byte watchable region.  */
-  if ((addr + len) > (addr & ~7) + 8)
+  /* addr+len must fall in the 8 byte watchable region for DABR-based
+     processors.  DAC-based processors, like the PowerPC 440, will use
+     addresses aligned to 4-bytes due to the way the read/write flags are
+     passed at the moment.  */
+  if (((ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+      && (addr + len) > (addr & ~3) + 4)
+      || (addr + len) > (addr & ~7) + 8)
     return 0;
 
   return 1;
@@ -1038,21 +1062,37 @@
   struct lwp_info *lp;
   ptid_t ptid;
   long dabr_value;
+  long read_mode, write_mode;
 
-  dabr_value = addr & ~7;
+  if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+  {
+  /* PowerPC 440 requires only the read/write flags to be passed
+     to the kernel.  */
+    read_mode  = 1;
+    write_mode = 2;
+  }
+  else
+  {
+  /* PowerPC 970 and other DABR-based processors are required to pass
+     the Breakpoint Translation bit together with the flags.  */
+    read_mode  = 5;
+    write_mode = 6;
+  }
+
+  dabr_value = addr & ~(read_mode | write_mode);
   switch (rw)
     {
     case hw_read:
       /* Set read and translate bits.  */
-      dabr_value |= 5;
+      dabr_value |= read_mode;
       break;
     case hw_write:
       /* Set write and translate bits.  */
-      dabr_value |= 6;
+      dabr_value |= write_mode;
       break;
     case hw_access:
       /* Set read, write and translate bits.  */
-      dabr_value |= 7;
+      dabr_value |= read_mode | write_mode;
       break;
     }
 
@@ -1112,9 +1152,17 @@
 					CORE_ADDR addr,
 					CORE_ADDR start, int length)
 {
-  addr &= ~7;
-  /* Check whether [start, start+length-1] intersects [addr, addr+7]. */
-  return start <= addr + 7 && start + length - 1 >= addr;
+  int mask;
+
+  if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+    mask = 3;
+  else
+    mask = 7;
+
+  addr &= ~mask;
+
+  /* Check whether [start, start+length-1] intersects [addr, addr+mask]. */
+  return start <= addr + mask && start + length - 1 >= addr;
 }
 
 static void

Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Kevin Buettner]

On Tue, 05 Aug 2008 16:56:39 -0300
Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:

> 2008-08-05  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> 	Define PPC_FEATURE_BOOKE.	
> 	(ppc_linux_get_hwcap): New function.
> 	(ppc_linux_region_ok_for_hw_watchpoint): Handle PowerPC 440
> 	4-bytes alignment restrictions.
> 	(ppc_linux_insert_watchpoint): Handle PowerPC 440-specific
> 	positioning of the read/write flags.
> 	(ppc_linux_watchpoint_addr_within_range): Handle PowerPC 440
> 	4-bytes alignment.

Okay.

Kevin

Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Pedro Alves]

Hi Luis,

On Tuesday 05 August 2008 20:56:39, Luis Machado wrote:

> 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".

This requires a Makefile.in update.

( or lobbying for the auto-dependencies patch to go in :-) Hint! Hint! )

-- 
Pedro Alves

Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Daniel Jacobowitz]

On Wed, Aug 06, 2008 at 06:40:04PM +0100, Pedro Alves wrote:
> Hi Luis,
> 
> On Tuesday 05 August 2008 20:56:39, Luis Machado wrote:
> 
> > 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> 
> This requires a Makefile.in update.
> 
> ( or lobbying for the auto-dependencies patch to go in :-) Hint! Hint! )

It's on top of my list.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Luis Machado]

On Wed, 2008-08-06 at 13:43 -0400, Daniel Jacobowitz wrote:
> On Wed, Aug 06, 2008 at 06:40:04PM +0100, Pedro Alves wrote:
> > Hi Luis,
> > 
> > On Tuesday 05 August 2008 20:56:39, Luis Machado wrote:
> > 
> > > 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> > 
> > This requires a Makefile.in update.
> > 
> > ( or lobbying for the auto-dependencies patch to go in :-) Hint! Hint! )
> 
> It's on top of my list.

So i don't need that fixed, right? :-)

Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Daniel Jacobowitz]

On Wed, Aug 06, 2008 at 03:14:38PM -0300, Luis Machado wrote:
> On Wed, 2008-08-06 at 13:43 -0400, Daniel Jacobowitz wrote:
> > On Wed, Aug 06, 2008 at 06:40:04PM +0100, Pedro Alves wrote:
> > > Hi Luis,
> > > 
> > > On Tuesday 05 August 2008 20:56:39, Luis Machado wrote:
> > > 
> > > > 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> > > 
> > > This requires a Makefile.in update.
> > > 
> > > ( or lobbying for the auto-dependencies patch to go in :-) Hint! Hint! )
> > 
> > It's on top of my list.
> 
> So i don't need that fixed, right? :-)

Please fix it if you commit before that part of the makefile goes away
:-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems

[Luis Machado]

On Wed, 2008-08-06 at 15:42 -0400, Daniel Jacobowitz wrote:
> On Wed, Aug 06, 2008 at 03:14:38PM -0300, Luis Machado wrote:
> > On Wed, 2008-08-06 at 13:43 -0400, Daniel Jacobowitz wrote:
> > > On Wed, Aug 06, 2008 at 06:40:04PM +0100, Pedro Alves wrote:
> > > > Hi Luis,
> > > > 
> > > > On Tuesday 05 August 2008 20:56:39, Luis Machado wrote:
> > > > 
> > > > > 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> > > > 
> > > > This requires a Makefile.in update.
> > > > 
> > > > ( or lobbying for the auto-dependencies patch to go in :-) Hint! Hint! )
> > > 
> > > It's on top of my list.
> > 
> > So i don't need that fixed, right? :-)
> 
> Please fix it if you commit before that part of the makefile goes away
> :-)

Thanks, checked in.

Luis

[commit] Fix broken watchpoints (Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems)

[Ulrich Weigand]

Luis Machado wrote:

> 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> 	Define PPC_FEATURE_BOOKE.	
> 	(ppc_linux_get_hwcap): New function.
> 	(ppc_linux_region_ok_for_hw_watchpoint): Handle PowerPC 440
> 	4-bytes alignment restrictions.
> 	(ppc_linux_insert_watchpoint): Handle PowerPC 440-specific
> 	positioning of the read/write flags.
> 	(ppc_linux_watchpoint_addr_within_range): Handle PowerPC 440
> 	4-bytes alignment.

unfortunately this patch completely broke watchpoint support for me on Cell.
The reason is that this routine:

>+/* Fetch the AT_HWCAP entry from the aux vector.  */
>+unsigned long ppc_linux_get_hwcap (void)
>+{
>+  CORE_ADDR field;
>+
>+  if (target_auxv_search (&current_target, AT_PLATFORM, &field))
>+    return (unsigned long) field;

really needs to fetch AT_HWCAP, *not* AT_PLATFORM (which is a string).

Fixed by the patch below.  Tested on powerpc-linux and powerpc64-linux
(on Cell/B.E.), checked into mainline.

Bye,
Ulrich


ChangeLog:

	* ppc-linux-nat.c (ppc_linux_get_hwcap): Really get AT_HWCAP.


diff -urNp gdb-orig/gdb/ppc-linux-nat.c gdb-head/gdb/ppc-linux-nat.c
--- gdb-orig/gdb/ppc-linux-nat.c	2008-08-12 19:13:15.793562000 +0200
+++ gdb-head/gdb/ppc-linux-nat.c	2008-08-12 19:49:17.363981345 +0200
@@ -835,7 +835,7 @@ unsigned long ppc_linux_get_hwcap (void)
 {
   CORE_ADDR field;
 
-  if (target_auxv_search (&current_target, AT_PLATFORM, &field))
+  if (target_auxv_search (&current_target, AT_HWCAP, &field))
     return (unsigned long) field;
 
   return 0;

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [commit] Fix broken watchpoints (Re: [patch-ppc] Hardware watchpoint support for 44x/BookE-based systems)

[Luis Machado]

Stupid typo.

Thanks for fixing this.

Luis
On Tue, 2008-08-12 at 20:23 +0200, Ulrich Weigand wrote:
> Luis Machado wrote:
> 
> > 	* ppc-linux-nat.c: Include "auxv.h" and "elf/common.h".
> > 	Define PPC_FEATURE_BOOKE.	
> > 	(ppc_linux_get_hwcap): New function.
> > 	(ppc_linux_region_ok_for_hw_watchpoint): Handle PowerPC 440
> > 	4-bytes alignment restrictions.
> > 	(ppc_linux_insert_watchpoint): Handle PowerPC 440-specific
> > 	positioning of the read/write flags.
> > 	(ppc_linux_watchpoint_addr_within_range): Handle PowerPC 440
> > 	4-bytes alignment.
> 
> unfortunately this patch completely broke watchpoint support for me on Cell.
> The reason is that this routine:
> 
> >+/* Fetch the AT_HWCAP entry from the aux vector.  */
> >+unsigned long ppc_linux_get_hwcap (void)
> >+{
> >+  CORE_ADDR field;
> >+
> >+  if (target_auxv_search (&current_target, AT_PLATFORM, &field))
> >+    return (unsigned long) field;
> 
> really needs to fetch AT_HWCAP, *not* AT_PLATFORM (which is a string).
> 
> Fixed by the patch below.  Tested on powerpc-linux and powerpc64-linux
> (on Cell/B.E.), checked into mainline.
> 
> Bye,
> Ulrich
> 
> 
> ChangeLog:
> 
> 	* ppc-linux-nat.c (ppc_linux_get_hwcap): Really get AT_HWCAP.
> 
> 
> diff -urNp gdb-orig/gdb/ppc-linux-nat.c gdb-head/gdb/ppc-linux-nat.c
> --- gdb-orig/gdb/ppc-linux-nat.c	2008-08-12 19:13:15.793562000 +0200
> +++ gdb-head/gdb/ppc-linux-nat.c	2008-08-12 19:49:17.363981345 +0200
> @@ -835,7 +835,7 @@ unsigned long ppc_linux_get_hwcap (void)
>  {
>    CORE_ADDR field;
> 
> -  if (target_auxv_search (&current_target, AT_PLATFORM, &field))
> +  if (target_auxv_search (&current_target, AT_HWCAP, &field))
>      return (unsigned long) field;
> 
>    return 0;
>