[RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

These patches implement an extention to the ptrace interface proposed by
Thiago Bauermann and the the PowerPC gdb team.

GDB intends to support the following hardware debug features of BookE
processors:

4 hardware breakpoints (IAC)
2 hardware watchpoints (read, write and read-write) (DAC)
2 value conditions for the hardware watchpoints (DVC)

For that, we need to extend ptrace so that GDB can query and set these
resources. Since we're extending, we're trying to create an interface
that's extendable and that covers both BookE and server processors, so
that GDB doesn't need to special-case each of them. We propose the
following 3 new ptrace requests described below.

There have been discussions of a generic hardware debug interface for the
kernel which would hopefully contemplate all the functionality below and
supersede it.  But we need something that works now, and which enables GDB
to be simpler and work with both Server and Embedded processors without
special cases.

1. PTRACE_PPC_GETHWDEBUGINFO

Query for GDB to discover the hardware debug features. The main info to
be returned here is the minimum alignment for the hardware watchpoints.
BookE processors don't have restrictions here, but server processors have
an 8-byte alignment restriction for hardware watchpoints. We'd like to avoid
adding special cases to GDB based on what it sees in AUXV.

Since we're at it, we added other useful info that the kernel can return to
GDB: this query will return the number of hardware breakpoints, hardware
watchpoints and whether it supports a range of addresses and a condition.
The query will fill the following structure provided by the requesting process:

struct ppc_debug_info {
       unit32_t version;
       unit32_t num_instruction_bps;
       unit32_t num_data_bps;
       unit32_t num_condition_regs;
       unit32_t data_bp_alignment;
       unit32_t sizeof_condition; /* size of the DVC register */
       uint64_t features; /* bitmask of the individual flags */
};

features will have bits indicating whether there is support for:

#define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
#define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
#define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
#define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8

2. PTRACE_SETHWDEBUG

Sets a hardware breakpoint or watchpoint, according to the provided structure:

struct ppc_hw_breakpoint {
        uint32_t version;
#define PPC_BREAKPOINT_TRIGGER_EXECUTE  0x1
#define PPC_BREAKPOINT_TRIGGER_READ     0x2
#define PPC_BREAKPOINT_TRIGGER_WRITE    0x4
        uint32_t trigger_type;       /* only some combinations allowed */
#define PPC_BREAKPOINT_MODE_EXACT               0x0
#define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE     0x1
#define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE     0x2
#define PPC_BREAKPOINT_MODE_MASK                0x3
        uint32_t addr_mode;          /* address match mode */

#define PPC_BREAKPOINT_CONDITION_NONE   0x0
#define PPC_BREAKPOINT_CONDITION_AND    0x1
#define PPC_BREAKPOINT_CONDITION_EXACT  0x1	/* different name for the same thing as above */
#define PPC_BREAKPOINT_CONDITION_OR     0x2
#define PPC_BREAKPOINT_CONDITION_AND_OR 0x3
#define PPC_BREAKPOINT_CONDITION_BE_ALL 0x00ff0000	/* byte enable bits */
#define PPC_BREAKPOINT_CONDITION_BE(n)  (1<<((n)+16))
        uint32_t condition_mode;     /* break/watchpoint condition flags */

        uint64_t addr;
        uint64_t addr2;
        uint64_t condition_value;
};

A request specifies one event, not necessarily just one register to be set.
For instance, if the request is for a watchpoint with a condition, both the
DAC and DVC registers will be set in the same request.

With this GDB can ask for all kinds of hardware breakpoints and watchpoints
that the BookE supports. COMEFROM breakpoints available in server processors
are not contemplated, but that is out of the scope of this work.

ptrace will return an integer (handle) uniquely identifying the breakpoint or
watchpoint just created. This integer will be used in the PTRACE_DELHWDEBUG
request to ask for its removal. Return -ENOSPC if the requested breakpoint
can't be allocated on the registers.

Some examples of using the structure to:

- set a breakpoint in the first breakpoint register

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  p.addr            = (uint64_t) address;
  p.addr2           = 0;
  p.condition_value = 0;

- set a watchpoint which triggers on reads in the second watchpoint register

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_READ;
  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  p.addr            = (uint64_t) address;
  p.addr2           = 0;
  p.condition_value = 0;

- set a watchpoint which triggers only with a specific value

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_READ;
  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_AND | PPC_BREAKPOINT_CONDITION_BE_ALL;
  p.addr            = (uint64_t) address;
  p.addr2           = 0;
  p.condition_value = (uint64_t) condition;

- set a ranged hardware breakpoint

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
  p.addr_mode       = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  p.addr            = (uint64_t) begin_range;
  p.addr2           = (uint64_t) end_range;
  p.condition_value = 0;

3. PTRACE_DELHWDEBUG

Takes an integer which identifies an existing breakpoint or watchpoint
(i.e., the value returned from PTRACE_SETHWDEBUG), and deletes the
corresponding breakpoint or watchpoint..

-- 
Dave Kleikamp
IBM Linux Technology Center

[RFC:PATCH 01/03] powerpc: Extended ptrace interface

[Dave Kleikamp]

powerpc: Extended ptrace interface

From: Torez Smith <lnxtorez@linux.vnet.ibm.com>

Add a new extended ptrace interface so that user-space has a single
interface for powerpc, without having to know the specific layout
of the debug registers.

Implement:
PPC_PTRACE_GETHWDEBUGINFO
PPC_PTRACE_SETHWDEBUG
PPC_PTRACE_DELHWDEBUG

Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
Cc: David Gibson <dwg@au1.ibm.com>
Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
---

 arch/powerpc/include/asm/ptrace.h |   75 ++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/ptrace.c      |   88 +++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+), 0 deletions(-)


diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 8c34149..7ae887b 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -24,6 +24,12 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#ifdef __KERNEL__
+#include <linux/types.h>
+#else
+#include <stdint.h>
+#endif
+
 #ifndef __ASSEMBLY__
 
 struct pt_regs {
@@ -292,4 +298,73 @@ extern void user_disable_single_step(struct task_struct *);
 
 #define PTRACE_SINGLEBLOCK	0x100	/* resume execution until next branch */
 
+#define PPC_PTRACE_GETHWDBGINFO	0x89
+#define PPC_PTRACE_SETHWDEBUG	0x88
+#define PPC_PTRACE_DELHWDEBUG	0x87
+
+#ifndef __ASSEMBLY__
+
+struct ppc_debug_info {
+	uint32_t version;		/* Only version 1 exists to date */
+	uint32_t num_instruction_bps;
+	uint32_t num_data_bps;
+	uint32_t num_condition_regs;
+	uint32_t data_bp_alignment;
+	uint32_t sizeof_condition;	/* size of the DVC register */
+	uint64_t features;
+};
+
+#endif /* __ASSEMBLY__ */
+
+/*
+ * features will have bits indication whether there is support for:
+ */
+#define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
+#define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
+#define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
+#define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
+
+#ifndef __ASSEMBLY__
+
+struct ppc_hw_breakpoint {
+	uint32_t version;		/* currently, version must be 1 */
+	uint32_t trigger_type;		/* only some combinations allowed */
+	uint32_t addr_mode;		/* address match mode */
+	uint32_t condition_mode;	/* break/watchpoint condition flags */
+	uint64_t addr;			/* break/watchpoint address */
+	uint64_t addr2;			/* range end or mask */
+	uint64_t condition_value;	/* contents of the DVC register */
+};
+
+#endif /* __ASSEMBLY__ */
+
+/*
+ * Trigger Type
+ */
+#define PPC_BREAKPOINT_TRIGGER_EXECUTE	0x1
+#define PPC_BREAKPOINT_TRIGGER_READ	0x2
+#define PPC_BREAKPOINT_TRIGGER_WRITE	0x4
+#define PPC_BREAKPOINT_TRIGGER_RW	0x6
+
+/*
+ * Address Mode
+ */
+#define PPC_BREAKPOINT_MODE_EXACT		0x0
+#define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE	0x1
+#define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE	0x2
+#define PPC_BREAKPOINT_MODE_MASK		0x3
+
+/*
+ * Condition Mode
+ */
+#define PPC_BREAKPOINT_CONDITION_NONE	0x0
+#define PPC_BREAKPOINT_CONDITION_AND	0x1
+#define PPC_BREAKPOINT_CONDITION_EXACT	0x1
+#define PPC_BREAKPOINT_CONDITION_OR	0x2
+#define PPC_BREAKPOINT_CONDITION_AND_OR	0x3
+#define PPC_BREAKPOINT_CONDITION_BE_ALL	0x00ff0000
+#define PPC_BREAKPOINT_CONDITION_BE_SHIFT	16
+#define PPC_BREAKPOINT_CONDITION_BE(n)	\
+	(1<<((n)+PPC_BREAKPOINT_CONDITION_BE_SHIFT))
+
 #endif /* _ASM_POWERPC_PTRACE_H */
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index ef14988..6be2ce0 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -839,6 +839,50 @@ void ptrace_disable(struct task_struct *child)
 	user_disable_single_step(child);
 }
 
+static long ppc_set_hwdebug(struct task_struct *child,
+		     struct ppc_hw_breakpoint *bp_info)
+{
+	/*
+	 * We currently support one data breakpoint
+	 */
+	if (((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0) ||
+	    ((bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0) ||
+	    (bp_info->trigger_type != PPC_BREAKPOINT_TRIGGER_WRITE) ||
+	    (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;
+
+	child->thread.dabr = (unsigned long)bp_info->addr;
+#ifdef CONFIG_BOOKE
+	child->thread.dbcr0 = DBCR0_IDM;
+	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
+		child->thread.dbcr0 |= DBSR_DAC1R;
+	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
+		child->thread.dbcr0 |= DBSR_DAC1W;
+	child->thread.regs->msr |= MSR_DE;
+#endif
+	return 1;
+}
+
+static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
+{
+	if ((data != 1) || (child->thread.dabr == 0))
+		return -EINVAL;
+
+	child->thread.dabr = 0;
+#ifdef CONFIG_BOOKE
+	child->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
+	child->thread.regs->msr &= ~MSR_DE;
+#endif
+	return 0;
+}
+
 /*
  * Here are the old "legacy" powerpc specific getregs/setregs ptrace calls,
  * we mark them as obsolete now, they will be removed in a future version
@@ -932,6 +976,50 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		break;
 	}
 
+	case PPC_PTRACE_GETHWDBGINFO: {
+		struct ppc_debug_info dbginfo;
+
+		dbginfo.version = 1;
+		dbginfo.num_instruction_bps = 0;
+		dbginfo.num_data_bps = 1;
+		dbginfo.num_condition_regs = 0;
+#ifdef CONFIG_PPC64
+		dbginfo.data_bp_alignment = 8;
+#else
+		dbginfo.data_bp_alignment = 0;
+#endif
+		dbginfo.sizeof_condition = 0;
+		dbginfo.features = 0;
+
+		if (!access_ok(VERIFY_WRITE, data,
+			       sizeof(struct ppc_debug_info)))
+			return -EFAULT;
+		ret = __copy_to_user((struct ppc_debug_info __user *)data,
+				     &dbginfo, sizeof(struct ppc_debug_info)) ?
+		      -EFAULT : 0;
+		break;
+	}
+
+	case PPC_PTRACE_SETHWDEBUG: {
+		struct ppc_hw_breakpoint bp_info;
+
+		if (!access_ok(VERIFY_READ, data,
+			       sizeof(struct ppc_hw_breakpoint)))
+			return -EFAULT;
+		ret = __copy_from_user(&bp_info,
+				       (struct ppc_hw_breakpoint __user *)data,
+				       sizeof(struct ppc_hw_breakpoint)) ?
+		      -EFAULT : 0;
+		if (!ret)
+			ret = ppc_set_hwdebug(child, &bp_info);
+		break;
+	}
+
+	case PPC_PTRACE_DELHWDEBUG: {
+		ret = ppc_del_hwdebug(child, addr, data);
+		break;
+	}
+
 	case PTRACE_GET_DEBUGREG: {
 		ret = -EINVAL;
 		/* We only support one DABR and no IABRS at the moment */

-- 
Dave Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 01/03] powerpc: Extended ptrace interface

[David Gibson]

On Thu, Dec 10, 2009 at 01:57:15PM -0200, Dave Kleikamp wrote:
> powerpc: Extended ptrace interface
> 
> From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
> 
> Add a new extended ptrace interface so that user-space has a single
> interface for powerpc, without having to know the specific layout
> of the debug registers.


> Implement:
> PPC_PTRACE_GETHWDEBUGINFO
> PPC_PTRACE_SETHWDEBUG
> PPC_PTRACE_DELHWDEBUG
> 
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>

Apart from the data breakpoint alignment for 32-bit systems, all the
comments below are trivial nits, so:

Acked-by: David Gibson <dwg@au1.ibm.com>

[snip]
> +/*
> + * Trigger Type
> + */
> +#define PPC_BREAKPOINT_TRIGGER_EXECUTE	0x1
> +#define PPC_BREAKPOINT_TRIGGER_READ	0x2
> +#define PPC_BREAKPOINT_TRIGGER_WRITE	0x4
> +#define PPC_BREAKPOINT_TRIGGER_RW	0x6

For a little extra safety, I'd tend towards defining the RW constant
in terms of the READ and WRITE constants.

> +
> +/*
> + * Address Mode
> + */
> +#define PPC_BREAKPOINT_MODE_EXACT		0x0
> +#define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE	0x1
> +#define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE	0x2
> +#define PPC_BREAKPOINT_MODE_MASK		0x3
> +
> +/*
> + * Condition Mode
> + */
> +#define PPC_BREAKPOINT_CONDITION_NONE	0x0
> +#define PPC_BREAKPOINT_CONDITION_AND	0x1
> +#define PPC_BREAKPOINT_CONDITION_EXACT	0x1

And likewuse define EXACT in terms of AND.

> +#define PPC_BREAKPOINT_CONDITION_OR	0x2
> +#define PPC_BREAKPOINT_CONDITION_AND_OR	0x3
> +#define PPC_BREAKPOINT_CONDITION_BE_ALL	0x00ff0000
> +#define PPC_BREAKPOINT_CONDITION_BE_SHIFT	16
> +#define PPC_BREAKPOINT_CONDITION_BE(n)	\
> +	(1<<((n)+PPC_BREAKPOINT_CONDITION_BE_SHIFT))

[snip]
> +	case PPC_PTRACE_GETHWDBGINFO: {
> +		struct ppc_debug_info dbginfo;
> +
> +		dbginfo.version = 1;
> +		dbginfo.num_instruction_bps = 0;
> +		dbginfo.num_data_bps = 1;
> +		dbginfo.num_condition_regs = 0;
> +#ifdef CONFIG_PPC64
> +		dbginfo.data_bp_alignment = 8;
> +#else
> +		dbginfo.data_bp_alignment = 0;

Uh.. this looks wrong.  Surely it should be 4.

-- 
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

Re: [RFC:PATCH 01/03] powerpc: Extended ptrace interface

[Kumar Gala]

On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:

> powerpc: Extended ptrace interface
>=20
> From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
>=20
> Add a new extended ptrace interface so that user-space has a single
> interface for powerpc, without having to know the specific layout
> of the debug registers.
>=20
> Implement:
> PPC_PTRACE_GETHWDEBUGINFO
> PPC_PTRACE_SETHWDEBUG
> PPC_PTRACE_DELHWDEBUG
>=20
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
> Cc: David Gibson <dwg@au1.ibm.com>
> Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
> ---
>=20
> arch/powerpc/include/asm/ptrace.h |   75 =
++++++++++++++++++++++++++++++++
> arch/powerpc/kernel/ptrace.c      |   88 =
+++++++++++++++++++++++++++++++++++++
> 2 files changed, 163 insertions(+), 0 deletions(-)
>=20
>=20
> diff --git a/arch/powerpc/include/asm/ptrace.h =
b/arch/powerpc/include/asm/ptrace.h
> index 8c34149..7ae887b 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -24,6 +24,12 @@
>  * 2 of the License, or (at your option) any later version.
>  */
>=20
> +#ifdef __KERNEL__
> +#include <linux/types.h>
> +#else
> +#include <stdint.h>
> +#endif
> +
> #ifndef __ASSEMBLY__
>=20
> struct pt_regs {
> @@ -292,4 +298,73 @@ extern void user_disable_single_step(struct =
task_struct *);
>=20
> #define PTRACE_SINGLEBLOCK	0x100	/* resume execution until next =
branch */
>=20
> +#define PPC_PTRACE_GETHWDBGINFO	0x89
> +#define PPC_PTRACE_SETHWDEBUG	0x88
> +#define PPC_PTRACE_DELHWDEBUG	0x87
> +
> +#ifndef __ASSEMBLY__
> +
> +struct ppc_debug_info {
> +	uint32_t version;		/* Only version 1 exists to date =
*/
> +	uint32_t num_instruction_bps;
> +	uint32_t num_data_bps;
> +	uint32_t num_condition_regs;
> +	uint32_t data_bp_alignment;
> +	uint32_t sizeof_condition;	/* size of the DVC register */
> +	uint64_t features;
> +};
> +
> +#endif /* __ASSEMBLY__ */
> +
> +/*
> + * features will have bits indication whether there is support for:
> + */
> +#define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> +#define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> +#define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> +#define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8

Pad these out.

#define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x0000000000000001
#define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x0000000000000002

etc..


> +
> +#ifndef __ASSEMBLY__
> +
> +struct ppc_hw_breakpoint {
> +	uint32_t version;		/* currently, version must be 1 =
*/
> +	uint32_t trigger_type;		/* only some combinations =
allowed */
> +	uint32_t addr_mode;		/* address match mode */
> +	uint32_t condition_mode;	/* break/watchpoint condition =
flags */
> +	uint64_t addr;			/* break/watchpoint address */
> +	uint64_t addr2;			/* range end or mask */
> +	uint64_t condition_value;	/* contents of the DVC register =
*/
> +};
> +
> +#endif /* __ASSEMBLY__ */
> +
> +/*
> + * Trigger Type
> + */
> +#define PPC_BREAKPOINT_TRIGGER_EXECUTE	0x1
> +#define PPC_BREAKPOINT_TRIGGER_READ	0x2
> +#define PPC_BREAKPOINT_TRIGGER_WRITE	0x4
> +#define PPC_BREAKPOINT_TRIGGER_RW	0x6

(ditto on the padding)

> +
> +/*
> + * Address Mode
> + */
> +#define PPC_BREAKPOINT_MODE_EXACT		0x0
> +#define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE	0x1
> +#define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE	0x2
> +#define PPC_BREAKPOINT_MODE_MASK		0x3
> +

(ditto on the padding)

> +/*
> + * Condition Mode
> + */
> +#define PPC_BREAKPOINT_CONDITION_NONE	0x0
> +#define PPC_BREAKPOINT_CONDITION_AND	0x1
> +#define PPC_BREAKPOINT_CONDITION_EXACT	0x1
> +#define PPC_BREAKPOINT_CONDITION_OR	0x2
> +#define PPC_BREAKPOINT_CONDITION_AND_OR	0x3

(ditto on the padding)

> +#define PPC_BREAKPOINT_CONDITION_BE_ALL	0x00ff0000
> +#define PPC_BREAKPOINT_CONDITION_BE_SHIFT	16
> +#define PPC_BREAKPOINT_CONDITION_BE(n)	\
> +	(1<<((n)+PPC_BREAKPOINT_CONDITION_BE_SHIFT))
> +
> #endif /* _ASM_POWERPC_PTRACE_H */
> diff --git a/arch/powerpc/kernel/ptrace.c =
b/arch/powerpc/kernel/ptrace.c
> index ef14988..6be2ce0 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -839,6 +839,50 @@ void ptrace_disable(struct task_struct *child)
> 	user_disable_single_step(child);
> }
>=20
> +static long ppc_set_hwdebug(struct task_struct *child,
> +		     struct ppc_hw_breakpoint *bp_info)
> +{
> +	/*
> +	 * We currently support one data breakpoint
> +	 */
> +	if (((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) =3D=3D =
0) ||
> +	    ((bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) !=3D =
0) ||
> +	    (bp_info->trigger_type !=3D PPC_BREAKPOINT_TRIGGER_WRITE) ||
> +	    (bp_info->addr_mode !=3D PPC_BREAKPOINT_MODE_EXACT) ||
> +	    (bp_info->condition_mode !=3D =
PPC_BREAKPOINT_CONDITION_NONE))
> +		return -EINVAL;
> +
> +	if (child->thread.dabr)
> +		return -ENOSPC;
> +
> +	if ((unsigned long)bp_info->addr >=3D TASK_SIZE)
> +		return -EIO;
> +
> +	child->thread.dabr =3D (unsigned long)bp_info->addr;
> +#ifdef CONFIG_BOOKE
> +	child->thread.dbcr0 =3D DBCR0_IDM;
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> +		child->thread.dbcr0 |=3D DBSR_DAC1R;
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> +		child->thread.dbcr0 |=3D DBSR_DAC1W;
> +	child->thread.regs->msr |=3D MSR_DE;
> +#endif
> +	return 1;
> +}
> +
> +static long ppc_del_hwdebug(struct task_struct *child, long addr, =
long data)
> +{
> +	if ((data !=3D 1) || (child->thread.dabr =3D=3D 0))
> +		return -EINVAL;
> +
> +	child->thread.dabr =3D 0;
> +#ifdef CONFIG_BOOKE
> +	child->thread.dbcr0 &=3D ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> +	child->thread.regs->msr &=3D ~MSR_DE;
> +#endif
> +	return 0;
> +}
> +
> /*
>  * Here are the old "legacy" powerpc specific getregs/setregs ptrace =
calls,
>  * we mark them as obsolete now, they will be removed in a future =
version
> @@ -932,6 +976,50 @@ long arch_ptrace(struct task_struct *child, long =
request, long addr, long data)
> 		break;
> 	}
>=20
> +	case PPC_PTRACE_GETHWDBGINFO: {
> +		struct ppc_debug_info dbginfo;
> +
> +		dbginfo.version =3D 1;
> +		dbginfo.num_instruction_bps =3D 0;
> +		dbginfo.num_data_bps =3D 1;
> +		dbginfo.num_condition_regs =3D 0;
> +#ifdef CONFIG_PPC64
> +		dbginfo.data_bp_alignment =3D 8;
> +#else
> +		dbginfo.data_bp_alignment =3D 0;
> +#endif
> +		dbginfo.sizeof_condition =3D 0;
> +		dbginfo.features =3D 0;
> +
> +		if (!access_ok(VERIFY_WRITE, data,
> +			       sizeof(struct ppc_debug_info)))
> +			return -EFAULT;
> +		ret =3D __copy_to_user((struct ppc_debug_info __user =
*)data,
> +				     &dbginfo, sizeof(struct =
ppc_debug_info)) ?
> +		      -EFAULT : 0;
> +		break;
> +	}
> +
> +	case PPC_PTRACE_SETHWDEBUG: {
> +		struct ppc_hw_breakpoint bp_info;
> +
> +		if (!access_ok(VERIFY_READ, data,
> +			       sizeof(struct ppc_hw_breakpoint)))
> +			return -EFAULT;
> +		ret =3D __copy_from_user(&bp_info,
> +				       (struct ppc_hw_breakpoint __user =
*)data,
> +				       sizeof(struct ppc_hw_breakpoint)) =
?
> +		      -EFAULT : 0;
> +		if (!ret)
> +			ret =3D ppc_set_hwdebug(child, &bp_info);
> +		break;
> +	}
> +
> +	case PPC_PTRACE_DELHWDEBUG: {
> +		ret =3D ppc_del_hwdebug(child, addr, data);
> +		break;
> +	}
> +
> 	case PTRACE_GET_DEBUGREG: {
> 		ret =3D -EINVAL;
> 		/* We only support one DABR and no IABRS at the moment =
*/
>=20
> --=20
> Dave Kleikamp
> IBM Linux Technology Center
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Dave Kleikamp]

powerpc: Add definitions for Debug Registers on BookE Platforms

From: Torez Smith <lnxtorez@linux.vnet.ibm.com>

This patch adds additional definitions for BookE Debug Registers
to the reg_booke.h header file.

Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
Cc: David Gibson <dwg@au1.ibm.com>
Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
---

 arch/powerpc/include/asm/processor.h |   30 +++++-
 arch/powerpc/include/asm/reg_booke.h |  176 +++++++++++++++++++++++++++++-----
 2 files changed, 178 insertions(+), 28 deletions(-)


diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 9eed29e..1393307 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -161,9 +161,35 @@ struct thread_struct {
 #ifdef CONFIG_PPC32
 	void		*pgdir;		/* root of page-table tree */
 #endif
-#if defined(CONFIG_4xx) || defined (CONFIG_BOOKE)
-	unsigned long	dbcr0;		/* debug control register values */
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/*
+	 * The following help to manage the use of Debug Control Registers
+	 * om the BookE platforms.
+	 */
+	unsigned long	dbcr0;
 	unsigned long	dbcr1;
+	unsigned long	dbcr2;
+	/*
+	 * The stored value of the DBSR register will be the value at the
+	 * last debug interrupt. This register can only be read from the
+	 * user (will never be written to) and has value while helping to
+	 * describe the reason for the last debug trap.  Torez
+	 */
+	unsigned long	dbsr;
+	/*
+	 * The following will contain addresses used by debug applications
+	 * to help trace and trap on particular address locations.
+	 * The bits in the Debug Control Registers above help define which
+	 * of the following registers will contain valid data and/or addresses.
+	 */
+	unsigned long	iac1;
+	unsigned long	iac2;
+	unsigned long	iac3;
+	unsigned long	iac4;
+	unsigned long	dac1;
+	unsigned long	dac2;
+	unsigned long	dvc1;
+	unsigned long	dvc2;
 #endif
 	/* FP and VSX 0-31 register set */
 	double		fpr[32][TS_FPRWIDTH];
diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
index 3bf7835..7f8c71f 100644
--- a/arch/powerpc/include/asm/reg_booke.h
+++ b/arch/powerpc/include/asm/reg_booke.h
@@ -248,6 +248,8 @@
 #define DBSR_RET	0x00008000	/* Return Debug Event */
 #define DBSR_CIRPT	0x00000040	/* Critical Interrupt Taken Event */
 #define DBSR_CRET	0x00000020	/* Critical Return Debug Event */
+#define DBSR_IAC12ATS	0x00000002	/* Instr Address Compare 1/2 Toggle */
+#define DBSR_IAC34ATS	0x00000001	/* Instr Address Compare 3/4 Toggle */
 #endif
 #ifdef CONFIG_40x
 #define DBSR_IC		0x80000000	/* Instruction Completion */
@@ -294,25 +296,68 @@
 #define DBCR0_IC	0x08000000	/* Instruction Completion */
 #define DBCR0_ICMP	DBCR0_IC
 #define DBCR0_BT	0x04000000	/* Branch Taken */
-#define DBCR0_BRT	DBCR0_BT
 #define DBCR0_EDE	0x02000000	/* Exception Debug Event */
 #define DBCR0_IRPT	DBCR0_EDE
 #define DBCR0_TDE	0x01000000	/* TRAP Debug Event */
-#define DBCR0_IA1	0x00800000	/* Instr Addr compare 1 enable */
-#define DBCR0_IAC1	DBCR0_IA1
-#define DBCR0_IA2	0x00400000	/* Instr Addr compare 2 enable */
-#define DBCR0_IAC2	DBCR0_IA2
-#define DBCR0_IA12	0x00200000	/* Instr Addr 1-2 range enable */
-#define DBCR0_IA12X	0x00100000	/* Instr Addr 1-2 range eXclusive */
-#define DBCR0_IA3	0x00080000	/* Instr Addr compare 3 enable */
-#define DBCR0_IAC3	DBCR0_IA3
-#define DBCR0_IA4	0x00040000	/* Instr Addr compare 4 enable */
-#define DBCR0_IAC4	DBCR0_IA4
-#define DBCR0_IA34	0x00020000	/* Instr Addr 3-4 range Enable */
-#define DBCR0_IA34X	0x00010000	/* Instr Addr 3-4 range eXclusive */
-#define DBCR0_IA12T	0x00008000	/* Instr Addr 1-2 range Toggle */
-#define DBCR0_IA34T	0x00004000	/* Instr Addr 3-4 range Toggle */
+#define DBCR0_IAC1	0x00800000	/* Instr Addr compare 1 enable */
+#define DBCR0_IAC2	0x00400000	/* Instr Addr compare 2 enable */
+#define DBCR0_IAC12M	0x00300000	/* Instr Addr 1-2 range enable */
+#define DBCR0_IAC12M_R	0x00100000	/* Instr Addr 1-2 Reserved state */
+#define DBCR0_IAC12M_I	0x00200000	/* Instr Addr 1-2 range Inclusive */
+#define DBCR0_IAC12M_X	0x00300000	/* Instr Addr 1-2 range eXclusive */
+#define DBCR0_IAC3	0x00080000	/* Instr Addr compare 3 enable */
+#define DBCR0_IAC4	0x00040000	/* Instr Addr compare 4 enable */
+#define DBCR0_IAC34	0x00020000	/* Instr Addr 3-4 range Enable */
+#define DBCR0_IAC34X	0x00010000	/* Instr Addr 3-4 range eXclusive */
+#define DBCR0_IAC12T	0x00008000	/* Instr Addr 1-2 range Toggle */
+#define DBCR0_IAC34T	0x00004000	/* Instr Addr 3-4 range Toggle */
 #define DBCR0_FT	0x00000001	/* Freeze Timers on debug event */
+
+#define DBCR0_USER_DEBUG	(DBCR0_IDM | DBCR0_ICMP | DBCR0_IAC1 | \
+				 DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4)
+#define DBCR0_BASE_REG_VALUE	0
+
+#define dbcr_iac_range(task)	((task)->thread.dbcr0)
+#define DBCR_IAC12M	DBCR0_IAC12M
+#define DBCR_IAC12M_I	DBCR0_IAC12M_I
+#define DBCR_IAC12M_X	DBCR0_IAC12M_X
+#define DBCR_IAC34M	DBCR0_IAC34M
+#define DBCR_IAC34M_I	DBCR0_IAC34M_I
+#define DBCR_IAC34M_X	DBCR0_IAC34M_X
+
+/* Bit definitions related to the DBCR1. */
+#define DBCR1_D1R	0x80000000	/* DAC1 Read Debug Event */
+#define DBCR1_DAC1R	DBCR1_D1R
+#define DBCR1_D2R	0x40000000	/* DAC2 Read Debug Event */
+#define DBCR1_DAC2R	DBCR1_D2R
+#define DBCR1_D1W	0x20000000	/* DAC1 Write Debug Event */
+#define DBCR1_DAC1W	DBCR1_D1W
+#define DBCR1_D2W	0x10000000	/* DAC2 Write Debug Event */
+#define DBCR1_DAC2W	DBCR1_D2W
+
+#define DBCR1_USER_DEBUG	(DBCR1_DAC1R | DBCR1_DAC2R | DBCR1_DAC1W | \
+				 DBCR1_DAC2W)
+#define DBCR1_BASE_REG_VALUE	0
+
+#define dbcr_dac(task)	((task)->thread.dbcr1)
+#define DBCR_DAC1R	DBCR1_DAC1R
+#define DBCR_DAC1W	DBCR1_DAC1W
+#define DBCR_DAC2R	DBCR1_DAC2R
+#define DBCR_DAC2W	DBCR1_DAC2W
+
+#define DBCR2_USER_DEBUG	0
+#define DBCR2_BASE_REG_VALUE	0
+
+/*
+ * Are there any active Debug Events represented in the
+ * Debug Control Registers?
+ */
+#define DBCR0_ACTIVE_EVENTS	(DBCR0_ICMP | DBCR0_IAC1 | DBCR0_IAC2 | \
+				 DBCR0_IAC3 | DBCR0_IAC4)
+#define DBCR1_ACTIVE_EVENTS	(DBCR1_D1R | DBCR1_D2R | DBCR1_D1W | DBCR1_D2W)
+#define DBCR_ACTIVE_EVENTS(dbcr0, dbcr1)  (((dbcr0) & DBCR0_ACTIVE_EVENTS) || \
+					   ((dbcr1) & DBCR1_ACTIVE_EVENTS))
+
 #elif defined(CONFIG_BOOKE)
 #define DBCR0_EDM	0x80000000	/* External Debug Mode */
 #define DBCR0_IDM	0x40000000	/* Internal Debug Mode */
@@ -324,8 +369,7 @@
 #define DBCR0_RST_NONE	0x00000000	/* No Reset */
 #define DBCR0_ICMP	0x08000000	/* Instruction Completion */
 #define DBCR0_IC	DBCR0_ICMP
-#define DBCR0_BRT	0x04000000	/* Branch Taken */
-#define DBCR0_BT	DBCR0_BRT
+#define DBCR0_BT	0x04000000	/* Branch Taken */
 #define DBCR0_IRPT	0x02000000	/* Exception Debug Event */
 #define DBCR0_TDE	0x01000000	/* TRAP Debug Event */
 #define DBCR0_TIE	DBCR0_TDE
@@ -342,19 +386,99 @@
 #define DBCR0_CRET	0x00000020	/* Critical Return Debug Event */
 #define DBCR0_FT	0x00000001	/* Freeze Timers on debug event */
 
+#define DBCR0_USER_DEBUG	(DBCR0_IDM | DBCR0_ICMP | DBCR0_IAC1 | \
+				 DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
+				 DBCR0_DAC1R | DBCR0_DAC1W  | DBCR0_DAC2R | \
+				 DBCR0_DAC2W)
+#define DBCR0_BASE_REG_VALUE	0
+
+#define dbcr_dac(task)	((task)->thread.dbcr0)
+#define DBCR_DAC1R	DBCR0_DAC1R
+#define DBCR_DAC1W	DBCR0_DAC1W
+#define DBCR_DAC2R	DBCR0_DAC2R
+#define DBCR_DAC2W	DBCR0_DAC2W
+
 /* Bit definitions related to the DBCR1. */
-#define DBCR1_IAC12M	0x00800000	/* Instr Addr 1-2 range enable */
-#define DBCR1_IAC12MX	0x00C00000	/* Instr Addr 1-2 range eXclusive */
-#define DBCR1_IAC12AT	0x00010000	/* Instr Addr 1-2 range Toggle */
-#define DBCR1_IAC34M	0x00000080	/* Instr Addr 3-4 range enable */
-#define DBCR1_IAC34MX	0x000000C0	/* Instr Addr 3-4 range eXclusive */
-#define DBCR1_IAC34AT	0x00000001	/* Instr Addr 3-4 range Toggle */
+#define DBCR1_IAC1US	0xC0000000	/* Instr Addr Cmp 1 Sup/User   */
+#define DBCR1_IAC1ER	0x30000000	/* Instr Addr Cmp 1 Eff/Real */
+#define DBCR1_IAC1ER_01	0x10000000	/* reserved */
+#define DBCR1_IAC1ER_10	0x20000000	/* Instr Addr Cmp 1 Eff/Real MSR[IS]=0 */
+#define DBCR1_IAC1ER_11	0x30000000	/* Instr Addr Cmp 1 Eff/Real MSR[IS]=1 */
+#define DBCR1_IAC2US	0x0C000000	/* Instr Addr Cmp 2 Sup/User   */
+#define DBCR1_IAC2ER	0x03000000	/* Instr Addr Cmp 2 Eff/Real */
+#define DBCR1_IAC2ER_01	0x01000000	/* reserved */
+#define DBCR1_IAC2ER_10	0x02000000	/* Instr Addr Cmp 2 Eff/Real MSR[IS]=0 */
+#define DBCR1_IAC2ER_11	0x03000000	/* Instr Addr Cmp 2 Eff/Real MSR[IS]=1 */
+#define DBCR1_IAC12M	0x00C00000	/* Instr Addr 1-2 range enable */
+#define DBCR1_IAC12M_R	0x00400000	/* Instr Addr 1-2 reserved state */
+#define DBCR1_IAC12M_I	0x00800000	/* Instr Addr 1-2 range inclusive */
+#define DBCR1_IAC12M_X	0x00C00000	/* Instr Addr 1-2 range eXclusive */
+#define DBCR1_IAC12A_T	0x00010000	/* Instr Addr 1-2 range Toggle */
+#define DBCR1_IAC3US	0x0000C000	/* Instr Addr Cmp 3 Sup/User   */
+#define DBCR1_IAC3ER	0x00003000	/* Instr Addr Cmp 3 Eff/Real */
+#define DBCR1_IAC3ER_01	0x00001000	/* reserved */
+#define DBCR1_IAC3ER_10	0x00002000	/* Instr Addr Cmp 3 Eff/Real MSR[IS]=0 */
+#define DBCR1_IAC3ER_11	0x00003000	/* Instr Addr Cmp 3 Eff/Real MSR[IS]=1 */
+#define DBCR1_IAC4US	0x00000C00	/* Instr Addr Cmp 4 Sup/User   */
+#define DBCR1_IAC4ER	0x00000300	/* Instr Addr Cmp 4 Eff/Real */
+#define DBCR1_IAC4ER_01	0x00000100	/* Instr Addr Cmp 4 Eff/Real MSR[IS]=0 */
+#define DBCR1_IAC4ER_10	0x00000200	/* Instr Addr Cmp 4 Eff/Real MSR[IS]=0 */
+#define DBCR1_IAC4ER_11	0x00000300	/* Instr Addr Cmp 4 Eff/Real MSR[IS]=1 */
+#define DBCR1_IAC34M	0x000000C0	/* Instr Addr 3-4 range enable */
+#define DBCR1_IAC34M_R	0x00000040	/* Instr Addr 3-4 reserved state */
+#define DBCR1_IAC34M_I	0x00000080	/* Instr Addr 3-4 range inclusive */
+#define DBCR1_IAC34M_X	0x000000C0	/* Instr Addr 3-4 range eXclusive */
+#define DBCR1_IAC34A_T	0x00000001	/* Instr Addr 3-4 range Toggle */
+
+#define DBCR1_USER_DEBUG	(DBCR1_IAC12M | DBCR1_IAC34M)
+#define DBCR1_BASE_REG_VALUE	(DBCR1_IAC1US | DBCR1_IAC1ER_10 | \
+				 DBCR1_IAC2US | DBCR1_IAC2ER_10 | \
+				 DBCR1_IAC3US | DBCR1_IAC3ER_10 | \
+				 DBCR1_IAC4US | DBCR1_IAC4ER_10)
+
+#define dbcr_iac_range(task)	((task)->thread.dbcr1)
+#define DBCR_IAC12M	DBCR1_IAC12M
+#define DBCR_IAC12M_I	DBCR1_IAC12M_I
+#define DBCR_IAC12M_X	DBCR1_IAC12M_X
+#define DBCR_IAC34M	DBCR1_IAC34M
+#define DBCR_IAC34M_I	DBCR1_IAC34M_I
+#define DBCR_IAC34M_X	DBCR1_IAC34M_X
 
 /* Bit definitions related to the DBCR2. */
-#define DBCR2_DAC12M	0x00800000	/* DAC 1-2 range enable */
-#define DBCR2_DAC12MX	0x00C00000	/* DAC 1-2 range eXclusive */
+#define DBCR2_DAC1US	0xC0000000	/* Data Addr Cmp 1 Sup/User   */
+#define DBCR2_DAC1ER	0x30000000	/* Data Addr Cmp 1 Eff/Real */
+#define DBCR2_DAC2US	0x00000000	/* Data Addr Cmp 2 Sup/User   */
+#define DBCR2_DAC2ER	0x00000000	/* Data Addr Cmp 2 Eff/Real */
+#define DBCR2_DAC12MODE	0x00C00000	/* DAC 1-2 Mode Bits */
+#define DBCR2_DAC12MASK	0x00400000	/* DAC 1-2 Mask mode*/
+#define DBCR2_DAC12R	0x00800000	/* DAC 1-2 range enable */
+#define DBCR2_DAC12RX	0x00C00000	/* DAC 1-2 range eXclusive */
 #define DBCR2_DAC12A	0x00200000	/* DAC 1-2 Asynchronous */
-#endif
+#define DBCR2_DVC1M	0x000C0000	/* Data Value Comp 1 Mode */
+#define DBCR2_DVC1M_SHIFT	18	/* # of bits to shift DBCR2_DVC1M */
+#define DBCR2_DVC2M	0x00030000	/* Data Value Comp 2 Mode */
+#define DBCR2_DVC2M_SHIFT	16	/* # of bits to shift DBCR2_DVC2M */
+#define DBCR2_DVC1BE	0x00000F00	/* Data Value Comp 1 Byte */
+#define DBCR2_DVC1BE_SHIFT	8	/* # of bits to shift DBCR2_DVC1BE */
+#define DBCR2_DVC2BE	0x0000000F	/* Data Value Comp 2 Byte */
+#define DBCR2_DVC2BE_SHIFT	0	/* # of bits to shift DBCR2_DVC2BE */
+
+#define DBCR2_USER_DEBUG	(DBCR2_DAC12MODE | DBCR2_DVC1M | DBCR2_DVC2M | \
+				 DBCR2_DVC1BE | DBCR2_DVC2BE)
+#define DBCR2_BASE_REG_VALUE	(DBCR2_DAC1US | DBCR2_DAC2US)
+
+/*
+ * Are there any active Debug Events represented in the
+ * Debug Control Registers?
+ */
+#define DBCR0_ACTIVE_EVENTS  (DBCR0_ICMP | DBCR0_IAC1 | DBCR0_IAC2 | \
+			      DBCR0_IAC3 | DBCR0_IAC4 | DBCR0_DAC1R | \
+			      DBCR0_DAC1W  | DBCR0_DAC2R | DBCR0_DAC2W)
+#define DBCR1_ACTIVE_EVENTS	0
+
+#define DBCR_ACTIVE_EVENTS(dbcr0, dbcr1)  (((dbcr0) & DBCR0_ACTIVE_EVENTS) || \
+					   ((dbcr1) & DBCR1_ACTIVE_EVENTS))
+#endif /* #elif defined(CONFIG_BOOKE) */
 
 /* Bit definitions related to the TCR. */
 #define TCR_WP(x)	(((x)&0x3)<<30)	/* WDT Period */

-- 
Dave Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Josh Boyer]

On Thu, Dec 10, 2009 at 01:57:21PM -0200, Dave Kleikamp wrote:
>diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>index 9eed29e..1393307 100644
>--- a/arch/powerpc/include/asm/processor.h
>+++ b/arch/powerpc/include/asm/processor.h
>@@ -161,9 +161,35 @@ struct thread_struct {
> #ifdef CONFIG_PPC32
> 	void		*pgdir;		/* root of page-table tree */
> #endif
>-#if defined(CONFIG_4xx) || defined (CONFIG_BOOKE)
>-	unsigned long	dbcr0;		/* debug control register values */
>+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
>+	/*
>+	 * The following help to manage the use of Debug Control Registers
>+	 * om the BookE platforms.
>+	 */
>+	unsigned long	dbcr0;
> 	unsigned long	dbcr1;
>+	unsigned long	dbcr2;

This is wrong for 405.  405 only has dbcr0 and dbcr1.  I don't see why you'd
change the #define values to be more explicit and then include things that
don't make sense.

>+	/*
>+	 * The stored value of the DBSR register will be the value at the
>+	 * last debug interrupt. This register can only be read from the
>+	 * user (will never be written to) and has value while helping to
>+	 * describe the reason for the last debug trap.  Torez
>+	 */
>+	unsigned long	dbsr;
>+	/*
>+	 * The following will contain addresses used by debug applications
>+	 * to help trace and trap on particular address locations.
>+	 * The bits in the Debug Control Registers above help define which
>+	 * of the following registers will contain valid data and/or addresses.
>+	 */
>+	unsigned long	iac1;
>+	unsigned long	iac2;
>+	unsigned long	iac3;
>+	unsigned long	iac4;
>+	unsigned long	dac1;
>+	unsigned long	dac2;
>+	unsigned long	dvc1;
>+	unsigned long	dvc2;
> #endif

Without digging much, I'm wondering if we could just use a pointer to a debug
register structure here instead of growing struct thread more.

> 	/* FP and VSX 0-31 register set */
> 	double		fpr[32][TS_FPRWIDTH];
>diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
>index 3bf7835..7f8c71f 100644
>--- a/arch/powerpc/include/asm/reg_booke.h
>+++ b/arch/powerpc/include/asm/reg_booke.h
>@@ -248,6 +248,8 @@
> #define DBSR_RET	0x00008000	/* Return Debug Event */
> #define DBSR_CIRPT	0x00000040	/* Critical Interrupt Taken Event */
> #define DBSR_CRET	0x00000020	/* Critical Return Debug Event */
>+#define DBSR_IAC12ATS	0x00000002	/* Instr Address Compare 1/2 Toggle */
>+#define DBSR_IAC34ATS	0x00000001	/* Instr Address Compare 3/4 Toggle */
> #endif
> #ifdef CONFIG_40x
> #define DBSR_IC		0x80000000	/* Instruction Completion */
>@@ -294,25 +296,68 @@
> #define DBCR0_IC	0x08000000	/* Instruction Completion */
> #define DBCR0_ICMP	DBCR0_IC
> #define DBCR0_BT	0x04000000	/* Branch Taken */
>-#define DBCR0_BRT	DBCR0_BT
> #define DBCR0_EDE	0x02000000	/* Exception Debug Event */
> #define DBCR0_IRPT	DBCR0_EDE
> #define DBCR0_TDE	0x01000000	/* TRAP Debug Event */
>-#define DBCR0_IA1	0x00800000	/* Instr Addr compare 1 enable */
>-#define DBCR0_IAC1	DBCR0_IA1
>-#define DBCR0_IA2	0x00400000	/* Instr Addr compare 2 enable */
>-#define DBCR0_IAC2	DBCR0_IA2
>-#define DBCR0_IA12	0x00200000	/* Instr Addr 1-2 range enable */
>-#define DBCR0_IA12X	0x00100000	/* Instr Addr 1-2 range eXclusive */
>-#define DBCR0_IA3	0x00080000	/* Instr Addr compare 3 enable */
>-#define DBCR0_IAC3	DBCR0_IA3
>-#define DBCR0_IA4	0x00040000	/* Instr Addr compare 4 enable */
>-#define DBCR0_IAC4	DBCR0_IA4
>-#define DBCR0_IA34	0x00020000	/* Instr Addr 3-4 range Enable */
>-#define DBCR0_IA34X	0x00010000	/* Instr Addr 3-4 range eXclusive */
>-#define DBCR0_IA12T	0x00008000	/* Instr Addr 1-2 range Toggle */
>-#define DBCR0_IA34T	0x00004000	/* Instr Addr 3-4 range Toggle */
>+#define DBCR0_IAC1	0x00800000	/* Instr Addr compare 1 enable */
>+#define DBCR0_IAC2	0x00400000	/* Instr Addr compare 2 enable */
>+#define DBCR0_IAC12M	0x00300000	/* Instr Addr 1-2 range enable */
>+#define DBCR0_IAC12M_R	0x00100000	/* Instr Addr 1-2 Reserved state */
>+#define DBCR0_IAC12M_I	0x00200000	/* Instr Addr 1-2 range Inclusive */
>+#define DBCR0_IAC12M_X	0x00300000	/* Instr Addr 1-2 range eXclusive */
>+#define DBCR0_IAC3	0x00080000	/* Instr Addr compare 3 enable */
>+#define DBCR0_IAC4	0x00040000	/* Instr Addr compare 4 enable */
>+#define DBCR0_IAC34	0x00020000	/* Instr Addr 3-4 range Enable */
>+#define DBCR0_IAC34X	0x00010000	/* Instr Addr 3-4 range eXclusive */
>+#define DBCR0_IAC12T	0x00008000	/* Instr Addr 1-2 range Toggle */
>+#define DBCR0_IAC34T	0x00004000	/* Instr Addr 3-4 range Toggle */

A lot of this seems to just be cleanup... would it be possible to factor out
the cleanup parts so that the additions are easier to review?

> #define DBCR0_FT	0x00000001	/* Freeze Timers on debug event */
>+
>+#define DBCR0_USER_DEBUG	(DBCR0_IDM | DBCR0_ICMP | DBCR0_IAC1 | \
>+				 DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4)
>+#define DBCR0_BASE_REG_VALUE	0
>+
>+#define dbcr_iac_range(task)	((task)->thread.dbcr0)
>+#define DBCR_IAC12M	DBCR0_IAC12M
>+#define DBCR_IAC12M_I	DBCR0_IAC12M_I
>+#define DBCR_IAC12M_X	DBCR0_IAC12M_X
>+#define DBCR_IAC34M	DBCR0_IAC34M
>+#define DBCR_IAC34M_I	DBCR0_IAC34M_I
>+#define DBCR_IAC34M_X	DBCR0_IAC34M_X
>+
>+/* Bit definitions related to the DBCR1. */
>+#define DBCR1_D1R	0x80000000	/* DAC1 Read Debug Event */
>+#define DBCR1_DAC1R	DBCR1_D1R
>+#define DBCR1_D2R	0x40000000	/* DAC2 Read Debug Event */
>+#define DBCR1_DAC2R	DBCR1_D2R
>+#define DBCR1_D1W	0x20000000	/* DAC1 Write Debug Event */
>+#define DBCR1_DAC1W	DBCR1_D1W
>+#define DBCR1_D2W	0x10000000	/* DAC2 Write Debug Event */
>+#define DBCR1_DAC2W	DBCR1_D2W
>+
>+#define DBCR1_USER_DEBUG	(DBCR1_DAC1R | DBCR1_DAC2R | DBCR1_DAC1W | \
>+				 DBCR1_DAC2W)
>+#define DBCR1_BASE_REG_VALUE	0
>+
>+#define dbcr_dac(task)	((task)->thread.dbcr1)
>+#define DBCR_DAC1R	DBCR1_DAC1R
>+#define DBCR_DAC1W	DBCR1_DAC1W
>+#define DBCR_DAC2R	DBCR1_DAC2R
>+#define DBCR_DAC2W	DBCR1_DAC2W
>+
>+#define DBCR2_USER_DEBUG	0
>+#define DBCR2_BASE_REG_VALUE	0

Why are these defined for 405?

>+/*
>+ * Are there any active Debug Events represented in the
>+ * Debug Control Registers?
>+ */
>+#define DBCR0_ACTIVE_EVENTS	(DBCR0_ICMP | DBCR0_IAC1 | DBCR0_IAC2 | \
>+				 DBCR0_IAC3 | DBCR0_IAC4)
>+#define DBCR1_ACTIVE_EVENTS	(DBCR1_D1R | DBCR1_D2R | DBCR1_D1W | DBCR1_D2W)
>+#define DBCR_ACTIVE_EVENTS(dbcr0, dbcr1)  (((dbcr0) & DBCR0_ACTIVE_EVENTS) || \
>+					   ((dbcr1) & DBCR1_ACTIVE_EVENTS))
>+
> #elif defined(CONFIG_BOOKE)
> #define DBCR0_EDM	0x80000000	/* External Debug Mode */
> #define DBCR0_IDM	0x40000000	/* Internal Debug Mode */
>@@ -324,8 +369,7 @@
> #define DBCR0_RST_NONE	0x00000000	/* No Reset */
> #define DBCR0_ICMP	0x08000000	/* Instruction Completion */
> #define DBCR0_IC	DBCR0_ICMP
>-#define DBCR0_BRT	0x04000000	/* Branch Taken */
>-#define DBCR0_BT	DBCR0_BRT
>+#define DBCR0_BT	0x04000000	/* Branch Taken */

This seems like just cleanup of DBCR0_BRT?

> #define DBCR0_IRPT	0x02000000	/* Exception Debug Event */
> #define DBCR0_TDE	0x01000000	/* TRAP Debug Event */
> #define DBCR0_TIE	DBCR0_TDE
>@@ -342,19 +386,99 @@
> #define DBCR0_CRET	0x00000020	/* Critical Return Debug Event */
> #define DBCR0_FT	0x00000001	/* Freeze Timers on debug event */
>
>+#define DBCR0_USER_DEBUG	(DBCR0_IDM | DBCR0_ICMP | DBCR0_IAC1 | \
>+				 DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
>+				 DBCR0_DAC1R | DBCR0_DAC1W  | DBCR0_DAC2R | \
>+				 DBCR0_DAC2W)
>+#define DBCR0_BASE_REG_VALUE	0
>+
>+#define dbcr_dac(task)	((task)->thread.dbcr0)
>+#define DBCR_DAC1R	DBCR0_DAC1R
>+#define DBCR_DAC1W	DBCR0_DAC1W
>+#define DBCR_DAC2R	DBCR0_DAC2R
>+#define DBCR_DAC2W	DBCR0_DAC2W
>+
> /* Bit definitions related to the DBCR1. */

I'll try to review these a bit later.  Changing #defines makes for hard patch
review :)

josh

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Dave Kleikamp]

On Thu, 2009-12-10 at 12:07 -0500, Josh Boyer wrote:
> On Thu, Dec 10, 2009 at 01:57:21PM -0200, Dave Kleikamp wrote:

> >+	/*
> >+	 * The stored value of the DBSR register will be the value at the
> >+	 * last debug interrupt. This register can only be read from the
> >+	 * user (will never be written to) and has value while helping to
> >+	 * describe the reason for the last debug trap.  Torez
> >+	 */
> >+	unsigned long	dbsr;
> >+	/*
> >+	 * The following will contain addresses used by debug applications
> >+	 * to help trace and trap on particular address locations.
> >+	 * The bits in the Debug Control Registers above help define which
> >+	 * of the following registers will contain valid data and/or addresses.
> >+	 */
> >+	unsigned long	iac1;
> >+	unsigned long	iac2;
> >+	unsigned long	iac3;
> >+	unsigned long	iac4;
> >+	unsigned long	dac1;
> >+	unsigned long	dac2;
> >+	unsigned long	dvc1;
> >+	unsigned long	dvc2;
> > #endif
> 
> Without digging much, I'm wondering if we could just use a pointer to a debug
> register structure here instead of growing struct thread more.

I didn't do this (yet).  I'm not sure if it is worth the trouble.  Don't
hesitate to change my mind on this.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[David Gibson]

On Thu, Dec 10, 2009 at 01:57:21PM -0200, Dave Kleikamp wrote:
> powerpc: Add definitions for Debug Registers on BookE Platforms
> 
> From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
> 
> This patch adds additional definitions for BookE Debug Registers
> to the reg_booke.h header file.
> 
> Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>

As with patch 1/3, none of the comments below is anything that
couldn't be fixed up after merging.  So,

Acked-by: David Gibson <dwg@au1.ibm.com>

> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
> Cc: David Gibson <dwg@au1.ibm.com>
> Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
> ---
> 
>  arch/powerpc/include/asm/processor.h |   30 +++++-
>  arch/powerpc/include/asm/reg_booke.h |  176 +++++++++++++++++++++++++++++-----
>  2 files changed, 178 insertions(+), 28 deletions(-)

[snip]
> +	/*
> +	 * The following will contain addresses used by debug applications
> +	 * to help trace and trap on particular address locations.
> +	 * The bits in the Debug Control Registers above help define which
> +	 * of the following registers will contain valid data and/or addresses.
> +	 */
> +	unsigned long	iac1;
> +	unsigned long	iac2;
> +	unsigned long	iac3;
> +	unsigned long	iac4;
> +	unsigned long	dac1;
> +	unsigned long	dac2;
> +	unsigned long	dvc1;
> +	unsigned long	dvc2;

I think you'd make the logic in patch 3 substantially easier, if you
defined these as
	unsigned long iac[4];
	unsigned long dac[2];
	unsigned long dvc[2];
instead of as individual structure members.

[snip]
> +#define DBCR0_USER_DEBUG	(DBCR0_IDM | DBCR0_ICMP | DBCR0_IAC1 | \
> +				 DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4)
> +#define DBCR0_BASE_REG_VALUE	0

These constants are left over from when the interface allowed
more-or-less direct access to the debug regs.  I don't think the
USER_DEBUG constant is used at all any more, and the BASE_REG_VALUE is
just used in the load_default function, and might as well be inline
there.

[snip]
> +
> +#define dbcr_iac_range(task)	((task)->thread.dbcr0)

Hrm, I think the way these macros work to do the 40x vs. BookE
abstration is kind of ugly.  But an unequivocally better way doesn't
immediately occur to me.

-- 
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

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Dave Kleikamp]

On Fri, 2009-12-11 at 11:53 +1100, David Gibson wrote:
> On Thu, Dec 10, 2009 at 01:57:21PM -0200, Dave Kleikamp wrote:
> > powerpc: Add definitions for Debug Registers on BookE Platforms
> > 
> > From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
> > 
> > This patch adds additional definitions for BookE Debug Registers
> > to the reg_booke.h header file.
> > 
> > Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
> > Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> 
> As with patch 1/3, none of the comments below is anything that
> couldn't be fixed up after merging.  So,
> 
> Acked-by: David Gibson <dwg@au1.ibm.com>
> 
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
> > Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
> > Cc: David Gibson <dwg@au1.ibm.com>
> > Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
> > ---
> > 
> >  arch/powerpc/include/asm/processor.h |   30 +++++-
> >  arch/powerpc/include/asm/reg_booke.h |  176 +++++++++++++++++++++++++++++-----
> >  2 files changed, 178 insertions(+), 28 deletions(-)
> 
> [snip]
> > +	/*
> > +	 * The following will contain addresses used by debug applications
> > +	 * to help trace and trap on particular address locations.
> > +	 * The bits in the Debug Control Registers above help define which
> > +	 * of the following registers will contain valid data and/or addresses.
> > +	 */
> > +	unsigned long	iac1;
> > +	unsigned long	iac2;
> > +	unsigned long	iac3;
> > +	unsigned long	iac4;
> > +	unsigned long	dac1;
> > +	unsigned long	dac2;
> > +	unsigned long	dvc1;
> > +	unsigned long	dvc2;
> 
> I think you'd make the logic in patch 3 substantially easier, if you
> defined these as
> 	unsigned long iac[4];
> 	unsigned long dac[2];
> 	unsigned long dvc[2];
> instead of as individual structure members.

I'll give that a look.  You're probably right.

> [snip]
> > +#define DBCR0_USER_DEBUG	(DBCR0_IDM | DBCR0_ICMP | DBCR0_IAC1 | \
> > +				 DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4)
> > +#define DBCR0_BASE_REG_VALUE	0
> 
> These constants are left over from when the interface allowed
> more-or-less direct access to the debug regs.  I don't think the
> USER_DEBUG constant is used at all any more, and the BASE_REG_VALUE is
> just used in the load_default function, and might as well be inline
> there.

Right.

> [snip]
> > +
> > +#define dbcr_iac_range(task)	((task)->thread.dbcr0)
> 
> Hrm, I think the way these macros work to do the 40x vs. BookE
> abstration is kind of ugly.  But an unequivocally better way doesn't
> immediately occur to me.

Without this, the ifdef's were horrendous.  I'm open to renaming this or
redefining it to be more intuitive if anyone has a better idea.
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Dave Kleikamp]

On Fri, 2009-12-11 at 11:53 +1100, David Gibson wrote:
> On Thu, Dec 10, 2009 at 01:57:21PM -0200, Dave Kleikamp wrote:
> > powerpc: Add definitions for Debug Registers on BookE Platforms
> > 
> > From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
> > 
> > This patch adds additional definitions for BookE Debug Registers
> > to the reg_booke.h header file.
> > 
> > Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
> > Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
 
> >  arch/powerpc/include/asm/processor.h |   30 +++++-
> >  arch/powerpc/include/asm/reg_booke.h |  176 +++++++++++++++++++++++++++++-----
> >  2 files changed, 178 insertions(+), 28 deletions(-)
> 
> [snip]
> > +	/*
> > +	 * The following will contain addresses used by debug applications
> > +	 * to help trace and trap on particular address locations.
> > +	 * The bits in the Debug Control Registers above help define which
> > +	 * of the following registers will contain valid data and/or addresses.
> > +	 */
> > +	unsigned long	iac1;
> > +	unsigned long	iac2;
> > +	unsigned long	iac3;
> > +	unsigned long	iac4;
> > +	unsigned long	dac1;
> > +	unsigned long	dac2;
> > +	unsigned long	dvc1;
> > +	unsigned long	dvc2;
> 
> I think you'd make the logic in patch 3 substantially easier, if you
> defined these as
> 	unsigned long iac[4];
> 	unsigned long dac[2];
> 	unsigned long dvc[2];
> instead of as individual structure members.

I've cleaned up the logic a bit without having to change this.  Any
further simplification of the code would also involve abstracting the
#defines further and I'm not sure it would make anything more clear.

> [snip]
> > +
> > +#define dbcr_iac_range(task)	((task)->thread.dbcr0)
> 
> Hrm, I think the way these macros work to do the 40x vs. BookE
> abstration is kind of ugly.  But an unequivocally better way doesn't
> immediately occur to me.

I haven't changed this.  I don't have a better solution.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Kumar Gala]

On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:

> +#define DBCR1_IAC1US	0xC0000000	/* Instr Addr Cmp 1 Sup/User   =
*/
> +#define DBCR1_IAC1ER	0x30000000	/* Instr Addr Cmp 1 Eff/Real */
> +#define DBCR1_IAC1ER_01	0x10000000	/* reserved */
> +#define DBCR1_IAC1ER_10	0x20000000	/* Instr Addr Cmp 1 =
Eff/Real MSR[IS]=3D0 */
> +#define DBCR1_IAC1ER_11	0x30000000	/* Instr Addr Cmp 1 =
Eff/Real MSR[IS]=3D1 */
> +#define DBCR1_IAC2US	0x0C000000	/* Instr Addr Cmp 2 Sup/User   =
*/
> +#define DBCR1_IAC2ER	0x03000000	/* Instr Addr Cmp 2 Eff/Real */
> +#define DBCR1_IAC2ER_01	0x01000000	/* reserved */
> +#define DBCR1_IAC2ER_10	0x02000000	/* Instr Addr Cmp 2 =
Eff/Real MSR[IS]=3D0 */
> +#define DBCR1_IAC2ER_11	0x03000000	/* Instr Addr Cmp 2 =
Eff/Real MSR[IS]=3D1 */
> +#define DBCR1_IAC12M	0x00C00000	/* Instr Addr 1-2 range enable =
*/
> +#define DBCR1_IAC12M_R	0x00400000	/* Instr Addr 1-2 =
reserved state */
> +#define DBCR1_IAC12M_I	0x00800000	/* Instr Addr 1-2 range =
inclusive */
> +#define DBCR1_IAC12M_X	0x00C00000	/* Instr Addr 1-2 range =
eXclusive */
> +#define DBCR1_IAC12A_T	0x00010000	/* Instr Addr 1-2 range =
Toggle */
> +#define DBCR1_IAC3US	0x0000C000	/* Instr Addr Cmp 3 Sup/User   =
*/
> +#define DBCR1_IAC3ER	0x00003000	/* Instr Addr Cmp 3 Eff/Real */
> +#define DBCR1_IAC3ER_01	0x00001000	/* reserved */
> +#define DBCR1_IAC3ER_10	0x00002000	/* Instr Addr Cmp 3 =
Eff/Real MSR[IS]=3D0 */
> +#define DBCR1_IAC3ER_11	0x00003000	/* Instr Addr Cmp 3 =
Eff/Real MSR[IS]=3D1 */
> +#define DBCR1_IAC4US	0x00000C00	/* Instr Addr Cmp 4 Sup/User   =
*/
> +#define DBCR1_IAC4ER	0x00000300	/* Instr Addr Cmp 4 Eff/Real */
> +#define DBCR1_IAC4ER_01	0x00000100	/* Instr Addr Cmp 4 =
Eff/Real MSR[IS]=3D0 */
> +#define DBCR1_IAC4ER_10	0x00000200	/* Instr Addr Cmp 4 =
Eff/Real MSR[IS]=3D0 */
> +#define DBCR1_IAC4ER_11	0x00000300	/* Instr Addr Cmp 4 =
Eff/Real MSR[IS]=3D1 */
> +#define DBCR1_IAC34M	0x000000C0	/* Instr Addr 3-4 range enable =
*/
> +#define DBCR1_IAC34M_R	0x00000040	/* Instr Addr 3-4 =
reserved state */
> +#define DBCR1_IAC34M_I	0x00000080	/* Instr Addr 3-4 range =
inclusive */
> +#define DBCR1_IAC34M_X	0x000000C0	/* Instr Addr 3-4 range =
eXclusive */
> +#define DBCR1_IAC34A_T	0x00000001	/* Instr Addr 3-4 range =
Toggle */
> +
> +#define DBCR1_USER_DEBUG	(DBCR1_IAC12M | DBCR1_IAC34M)
> +#define DBCR1_BASE_REG_VALUE	(DBCR1_IAC1US | DBCR1_IAC1ER_10 | \
> +				 DBCR1_IAC2US | DBCR1_IAC2ER_10 | \
> +				 DBCR1_IAC3US | DBCR1_IAC3ER_10 | \
> +				 DBCR1_IAC4US | DBCR1_IAC4ER_10)

We are we using MSR[IS] IS=3D0, why not just any Eff address?  In the =
future we might have user as IS =3D 1, and kernel as IS =3D 0.

- k=

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[David Gibson]

On Thu, Dec 10, 2009 at 08:41:53PM -0600, Kumar Gala wrote:
[snip]
> > +#define DBCR1_USER_DEBUG	(DBCR1_IAC12M | DBCR1_IAC34M)
> > +#define DBCR1_BASE_REG_VALUE	(DBCR1_IAC1US | DBCR1_IAC1ER_10 | \
> > +				 DBCR1_IAC2US | DBCR1_IAC2ER_10 | \
> > +				 DBCR1_IAC3US | DBCR1_IAC3ER_10 | \
> > +				 DBCR1_IAC4US | DBCR1_IAC4ER_10)
> 
> We are we using MSR[IS] IS=0, why not just any Eff address?  In the
> future we might have user as IS = 1, and kernel as IS = 0.

Since the user can't control that directly, we can update this when
and if we change our use of address spaces.

-- 
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

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[Kumar Gala]

On Dec 10, 2009, at 9:28 PM, David Gibson wrote:

> On Thu, Dec 10, 2009 at 08:41:53PM -0600, Kumar Gala wrote:
> [snip]
>>> +#define DBCR1_USER_DEBUG	(DBCR1_IAC12M | DBCR1_IAC34M)
>>> +#define DBCR1_BASE_REG_VALUE	(DBCR1_IAC1US | DBCR1_IAC1ER_10 =
| \
>>> +				 DBCR1_IAC2US | DBCR1_IAC2ER_10 | \
>>> +				 DBCR1_IAC3US | DBCR1_IAC3ER_10 | \
>>> +				 DBCR1_IAC4US | DBCR1_IAC4ER_10)
>>=20
>> We are we using MSR[IS] IS=3D0, why not just any Eff address?  In the
>> future we might have user as IS =3D 1, and kernel as IS =3D 0.
>=20
> Since the user can't control that directly, we can update this when
> and if we change our use of address spaces.

That's such a subtle issue (easy to forgot about and miss in the =
future).  Why do we need this now?

- k=

Re: [RFC:PATCH 02/03] powerpc: Add definitions for Debug Registers on BookE Platforms

[David Gibson]

On Fri, Dec 11, 2009 at 08:35:35AM -0600, Kumar Gala wrote:
> 
> On Dec 10, 2009, at 9:28 PM, David Gibson wrote:
> 
> > On Thu, Dec 10, 2009 at 08:41:53PM -0600, Kumar Gala wrote:
> > [snip]
> >>> +#define DBCR1_USER_DEBUG	(DBCR1_IAC12M | DBCR1_IAC34M)
> >>> +#define DBCR1_BASE_REG_VALUE	(DBCR1_IAC1US | DBCR1_IAC1ER_10 | \
> >>> +				 DBCR1_IAC2US | DBCR1_IAC2ER_10 | \
> >>> +				 DBCR1_IAC3US | DBCR1_IAC3ER_10 | \
> >>> +				 DBCR1_IAC4US | DBCR1_IAC4ER_10)
> >> 
> >> We are we using MSR[IS] IS=0, why not just any Eff address?  In the
> >> future we might have user as IS = 1, and kernel as IS = 0.
> > 
> > Since the user can't control that directly, we can update this when
> > and if we change our use of address spaces.
> 
> That's such a subtle issue (easy to forgot about and miss in the
> future).  Why do we need this now?

Um.. I guess we don't really.  I was thinking of this default value a)
in the context of the old explicit register setting interface, where
we wanted to force what userspace could and could not set here and b)
half in the context of a system with real mode, in which we certainly
wouldn't want userspace to be able to set real address breakpoints.

I don't think it matters that much.  And, if we use AS1 for userspace
in the future, we arguably still want to change this to make it _01,
so that userspace can't set breakpoints in the kernel AS (although I
guess as long as we force the PR state in which the breakpoint
triggers, it doesn't really matter).

-- 
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

[RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Dave Kleikamp]

powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

From: Torez Smith <lnxtorez@linux.vnet.ibm.com>

This patch defines context switch and trap related functionality
for BookE specific Debug Registers. It adds support to ptrace()
for setting and getting BookE related Debug Registers

Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
Cc: David Gibson <dwg@au1.ibm.com>
Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
---

 arch/powerpc/include/asm/system.h |    2 
 arch/powerpc/kernel/process.c     |  109 ++++++++-
 arch/powerpc/kernel/ptrace.c      |  435 ++++++++++++++++++++++++++++++++++---
 arch/powerpc/kernel/signal.c      |    6 -
 arch/powerpc/kernel/signal_32.c   |    8 +
 arch/powerpc/kernel/traps.c       |   86 ++++++-
 6 files changed, 564 insertions(+), 82 deletions(-)


diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h
index bb8e006..474bf23 100644
--- a/arch/powerpc/include/asm/system.h
+++ b/arch/powerpc/include/asm/system.h
@@ -114,6 +114,8 @@ static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
 extern int set_dabr(unsigned long dabr);
 extern void do_dabr(struct pt_regs *regs, unsigned long address,
 		    unsigned long error_code);
+extern void do_send_trap(struct pt_regs *regs, unsigned long address,
+			 unsigned long error_code, int signal_code, int errno);
 extern void print_backtrace(unsigned long *);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index c930ac3..a0dbb09 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -245,6 +245,24 @@ void discard_lazy_cpu_state(void)
 }
 #endif /* CONFIG_SMP */
 
+void do_send_trap(struct pt_regs *regs, unsigned long address,
+		  unsigned long error_code, int signal_code, int errno)
+{
+	siginfo_t info;
+
+	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
+			11, SIGSEGV) == NOTIFY_STOP)
+		return;
+
+	/* Deliver the signal to userspace */
+	info.si_signo = SIGTRAP;
+	info.si_errno = errno;
+	info.si_code = signal_code;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGTRAP, &info, current);
+}
+
+#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
 void do_dabr(struct pt_regs *regs, unsigned long address,
 		    unsigned long error_code)
 {
@@ -257,12 +275,6 @@ void do_dabr(struct pt_regs *regs, unsigned long address,
 	if (debugger_dabr_match(regs))
 		return;
 
-	/* Clear the DAC and struct entries.  One shot trigger */
-#if defined(CONFIG_BOOKE)
-	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
-							| DBCR0_IDM));
-#endif
-
 	/* Clear the DABR */
 	set_dabr(0);
 
@@ -273,9 +285,71 @@ void do_dabr(struct pt_regs *regs, unsigned long address,
 	info.si_addr = (void __user *)address;
 	force_sig_info(SIGTRAP, &info, current);
 }
+#endif
 
 static DEFINE_PER_CPU(unsigned long, current_dabr);
 
+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
+/*
+ * Set the debug registers back to their default "safe" values.
+ */
+static void set_debug_reg_defaults(struct thread_struct *thread)
+{
+	thread->iac1 = thread->iac2 = thread->iac3 = thread->iac4 = 0;
+	thread->dac1 = thread->dac2 = 0;
+	thread->dvc1 = thread->dvc2 = 0;
+	/*
+	 * reset the DBCR0, DBCR1 and DBCR2 registers. All bits with
+	 * the exception of the reserved bits should be cleared out
+	 * and set to 0.
+	 *
+	 * For the DBCR0 register, the reserved bits are bits 17:30.
+	 * Reserved bits for DBCR1 are bits 10:14 and bits 26:30.
+	 * And, bits 10:11 for DBCR2.
+	 */
+	thread->dbcr0 = DBCR0_BASE_REG_VALUE;
+	/*
+	 * First clear all "non reserved" bits from DBCR1 then initialize reg
+	 * to force User/Supervisor bits to b11 (user-only MSR[PR]=1) and
+	 * Effective/Real * bits to b10 (trap only if IS==0)
+	 */
+	thread->dbcr1 = DBCR1_BASE_REG_VALUE;
+	/*
+	 * Force Data Address Compare User/Supervisor bits to be User-only
+	 * (0b11 MSR[PR]=1) and set all other bits in DBCR2 register to be 0.
+	 * This sets the Data Address Compare Effective/Real bits to be 0b00
+	 * (Effective, MSR[DS]=don't care).
+	 */
+	thread->dbcr2 = DBCR2_BASE_REG_VALUE;
+}
+
+static void prime_debug_regs(struct thread_struct *thread)
+{
+	mtspr(SPRN_IAC1, thread->iac1);
+	mtspr(SPRN_IAC2, thread->iac2);
+	mtspr(SPRN_IAC3, thread->iac3);
+	mtspr(SPRN_IAC4, thread->iac4);
+	mtspr(SPRN_DAC1, thread->dac1);
+	mtspr(SPRN_DAC2, thread->dac2);
+	mtspr(SPRN_DVC1, thread->dvc1);
+	mtspr(SPRN_DVC2, thread->dvc2);
+	mtspr(SPRN_DBCR0, thread->dbcr0);
+	mtspr(SPRN_DBCR1, thread->dbcr1);
+	mtspr(SPRN_DBCR2, thread->dbcr2);
+}
+/*
+ * Unless neither the old or new thread are making use of the
+ * debug registers, set the debug registers from the values
+ * stored in the new thread.
+ */
+static void switch_booke_debug_regs(struct thread_struct *new_thread)
+{
+	if ((current->thread.dbcr0 & DBCR0_IDM)
+		|| (new_thread->dbcr0 & DBCR0_IDM))
+			prime_debug_regs(new_thread);
+}
+#endif
+
 int set_dabr(unsigned long dabr)
 {
 	__get_cpu_var(current_dabr) = dabr;
@@ -284,7 +358,7 @@ int set_dabr(unsigned long dabr)
 		return ppc_md.set_dabr(dabr);
 
 	/* XXX should we have a CPU_FTR_HAS_DABR ? */
-#if defined(CONFIG_BOOKE)
+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
 	mtspr(SPRN_DAC1, dabr);
 #elif defined(CONFIG_PPC_BOOK3S)
 	mtspr(SPRN_DABR, dabr);
@@ -371,10 +445,8 @@ struct task_struct *__switch_to(struct task_struct *prev,
 
 #endif /* CONFIG_SMP */
 
-#if defined(CONFIG_BOOKE)
-	/* If new thread DAC (HW breakpoint) is the same then leave it */
-	if (new->thread.dabr)
-		set_dabr(new->thread.dabr);
+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
+	switch_booke_debug_regs(&new->thread);
 #else
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
@@ -514,7 +586,7 @@ void show_regs(struct pt_regs * regs)
 	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
 	trap = TRAP(regs);
 	if (trap == 0x300 || trap == 0x600)
-#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
 		printk("DEAR: "REG", ESR: "REG"\n", regs->dar, regs->dsisr);
 #else
 		printk("DAR: "REG", DSISR: "REG"\n", regs->dar, regs->dsisr);
@@ -568,14 +640,19 @@ void flush_thread(void)
 
 	discard_lazy_cpu_state();
 
+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
+	/*
+	 * flush_thread() is called on exec() to reset the
+	 * thread's status. Set all debug regs back to their
+	 * default values....Torez
+	 */
+	set_debug_reg_defaults(&current->thread);
+#else
 	if (current->thread.dabr) {
 		current->thread.dabr = 0;
 		set_dabr(0);
-
-#if defined(CONFIG_BOOKE)
-		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
-#endif
 	}
+#endif
 }
 
 void
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 6be2ce0..6710a69 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -737,17 +737,25 @@ void user_disable_single_step(struct task_struct *task)
 	struct pt_regs *regs = task->thread.regs;
 
 	if (regs != NULL) {
-#if defined(CONFIG_BOOKE)
-		/* If DAC don't clear DBCRO_IDM or MSR_DE */
-		if (task->thread.dabr)
-			task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_BT);
-		else {
-			task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_BT | DBCR0_IDM);
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		/*
+		 * The logic to disable single stepping should be as
+		 * simple as turning off the Instruction Complete flag.
+		 * And, after doing so, if all debug flags are off, turn
+		 * off DBCR0(IDM) and MSR(DE) .... Torez
+		 */
+		task->thread.dbcr0 &= ~DBCR0_IC;
+		/*
+		 * Test to see if any of the DBCR_ACTIVE_EVENTS bits are set.
+		 */
+		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
+					task->thread.dbcr1)) {
+			/*
+			 * All debug events were off.....
+			 */
+			task->thread.dbcr0 &= ~DBCR0_IDM;
 			regs->msr &= ~MSR_DE;
 		}
-#elif defined(CONFIG_40x)
-		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_BT | DBCR0_IDM);
-		regs->msr &= ~MSR_DE;
 #else
 		regs->msr &= ~(MSR_SE | MSR_BE);
 #endif
@@ -769,8 +777,7 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 	if ((data & ~0x7UL) >= TASK_SIZE)
 		return -EIO;
 
-#ifndef CONFIG_BOOKE
-
+#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
 	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
 	 *  It was assumed, on previous implementations, that 3 bits were
 	 *  passed together with the data address, fitting the design of the
@@ -789,21 +796,22 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 
 	/* Move contents to the DABR register */
 	task->thread.dabr = data;
-
-#endif
-#if defined(CONFIG_BOOKE)
-
+#else
 	/* As described above, it was assumed 3 bits were passed with the data
 	 *  address, but we will assume only the mode bits will be passed
 	 *  as to not cause alignment restrictions for DAC-based processors.
 	 */
 
 	/* DAC's hold the whole address without any mode flags */
-	task->thread.dabr = data & ~0x3UL;
-
-	if (task->thread.dabr == 0) {
-		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
-		task->thread.regs->msr &= ~MSR_DE;
+	task->thread.dac1 = data & ~0x3UL;
+
+	if (task->thread.dac1 == 0) {
+		dbcr_dac(task) &= ~(DBCR_DAC1R | DBCR_DAC1W);
+		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
+					task->thread.dbcr1)) {
+			task->thread.regs->msr &= ~MSR_DE;
+			task->thread.dbcr0 &= ~DBCR0_IDM;
+		}
 		return 0;
 	}
 
@@ -814,15 +822,15 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 
 	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
 	   register */
-	task->thread.dbcr0 = DBCR0_IDM;
+	task->thread.dbcr0 |= DBCR0_IDM;
 
 	/* Check for write and read flags and set DBCR0
 	   accordingly */
+	dbcr_dac(task) &= ~(DBCR_DAC1R|DBCR_DAC1W);
 	if (data & 0x1UL)
-		task->thread.dbcr0 |= DBSR_DAC1R;
+		dbcr_dac(task) |= DBCR_DAC1R;
 	if (data & 0x2UL)
-		task->thread.dbcr0 |= DBSR_DAC1W;
-
+		dbcr_dac(task) |= DBCR_DAC1W;
 	task->thread.regs->msr |= MSR_DE;
 #endif
 	return 0;
@@ -839,11 +847,324 @@ void ptrace_disable(struct task_struct *child)
 	user_disable_single_step(child);
 }
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+static long set_intruction_bp(struct task_struct *child,
+			      struct ppc_hw_breakpoint *bp_info)
+{
+	int slots_needed;
+	int slot;
+	int free_slot = 0;
+
+	/*
+	 * Find an avalailable slot for the breakpoint.
+	 * If possible, reserve consecutive slots, 1 & 2, for a range
+	 * breakpoint.  (Can this be done simpler?)
+	 */
+	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
+		slots_needed = 1;
+	else
+		slots_needed = 2;
+
+	if ((child->thread.dbcr0 & DBCR0_IAC1) == 0) {
+		if (slots_needed == 1) {
+			if (child->thread.dbcr0 & DBCR0_IAC2) {
+				slot = 1;
+				goto found;
+			}
+			/* Try to save slots 1 & 2 for range */
+			free_slot = 1;
+		} else
+			if ((child->thread.dbcr0 & DBCR0_IAC2) == 0) {
+				slot = 1;
+				goto found;
+			}
+	} else if ((slots_needed == 1) &&
+		   ((child->thread.dbcr0 & DBCR0_IAC2) == 0)) {
+		slot = 2;
+		goto found;
+	}
+	if ((child->thread.dbcr0 & DBCR0_IAC3) == 0) {
+		if (slots_needed == 1) {
+			slot = 3;
+			goto found;
+		}
+		if ((child->thread.dbcr0 & DBCR0_IAC4) == 0) {
+			slot = 3;
+			goto found;
+		}
+		return -ENOSPC;
+	} else if (slots_needed == 2)
+		return -ENOSPC;
+	if ((child->thread.dbcr0 & DBCR0_IAC4) == 0) {
+		slot = 4;
+	} else if (free_slot)
+		slot = free_slot;
+	else
+		return -ENOSPC;
+found:
+	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT) {
+		switch (slot) {
+			case 1:
+				child->thread.iac1 = bp_info->addr;
+				child->thread.dbcr0 |= DBCR0_IAC1;
+				break;
+			case 2:
+				child->thread.iac2 = bp_info->addr;
+				child->thread.dbcr0 |= DBCR0_IAC2;
+				break;
+			case 3:
+				child->thread.iac3 = bp_info->addr;
+				child->thread.dbcr0 |= DBCR0_IAC3;
+				break;
+			case 4:
+				child->thread.iac4 = bp_info->addr;
+				child->thread.dbcr0 |= DBCR0_IAC4;
+				break;
+		}
+	} else if (slot == 1) {
+		child->thread.iac1 = bp_info->addr;
+		child->thread.iac2 = bp_info->addr2;
+		child->thread.dbcr0 |= (DBCR0_IAC1 | DBCR0_IAC2);
+		if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
+			dbcr_iac_range(child) |= DBCR_IAC12M_X;
+		else
+			dbcr_iac_range(child) |= DBCR_IAC12M_I;
+	} else { /* slot == 3 */
+		child->thread.iac3 = bp_info->addr;
+		child->thread.iac4 = bp_info->addr2;
+		child->thread.dbcr0 |= (DBCR0_IAC3 | DBCR0_IAC4);
+		if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
+			dbcr_iac_range(child) |= DBCR_IAC34M_X;
+		else
+			dbcr_iac_range(child) |= DBCR_IAC34M_I;
+	}
+	child->thread.dbcr0 |= DBCR0_IDM;
+	child->thread.regs->msr |= MSR_DE;
+
+	return slot;
+}
+
+static int del_instruction_bp(struct task_struct *child, int slot)
+{
+	switch (slot) {
+	case 1:
+		if (dbcr_iac_range(child) & DBCR_IAC12M) {
+			/* address range - clear slots 1 & 2 */
+			child->thread.iac2 = 0;
+			child->thread.dbcr0 &= ~DBCR0_IAC2;
+			dbcr_iac_range(child) &= ~DBCR_IAC12M;
+		}
+		child->thread.iac1 = 0;
+		child->thread.dbcr0 &= ~DBCR0_IAC1;
+		break;
+	case 2:
+		if (dbcr_iac_range(child) & DBCR_IAC12M)
+			/* used in a range */
+			return -EINVAL;
+		child->thread.iac2 = 0;
+		child->thread.dbcr0 &= ~DBCR0_IAC2;
+		break;
+	case 3:
+		if (dbcr_iac_range(child) & DBCR_IAC34M) {
+			/* address range - clear slots 3 & 4 */
+			child->thread.iac4 = 0;
+			child->thread.dbcr0 &= ~DBCR0_IAC4;
+			dbcr_iac_range(child) &= ~DBCR_IAC34M;
+		}
+		child->thread.iac3 = 0;
+		child->thread.dbcr0 &= ~DBCR0_IAC3;
+		break;
+	case 4:
+		if (dbcr_iac_range(child) & DBCR_IAC34M)
+			/* Used in a range */
+			return -EINVAL;
+		child->thread.iac4 = 0;
+		child->thread.dbcr0 &= ~DBCR0_IAC4;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int set_dac(struct task_struct *child, struct ppc_hw_breakpoint *bp_info)
+{
+	int byte_enable =
+		(bp_info->condition_mode >> PPC_BREAKPOINT_CONDITION_BE_SHIFT)
+		& 0xf;
+	int condition_mode =
+		bp_info->condition_mode & PPC_BREAKPOINT_CONDITION_AND_OR;
+	int slot;
+
+	if (byte_enable && (condition_mode == 0))
+		return -EINVAL;
+
+	if (bp_info->addr >= TASK_SIZE)
+		return -EIO;
+
+	if ((dbcr_dac(child) & (DBCR_DAC1R | DBCR_DAC1W)) == 0) {
+		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
+			dbcr_dac(child) |= DBCR_DAC1R;
+		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
+			dbcr_dac(child) |= DBCR_DAC1W;
+		child->thread.dac1 = (unsigned long)bp_info->addr;
+#ifdef CONFIG_BOOKE
+		if (byte_enable) {
+			child->thread.dvc1 =
+				(unsigned long)bp_info->condition_value;
+			child->thread.dbcr2 |=
+				((byte_enable << DBCR2_DVC1BE_SHIFT) |
+				 (condition_mode << DBCR2_DVC1M_SHIFT));
+		}
+#endif
+		slot = 1;
+	} else if ((dbcr_dac(child) & (DBCR_DAC2R | DBCR_DAC2W)) == 0) {
+		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
+			dbcr_dac(child) |= DBCR_DAC2R;
+		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
+			dbcr_dac(child) |= DBCR_DAC2W;
+		child->thread.dac2 = (unsigned long)bp_info->addr;
+#ifdef CONFIG_BOOKE
+		if (byte_enable) {
+			child->thread.dvc2 =
+				(unsigned long)bp_info->condition_value;
+			child->thread.dbcr2 |=
+				((byte_enable << DBCR2_DVC2BE_SHIFT) |
+				 (condition_mode << DBCR2_DVC2M_SHIFT));
+		}
+#endif
+		slot = 2;
+	} else
+		return -ENOSPC;
+	child->thread.dbcr0 |= DBCR0_IDM;
+	child->thread.regs->msr |= MSR_DE;
+
+	return slot + 4;
+}
+
+static int del_dac(struct task_struct *child, int slot)
+{
+	if (slot == 1) {
+#ifdef CONFIG_BOOKE
+		if (child->thread.dbcr2 & DBCR2_DAC12MODE) {
+			child->thread.dac1 = 0;
+			child->thread.dac2 = 0;
+			child->thread.dbcr0 &= ~(DBCR0_DAC1R | DBCR0_DAC1W |
+						 DBCR0_DAC2R | DBCR0_DAC2W);
+			child->thread.dbcr2 &= ~DBCR2_DAC12MODE;
+			return 0;
+		}
+		child->thread.dbcr2 &= ~(DBCR2_DVC1M | DBCR2_DVC1BE);
+		child->thread.dvc1 = 0;
+#endif
+		child->thread.dac1 = 0;
+		dbcr_dac(child) &= ~(DBCR_DAC1R | DBCR_DAC1W);
+	} else if (slot == 2) {
+#ifdef CONFIG_BOOKE
+		if (child->thread.dbcr2 & DBCR2_DAC12MODE)
+			/* Part of a range */
+			return -EINVAL;
+		child->thread.dbcr2 &= ~(DBCR2_DVC2M | DBCR2_DVC2BE);
+		child->thread.dvc2 = 0;
+#endif
+		child->thread.dac2 = 0;
+		dbcr_dac(child) &= ~(DBCR_DAC2R | DBCR_DAC2W);
+	} else
+		return -EINVAL;
+
+	return 0;
+}
+#endif /* CONFIG_40x || CONFIG_BOOKE */
+
+#ifdef CONFIG_BOOKE
+static int set_dac_range(struct task_struct *child,
+			 struct ppc_hw_breakpoint *bp_info)
+{
+	int mode = bp_info->addr_mode & PPC_BREAKPOINT_MODE_MASK;
+
+	/* We don't allow range watchpoints to be used with DVC */
+	if (bp_info->condition_mode && PPC_BREAKPOINT_CONDITION_BE_ALL)
+		return -EINVAL;
+
+	if (bp_info->addr >= TASK_SIZE)
+		return -EIO;
+
+	if (child->thread.dbcr0 &
+	    (DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W))
+		return -ENOSPC;
+
+	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
+		child->thread.dbcr0 |= (DBCR0_DAC1R | DBCR0_DAC2R | DBCR0_IDM);
+	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
+		child->thread.dbcr0 |= (DBCR0_DAC1W | DBCR0_DAC2W | DBCR0_IDM);
+	child->thread.dac1 = bp_info->addr;
+	child->thread.dac2 = bp_info->addr2;
+	if (mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+		child->thread.dbcr2  |= DBCR2_DAC12R;
+	else if (mode == PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
+		child->thread.dbcr2  |= DBCR2_DAC12RX;
+	else	/* PPC_BREAKPOINT_MODE_MASK */
+		child->thread.dbcr2  |= DBCR2_DAC12MASK;
+	child->thread.regs->msr |= MSR_DE;
+
+	return 5;
+}
+#endif /* CONFIG_BOOKE */
+
 static long ppc_set_hwdebug(struct task_struct *child,
 		     struct ppc_hw_breakpoint *bp_info)
 {
+	if (bp_info->version != 1)
+		return -ENOTSUPP;
+
+#ifdef CONFIG_BOOKE
+	/*
+	 * Check for invalid flags and combinations
+	 */
+	if ((bp_info->trigger_type == 0) ||
+	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
+				       PPC_BREAKPOINT_TRIGGER_RW)) ||
+	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
+	    (bp_info->condition_mode &
+	     ~(PPC_BREAKPOINT_CONDITION_AND_OR |
+	       PPC_BREAKPOINT_CONDITION_BE_ALL)))
+		return -EINVAL;
+
+	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
+		if (bp_info->trigger_type != PPC_BREAKPOINT_TRIGGER_EXECUTE)
+			/* At least another bit was set */
+			return -EINVAL;
+		return set_intruction_bp(child, bp_info);
+	}
+
+	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
+		return set_dac(child, bp_info);
+
+	return set_dac_range(child, bp_info);
+#elif defined(CONFIG_40x)
+	/*
+	 * Check for invalid flags and combinations
+	 */
+	if ((bp_info->trigger_type == 0) ||
+	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
+				       PPC_BREAKPOINT_TRIGGER_RW)) ||
+	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
+	    (bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE))
+		return -EINVAL;
+
+	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
+		if (bp_info->trigger_type != PPC_BREAKPOINT_TRIGGER_EXECUTE)
+			/* At least another bit was set */
+			return -EINVAL;
+		return set_intruction_bp(child, bp_info);
+	}
+	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
+		return -EINVAL;
+
+	return set_dac(child, bp_info);
+#else
 	/*
-	 * We currently support one data breakpoint
+	 * We only support one data breakpoint
 	 */
 	if (((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0) ||
 	    ((bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0) ||
@@ -859,28 +1180,35 @@ static long ppc_set_hwdebug(struct task_struct *child,
 		return -EIO;
 
 	child->thread.dabr = (unsigned long)bp_info->addr;
-#ifdef CONFIG_BOOKE
-	child->thread.dbcr0 = DBCR0_IDM;
-	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
-		child->thread.dbcr0 |= DBSR_DAC1R;
-	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
-		child->thread.dbcr0 |= DBSR_DAC1W;
-	child->thread.regs->msr |= MSR_DE;
-#endif
 	return 1;
+#endif
 }
 
 static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
 {
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	int rc;
+
+	if (data <= 4)
+		rc = del_instruction_bp(child, (int)data);
+	else
+		rc = del_dac(child, (int)data - 4);
+
+	if (!rc) {
+		if (!DBCR_ACTIVE_EVENTS(child->thread.dbcr0,
+					child->thread.dbcr1)) {
+			child->thread.dbcr0 &= ~DBCR0_IDM;
+			child->thread.regs->msr &= ~MSR_DE;
+		}
+	}
+	return rc;
+#else
 	if ((data != 1) || (child->thread.dabr == 0))
 		return -EINVAL;
 
 	child->thread.dabr = 0;
-#ifdef CONFIG_BOOKE
-	child->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
-	child->thread.regs->msr &= ~MSR_DE;
-#endif
 	return 0;
+#endif
 }
 
 /*
@@ -980,16 +1308,36 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		struct ppc_debug_info dbginfo;
 
 		dbginfo.version = 1;
+#ifdef CONFIG_BOOKE
+		dbginfo.num_instruction_bps = 4;
+		dbginfo.num_data_bps = 2;
+		dbginfo.num_condition_regs = 2;
+		dbginfo.data_bp_alignment = 0;
+		dbginfo.sizeof_condition = 4;
+		dbginfo.features = PPC_DEBUG_FEATURE_INSN_BP_RANGE |
+				   PPC_DEBUG_FEATURE_INSN_BP_MASK |
+				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
+				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
+#elif defined(CONFIG_40x)
+		/*
+		 * I don't know how the DVCs work on 40x, I'm not going
+		 * to support it now. -- Shaggy
+		 */
+		dbginfo.num_instruction_bps = 4;
+		dbginfo.num_data_bps = 2;
+		dbginfo.num_condition_regs = 0;
+		dbginfo.data_bp_alignment = 0;
+		dbginfo.sizeof_condition = 0;
+		dbginfo.features = PPC_DEBUG_FEATURE_INSN_BP_RANGE |
+				   PPC_DEBUG_FEATURE_INSN_BP_MASK;
+#else
 		dbginfo.num_instruction_bps = 0;
 		dbginfo.num_data_bps = 1;
 		dbginfo.num_condition_regs = 0;
-#ifdef CONFIG_PPC64
 		dbginfo.data_bp_alignment = 8;
-#else
-		dbginfo.data_bp_alignment = 0;
-#endif
 		dbginfo.sizeof_condition = 0;
 		dbginfo.features = 0;
+#endif
 
 		if (!access_ok(VERIFY_WRITE, data,
 			       sizeof(struct ppc_debug_info)))
@@ -1025,8 +1373,13 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		/* We only support one DABR and no IABRS at the moment */
 		if (addr > 0)
 			break;
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		ret = put_user(child->thread.dac1,
+			       (unsigned long __user *)data);
+#else
 		ret = put_user(child->thread.dabr,
 			       (unsigned long __user *)data);
+#endif
 		break;
 	}
 
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 00b5078..94df779 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -140,17 +140,15 @@ static int do_signal_pending(sigset_t *oldset, struct pt_regs *regs)
 		return 0;               /* no signals delivered */
 	}
 
+#if !(defined(CONFIG_BOOKE) || defined(CONFIG_40x))
         /*
 	 * Reenable the DABR before delivering the signal to
 	 * 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_BOOKE)
-		mtspr(SPRN_DBCR0, current->thread.dbcr0);
 #endif
-	}
 
 	if (is32) {
         	if (ka.sa.sa_flags & SA_SIGINFO)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index d670429..6cc6e81 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -1092,8 +1092,12 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
 				new_msr |= MSR_DE;
 				new_dbcr0 |= (DBCR0_IDM | DBCR0_IC);
 			} else {
-				new_msr &= ~MSR_DE;
-				new_dbcr0 &= ~(DBCR0_IDM | DBCR0_IC);
+				new_dbcr0 &= ~DBCR0_IC;
+				if (!DBCR_ACTIVE_EVENTS(new_dbcr0,
+						current->thread.dbcr1)) {
+					new_msr &= ~MSR_DE;
+					new_dbcr0 &= ~DBCR0_IDM;
+				}
 			}
 #else
 			if (op.dbg_value)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index a81c743..d919571 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1016,9 +1016,63 @@ void SoftwareEmulation(struct pt_regs *regs)
 #endif /* CONFIG_8xx */
 
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+static void handle_debug(struct pt_regs *regs, unsigned long debug_status)
+{
+	int changed = 0;
+	/*
+	 * Determine the cause of the debug event, clear the
+	 * event flags and send a trap to the handler. Torez
+	 */
+	if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		dbcr_dac(current) &= ~(DBCR_DAC1R | DBCR_DAC1W);
+		do_send_trap(regs, mfspr(SPRN_DAC1), debug_status, TRAP_HWBKPT,
+			     5);
+		changed |= 0x01;
+	}  else if (debug_status & (DBSR_DAC2R | DBSR_DAC2W)) {
+		dbcr_dac(current) &= ~(DBCR_DAC2R | DBCR_DAC2W);
+		do_send_trap(regs, mfspr(SPRN_DAC2), debug_status, TRAP_HWBKPT,
+			     6);
+		changed |= 0x01;
+	}  else if (debug_status & DBSR_IAC1) {
+		current->thread.dbcr0 &= ~DBCR0_IAC1;
+		do_send_trap(regs, mfspr(SPRN_IAC1), debug_status, TRAP_HWBKPT,
+			     1);
+		changed |= 0x01;
+	}  else if (debug_status & DBSR_IAC2) {
+		current->thread.dbcr0 &= ~DBCR0_IAC2;
+		do_send_trap(regs, mfspr(SPRN_IAC2), debug_status, TRAP_HWBKPT,
+			     2);
+		changed |= 0x01;
+	}  else if (debug_status & DBSR_IAC3) {
+		current->thread.dbcr0 &= ~DBCR0_IAC3;
+		do_send_trap(regs, mfspr(SPRN_IAC3), debug_status, TRAP_HWBKPT,
+			     3);
+		changed |= 0x01;
+	}  else if (debug_status & DBSR_IAC4) {
+		current->thread.dbcr0 &= ~DBCR0_IAC4;
+		do_send_trap(regs, mfspr(SPRN_IAC4), debug_status, TRAP_HWBKPT,
+			     4);
+		changed |= 0x01;
+	}
+	/*
+	 * At the point this routine was called, the MSR(DE) was turned off.
+	 * Check all other debug flags and see if that bit needs to be turned
+	 * back on or not.
+	 */
+	if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0, current->thread.dbcr1))
+		regs->msr |= MSR_DE;
+	else
+		/* Make sure the IDM flag is off */
+		current->thread.dbcr0 &= ~DBCR0_IDM;
+
+	if (changed & 0x01)
+		mtspr(SPRN_DBCR0, current->thread.dbcr0);
+}
 
 void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 {
+	current->thread.dbsr = debug_status;
+
 	/* Hack alert: On BookE, Branch Taken stops on the branch itself, while
 	 * on server, it stops on the target of the branch. In order to simulate
 	 * the server behaviour, we thus restart right away with a single step
@@ -1062,27 +1116,21 @@ void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 		if (debugger_sstep(regs))
 			return;
 
-		if (user_mode(regs))
-			current->thread.dbcr0 &= ~(DBCR0_IC);
-
-		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
-	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
-		regs->msr &= ~MSR_DE;
-
 		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));
+			current->thread.dbcr0 &= ~DBCR0_IC;
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+			if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0,
+					       current->thread.dbcr1))
+				regs->msr |= MSR_DE;
+			else
+				/* Make sure the IDM bit is off */
+				current->thread.dbcr0 &= ~DBCR0_IDM;
+#endif
 		}
-		/* Setup and send the trap to the handler */
-		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
-	}
+
+		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
+	} else
+		handle_debug(regs, debug_status);
 }
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 

-- 
Dave Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Josh Boyer]

On Thu, Dec 10, 2009 at 01:57:27PM -0200, Dave Kleikamp wrote:
>powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace
>
>From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
>
>This patch defines context switch and trap related functionality
>for BookE specific Debug Registers. It adds support to ptrace()
>for setting and getting BookE related Debug Registers
>
>Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
>Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
>Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
>Cc: David Gibson <dwg@au1.ibm.com>
>Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
>---
>
> arch/powerpc/include/asm/system.h |    2 
> arch/powerpc/kernel/process.c     |  109 ++++++++-
> arch/powerpc/kernel/ptrace.c      |  435 ++++++++++++++++++++++++++++++++++---
> arch/powerpc/kernel/signal.c      |    6 -
> arch/powerpc/kernel/signal_32.c   |    8 +
> arch/powerpc/kernel/traps.c       |   86 ++++++-
> 6 files changed, 564 insertions(+), 82 deletions(-)
>
>
>diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h
>index bb8e006..474bf23 100644
>--- a/arch/powerpc/include/asm/system.h
>+++ b/arch/powerpc/include/asm/system.h
>@@ -114,6 +114,8 @@ static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
> extern int set_dabr(unsigned long dabr);
> extern void do_dabr(struct pt_regs *regs, unsigned long address,
> 		    unsigned long error_code);
>+extern void do_send_trap(struct pt_regs *regs, unsigned long address,
>+			 unsigned long error_code, int signal_code, int errno);
> extern void print_backtrace(unsigned long *);
> extern void show_regs(struct pt_regs * regs);
> extern void flush_instruction_cache(void);
>diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>index c930ac3..a0dbb09 100644
>--- a/arch/powerpc/kernel/process.c
>+++ b/arch/powerpc/kernel/process.c
>@@ -245,6 +245,24 @@ void discard_lazy_cpu_state(void)
> }
> #endif /* CONFIG_SMP */
>
>+void do_send_trap(struct pt_regs *regs, unsigned long address,
>+		  unsigned long error_code, int signal_code, int errno)
>+{
>+	siginfo_t info;
>+
>+	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
>+			11, SIGSEGV) == NOTIFY_STOP)
>+		return;
>+
>+	/* Deliver the signal to userspace */
>+	info.si_signo = SIGTRAP;
>+	info.si_errno = errno;
>+	info.si_code = signal_code;
>+	info.si_addr = (void __user *)address;
>+	force_sig_info(SIGTRAP, &info, current);
>+}
>+
>+#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
> void do_dabr(struct pt_regs *regs, unsigned long address,
> 		    unsigned long error_code)
> {
>@@ -257,12 +275,6 @@ void do_dabr(struct pt_regs *regs, unsigned long address,
> 	if (debugger_dabr_match(regs))
> 		return;
>
>-	/* Clear the DAC and struct entries.  One shot trigger */
>-#if defined(CONFIG_BOOKE)
>-	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
>-							| DBCR0_IDM));
>-#endif
>-
> 	/* Clear the DABR */
> 	set_dabr(0);
>
>@@ -273,9 +285,71 @@ void do_dabr(struct pt_regs *regs, unsigned long address,
> 	info.si_addr = (void __user *)address;
> 	force_sig_info(SIGTRAP, &info, current);
> }
>+#endif
>
> static DEFINE_PER_CPU(unsigned long, current_dabr);
>
>+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
>+/*
>+ * Set the debug registers back to their default "safe" values.
>+ */
>+static void set_debug_reg_defaults(struct thread_struct *thread)
>+{
>+	thread->iac1 = thread->iac2 = thread->iac3 = thread->iac4 = 0;
>+	thread->dac1 = thread->dac2 = 0;
>+	thread->dvc1 = thread->dvc2 = 0;
>+	/*
>+	 * reset the DBCR0, DBCR1 and DBCR2 registers. All bits with
>+	 * the exception of the reserved bits should be cleared out
>+	 * and set to 0.
>+	 *
>+	 * For the DBCR0 register, the reserved bits are bits 17:30.
>+	 * Reserved bits for DBCR1 are bits 10:14 and bits 26:30.
>+	 * And, bits 10:11 for DBCR2.
>+	 */
>+	thread->dbcr0 = DBCR0_BASE_REG_VALUE;
>+	/*
>+	 * First clear all "non reserved" bits from DBCR1 then initialize reg
>+	 * to force User/Supervisor bits to b11 (user-only MSR[PR]=1) and
>+	 * Effective/Real * bits to b10 (trap only if IS==0)
>+	 */
>+	thread->dbcr1 = DBCR1_BASE_REG_VALUE;
>+	/*
>+	 * Force Data Address Compare User/Supervisor bits to be User-only
>+	 * (0b11 MSR[PR]=1) and set all other bits in DBCR2 register to be 0.
>+	 * This sets the Data Address Compare Effective/Real bits to be 0b00
>+	 * (Effective, MSR[DS]=don't care).
>+	 */
>+	thread->dbcr2 = DBCR2_BASE_REG_VALUE;
>+}
>+
>+static void prime_debug_regs(struct thread_struct *thread)
>+{
>+	mtspr(SPRN_IAC1, thread->iac1);
>+	mtspr(SPRN_IAC2, thread->iac2);
>+	mtspr(SPRN_IAC3, thread->iac3);
>+	mtspr(SPRN_IAC4, thread->iac4);
>+	mtspr(SPRN_DAC1, thread->dac1);
>+	mtspr(SPRN_DAC2, thread->dac2);
>+	mtspr(SPRN_DVC1, thread->dvc1);
>+	mtspr(SPRN_DVC2, thread->dvc2);
>+	mtspr(SPRN_DBCR0, thread->dbcr0);
>+	mtspr(SPRN_DBCR1, thread->dbcr1);
>+	mtspr(SPRN_DBCR2, thread->dbcr2);

405 has no DBCR2, so I doubt setting it via mtspr would be good.  Does this
compile for 40x and did you test it at all?

Also, looking at the current kernel it seems SPRN_DBCR2 is only defined for
CONFIG_PPC_BOOK3E_64 which is also inaccurate.  I don't see that fixed in any
of the other patches.  Are you missing a patch, or was this generated against
and old kernel or something?

>@@ -514,7 +586,7 @@ void show_regs(struct pt_regs * regs)
> 	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
> 	trap = TRAP(regs);
> 	if (trap == 0x300 || trap == 0x600)
>-#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
>+#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> 		printk("DEAR: "REG", ESR: "REG"\n", regs->dar, regs->dsisr);
> #else
> 		printk("DAR: "REG", DSISR: "REG"\n", regs->dar, regs->dsisr);

This hunk seems unnecessary.

I need to find some time to sit down and look over the rest still.  I do have
some questions on how/if this will interact correctly with an older GDB still,
and some of the things I've pointed out make me wonder how much this has all
been tested and on what platforms.

josh

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Josh Boyer]

On Thu, Dec 10, 2009 at 12:27:11PM -0500, Josh Boyer wrote:
>On Thu, Dec 10, 2009 at 01:57:27PM -0200, Dave Kleikamp wrote:
>>+static void prime_debug_regs(struct thread_struct *thread)
>>+{
>>+	mtspr(SPRN_IAC1, thread->iac1);
>>+	mtspr(SPRN_IAC2, thread->iac2);
>>+	mtspr(SPRN_IAC3, thread->iac3);
>>+	mtspr(SPRN_IAC4, thread->iac4);
>>+	mtspr(SPRN_DAC1, thread->dac1);
>>+	mtspr(SPRN_DAC2, thread->dac2);
>>+	mtspr(SPRN_DVC1, thread->dvc1);
>>+	mtspr(SPRN_DVC2, thread->dvc2);
>>+	mtspr(SPRN_DBCR0, thread->dbcr0);
>>+	mtspr(SPRN_DBCR1, thread->dbcr1);
>>+	mtspr(SPRN_DBCR2, thread->dbcr2);
>
>405 has no DBCR2, so I doubt setting it via mtspr would be good.  Does this
>compile for 40x and did you test it at all?
>
>Also, looking at the current kernel it seems SPRN_DBCR2 is only defined for
>CONFIG_PPC_BOOK3E_64 which is also inaccurate.

I mis-read the code on this part.  SPRN_DBCR2 gets defined unconditionally,
so Book-E should be fine.  Still seems broken on 405 though.

josh

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Dave Kleikamp]

On Thu, 2009-12-10 at 12:41 -0500, Josh Boyer wrote:
> On Thu, Dec 10, 2009 at 12:27:11PM -0500, Josh Boyer wrote:
> >On Thu, Dec 10, 2009 at 01:57:27PM -0200, Dave Kleikamp wrote:
> >>+static void prime_debug_regs(struct thread_struct *thread)
> >>+{
> >>+	mtspr(SPRN_IAC1, thread->iac1);
> >>+	mtspr(SPRN_IAC2, thread->iac2);
> >>+	mtspr(SPRN_IAC3, thread->iac3);
> >>+	mtspr(SPRN_IAC4, thread->iac4);
> >>+	mtspr(SPRN_DAC1, thread->dac1);
> >>+	mtspr(SPRN_DAC2, thread->dac2);
> >>+	mtspr(SPRN_DVC1, thread->dvc1);
> >>+	mtspr(SPRN_DVC2, thread->dvc2);
> >>+	mtspr(SPRN_DBCR0, thread->dbcr0);
> >>+	mtspr(SPRN_DBCR1, thread->dbcr1);
> >>+	mtspr(SPRN_DBCR2, thread->dbcr2);
> >
> >405 has no DBCR2, so I doubt setting it via mtspr would be good.  Does this
> >compile for 40x and did you test it at all?
> >
> >Also, looking at the current kernel it seems SPRN_DBCR2 is only defined for
> >CONFIG_PPC_BOOK3E_64 which is also inaccurate.
> 
> I mis-read the code on this part.  SPRN_DBCR2 gets defined unconditionally,
> so Book-E should be fine.  Still seems broken on 405 though.

Agreed.  I'll rushed through the 405 stuff a bit.  I'll scrutinize and
test it before I post again.

Thanks,
Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Kumar Gala]

On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:

> powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace
>=20
> From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
>=20
> This patch defines context switch and trap related functionality
> for BookE specific Debug Registers. It adds support to ptrace()
> for setting and getting BookE related Debug Registers
>=20
> Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
> Cc: David Gibson <dwg@au1.ibm.com>
> Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
> ---
>=20
> arch/powerpc/include/asm/system.h |    2=20
> arch/powerpc/kernel/process.c     |  109 ++++++++-
> arch/powerpc/kernel/ptrace.c      |  435 =
++++++++++++++++++++++++++++++++++---
> arch/powerpc/kernel/signal.c      |    6 -
> arch/powerpc/kernel/signal_32.c   |    8 +
> arch/powerpc/kernel/traps.c       |   86 ++++++-
> 6 files changed, 564 insertions(+), 82 deletions(-)
>=20
>=20
> diff --git a/arch/powerpc/include/asm/system.h =
b/arch/powerpc/include/asm/system.h
> index bb8e006..474bf23 100644
> --- a/arch/powerpc/include/asm/system.h
> +++ b/arch/powerpc/include/asm/system.h
> @@ -114,6 +114,8 @@ static inline int debugger_fault_handler(struct =
pt_regs *regs) { return 0; }
> extern int set_dabr(unsigned long dabr);
> extern void do_dabr(struct pt_regs *regs, unsigned long address,
> 		    unsigned long error_code);
> +extern void do_send_trap(struct pt_regs *regs, unsigned long address,
> +			 unsigned long error_code, int signal_code, int =
errno);
> extern void print_backtrace(unsigned long *);
> extern void show_regs(struct pt_regs * regs);
> extern void flush_instruction_cache(void);
> diff --git a/arch/powerpc/kernel/process.c =
b/arch/powerpc/kernel/process.c
> index c930ac3..a0dbb09 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -245,6 +245,24 @@ void discard_lazy_cpu_state(void)
> }
> #endif /* CONFIG_SMP */
>=20
> +void do_send_trap(struct pt_regs *regs, unsigned long address,
> +		  unsigned long error_code, int signal_code, int errno)
> +{
> +	siginfo_t info;
> +
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) =3D=3D NOTIFY_STOP)
> +		return;
> +
> +	/* Deliver the signal to userspace */
> +	info.si_signo =3D SIGTRAP;
> +	info.si_errno =3D errno;
> +	info.si_code =3D signal_code;
> +	info.si_addr =3D (void __user *)address;
> +	force_sig_info(SIGTRAP, &info, current);
> +}
> +
> +#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
> void do_dabr(struct pt_regs *regs, unsigned long address,
> 		    unsigned long error_code)
> {
> @@ -257,12 +275,6 @@ void do_dabr(struct pt_regs *regs, unsigned long =
address,
> 	if (debugger_dabr_match(regs))
> 		return;
>=20
> -	/* Clear the DAC and struct entries.  One shot trigger */
> -#if defined(CONFIG_BOOKE)
> -	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> -							| DBCR0_IDM));
> -#endif
> -
> 	/* Clear the DABR */
> 	set_dabr(0);
>=20
> @@ -273,9 +285,71 @@ void do_dabr(struct pt_regs *regs, unsigned long =
address,
> 	info.si_addr =3D (void __user *)address;
> 	force_sig_info(SIGTRAP, &info, current);
> }
> +#endif
>=20
> static DEFINE_PER_CPU(unsigned long, current_dabr);
>=20
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +/*
> + * Set the debug registers back to their default "safe" values.
> + */
> +static void set_debug_reg_defaults(struct thread_struct *thread)
> +{
> +	thread->iac1 =3D thread->iac2 =3D thread->iac3 =3D thread->iac4 =
=3D 0;
> +	thread->dac1 =3D thread->dac2 =3D 0;
> +	thread->dvc1 =3D thread->dvc2 =3D 0;
> +	/*
> +	 * reset the DBCR0, DBCR1 and DBCR2 registers. All bits with
> +	 * the exception of the reserved bits should be cleared out
> +	 * and set to 0.
> +	 *
> +	 * For the DBCR0 register, the reserved bits are bits 17:30.
> +	 * Reserved bits for DBCR1 are bits 10:14 and bits 26:30.
> +	 * And, bits 10:11 for DBCR2.
> +	 */
> +	thread->dbcr0 =3D DBCR0_BASE_REG_VALUE;

This seems to always be 0, why have a special #define for it.

> +	/*
> +	 * First clear all "non reserved" bits from DBCR1 then =
initialize reg
> +	 * to force User/Supervisor bits to b11 (user-only MSR[PR]=3D1) =
and
> +	 * Effective/Real * bits to b10 (trap only if IS=3D=3D0)
> +	 */
> +	thread->dbcr1 =3D DBCR1_BASE_REG_VALUE;
> +	/*
> +	 * Force Data Address Compare User/Supervisor bits to be =
User-only
> +	 * (0b11 MSR[PR]=3D1) and set all other bits in DBCR2 register =
to be 0.
> +	 * This sets the Data Address Compare Effective/Real bits to be =
0b00
> +	 * (Effective, MSR[DS]=3Ddon't care).
> +	 */
> +	thread->dbcr2 =3D DBCR2_BASE_REG_VALUE;
> +}
> +
> +static void prime_debug_regs(struct thread_struct *thread)
> +{
> +	mtspr(SPRN_IAC1, thread->iac1);
> +	mtspr(SPRN_IAC2, thread->iac2);
> +	mtspr(SPRN_IAC3, thread->iac3);
> +	mtspr(SPRN_IAC4, thread->iac4);
> +	mtspr(SPRN_DAC1, thread->dac1);
> +	mtspr(SPRN_DAC2, thread->dac2);
> +	mtspr(SPRN_DVC1, thread->dvc1);
> +	mtspr(SPRN_DVC2, thread->dvc2);
> +	mtspr(SPRN_DBCR0, thread->dbcr0);
> +	mtspr(SPRN_DBCR1, thread->dbcr1);
> +	mtspr(SPRN_DBCR2, thread->dbcr2);

We should probably look at dbginfo.num_condition_regs, =
dbginfo.num_instruction_bps, & dbginfo.num_data_bps and set these =
accordingly.

> +}
> +/*
> + * Unless neither the old or new thread are making use of the
> + * debug registers, set the debug registers from the values
> + * stored in the new thread.
> + */
> +static void switch_booke_debug_regs(struct thread_struct *new_thread)
> +{
> +	if ((current->thread.dbcr0 & DBCR0_IDM)
> +		|| (new_thread->dbcr0 & DBCR0_IDM))
> +			prime_debug_regs(new_thread);
> +}
> +#endif
> +
> int set_dabr(unsigned long dabr)
> {
> 	__get_cpu_var(current_dabr) =3D dabr;
> @@ -284,7 +358,7 @@ int set_dabr(unsigned long dabr)
> 		return ppc_md.set_dabr(dabr);
>=20
> 	/* XXX should we have a CPU_FTR_HAS_DABR ? */
> -#if defined(CONFIG_BOOKE)
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> 	mtspr(SPRN_DAC1, dabr);
> #elif defined(CONFIG_PPC_BOOK3S)
> 	mtspr(SPRN_DABR, dabr);
> @@ -371,10 +445,8 @@ struct task_struct *__switch_to(struct =
task_struct *prev,
>=20
> #endif /* CONFIG_SMP */
>=20
> -#if defined(CONFIG_BOOKE)
> -	/* If new thread DAC (HW breakpoint) is the same then leave it =
*/
> -	if (new->thread.dabr)
> -		set_dabr(new->thread.dabr);
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +	switch_booke_debug_regs(&new->thread);
> #else
> 	if (unlikely(__get_cpu_var(current_dabr) !=3D new->thread.dabr))
> 		set_dabr(new->thread.dabr);
> @@ -514,7 +586,7 @@ void show_regs(struct pt_regs * regs)
> 	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
> 	trap =3D TRAP(regs);
> 	if (trap =3D=3D 0x300 || trap =3D=3D 0x600)
> -#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> 		printk("DEAR: "REG", ESR: "REG"\n", regs->dar, =
regs->dsisr);
> #else
> 		printk("DAR: "REG", DSISR: "REG"\n", regs->dar, =
regs->dsisr);
> @@ -568,14 +640,19 @@ void flush_thread(void)
>=20
> 	discard_lazy_cpu_state();
>=20
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +	/*
> +	 * flush_thread() is called on exec() to reset the
> +	 * thread's status. Set all debug regs back to their
> +	 * default values....Torez
> +	 */
> +	set_debug_reg_defaults(&current->thread);
> +#else
> 	if (current->thread.dabr) {
> 		current->thread.dabr =3D 0;
> 		set_dabr(0);
> -
> -#if defined(CONFIG_BOOKE)
> -		current->thread.dbcr0 &=3D ~(DBSR_DAC1R | DBSR_DAC1W);
> -#endif
> 	}
> +#endif
> }
>=20
> void
> diff --git a/arch/powerpc/kernel/ptrace.c =
b/arch/powerpc/kernel/ptrace.c
> index 6be2ce0..6710a69 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -737,17 +737,25 @@ void user_disable_single_step(struct task_struct =
*task)
> 	struct pt_regs *regs =3D task->thread.regs;
>=20
> 	if (regs !=3D NULL) {
> -#if defined(CONFIG_BOOKE)
> -		/* If DAC don't clear DBCRO_IDM or MSR_DE */
> -		if (task->thread.dabr)
> -			task->thread.dbcr0 &=3D ~(DBCR0_IC | DBCR0_BT);
> -		else {
> -			task->thread.dbcr0 &=3D ~(DBCR0_IC | DBCR0_BT | =
DBCR0_IDM);
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		/*
> +		 * The logic to disable single stepping should be as
> +		 * simple as turning off the Instruction Complete flag.
> +		 * And, after doing so, if all debug flags are off, turn
> +		 * off DBCR0(IDM) and MSR(DE) .... Torez
> +		 */
> +		task->thread.dbcr0 &=3D ~DBCR0_IC;
> +		/*
> +		 * Test to see if any of the DBCR_ACTIVE_EVENTS bits are =
set.
> +		 */
> +		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
> +					task->thread.dbcr1)) {
> +			/*
> +			 * All debug events were off.....
> +			 */
> +			task->thread.dbcr0 &=3D ~DBCR0_IDM;
> 			regs->msr &=3D ~MSR_DE;
> 		}
> -#elif defined(CONFIG_40x)
> -		task->thread.dbcr0 &=3D ~(DBCR0_IC | DBCR0_BT | =
DBCR0_IDM);
> -		regs->msr &=3D ~MSR_DE;
> #else
> 		regs->msr &=3D ~(MSR_SE | MSR_BE);
> #endif
> @@ -769,8 +777,7 @@ int ptrace_set_debugreg(struct task_struct *task, =
unsigned long addr,
> 	if ((data & ~0x7UL) >=3D TASK_SIZE)
> 		return -EIO;
>=20
> -#ifndef CONFIG_BOOKE
> -
> +#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
> 	/* For processors using DABR (i.e. 970), the bottom 3 bits are =
flags.
> 	 *  It was assumed, on previous implementations, that 3 bits =
were
> 	 *  passed together with the data address, fitting the design of =
the
> @@ -789,21 +796,22 @@ int ptrace_set_debugreg(struct task_struct =
*task, unsigned long addr,
>=20
> 	/* Move contents to the DABR register */
> 	task->thread.dabr =3D data;
> -
> -#endif
> -#if defined(CONFIG_BOOKE)
> -
> +#else
> 	/* As described above, it was assumed 3 bits were passed with =
the data
> 	 *  address, but we will assume only the mode bits will be =
passed
> 	 *  as to not cause alignment restrictions for DAC-based =
processors.
> 	 */
>=20
> 	/* DAC's hold the whole address without any mode flags */
> -	task->thread.dabr =3D data & ~0x3UL;
> -
> -	if (task->thread.dabr =3D=3D 0) {
> -		task->thread.dbcr0 &=3D ~(DBSR_DAC1R | DBSR_DAC1W | =
DBCR0_IDM);
> -		task->thread.regs->msr &=3D ~MSR_DE;
> +	task->thread.dac1 =3D data & ~0x3UL;
> +
> +	if (task->thread.dac1 =3D=3D 0) {
> +		dbcr_dac(task) &=3D ~(DBCR_DAC1R | DBCR_DAC1W);
> +		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
> +					task->thread.dbcr1)) {
> +			task->thread.regs->msr &=3D ~MSR_DE;
> +			task->thread.dbcr0 &=3D ~DBCR0_IDM;
> +		}
> 		return 0;
> 	}
>=20
> @@ -814,15 +822,15 @@ int ptrace_set_debugreg(struct task_struct =
*task, unsigned long addr,
>=20
> 	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
> 	   register */
> -	task->thread.dbcr0 =3D DBCR0_IDM;
> +	task->thread.dbcr0 |=3D DBCR0_IDM;
>=20
> 	/* Check for write and read flags and set DBCR0
> 	   accordingly */
> +	dbcr_dac(task) &=3D ~(DBCR_DAC1R|DBCR_DAC1W);
> 	if (data & 0x1UL)
> -		task->thread.dbcr0 |=3D DBSR_DAC1R;
> +		dbcr_dac(task) |=3D DBCR_DAC1R;
> 	if (data & 0x2UL)
> -		task->thread.dbcr0 |=3D DBSR_DAC1W;
> -
> +		dbcr_dac(task) |=3D DBCR_DAC1W;
> 	task->thread.regs->msr |=3D MSR_DE;
> #endif
> 	return 0;
> @@ -839,11 +847,324 @@ void ptrace_disable(struct task_struct *child)
> 	user_disable_single_step(child);
> }
>=20
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +static long set_intruction_bp(struct task_struct *child,
> +			      struct ppc_hw_breakpoint *bp_info)
> +{
> +	int slots_needed;
> +	int slot;
> +	int free_slot =3D 0;
> +
> +	/*
> +	 * Find an avalailable slot for the breakpoint.
> +	 * If possible, reserve consecutive slots, 1 & 2, for a range
> +	 * breakpoint.  (Can this be done simpler?)
> +	 */
> +	if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_EXACT)
> +		slots_needed =3D 1;
> +	else
> +		slots_needed =3D 2;
> +
> +	if ((child->thread.dbcr0 & DBCR0_IAC1) =3D=3D 0) {
> +		if (slots_needed =3D=3D 1) {
> +			if (child->thread.dbcr0 & DBCR0_IAC2) {
> +				slot =3D 1;
> +				goto found;
> +			}
> +			/* Try to save slots 1 & 2 for range */
> +			free_slot =3D 1;
> +		} else
> +			if ((child->thread.dbcr0 & DBCR0_IAC2) =3D=3D 0) =
{
> +				slot =3D 1;
> +				goto found;
> +			}
> +	} else if ((slots_needed =3D=3D 1) &&
> +		   ((child->thread.dbcr0 & DBCR0_IAC2) =3D=3D 0)) {
> +		slot =3D 2;
> +		goto found;
> +	}
> +	if ((child->thread.dbcr0 & DBCR0_IAC3) =3D=3D 0) {
> +		if (slots_needed =3D=3D 1) {
> +			slot =3D 3;
> +			goto found;
> +		}
> +		if ((child->thread.dbcr0 & DBCR0_IAC4) =3D=3D 0) {
> +			slot =3D 3;
> +			goto found;
> +		}
> +		return -ENOSPC;
> +	} else if (slots_needed =3D=3D 2)
> +		return -ENOSPC;
> +	if ((child->thread.dbcr0 & DBCR0_IAC4) =3D=3D 0) {
> +		slot =3D 4;
> +	} else if (free_slot)
> +		slot =3D free_slot;
> +	else
> +		return -ENOSPC;

Need to factor in if # of IACs is only 2.

> +found:
> +	if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_EXACT) {
> +		switch (slot) {
> +			case 1:
> +				child->thread.iac1 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC1;
> +				break;
> +			case 2:
> +				child->thread.iac2 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC2;
> +				break;
> +			case 3:
> +				child->thread.iac3 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC3;
> +				break;
> +			case 4:
> +				child->thread.iac4 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC4;
> +				break;
> +		}
> +	} else if (slot =3D=3D 1) {
> +		child->thread.iac1 =3D bp_info->addr;
> +		child->thread.iac2 =3D bp_info->addr2;
> +		child->thread.dbcr0 |=3D (DBCR0_IAC1 | DBCR0_IAC2);
> +		if (bp_info->addr_mode =3D=3D =
PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
> +			dbcr_iac_range(child) |=3D DBCR_IAC12M_X;
> +		else
> +			dbcr_iac_range(child) |=3D DBCR_IAC12M_I;
> +	} else { /* slot =3D=3D 3 */
> +		child->thread.iac3 =3D bp_info->addr;
> +		child->thread.iac4 =3D bp_info->addr2;
> +		child->thread.dbcr0 |=3D (DBCR0_IAC3 | DBCR0_IAC4);
> +		if (bp_info->addr_mode =3D=3D =
PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
> +			dbcr_iac_range(child) |=3D DBCR_IAC34M_X;
> +		else
> +			dbcr_iac_range(child) |=3D DBCR_IAC34M_I;
> +	}
> +	child->thread.dbcr0 |=3D DBCR0_IDM;
> +	child->thread.regs->msr |=3D MSR_DE;
> +
> +	return slot;
> +}
> +
> +static int del_instruction_bp(struct task_struct *child, int slot)
> +{
> +	switch (slot) {
> +	case 1:
> +		if (dbcr_iac_range(child) & DBCR_IAC12M) {
> +			/* address range - clear slots 1 & 2 */
> +			child->thread.iac2 =3D 0;
> +			child->thread.dbcr0 &=3D ~DBCR0_IAC2;
> +			dbcr_iac_range(child) &=3D ~DBCR_IAC12M;
> +		}
> +		child->thread.iac1 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC1;
> +		break;
> +	case 2:
> +		if (dbcr_iac_range(child) & DBCR_IAC12M)
> +			/* used in a range */
> +			return -EINVAL;
> +		child->thread.iac2 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC2;
> +		break;
> +	case 3:
> +		if (dbcr_iac_range(child) & DBCR_IAC34M) {
> +			/* address range - clear slots 3 & 4 */
> +			child->thread.iac4 =3D 0;
> +			child->thread.dbcr0 &=3D ~DBCR0_IAC4;
> +			dbcr_iac_range(child) &=3D ~DBCR_IAC34M;
> +		}
> +		child->thread.iac3 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC3;
> +		break;
> +	case 4:
> +		if (dbcr_iac_range(child) & DBCR_IAC34M)
> +			/* Used in a range */
> +			return -EINVAL;
> +		child->thread.iac4 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int set_dac(struct task_struct *child, struct =
ppc_hw_breakpoint *bp_info)
> +{
> +	int byte_enable =3D
> +		(bp_info->condition_mode >> =
PPC_BREAKPOINT_CONDITION_BE_SHIFT)
> +		& 0xf;
> +	int condition_mode =3D
> +		bp_info->condition_mode & =
PPC_BREAKPOINT_CONDITION_AND_OR;
> +	int slot;
> +
> +	if (byte_enable && (condition_mode =3D=3D 0))
> +		return -EINVAL;
> +
> +	if (bp_info->addr >=3D TASK_SIZE)
> +		return -EIO;
> +
> +	if ((dbcr_dac(child) & (DBCR_DAC1R | DBCR_DAC1W)) =3D=3D 0) {
> +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> +			dbcr_dac(child) |=3D DBCR_DAC1R;
> +		if (bp_info->trigger_type & =
PPC_BREAKPOINT_TRIGGER_WRITE)
> +			dbcr_dac(child) |=3D DBCR_DAC1W;
> +		child->thread.dac1 =3D (unsigned long)bp_info->addr;
> +#ifdef CONFIG_BOOKE
> +		if (byte_enable) {
> +			child->thread.dvc1 =3D
> +				(unsigned long)bp_info->condition_value;
> +			child->thread.dbcr2 |=3D
> +				((byte_enable << DBCR2_DVC1BE_SHIFT) |
> +				 (condition_mode << DBCR2_DVC1M_SHIFT));
> +		}
> +#endif
> +		slot =3D 1;
> +	} else if ((dbcr_dac(child) & (DBCR_DAC2R | DBCR_DAC2W)) =3D=3D =
0) {
> +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> +			dbcr_dac(child) |=3D DBCR_DAC2R;
> +		if (bp_info->trigger_type & =
PPC_BREAKPOINT_TRIGGER_WRITE)
> +			dbcr_dac(child) |=3D DBCR_DAC2W;
> +		child->thread.dac2 =3D (unsigned long)bp_info->addr;
> +#ifdef CONFIG_BOOKE
> +		if (byte_enable) {
> +			child->thread.dvc2 =3D
> +				(unsigned long)bp_info->condition_value;
> +			child->thread.dbcr2 |=3D
> +				((byte_enable << DBCR2_DVC2BE_SHIFT) |
> +				 (condition_mode << DBCR2_DVC2M_SHIFT));
> +		}
> +#endif
> +		slot =3D 2;
> +	} else
> +		return -ENOSPC;
> +	child->thread.dbcr0 |=3D DBCR0_IDM;
> +	child->thread.regs->msr |=3D MSR_DE;
> +
> +	return slot + 4;
> +}
> +
> +static int del_dac(struct task_struct *child, int slot)
> +{
> +	if (slot =3D=3D 1) {
> +#ifdef CONFIG_BOOKE
> +		if (child->thread.dbcr2 & DBCR2_DAC12MODE) {
> +			child->thread.dac1 =3D 0;
> +			child->thread.dac2 =3D 0;
> +			child->thread.dbcr0 &=3D ~(DBCR0_DAC1R | =
DBCR0_DAC1W |
> +						 DBCR0_DAC2R | =
DBCR0_DAC2W);
> +			child->thread.dbcr2 &=3D ~DBCR2_DAC12MODE;
> +			return 0;
> +		}
> +		child->thread.dbcr2 &=3D ~(DBCR2_DVC1M | DBCR2_DVC1BE);
> +		child->thread.dvc1 =3D 0;
> +#endif
> +		child->thread.dac1 =3D 0;
> +		dbcr_dac(child) &=3D ~(DBCR_DAC1R | DBCR_DAC1W);
> +	} else if (slot =3D=3D 2) {
> +#ifdef CONFIG_BOOKE
> +		if (child->thread.dbcr2 & DBCR2_DAC12MODE)
> +			/* Part of a range */
> +			return -EINVAL;
> +		child->thread.dbcr2 &=3D ~(DBCR2_DVC2M | DBCR2_DVC2BE);
> +		child->thread.dvc2 =3D 0;
> +#endif
> +		child->thread.dac2 =3D 0;
> +		dbcr_dac(child) &=3D ~(DBCR_DAC2R | DBCR_DAC2W);
> +	} else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_40x || CONFIG_BOOKE */
> +
> +#ifdef CONFIG_BOOKE
> +static int set_dac_range(struct task_struct *child,
> +			 struct ppc_hw_breakpoint *bp_info)
> +{
> +	int mode =3D bp_info->addr_mode & PPC_BREAKPOINT_MODE_MASK;
> +
> +	/* We don't allow range watchpoints to be used with DVC */
> +	if (bp_info->condition_mode && PPC_BREAKPOINT_CONDITION_BE_ALL)
> +		return -EINVAL;
> +
> +	if (bp_info->addr >=3D TASK_SIZE)
> +		return -EIO;
> +
> +	if (child->thread.dbcr0 &
> +	    (DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W))
> +		return -ENOSPC;
> +
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> +		child->thread.dbcr0 |=3D (DBCR0_DAC1R | DBCR0_DAC2R | =
DBCR0_IDM);
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> +		child->thread.dbcr0 |=3D (DBCR0_DAC1W | DBCR0_DAC2W | =
DBCR0_IDM);
> +	child->thread.dac1 =3D bp_info->addr;
> +	child->thread.dac2 =3D bp_info->addr2;
> +	if (mode =3D=3D PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
> +		child->thread.dbcr2  |=3D DBCR2_DAC12R;
> +	else if (mode =3D=3D PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
> +		child->thread.dbcr2  |=3D DBCR2_DAC12RX;
> +	else	/* PPC_BREAKPOINT_MODE_MASK */
> +		child->thread.dbcr2  |=3D DBCR2_DAC12MASK;
> +	child->thread.regs->msr |=3D MSR_DE;
> +
> +	return 5;
> +}
> +#endif /* CONFIG_BOOKE */
> +
> static long ppc_set_hwdebug(struct task_struct *child,
> 		     struct ppc_hw_breakpoint *bp_info)
> {
> +	if (bp_info->version !=3D 1)
> +		return -ENOTSUPP;
> +
> +#ifdef CONFIG_BOOKE
> +	/*
> +	 * Check for invalid flags and combinations
> +	 */
> +	if ((bp_info->trigger_type =3D=3D 0) ||
> +	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
> +				       PPC_BREAKPOINT_TRIGGER_RW)) ||
> +	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
> +	    (bp_info->condition_mode &
> +	     ~(PPC_BREAKPOINT_CONDITION_AND_OR |
> +	       PPC_BREAKPOINT_CONDITION_BE_ALL)))
> +		return -EINVAL;

We should add a sanity check for bp_info->condition_mode !=3D =
PPC_BREAKPOINT_CONDITION_NONE if dbginfo.num_condition_regs =3D 0.

> +
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
> +		if (bp_info->trigger_type !=3D =
PPC_BREAKPOINT_TRIGGER_EXECUTE)
> +			/* At least another bit was set */
> +			return -EINVAL;
> +		return set_intruction_bp(child, bp_info);
> +	}
> +
> +	if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_EXACT)
> +		return set_dac(child, bp_info);
> +
> +	return set_dac_range(child, bp_info);
> +#elif defined(CONFIG_40x)
> +	/*
> +	 * Check for invalid flags and combinations
> +	 */
> +	if ((bp_info->trigger_type =3D=3D 0) ||
> +	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
> +				       PPC_BREAKPOINT_TRIGGER_RW)) ||
> +	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
> +	    (bp_info->condition_mode !=3D =
PPC_BREAKPOINT_CONDITION_NONE))
> +		return -EINVAL;
> +
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
> +		if (bp_info->trigger_type !=3D =
PPC_BREAKPOINT_TRIGGER_EXECUTE)
> +			/* At least another bit was set */
> +			return -EINVAL;
> +		return set_intruction_bp(child, bp_info);
> +	}
> +	if (bp_info->addr_mode !=3D PPC_BREAKPOINT_MODE_EXACT)
> +		return -EINVAL;
> +
> +	return set_dac(child, bp_info);
> +#else
> 	/*
> -	 * We currently support one data breakpoint
> +	 * We only support one data breakpoint
> 	 */
> 	if (((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) =3D=3D =
0) ||
> 	    ((bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) !=3D =
0) ||
> @@ -859,28 +1180,35 @@ static long ppc_set_hwdebug(struct task_struct =
*child,
> 		return -EIO;
>=20
> 	child->thread.dabr =3D (unsigned long)bp_info->addr;
> -#ifdef CONFIG_BOOKE
> -	child->thread.dbcr0 =3D DBCR0_IDM;
> -	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> -		child->thread.dbcr0 |=3D DBSR_DAC1R;
> -	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> -		child->thread.dbcr0 |=3D DBSR_DAC1W;
> -	child->thread.regs->msr |=3D MSR_DE;
> -#endif
> 	return 1;
> +#endif
> }
>=20
> static long ppc_del_hwdebug(struct task_struct *child, long addr, long =
data)
> {
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +	int rc;
> +
> +	if (data <=3D 4)
> +		rc =3D del_instruction_bp(child, (int)data);
> +	else
> +		rc =3D del_dac(child, (int)data - 4);
> +
> +	if (!rc) {
> +		if (!DBCR_ACTIVE_EVENTS(child->thread.dbcr0,
> +					child->thread.dbcr1)) {
> +			child->thread.dbcr0 &=3D ~DBCR0_IDM;
> +			child->thread.regs->msr &=3D ~MSR_DE;
> +		}
> +	}
> +	return rc;
> +#else
> 	if ((data !=3D 1) || (child->thread.dabr =3D=3D 0))
> 		return -EINVAL;
>=20
> 	child->thread.dabr =3D 0;
> -#ifdef CONFIG_BOOKE
> -	child->thread.dbcr0 &=3D ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> -	child->thread.regs->msr &=3D ~MSR_DE;
> -#endif
> 	return 0;
> +#endif
> }
>=20
> /*
> @@ -980,16 +1308,36 @@ long arch_ptrace(struct task_struct *child, =
long request, long addr, long data)
> 		struct ppc_debug_info dbginfo;
>=20
> 		dbginfo.version =3D 1;
> +#ifdef CONFIG_BOOKE
> +		dbginfo.num_instruction_bps =3D 4;
> +		dbginfo.num_data_bps =3D 2;
> +		dbginfo.num_condition_regs =3D 2;
> +		dbginfo.data_bp_alignment =3D 0;
> +		dbginfo.sizeof_condition =3D 4;
> +		dbginfo.features =3D PPC_DEBUG_FEATURE_INSN_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_INSN_BP_MASK |
> +				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
> +#elif defined(CONFIG_40x)
> +		/*
> +		 * I don't know how the DVCs work on 40x, I'm not going
> +		 * to support it now. -- Shaggy
> +		 */
> +		dbginfo.num_instruction_bps =3D 4;
> +		dbginfo.num_data_bps =3D 2;
> +		dbginfo.num_condition_regs =3D 0;
> +		dbginfo.data_bp_alignment =3D 0;
> +		dbginfo.sizeof_condition =3D 0;
> +		dbginfo.features =3D PPC_DEBUG_FEATURE_INSN_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_INSN_BP_MASK;
> +#else
> 		dbginfo.num_instruction_bps =3D 0;
> 		dbginfo.num_data_bps =3D 1;
> 		dbginfo.num_condition_regs =3D 0;
> -#ifdef CONFIG_PPC64
> 		dbginfo.data_bp_alignment =3D 8;
> -#else
> -		dbginfo.data_bp_alignment =3D 0;
> -#endif
> 		dbginfo.sizeof_condition =3D 0;
> 		dbginfo.features =3D 0;
> +#endif

This is a bit ugly and BOOKE 64 parts probably don't have the 8 byte =
alignment.

Should we push some of this into cputable?

>=20
> 		if (!access_ok(VERIFY_WRITE, data,
> 			       sizeof(struct ppc_debug_info)))
> @@ -1025,8 +1373,13 @@ long arch_ptrace(struct task_struct *child, =
long request, long addr, long data)
> 		/* We only support one DABR and no IABRS at the moment =
*/
> 		if (addr > 0)
> 			break;
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		ret =3D put_user(child->thread.dac1,
> +			       (unsigned long __user *)data);
> +#else
> 		ret =3D put_user(child->thread.dabr,
> 			       (unsigned long __user *)data);
> +#endif
> 		break;
> 	}
>=20
> diff --git a/arch/powerpc/kernel/signal.c =
b/arch/powerpc/kernel/signal.c
> index 00b5078..94df779 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -140,17 +140,15 @@ static int do_signal_pending(sigset_t *oldset, =
struct pt_regs *regs)
> 		return 0;               /* no signals delivered */
> 	}
>=20
> +#if !(defined(CONFIG_BOOKE) || defined(CONFIG_40x))
>         /*
> 	 * Reenable the DABR before delivering the signal to
> 	 * 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_BOOKE)
> -		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> #endif
> -	}
>=20
> 	if (is32) {
>         	if (ka.sa.sa_flags & SA_SIGINFO)
> diff --git a/arch/powerpc/kernel/signal_32.c =
b/arch/powerpc/kernel/signal_32.c
> index d670429..6cc6e81 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -1092,8 +1092,12 @@ int sys_debug_setcontext(struct ucontext __user =
*ctx,
> 				new_msr |=3D MSR_DE;
> 				new_dbcr0 |=3D (DBCR0_IDM | DBCR0_IC);
> 			} else {
> -				new_msr &=3D ~MSR_DE;
> -				new_dbcr0 &=3D ~(DBCR0_IDM | DBCR0_IC);
> +				new_dbcr0 &=3D ~DBCR0_IC;
> +				if (!DBCR_ACTIVE_EVENTS(new_dbcr0,
> +						current->thread.dbcr1)) =
{
> +					new_msr &=3D ~MSR_DE;
> +					new_dbcr0 &=3D ~DBCR0_IDM;
> +				}
> 			}
> #else
> 			if (op.dbg_value)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index a81c743..d919571 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1016,9 +1016,63 @@ void SoftwareEmulation(struct pt_regs *regs)
> #endif /* CONFIG_8xx */
>=20
> #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +static void handle_debug(struct pt_regs *regs, unsigned long =
debug_status)
> +{
> +	int changed =3D 0;
> +	/*
> +	 * Determine the cause of the debug event, clear the
> +	 * event flags and send a trap to the handler. Torez
> +	 */
> +	if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
> +		dbcr_dac(current) &=3D ~(DBCR_DAC1R | DBCR_DAC1W);
> +		do_send_trap(regs, mfspr(SPRN_DAC1), debug_status, =
TRAP_HWBKPT,
> +			     5);
> +		changed |=3D 0x01;
> +	}  else if (debug_status & (DBSR_DAC2R | DBSR_DAC2W)) {
> +		dbcr_dac(current) &=3D ~(DBCR_DAC2R | DBCR_DAC2W);
> +		do_send_trap(regs, mfspr(SPRN_DAC2), debug_status, =
TRAP_HWBKPT,
> +			     6);
> +		changed |=3D 0x01;
> +	}  else if (debug_status & DBSR_IAC1) {
> +		current->thread.dbcr0 &=3D ~DBCR0_IAC1;
> +		do_send_trap(regs, mfspr(SPRN_IAC1), debug_status, =
TRAP_HWBKPT,
> +			     1);
> +		changed |=3D 0x01;
> +	}  else if (debug_status & DBSR_IAC2) {
> +		current->thread.dbcr0 &=3D ~DBCR0_IAC2;
> +		do_send_trap(regs, mfspr(SPRN_IAC2), debug_status, =
TRAP_HWBKPT,
> +			     2);
> +		changed |=3D 0x01;
> +	}  else if (debug_status & DBSR_IAC3) {
> +		current->thread.dbcr0 &=3D ~DBCR0_IAC3;
> +		do_send_trap(regs, mfspr(SPRN_IAC3), debug_status, =
TRAP_HWBKPT,
> +			     3);
> +		changed |=3D 0x01;
> +	}  else if (debug_status & DBSR_IAC4) {
> +		current->thread.dbcr0 &=3D ~DBCR0_IAC4;
> +		do_send_trap(regs, mfspr(SPRN_IAC4), debug_status, =
TRAP_HWBKPT,
> +			     4);
> +		changed |=3D 0x01;
> +	}
> +	/*
> +	 * At the point this routine was called, the MSR(DE) was turned =
off.
> +	 * Check all other debug flags and see if that bit needs to be =
turned
> +	 * back on or not.
> +	 */
> +	if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0, =
current->thread.dbcr1))
> +		regs->msr |=3D MSR_DE;
> +	else
> +		/* Make sure the IDM flag is off */
> +		current->thread.dbcr0 &=3D ~DBCR0_IDM;
> +
> +	if (changed & 0x01)
> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> +}
>=20
> void __kprobes DebugException(struct pt_regs *regs, unsigned long =
debug_status)
> {
> +	current->thread.dbsr =3D debug_status;
> +
> 	/* Hack alert: On BookE, Branch Taken stops on the branch =
itself, while
> 	 * on server, it stops on the target of the branch. In order to =
simulate
> 	 * the server behaviour, we thus restart right away with a =
single step
> @@ -1062,27 +1116,21 @@ void __kprobes DebugException(struct pt_regs =
*regs, unsigned long debug_status)
> 		if (debugger_sstep(regs))
> 			return;
>=20
> -		if (user_mode(regs))
> -			current->thread.dbcr0 &=3D ~(DBCR0_IC);
> -
> -		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
> -	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
> -		regs->msr &=3D ~MSR_DE;
> -
> 		if (user_mode(regs)) {
> -			current->thread.dbcr0 &=3D ~(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));
> +			current->thread.dbcr0 &=3D ~DBCR0_IC;
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +			if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0,
> +					       current->thread.dbcr1))
> +				regs->msr |=3D MSR_DE;
> +			else
> +				/* Make sure the IDM bit is off */
> +				current->thread.dbcr0 &=3D ~DBCR0_IDM;
> +#endif
> 		}
> -		/* Setup and send the trap to the handler */
> -		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
> -	}
> +
> +		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
> +	} else
> +		handle_debug(regs, debug_status);
> }
> #endif /* CONFIG_4xx || CONFIG_BOOKE */
>=20
>=20
> --=20
> Dave Kleikamp
> IBM Linux Technology Center
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Dave Kleikamp]

On Thu, 2009-12-10 at 20:50 -0600, Kumar Gala wrote:
> On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
> 
> > powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace
> > 
> > From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
> > 
> > This patch defines context switch and trap related functionality
> > for BookE specific Debug Registers. It adds support to ptrace()
> > for setting and getting BookE related Debug Registers

snip

> > +static void prime_debug_regs(struct thread_struct *thread)
> > +{
> > +	mtspr(SPRN_IAC1, thread->iac1);
> > +	mtspr(SPRN_IAC2, thread->iac2);
> > +	mtspr(SPRN_IAC3, thread->iac3);
> > +	mtspr(SPRN_IAC4, thread->iac4);
> > +	mtspr(SPRN_DAC1, thread->dac1);
> > +	mtspr(SPRN_DAC2, thread->dac2);
> > +	mtspr(SPRN_DVC1, thread->dvc1);
> > +	mtspr(SPRN_DVC2, thread->dvc2);
> > +	mtspr(SPRN_DBCR0, thread->dbcr0);
> > +	mtspr(SPRN_DBCR1, thread->dbcr1);
> > +	mtspr(SPRN_DBCR2, thread->dbcr2);
> 
> We should probably look at dbginfo.num_condition_regs, dbginfo.num_instruction_bps, & dbginfo.num_data_bps and set these accordingly.

All I did here is make dbcr2 depend on CONFIG_BOOKE.  For the time
being, there's no run-time checks on this, only compile-time.

> > +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> > +static long set_intruction_bp(struct task_struct *child,
> > +			      struct ppc_hw_breakpoint *bp_info)
> > +{
> > +	int slots_needed;
> > +	int slot;
> > +	int free_slot = 0;
> > +
> > +	/*
> > +	 * Find an avalailable slot for the breakpoint.
> > +	 * If possible, reserve consecutive slots, 1 & 2, for a range
> > +	 * breakpoint.  (Can this be done simpler?)
> > +	 */
> > +	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
> > +		slots_needed = 1;
> > +	else
> > +		slots_needed = 2;
> > +
> > +	if ((child->thread.dbcr0 & DBCR0_IAC1) == 0) {
> > +		if (slots_needed == 1) {
> > +			if (child->thread.dbcr0 & DBCR0_IAC2) {
> > +				slot = 1;
> > +				goto found;
> > +			}
> > +			/* Try to save slots 1 & 2 for range */
> > +			free_slot = 1;
> > +		} else
> > +			if ((child->thread.dbcr0 & DBCR0_IAC2) == 0) {
> > +				slot = 1;
> > +				goto found;
> > +			}
> > +	} else if ((slots_needed == 1) &&
> > +		   ((child->thread.dbcr0 & DBCR0_IAC2) == 0)) {
> > +		slot = 2;
> > +		goto found;
> > +	}
> > +	if ((child->thread.dbcr0 & DBCR0_IAC3) == 0) {
> > +		if (slots_needed == 1) {
> > +			slot = 3;
> > +			goto found;
> > +		}
> > +		if ((child->thread.dbcr0 & DBCR0_IAC4) == 0) {
> > +			slot = 3;
> > +			goto found;
> > +		}
> > +		return -ENOSPC;
> > +	} else if (slots_needed == 2)
> > +		return -ENOSPC;
> > +	if ((child->thread.dbcr0 & DBCR0_IAC4) == 0) {
> > +		slot = 4;
> > +	} else if (free_slot)
> > +		slot = free_slot;
> > +	else
> > +		return -ENOSPC;
> 
> Need to factor in if # of IACs is only 2.

What cpu has 2 IACs?

> > static long ppc_set_hwdebug(struct task_struct *child,
> > 		     struct ppc_hw_breakpoint *bp_info)
> > {
> > +	if (bp_info->version != 1)
> > +		return -ENOTSUPP;
> > +
> > +#ifdef CONFIG_BOOKE
> > +	/*
> > +	 * Check for invalid flags and combinations
> > +	 */
> > +	if ((bp_info->trigger_type == 0) ||
> > +	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
> > +				       PPC_BREAKPOINT_TRIGGER_RW)) ||
> > +	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
> > +	    (bp_info->condition_mode &
> > +	     ~(PPC_BREAKPOINT_CONDITION_AND_OR |
> > +	       PPC_BREAKPOINT_CONDITION_BE_ALL)))
> > +		return -EINVAL;
> 
> We should add a sanity check for bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE if dbginfo.num_condition_regs = 0.

This falls into the run-time checks that I haven't implemented.  BOOKE
will not define dbginfo.num_condition_regs to be 0.

> > /*
> > @@ -980,16 +1308,36 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
> > 		struct ppc_debug_info dbginfo;
> > 
> > 		dbginfo.version = 1;
> > +#ifdef CONFIG_BOOKE
> > +		dbginfo.num_instruction_bps = 4;
> > +		dbginfo.num_data_bps = 2;
> > +		dbginfo.num_condition_regs = 2;
> > +		dbginfo.data_bp_alignment = 0;
> > +		dbginfo.sizeof_condition = 4;
> > +		dbginfo.features = PPC_DEBUG_FEATURE_INSN_BP_RANGE |
> > +				   PPC_DEBUG_FEATURE_INSN_BP_MASK |
> > +				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
> > +				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
> > +#elif defined(CONFIG_40x)
> > +		/*
> > +		 * I don't know how the DVCs work on 40x, I'm not going
> > +		 * to support it now. -- Shaggy
> > +		 */
> > +		dbginfo.num_instruction_bps = 4;
> > +		dbginfo.num_data_bps = 2;
> > +		dbginfo.num_condition_regs = 0;
> > +		dbginfo.data_bp_alignment = 0;
> > +		dbginfo.sizeof_condition = 0;
> > +		dbginfo.features = PPC_DEBUG_FEATURE_INSN_BP_RANGE |
> > +				   PPC_DEBUG_FEATURE_INSN_BP_MASK;
> > +#else
> > 		dbginfo.num_instruction_bps = 0;
> > 		dbginfo.num_data_bps = 1;
> > 		dbginfo.num_condition_regs = 0;
> > -#ifdef CONFIG_PPC64
> > 		dbginfo.data_bp_alignment = 8;
> > -#else
> > -		dbginfo.data_bp_alignment = 0;
> > -#endif
> > 		dbginfo.sizeof_condition = 0;
> > 		dbginfo.features = 0;
> > +#endif
> 
> This is a bit ugly and BOOKE 64 parts probably don't have the 8 byte alignment.
> 
> Should we push some of this into cputable?

I'm thinking about it.  It may clean up some of the ifdefs.  I'll give
it a try, and if it ends up cleaner, I'll submit a follow-up patch.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[David Gibson]

On Thu, Dec 10, 2009 at 01:57:27PM -0200, Dave Kleikamp wrote:
> powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace
>=20
> From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
>=20
> This patch defines context switch and trap related functionality
> for BookE specific Debug Registers. It adds support to ptrace()
> for setting and getting BookE related Debug Registers
>=20
> Signed-off-by: Torez Smith  <lnxtorez@linux.vnet.ibm.com>
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Sergio Durigan Junior <sergiodj@br.ibm.com>
> Cc: David Gibson <dwg@au1.ibm.com>
> Cc: linuxppc-dev list <Linuxppc-dev@ozlabs.org>
> ---
>=20
>  arch/powerpc/include/asm/system.h |    2=20
>  arch/powerpc/kernel/process.c     |  109 ++++++++-
>  arch/powerpc/kernel/ptrace.c      |  435 +++++++++++++++++++++++++++++++=
+++---
>  arch/powerpc/kernel/signal.c      |    6 -
>  arch/powerpc/kernel/signal_32.c   |    8 +
>  arch/powerpc/kernel/traps.c       |   86 ++++++-
>  6 files changed, 564 insertions(+), 82 deletions(-)

[snip]
> +void do_send_trap(struct pt_regs *regs, unsigned long address,
> +		  unsigned long error_code, int signal_code, int errno)
> +{
> +	siginfo_t info;
> +
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) =3D=3D NOTIFY_STOP)
> +		return;
> +
> +	/* Deliver the signal to userspace */
> +	info.si_signo =3D SIGTRAP;
> +	info.si_errno =3D errno;

We're using the errno siginfo field, but not for an errno, so possibly
the parameter should be called something else.

[snip]
> +#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
>  void do_dabr(struct pt_regs *regs, unsigned long address,
>  		    unsigned long error_code)
>  {
> @@ -257,12 +275,6 @@ void do_dabr(struct pt_regs *regs, unsigned long add=
ress,
>  	if (debugger_dabr_match(regs))
>  		return;
> =20
> -	/* Clear the DAC and struct entries.  One shot trigger */
> -#if defined(CONFIG_BOOKE)
> -	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> -							| DBCR0_IDM));
> -#endif
> -
>  	/* Clear the DABR */
>  	set_dabr(0);

Uh.. does this imply we're keeping the one-shot behaviour for
new-style breakpoints?  To me the interface really suggests they're
persistent, although dealing with the semantics of that at signal time
can get curly.

> =20
> @@ -273,9 +285,71 @@ void do_dabr(struct pt_regs *regs, unsigned long add=
ress,
>  	info.si_addr =3D (void __user *)address;
>  	force_sig_info(SIGTRAP, &info, current);
>  }
> +#endif
> =20
>  static DEFINE_PER_CPU(unsigned long, current_dabr);
> =20
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +/*
> + * Set the debug registers back to their default "safe" values.
> + */
> +static void set_debug_reg_defaults(struct thread_struct *thread)
> +{
> +	thread->iac1 =3D thread->iac2 =3D thread->iac3 =3D thread->iac4 =3D 0;
> +	thread->dac1 =3D thread->dac2 =3D 0;
> +	thread->dvc1 =3D thread->dvc2 =3D 0;
> +	/*
> +	 * reset the DBCR0, DBCR1 and DBCR2 registers. All bits with
> +	 * the exception of the reserved bits should be cleared out
> +	 * and set to 0.
> +	 *
> +	 * For the DBCR0 register, the reserved bits are bits 17:30.
> +	 * Reserved bits for DBCR1 are bits 10:14 and bits 26:30.
> +	 * And, bits 10:11 for DBCR2.
> +	 */
> +	thread->dbcr0 =3D DBCR0_BASE_REG_VALUE;

Since this is now the only place it's used, I'd pull the
BASE_REG_VALUE constant inline here.  Makes the actual definition sit
next to the describing comment which is a bonus.

> +	/*
> +	 * First clear all "non reserved" bits from DBCR1 then initialize reg
> +	 * to force User/Supervisor bits to b11 (user-only MSR[PR]=3D1) and
> +	 * Effective/Real * bits to b10 (trap only if IS=3D=3D0)
> +	 */
> +	thread->dbcr1 =3D DBCR1_BASE_REG_VALUE;
> +	/*
> +	 * Force Data Address Compare User/Supervisor bits to be User-only
> +	 * (0b11 MSR[PR]=3D1) and set all other bits in DBCR2 register to be 0.
> +	 * This sets the Data Address Compare Effective/Real bits to be 0b00
> +	 * (Effective, MSR[DS]=3Ddon't care).
> +	 */
> +	thread->dbcr2 =3D DBCR2_BASE_REG_VALUE;
> +}
> +
> +static void prime_debug_regs(struct thread_struct *thread)
> +{
> +	mtspr(SPRN_IAC1, thread->iac1);
> +	mtspr(SPRN_IAC2, thread->iac2);
> +	mtspr(SPRN_IAC3, thread->iac3);
> +	mtspr(SPRN_IAC4, thread->iac4);
> +	mtspr(SPRN_DAC1, thread->dac1);
> +	mtspr(SPRN_DAC2, thread->dac2);
> +	mtspr(SPRN_DVC1, thread->dvc1);
> +	mtspr(SPRN_DVC2, thread->dvc2);
> +	mtspr(SPRN_DBCR0, thread->dbcr0);
> +	mtspr(SPRN_DBCR1, thread->dbcr1);
> +	mtspr(SPRN_DBCR2, thread->dbcr2);

As Josh pointed out, you'll need to be a little more careful here to
only mtspr() registers which actually exist on the current platform.
We may be better off only implementing the new interface for BookE in
the first cut.

> +}
> +/*
> + * Unless neither the old or new thread are making use of the
> + * debug registers, set the debug registers from the values
> + * stored in the new thread.
> + */
> +static void switch_booke_debug_regs(struct thread_struct *new_thread)
> +{
> +	if ((current->thread.dbcr0 & DBCR0_IDM)
> +		|| (new_thread->dbcr0 & DBCR0_IDM))
> +			prime_debug_regs(new_thread);
> +}
> +#endif
> +
>  int set_dabr(unsigned long dabr)
>  {
>  	__get_cpu_var(current_dabr) =3D dabr;
> @@ -284,7 +358,7 @@ int set_dabr(unsigned long dabr)
>  		return ppc_md.set_dabr(dabr);
> =20
>  	/* XXX should we have a CPU_FTR_HAS_DABR ? */
> -#if defined(CONFIG_BOOKE)
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
>  	mtspr(SPRN_DAC1, dabr);

Uh.. this would seem to be wrong.  set_dabr(0) is called from the
debug exception - but nowadays that could be tiggered by a DAC other
than DAC1.

>  #elif defined(CONFIG_PPC_BOOK3S)
>  	mtspr(SPRN_DABR, dabr);
> @@ -371,10 +445,8 @@ struct task_struct *__switch_to(struct task_struct *=
prev,
> =20
>  #endif /* CONFIG_SMP */
> =20
> -#if defined(CONFIG_BOOKE)
> -	/* If new thread DAC (HW breakpoint) is the same then leave it */
> -	if (new->thread.dabr)
> -		set_dabr(new->thread.dabr);
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +	switch_booke_debug_regs(&new->thread);
>  #else
>  	if (unlikely(__get_cpu_var(current_dabr) !=3D new->thread.dabr))
>  		set_dabr(new->thread.dabr);
> @@ -514,7 +586,7 @@ void show_regs(struct pt_regs * regs)
>  	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
>  	trap =3D TRAP(regs);
>  	if (trap =3D=3D 0x300 || trap =3D=3D 0x600)
> -#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
>  		printk("DEAR: "REG", ESR: "REG"\n", regs->dar, regs->dsisr);
>  #else
>  		printk("DAR: "REG", DSISR: "REG"\n", regs->dar, regs->dsisr);
> @@ -568,14 +640,19 @@ void flush_thread(void)
> =20
>  	discard_lazy_cpu_state();
> =20
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +	/*
> +	 * flush_thread() is called on exec() to reset the
> +	 * thread's status. Set all debug regs back to their
> +	 * default values....Torez
> +	 */
> +	set_debug_reg_defaults(&current->thread);

Better to define this function as a nop on non-BookE.

[snip]
> +#else
>  	/* As described above, it was assumed 3 bits were passed with the data
>  	 *  address, but we will assume only the mode bits will be passed
>  	 *  as to not cause alignment restrictions for DAC-based processors.
>  	 */
> =20
>  	/* DAC's hold the whole address without any mode flags */
> -	task->thread.dabr =3D data & ~0x3UL;
> -
> -	if (task->thread.dabr =3D=3D 0) {
> -		task->thread.dbcr0 &=3D ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> -		task->thread.regs->msr &=3D ~MSR_DE;
> +	task->thread.dac1 =3D data & ~0x3UL;
> +
> +	if (task->thread.dac1 =3D=3D 0) {
> +		dbcr_dac(task) &=3D ~(DBCR_DAC1R | DBCR_DAC1W);
> +		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
> +					task->thread.dbcr1)) {
> +			task->thread.regs->msr &=3D ~MSR_DE;
> +			task->thread.dbcr0 &=3D ~DBCR0_IDM;
> +		}
>  		return 0;
>  	}

Ok, so effectively the old ptrace method of setting the DABR acts as a
bypass to set DAC1, rather than having the old interface being
implemented via the new interface.  This has some weirdness - you can
clobber a new-style breakpoint in DAC1 using the old interface, for
example.  Still, it might be the simplest approach for a first cut.

What *is* a problem though, is that this means that the SIGTRAP will
always give a slot number, even for a breakpoint established using the
old interface.  Part of the idea of encoding the registered breakpoint
number in the siginfo was to be able to distinguish between old-style
and new-style breakpoints at trap time.

[snip]
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +static long set_intruction_bp(struct task_struct *child,
> +			      struct ppc_hw_breakpoint *bp_info)
> +{
> +	int slots_needed;
> +	int slot;
> +	int free_slot =3D 0;
> +
> +	/*
> +	 * Find an avalailable slot for the breakpoint.
> +	 * If possible, reserve consecutive slots, 1 & 2, for a range
> +	 * breakpoint.  (Can this be done simpler?)
> +	 */
> +	if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_EXACT)
> +		slots_needed =3D 1;
> +	else
> +		slots_needed =3D 2;

Ugh.  This logic is pretty hard to follow.  I'm having trouble
convincing myself it gets all the cases right, although I haven't
found something definitely wrong.

> +	if ((child->thread.dbcr0 & DBCR0_IAC1) =3D=3D 0) {
> +		if (slots_needed =3D=3D 1) {
> +			if (child->thread.dbcr0 & DBCR0_IAC2) {
> +				slot =3D 1;
> +				goto found;
> +			}
> +			/* Try to save slots 1 & 2 for range */
> +			free_slot =3D 1;
> +		} else
> +			if ((child->thread.dbcr0 & DBCR0_IAC2) =3D=3D 0) {
> +				slot =3D 1;
> +				goto found;
> +			}
> +	} else if ((slots_needed =3D=3D 1) &&
> +		   ((child->thread.dbcr0 & DBCR0_IAC2) =3D=3D 0)) {
> +		slot =3D 2;
> +		goto found;
> +	}
> +	if ((child->thread.dbcr0 & DBCR0_IAC3) =3D=3D 0) {
> +		if (slots_needed =3D=3D 1) {
> +			slot =3D 3;
> +			goto found;
> +		}
> +		if ((child->thread.dbcr0 & DBCR0_IAC4) =3D=3D 0) {
> +			slot =3D 3;
> +			goto found;
> +		}
> +		return -ENOSPC;
> +	} else if (slots_needed =3D=3D 2)
> +		return -ENOSPC;
> +	if ((child->thread.dbcr0 & DBCR0_IAC4) =3D=3D 0) {
> +		slot =3D 4;
> +	} else if (free_slot)
> +		slot =3D free_slot;
> +	else
> +		return -ENOSPC;
> +found:
> +	if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_EXACT) {
> +		switch (slot) {
> +			case 1:
> +				child->thread.iac1 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC1;
> +				break;
> +			case 2:
> +				child->thread.iac2 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC2;
> +				break;
> +			case 3:
> +				child->thread.iac3 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC3;
> +				break;
> +			case 4:
> +				child->thread.iac4 =3D bp_info->addr;
> +				child->thread.dbcr0 |=3D DBCR0_IAC4;
> +				break;

By using an array instead of individual iac1..4 in the thread struct,
plus a suitable macro to generate the right bit, you could get rid of
this nasty switch.

> +		}
> +	} else if (slot =3D=3D 1) {
> +		child->thread.iac1 =3D bp_info->addr;
> +		child->thread.iac2 =3D bp_info->addr2;

You test that addr is a user address in the caller, but I don't think
you ever check addr2.

> +		child->thread.dbcr0 |=3D (DBCR0_IAC1 | DBCR0_IAC2);

Uh.. I thought for range breakpoints you only needed to enable the bit
for the first of the two IACs (plus the range enable bit, of course).

> +		if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
> +			dbcr_iac_range(child) |=3D DBCR_IAC12M_X;
> +		else
> +			dbcr_iac_range(child) |=3D DBCR_IAC12M_I;
> +	} else { /* slot =3D=3D 3 */
> +		child->thread.iac3 =3D bp_info->addr;
> +		child->thread.iac4 =3D bp_info->addr2;
> +		child->thread.dbcr0 |=3D (DBCR0_IAC3 | DBCR0_IAC4);
> +		if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
> +			dbcr_iac_range(child) |=3D DBCR_IAC34M_X;
> +		else
> +			dbcr_iac_range(child) |=3D DBCR_IAC34M_I;
> +	}
> +	child->thread.dbcr0 |=3D DBCR0_IDM;
> +	child->thread.regs->msr |=3D MSR_DE;
> +
> +	return slot;

Yeah, ok.  I think the idea of using the register number as the
breakpoint identifier is limiting and we'll need to do something else
eventually.  Nonetheless, since userspace should be treating it as
opaque, it will do for a first cut and we can rework this later.

> +}
> +
> +static int del_instruction_bp(struct task_struct *child, int slot)
> +{
> +	switch (slot) {
> +	case 1:
> +		if (dbcr_iac_range(child) & DBCR_IAC12M) {
> +			/* address range - clear slots 1 & 2 */
> +			child->thread.iac2 =3D 0;
> +			child->thread.dbcr0 &=3D ~DBCR0_IAC2;
> +			dbcr_iac_range(child) &=3D ~DBCR_IAC12M;
> +		}
> +		child->thread.iac1 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC1;
> +		break;

Uh.. as far as I can tell, you won't return an error if you try to
clear a breakpoint from a slot that isn't used.  That seems like
incorrect behaviour.

> +	case 2:
> +		if (dbcr_iac_range(child) & DBCR_IAC12M)
> +			/* used in a range */
> +			return -EINVAL;
> +		child->thread.iac2 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC2;
> +		break;
> +	case 3:
> +		if (dbcr_iac_range(child) & DBCR_IAC34M) {
> +			/* address range - clear slots 3 & 4 */
> +			child->thread.iac4 =3D 0;
> +			child->thread.dbcr0 &=3D ~DBCR0_IAC4;
> +			dbcr_iac_range(child) &=3D ~DBCR_IAC34M;
> +		}
> +		child->thread.iac3 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC3;
> +		break;
> +	case 4:
> +		if (dbcr_iac_range(child) & DBCR_IAC34M)
> +			/* Used in a range */
> +			return -EINVAL;
> +		child->thread.iac4 =3D 0;
> +		child->thread.dbcr0 &=3D ~DBCR0_IAC4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int set_dac(struct task_struct *child, struct ppc_hw_breakpoint *=
bp_info)
> +{
> +	int byte_enable =3D
> +		(bp_info->condition_mode >> PPC_BREAKPOINT_CONDITION_BE_SHIFT)
> +		& 0xf;
> +	int condition_mode =3D
> +		bp_info->condition_mode & PPC_BREAKPOINT_CONDITION_AND_OR;

Using the fact that AND_OR is also equal to a suitable mask is a bit
overly subtle.  Better to use a separately defined mask constant.

> +	int slot;
> +
> +	if (byte_enable && (condition_mode =3D=3D 0))
> +		return -EINVAL;
> +
> +	if (bp_info->addr >=3D TASK_SIZE)
> +		return -EIO;
> +
> +	if ((dbcr_dac(child) & (DBCR_DAC1R | DBCR_DAC1W)) =3D=3D 0) {
> +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> +			dbcr_dac(child) |=3D DBCR_DAC1R;
> +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> +			dbcr_dac(child) |=3D DBCR_DAC1W;
> +		child->thread.dac1 =3D (unsigned long)bp_info->addr;
> +#ifdef CONFIG_BOOKE

Better to have a runtime feature bit test here, than use an #ifdef to
distinguish the 40x and BookE cases.  Plus, you should return an error
if the user attempts to use a feature not supported on this hardware,
which doesn't seem to happen here.

=18[snip]
> +static int del_dac(struct task_struct *child, int slot)
> +{

Again, you never seem to generate an error if you attempt to delete a
watchpoint that was never set.

> +	if (slot =3D=3D 1) {
> +#ifdef CONFIG_BOOKE
> +		if (child->thread.dbcr2 & DBCR2_DAC12MODE) {
> +			child->thread.dac1 =3D 0;
> +			child->thread.dac2 =3D 0;
> +			child->thread.dbcr0 &=3D ~(DBCR0_DAC1R | DBCR0_DAC1W |
> +						 DBCR0_DAC2R | DBCR0_DAC2W);
> +			child->thread.dbcr2 &=3D ~DBCR2_DAC12MODE;
> +			return 0;
> +		}
> +		child->thread.dbcr2 &=3D ~(DBCR2_DVC1M | DBCR2_DVC1BE);
> +		child->thread.dvc1 =3D 0;

Since this will just clear fields which are never used on 40x, you
shouldn't need the ifdef.

> +#endif
> +		child->thread.dac1 =3D 0;
> +		dbcr_dac(child) &=3D ~(DBCR_DAC1R | DBCR_DAC1W);
> +	} else if (slot =3D=3D 2) {
> +#ifdef CONFIG_BOOKE
> +		if (child->thread.dbcr2 & DBCR2_DAC12MODE)
> +			/* Part of a range */
> +			return -EINVAL;
> +		child->thread.dbcr2 &=3D ~(DBCR2_DVC2M | DBCR2_DVC2BE);
> +		child->thread.dvc2 =3D 0;
> +#endif
> +		child->thread.dac2 =3D 0;
> +		dbcr_dac(child) &=3D ~(DBCR_DAC2R | DBCR_DAC2W);
> +	} else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_40x || CONFIG_BOOKE */
> +
> +#ifdef CONFIG_BOOKE
> +static int set_dac_range(struct task_struct *child,
> +			 struct ppc_hw_breakpoint *bp_info)
> +{
> +	int mode =3D bp_info->addr_mode & PPC_BREAKPOINT_MODE_MASK;
> +
> +	/* We don't allow range watchpoints to be used with DVC */
> +	if (bp_info->condition_mode && PPC_BREAKPOINT_CONDITION_BE_ALL)

Uh.. that condition really doesn't look right.  First, surely it
should be a bitwise, not a logical and, second the comment is talking
about range watchpoints, but the condition is about the byte enable
bits.

> +		return -EINVAL;
> +
> +	if (bp_info->addr >=3D TASK_SIZE)
> +		return -EIO;
> +
> +	if (child->thread.dbcr0 &
> +	    (DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W))
> +		return -ENOSPC;
> +
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> +		child->thread.dbcr0 |=3D (DBCR0_DAC1R | DBCR0_DAC2R | DBCR0_IDM);
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> +		child->thread.dbcr0 |=3D (DBCR0_DAC1W | DBCR0_DAC2W | DBCR0_IDM);

Again, I thought only the DAC1 R/W bits were used for range watchpoints.

> +	child->thread.dac1 =3D bp_info->addr;
> +	child->thread.dac2 =3D bp_info->addr2;
> +	if (mode =3D=3D PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
> +		child->thread.dbcr2  |=3D DBCR2_DAC12R;
> +	else if (mode =3D=3D PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE)
> +		child->thread.dbcr2  |=3D DBCR2_DAC12RX;
> +	else	/* PPC_BREAKPOINT_MODE_MASK */
> +		child->thread.dbcr2  |=3D DBCR2_DAC12MASK;
> +	child->thread.regs->msr |=3D MSR_DE;
> +
> +	return 5;
> +}
> +#endif /* CONFIG_BOOKE */
> +
>  static long ppc_set_hwdebug(struct task_struct *child,
>  		     struct ppc_hw_breakpoint *bp_info)
>  {
> +	if (bp_info->version !=3D 1)
> +		return -ENOTSUPP;
> +
> +#ifdef CONFIG_BOOKE
> +	/*
> +	 * Check for invalid flags and combinations
> +	 */
> +	if ((bp_info->trigger_type =3D=3D 0) ||
> +	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
> +				       PPC_BREAKPOINT_TRIGGER_RW)) ||
> +	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
> +	    (bp_info->condition_mode &
> +	     ~(PPC_BREAKPOINT_CONDITION_AND_OR |
> +	       PPC_BREAKPOINT_CONDITION_BE_ALL)))
> +		return -EINVAL;
> +
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
> +		if (bp_info->trigger_type !=3D PPC_BREAKPOINT_TRIGGER_EXECUTE)
> +			/* At least another bit was set */
> +			return -EINVAL;
> +		return set_intruction_bp(child, bp_info);
> +	}
> +
> +	if (bp_info->addr_mode =3D=3D PPC_BREAKPOINT_MODE_EXACT)
> +		return set_dac(child, bp_info);
> +
> +	return set_dac_range(child, bp_info);
> +#elif defined(CONFIG_40x)
> +	/*
> +	 * Check for invalid flags and combinations
> +	 */
> +	if ((bp_info->trigger_type =3D=3D 0) ||
> +	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
> +				       PPC_BREAKPOINT_TRIGGER_RW)) ||
> +	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
> +	    (bp_info->condition_mode !=3D PPC_BREAKPOINT_CONDITION_NONE))
> +		return -EINVAL;
> +
> +	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
> +		if (bp_info->trigger_type !=3D PPC_BREAKPOINT_TRIGGER_EXECUTE)
> +			/* At least another bit was set */
> +			return -EINVAL;
> +		return set_intruction_bp(child, bp_info);
> +	}
> +	if (bp_info->addr_mode !=3D PPC_BREAKPOINT_MODE_EXACT)
> +		return -EINVAL;
> +
> +	return set_dac(child, bp_info);
> +#else
>  	/*
> -	 * We currently support one data breakpoint
> +	 * We only support one data breakpoint

Uh.. you've updated this comment to something which is still wrong
with the new code...

[snip]
> @@ -980,16 +1308,36 @@ long arch_ptrace(struct task_struct *child, long r=
equest, long addr, long data)
>  		struct ppc_debug_info dbginfo;
> =20
>  		dbginfo.version =3D 1;
> +#ifdef CONFIG_BOOKE
> +		dbginfo.num_instruction_bps =3D 4;
> +		dbginfo.num_data_bps =3D 2;
> +		dbginfo.num_condition_regs =3D 2;
> +		dbginfo.data_bp_alignment =3D 0;

Surely that can't be right, I thought there was always a data bp
alignment constraint.

> +		dbginfo.sizeof_condition =3D 4;
> +		dbginfo.features =3D PPC_DEBUG_FEATURE_INSN_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_INSN_BP_MASK |
> +				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
> +#elif defined(CONFIG_40x)
> +		/*
> +		 * I don't know how the DVCs work on 40x, I'm not going
> +		 * to support it now. -- Shaggy
> +		 */
> +		dbginfo.num_instruction_bps =3D 4;
> +		dbginfo.num_data_bps =3D 2;
> +		dbginfo.num_condition_regs =3D 0;
> +		dbginfo.data_bp_alignment =3D 0;
> +		dbginfo.sizeof_condition =3D 0;
> +		dbginfo.features =3D PPC_DEBUG_FEATURE_INSN_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_INSN_BP_MASK;
> +#else
>  		dbginfo.num_instruction_bps =3D 0;
>  		dbginfo.num_data_bps =3D 1;
>  		dbginfo.num_condition_regs =3D 0;
> -#ifdef CONFIG_PPC64
>  		dbginfo.data_bp_alignment =3D 8;

Uh, this isn't quite right.  32-bit classic PPC can have DABRs, so
!CONFIG_PPC64 does not imply 40x|BOOKE, which means we still need the
32-bit case here.

--=20
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

Re: [RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

[Dave Kleikamp]

On Fri, 2009-12-11 at 14:26 +1100, David Gibson wrote:
> On Thu, Dec 10, 2009 at 01:57:27PM -0200, Dave Kleikamp wrote:
> > powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace
> > 
> > From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
> > 
> > This patch defines context switch and trap related functionality
> > for BookE specific Debug Registers. It adds support to ptrace()
> > for setting and getting BookE related Debug Registers

> [snip]
> > +#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
> >  void do_dabr(struct pt_regs *regs, unsigned long address,
> >  		    unsigned long error_code)
> >  {
> > @@ -257,12 +275,6 @@ void do_dabr(struct pt_regs *regs, unsigned long address,
> >  	if (debugger_dabr_match(regs))
> >  		return;
> >  
> > -	/* Clear the DAC and struct entries.  One shot trigger */
> > -#if defined(CONFIG_BOOKE)
> > -	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> > -							| DBCR0_IDM));
> > -#endif
> > -
> >  	/* Clear the DABR */
> >  	set_dabr(0);
> 
> Uh.. does this imply we're keeping the one-shot behaviour for
> new-style breakpoints?  To me the interface really suggests they're
> persistent, although dealing with the semantics of that at signal time
> can get curly.

I've left it as one-shot.  The gdb guys seem happy with that.

[snip]

> >  int set_dabr(unsigned long dabr)
> >  {
> >  	__get_cpu_var(current_dabr) = dabr;
> > @@ -284,7 +358,7 @@ int set_dabr(unsigned long dabr)
> >  		return ppc_md.set_dabr(dabr);
> >  
> >  	/* XXX should we have a CPU_FTR_HAS_DABR ? */
> > -#if defined(CONFIG_BOOKE)
> > +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> >  	mtspr(SPRN_DAC1, dabr);
> 
> Uh.. this would seem to be wrong.  set_dabr(0) is called from the
> debug exception - but nowadays that could be tiggered by a DAC other
> than DAC1.

Right now, the only place left that set_dabr() is called for BOOKE or
40x is from xmon.  I think that's already a problem in that it doesn't
play nicely with ptrace and it fails to set DBCR0_DAC1R or DBCR0_DAC1W.
We probably need an independent patch for that.


> [snip]
> > +#else
> >  	/* As described above, it was assumed 3 bits were passed with the data
> >  	 *  address, but we will assume only the mode bits will be passed
> >  	 *  as to not cause alignment restrictions for DAC-based processors.
> >  	 */
> >  
> >  	/* DAC's hold the whole address without any mode flags */
> > -	task->thread.dabr = data & ~0x3UL;
> > -
> > -	if (task->thread.dabr == 0) {
> > -		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> > -		task->thread.regs->msr &= ~MSR_DE;
> > +	task->thread.dac1 = data & ~0x3UL;
> > +
> > +	if (task->thread.dac1 == 0) {
> > +		dbcr_dac(task) &= ~(DBCR_DAC1R | DBCR_DAC1W);
> > +		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
> > +					task->thread.dbcr1)) {
> > +			task->thread.regs->msr &= ~MSR_DE;
> > +			task->thread.dbcr0 &= ~DBCR0_IDM;
> > +		}
> >  		return 0;
> >  	}
> 
> Ok, so effectively the old ptrace method of setting the DABR acts as a
> bypass to set DAC1, rather than having the old interface being
> implemented via the new interface.  This has some weirdness - you can
> clobber a new-style breakpoint in DAC1 using the old interface, for
> example.  Still, it might be the simplest approach for a first cut.

I really didn't consider a program using both the old and new interface.

> What *is* a problem though, is that this means that the SIGTRAP will
> always give a slot number, even for a breakpoint established using the
> old interface.  Part of the idea of encoding the registered breakpoint
> number in the siginfo was to be able to distinguish between old-style
> and new-style breakpoints at trap time.

Do we realistically expect both the old and new style breakpoints to be
used together?  I'm having trouble visualizing the scenerio.

> [snip]

> > +	int slot;
> > +
> > +	if (byte_enable && (condition_mode == 0))
> > +		return -EINVAL;
> > +
> > +	if (bp_info->addr >= TASK_SIZE)
> > +		return -EIO;
> > +
> > +	if ((dbcr_dac(child) & (DBCR_DAC1R | DBCR_DAC1W)) == 0) {
> > +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> > +			dbcr_dac(child) |= DBCR_DAC1R;
> > +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> > +			dbcr_dac(child) |= DBCR_DAC1W;
> > +		child->thread.dac1 = (unsigned long)bp_info->addr;
> > +#ifdef CONFIG_BOOKE
> 
> Better to have a runtime feature bit test here, than use an #ifdef to
> distinguish the 40x and BookE cases.  Plus, you should return an error
> if the user attempts to use a feature not supported on this hardware,
> which doesn't seem to happen here.

I'll take a look at making this a runtime feature.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Kumar Gala]

On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:

> These patches implement an extention to the ptrace interface proposed =
by
> Thiago Bauermann and the the PowerPC gdb team.
>=20
> GDB intends to support the following hardware debug features of BookE
> processors:
>=20
> 4 hardware breakpoints (IAC)
> 2 hardware watchpoints (read, write and read-write) (DAC)
> 2 value conditions for the hardware watchpoints (DVC)
>=20
> For that, we need to extend ptrace so that GDB can query and set these
> resources. Since we're extending, we're trying to create an interface
> that's extendable and that covers both BookE and server processors, so
> that GDB doesn't need to special-case each of them. We propose the
> following 3 new ptrace requests described below.
>=20
> There have been discussions of a generic hardware debug interface for =
the
> kernel which would hopefully contemplate all the functionality below =
and
> supersede it.  But we need something that works now, and which enables =
GDB
> to be simpler and work with both Server and Embedded processors =
without
> special cases.
>=20
> 1. PTRACE_PPC_GETHWDEBUGINFO
>=20
> Query for GDB to discover the hardware debug features. The main info =
to
> be returned here is the minimum alignment for the hardware =
watchpoints.
> BookE processors don't have restrictions here, but server processors =
have
> an 8-byte alignment restriction for hardware watchpoints. We'd like to =
avoid
> adding special cases to GDB based on what it sees in AUXV.
>=20
> Since we're at it, we added other useful info that the kernel can =
return to
> GDB: this query will return the number of hardware breakpoints, =
hardware
> watchpoints and whether it supports a range of addresses and a =
condition.
> The query will fill the following structure provided by the requesting =
process:
>=20
> struct ppc_debug_info {
>       unit32_t version;
>       unit32_t num_instruction_bps;
>       unit32_t num_data_bps;
>       unit32_t num_condition_regs;
>       unit32_t data_bp_alignment;
>       unit32_t sizeof_condition; /* size of the DVC register */
>       uint64_t features; /* bitmask of the individual flags */
> };
>=20
> features will have bits indicating whether there is support for:
>=20
> #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8

Is GDB smart enough to deal w/no condition_regs?  On some Book-E devices =
we have 2 IACs, 2 DACs, and 0 DVCs.  Does it need to be in the features?

- k=

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

On Thu, 2009-12-10 at 20:23 -0600, Kumar Gala wrote:
> On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
> 
> > These patches implement an extention to the ptrace interface proposed by
> > Thiago Bauermann and the the PowerPC gdb team.
> > 
> > GDB intends to support the following hardware debug features of BookE
> > processors:
> > 
> > 4 hardware breakpoints (IAC)
> > 2 hardware watchpoints (read, write and read-write) (DAC)
> > 2 value conditions for the hardware watchpoints (DVC)
> > 
> > For that, we need to extend ptrace so that GDB can query and set these
> > resources. Since we're extending, we're trying to create an interface
> > that's extendable and that covers both BookE and server processors, so
> > that GDB doesn't need to special-case each of them. We propose the
> > following 3 new ptrace requests described below.
> > 
> > There have been discussions of a generic hardware debug interface for the
> > kernel which would hopefully contemplate all the functionality below and
> > supersede it.  But we need something that works now, and which enables GDB
> > to be simpler and work with both Server and Embedded processors without
> > special cases.
> > 
> > 1. PTRACE_PPC_GETHWDEBUGINFO
> > 
> > Query for GDB to discover the hardware debug features. The main info to
> > be returned here is the minimum alignment for the hardware watchpoints.
> > BookE processors don't have restrictions here, but server processors have
> > an 8-byte alignment restriction for hardware watchpoints. We'd like to avoid
> > adding special cases to GDB based on what it sees in AUXV.
> > 
> > Since we're at it, we added other useful info that the kernel can return to
> > GDB: this query will return the number of hardware breakpoints, hardware
> > watchpoints and whether it supports a range of addresses and a condition.
> > The query will fill the following structure provided by the requesting process:
> > 
> > struct ppc_debug_info {
> >       unit32_t version;
> >       unit32_t num_instruction_bps;
> >       unit32_t num_data_bps;
> >       unit32_t num_condition_regs;
> >       unit32_t data_bp_alignment;
> >       unit32_t sizeof_condition; /* size of the DVC register */
> >       uint64_t features; /* bitmask of the individual flags */
> > };
> > 
> > features will have bits indicating whether there is support for:
> > 
> > #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> > #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> > #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> > #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
> 
> Is GDB smart enough to deal w/no condition_regs?  On some Book-E
> devices we have 2 IACs, 2 DACs, and 0 DVCs.  Does it need to be in the
> features?

I had discussed it with the gdb team.  I could easily add a feature
flag, but it would be equivalent to num_condition_regs > 0.  I don't
have a strong opinion either way.
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Thiago Jung Bauermann]

On Fri 11 Dec 2009 00:27:36 Dave Kleikamp wrote:
> On Thu, 2009-12-10 at 20:23 -0600, Kumar Gala wrote:
> > On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
> > > #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> > > #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> > > #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> > > #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
> >
> > Is GDB smart enough to deal w/no condition_regs?  On some Book-E
> > devices we have 2 IACs, 2 DACs, and 0 DVCs.  Does it need to be in the
> > features?
> 
> I had discussed it with the gdb team.  I could easily add a feature
> flag, but it would be equivalent to num_condition_regs > 0.  I don't
> have a strong opinion either way.

The current GDB code we have here uses num_condition_regs > 0 to discover if 
DVCs are supported, so a PPC_DEBUG_FEATURE constant for that is redundant 
IMHO.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Kumar Gala]

On Dec 11, 2009, at 2:07 PM, Thiago Jung Bauermann wrote:

> On Fri 11 Dec 2009 00:27:36 Dave Kleikamp wrote:
>> On Thu, 2009-12-10 at 20:23 -0600, Kumar Gala wrote:
>>> On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
>>>> #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
>>>> #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
>>>> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
>>>> #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
>>>=20
>>> Is GDB smart enough to deal w/no condition_regs?  On some Book-E
>>> devices we have 2 IACs, 2 DACs, and 0 DVCs.  Does it need to be in =
the
>>> features?
>>=20
>> I had discussed it with the gdb team.  I could easily add a feature
>> flag, but it would be equivalent to num_condition_regs > 0.  I don't
>> have a strong opinion either way.
>=20
> The current GDB code we have here uses num_condition_regs > 0 to =
discover if=20
> DVCs are supported, so a PPC_DEBUG_FEATURE constant for that is =
redundant=20
> IMHO.

That's fine, just want to make sure we are ok w/num_condition_regs =3D=3D =
0.

- k=

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

On Thu, 2009-12-10 at 20:23 -0600, Kumar Gala wrote:
> On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
> 
> > These patches implement an extention to the ptrace interface proposed by
> > Thiago Bauermann and the the PowerPC gdb team.
> > 
> > GDB intends to support the following hardware debug features of BookE
> > processors:
> > 
> > 4 hardware breakpoints (IAC)
> > 2 hardware watchpoints (read, write and read-write) (DAC)
> > 2 value conditions for the hardware watchpoints (DVC)
> > 
> > For that, we need to extend ptrace so that GDB can query and set these
> > resources. Since we're extending, we're trying to create an interface
> > that's extendable and that covers both BookE and server processors, so
> > that GDB doesn't need to special-case each of them. We propose the
> > following 3 new ptrace requests described below.
> > 
> > There have been discussions of a generic hardware debug interface for the
> > kernel which would hopefully contemplate all the functionality below and
> > supersede it.  But we need something that works now, and which enables GDB
> > to be simpler and work with both Server and Embedded processors without
> > special cases.
> > 
> > 1. PTRACE_PPC_GETHWDEBUGINFO
> > 
> > Query for GDB to discover the hardware debug features. The main info to
> > be returned here is the minimum alignment for the hardware watchpoints.
> > BookE processors don't have restrictions here, but server processors have
> > an 8-byte alignment restriction for hardware watchpoints. We'd like to avoid
> > adding special cases to GDB based on what it sees in AUXV.
> > 
> > Since we're at it, we added other useful info that the kernel can return to
> > GDB: this query will return the number of hardware breakpoints, hardware
> > watchpoints and whether it supports a range of addresses and a condition.
> > The query will fill the following structure provided by the requesting process:
> > 
> > struct ppc_debug_info {
> >       unit32_t version;
> >       unit32_t num_instruction_bps;
> >       unit32_t num_data_bps;
> >       unit32_t num_condition_regs;
> >       unit32_t data_bp_alignment;
> >       unit32_t sizeof_condition; /* size of the DVC register */
> >       uint64_t features; /* bitmask of the individual flags */
> > };
> > 
> > features will have bits indicating whether there is support for:
> > 
> > #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> > #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> > #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> > #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
> 
> Is GDB smart enough to deal w/no condition_regs?  On some Book-E
> devices we have 2 IACs, 2 DACs, and 0 DVCs.  Does it need to be in the
> features?

I wasn't aware that the bookE devices had varying numbers of these
registers.  I guess I will have to make it a runtime option.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

On Mon, 2010-01-18 at 16:34 -0600, Dave Kleikamp wrote:
> On Thu, 2009-12-10 at 20:23 -0600, Kumar Gala wrote:

> > Is GDB smart enough to deal w/no condition_regs?  On some Book-E
> > devices we have 2 IACs, 2 DACs, and 0 DVCs.  Does it need to be in the
> > features?
> 
> I wasn't aware that the bookE devices had varying numbers of these
> registers.  I guess I will have to make it a runtime option.

Kumar,

Can you tell me which bookE processors have 2 IAC's, and which have no
DVC's?

I think we still may be able to make these compile-time options as long
no two cpus that run on the same binary kernel vary in the number of
registers.  Right now I know the 403 only has 2 IAC's, and I don't
intend to expose the DVC's for the 40x processors anyway.

If they don't need to be run-time configurable, I think it would be
cleaner to define the number of each type of register in CONFIG_ flags
and put the logic into the Kconfig files.

Thanks,
Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Kumar Gala]

On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:

> These patches implement an extention to the ptrace interface proposed =
by
> Thiago Bauermann and the the PowerPC gdb team.
>=20
> GDB intends to support the following hardware debug features of BookE
> processors:
>=20
> 4 hardware breakpoints (IAC)
> 2 hardware watchpoints (read, write and read-write) (DAC)
> 2 value conditions for the hardware watchpoints (DVC)
>=20
> For that, we need to extend ptrace so that GDB can query and set these
> resources. Since we're extending, we're trying to create an interface
> that's extendable and that covers both BookE and server processors, so
> that GDB doesn't need to special-case each of them. We propose the
> following 3 new ptrace requests described below.
>=20
> There have been discussions of a generic hardware debug interface for =
the
> kernel which would hopefully contemplate all the functionality below =
and
> supersede it.  But we need something that works now, and which enables =
GDB
> to be simpler and work with both Server and Embedded processors =
without
> special cases.
>=20
> 1. PTRACE_PPC_GETHWDEBUGINFO
>=20
> Query for GDB to discover the hardware debug features. The main info =
to
> be returned here is the minimum alignment for the hardware =
watchpoints.
> BookE processors don't have restrictions here, but server processors =
have
> an 8-byte alignment restriction for hardware watchpoints. We'd like to =
avoid
> adding special cases to GDB based on what it sees in AUXV.
>=20
> Since we're at it, we added other useful info that the kernel can =
return to
> GDB: this query will return the number of hardware breakpoints, =
hardware
> watchpoints and whether it supports a range of addresses and a =
condition.
> The query will fill the following structure provided by the requesting =
process:
>=20
> struct ppc_debug_info {
>       unit32_t version;
>       unit32_t num_instruction_bps;
>       unit32_t num_data_bps;
>       unit32_t num_condition_regs;
>       unit32_t data_bp_alignment;
>       unit32_t sizeof_condition; /* size of the DVC register */
>       uint64_t features; /* bitmask of the individual flags */
> };
>=20
> features will have bits indicating whether there is support for:
>=20
> #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
>=20
> 2. PTRACE_SETHWDEBUG
>=20
> Sets a hardware breakpoint or watchpoint, according to the provided =
structure:
>=20
> struct ppc_hw_breakpoint {
>        uint32_t version;
> #define PPC_BREAKPOINT_TRIGGER_EXECUTE  0x1
> #define PPC_BREAKPOINT_TRIGGER_READ     0x2
> #define PPC_BREAKPOINT_TRIGGER_WRITE    0x4
>        uint32_t trigger_type;       /* only some combinations allowed =
*/
> #define PPC_BREAKPOINT_MODE_EXACT               0x0
> #define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE     0x1
> #define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE     0x2
> #define PPC_BREAKPOINT_MODE_MASK                0x3
>        uint32_t addr_mode;          /* address match mode */
>=20
> #define PPC_BREAKPOINT_CONDITION_NONE   0x0
> #define PPC_BREAKPOINT_CONDITION_AND    0x1
> #define PPC_BREAKPOINT_CONDITION_EXACT  0x1	/* different name for =
the same thing as above */
> #define PPC_BREAKPOINT_CONDITION_OR     0x2
> #define PPC_BREAKPOINT_CONDITION_AND_OR 0x3
> #define PPC_BREAKPOINT_CONDITION_BE_ALL 0x00ff0000	/* byte enable =
bits */
> #define PPC_BREAKPOINT_CONDITION_BE(n)  (1<<((n)+16))
>        uint32_t condition_mode;     /* break/watchpoint condition =
flags */
>=20
>        uint64_t addr;
>        uint64_t addr2;
>        uint64_t condition_value;
> };
>=20
> A request specifies one event, not necessarily just one register to be =
set.
> For instance, if the request is for a watchpoint with a condition, =
both the
> DAC and DVC registers will be set in the same request.
>=20
> With this GDB can ask for all kinds of hardware breakpoints and =
watchpoints
> that the BookE supports. COMEFROM breakpoints available in server =
processors
> are not contemplated, but that is out of the scope of this work.
>=20
> ptrace will return an integer (handle) uniquely identifying the =
breakpoint or
> watchpoint just created. This integer will be used in the =
PTRACE_DELHWDEBUG
> request to ask for its removal. Return -ENOSPC if the requested =
breakpoint
> can't be allocated on the registers.
>=20
> Some examples of using the structure to:
>=20
> - set a breakpoint in the first breakpoint register
>=20
>  p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>  p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_EXECUTE;
>  p.addr_mode       =3D PPC_BREAKPOINT_MODE_EXACT;
>  p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_NONE;
>  p.addr            =3D (uint64_t) address;
>  p.addr2           =3D 0;
>  p.condition_value =3D 0;
>=20
> - set a watchpoint which triggers on reads in the second watchpoint =
register
>=20
>  p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>  p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_READ;
>  p.addr_mode       =3D PPC_BREAKPOINT_MODE_EXACT;
>  p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_NONE;
>  p.addr            =3D (uint64_t) address;
>  p.addr2           =3D 0;
>  p.condition_value =3D 0;
>=20
> - set a watchpoint which triggers only with a specific value
>=20
>  p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>  p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_READ;
>  p.addr_mode       =3D PPC_BREAKPOINT_MODE_EXACT;
>  p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_AND | =
PPC_BREAKPOINT_CONDITION_BE_ALL;
>  p.addr            =3D (uint64_t) address;
>  p.addr2           =3D 0;
>  p.condition_value =3D (uint64_t) condition;
>=20
> - set a ranged hardware breakpoint
>=20
>  p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>  p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_EXECUTE;
>  p.addr_mode       =3D PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
>  p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_NONE;
>  p.addr            =3D (uint64_t) begin_range;
>  p.addr2           =3D (uint64_t) end_range;
>  p.condition_value =3D 0;
>=20
> 3. PTRACE_DELHWDEBUG
>=20
> Takes an integer which identifies an existing breakpoint or watchpoint
> (i.e., the value returned from PTRACE_SETHWDEBUG), and deletes the
> corresponding breakpoint or watchpoint..

This is a good write up.  We should have it as a commit message for the =
first patch.

- k

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

On Thu, 2009-12-10 at 20:24 -0600, Kumar Gala wrote:
> On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
> 
> > These patches implement an extention to the ptrace interface proposed by
> > Thiago Bauermann and the the PowerPC gdb team.
> > 
> > GDB intends to support the following hardware debug features of BookE
> > processors:
> > 
> > 4 hardware breakpoints (IAC)
> > 2 hardware watchpoints (read, write and read-write) (DAC)
> > 2 value conditions for the hardware watchpoints (DVC)
> > 
> > For that, we need to extend ptrace so that GDB can query and set these
> > resources. Since we're extending, we're trying to create an interface
> > that's extendable and that covers both BookE and server processors, so
> > that GDB doesn't need to special-case each of them. We propose the
> > following 3 new ptrace requests described below.
> > 
> > There have been discussions of a generic hardware debug interface for the
> > kernel which would hopefully contemplate all the functionality below and
> > supersede it.  But we need something that works now, and which enables GDB
> > to be simpler and work with both Server and Embedded processors without
> > special cases.
> > 
> > 1. PTRACE_PPC_GETHWDEBUGINFO
> > 
> > Query for GDB to discover the hardware debug features. The main info to
> > be returned here is the minimum alignment for the hardware watchpoints.
> > BookE processors don't have restrictions here, but server processors have
> > an 8-byte alignment restriction for hardware watchpoints. We'd like to avoid
> > adding special cases to GDB based on what it sees in AUXV.
> > 
> > Since we're at it, we added other useful info that the kernel can return to
> > GDB: this query will return the number of hardware breakpoints, hardware
> > watchpoints and whether it supports a range of addresses and a condition.
> > The query will fill the following structure provided by the requesting process:
> > 
> > struct ppc_debug_info {
> >       unit32_t version;
> >       unit32_t num_instruction_bps;
> >       unit32_t num_data_bps;
> >       unit32_t num_condition_regs;
> >       unit32_t data_bp_alignment;
> >       unit32_t sizeof_condition; /* size of the DVC register */
> >       uint64_t features; /* bitmask of the individual flags */
> > };
> > 
> > features will have bits indicating whether there is support for:
> > 
> > #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
> > #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
> > #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
> > #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
> > 
> > 2. PTRACE_SETHWDEBUG
> > 
> > Sets a hardware breakpoint or watchpoint, according to the provided structure:
> > 
> > struct ppc_hw_breakpoint {
> >        uint32_t version;
> > #define PPC_BREAKPOINT_TRIGGER_EXECUTE  0x1
> > #define PPC_BREAKPOINT_TRIGGER_READ     0x2
> > #define PPC_BREAKPOINT_TRIGGER_WRITE    0x4
> >        uint32_t trigger_type;       /* only some combinations allowed */
> > #define PPC_BREAKPOINT_MODE_EXACT               0x0
> > #define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE     0x1
> > #define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE     0x2
> > #define PPC_BREAKPOINT_MODE_MASK                0x3
> >        uint32_t addr_mode;          /* address match mode */
> > 
> > #define PPC_BREAKPOINT_CONDITION_NONE   0x0
> > #define PPC_BREAKPOINT_CONDITION_AND    0x1
> > #define PPC_BREAKPOINT_CONDITION_EXACT  0x1	/* different name for the same thing as above */
> > #define PPC_BREAKPOINT_CONDITION_OR     0x2
> > #define PPC_BREAKPOINT_CONDITION_AND_OR 0x3
> > #define PPC_BREAKPOINT_CONDITION_BE_ALL 0x00ff0000	/* byte enable bits */
> > #define PPC_BREAKPOINT_CONDITION_BE(n)  (1<<((n)+16))
> >        uint32_t condition_mode;     /* break/watchpoint condition flags */
> > 
> >        uint64_t addr;
> >        uint64_t addr2;
> >        uint64_t condition_value;
> > };
> > 
> > A request specifies one event, not necessarily just one register to be set.
> > For instance, if the request is for a watchpoint with a condition, both the
> > DAC and DVC registers will be set in the same request.
> > 
> > With this GDB can ask for all kinds of hardware breakpoints and watchpoints
> > that the BookE supports. COMEFROM breakpoints available in server processors
> > are not contemplated, but that is out of the scope of this work.
> > 
> > ptrace will return an integer (handle) uniquely identifying the breakpoint or
> > watchpoint just created. This integer will be used in the PTRACE_DELHWDEBUG
> > request to ask for its removal. Return -ENOSPC if the requested breakpoint
> > can't be allocated on the registers.
> > 
> > Some examples of using the structure to:
> > 
> > - set a breakpoint in the first breakpoint register
> > 
> >  p.version         = PPC_DEBUG_CURRENT_VERSION;
> >  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
> >  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
> >  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> >  p.addr            = (uint64_t) address;
> >  p.addr2           = 0;
> >  p.condition_value = 0;
> > 
> > - set a watchpoint which triggers on reads in the second watchpoint register
> > 
> >  p.version         = PPC_DEBUG_CURRENT_VERSION;
> >  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_READ;
> >  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
> >  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> >  p.addr            = (uint64_t) address;
> >  p.addr2           = 0;
> >  p.condition_value = 0;
> > 
> > - set a watchpoint which triggers only with a specific value
> > 
> >  p.version         = PPC_DEBUG_CURRENT_VERSION;
> >  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_READ;
> >  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
> >  p.condition_mode  = PPC_BREAKPOINT_CONDITION_AND | PPC_BREAKPOINT_CONDITION_BE_ALL;
> >  p.addr            = (uint64_t) address;
> >  p.addr2           = 0;
> >  p.condition_value = (uint64_t) condition;
> > 
> > - set a ranged hardware breakpoint
> > 
> >  p.version         = PPC_DEBUG_CURRENT_VERSION;
> >  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
> >  p.addr_mode       = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
> >  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> >  p.addr            = (uint64_t) begin_range;
> >  p.addr2           = (uint64_t) end_range;
> >  p.condition_value = 0;
> > 
> > 3. PTRACE_DELHWDEBUG
> > 
> > Takes an integer which identifies an existing breakpoint or watchpoint
> > (i.e., the value returned from PTRACE_SETHWDEBUG), and deletes the
> > corresponding breakpoint or watchpoint..
> 
> This is a good write up.  We should have it as a commit message for the first patch.

I have to give credit to Thiago for this.

Would it be worth adding to Documentation/powerpc/ ?
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Kumar Gala]

On Dec 10, 2009, at 8:29 PM, Dave Kleikamp wrote:

> On Thu, 2009-12-10 at 20:24 -0600, Kumar Gala wrote:
>> On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:
>>=20
>>> These patches implement an extention to the ptrace interface =
proposed by
>>> Thiago Bauermann and the the PowerPC gdb team.
>>>=20
>>> GDB intends to support the following hardware debug features of =
BookE
>>> processors:
>>>=20
>>> 4 hardware breakpoints (IAC)
>>> 2 hardware watchpoints (read, write and read-write) (DAC)
>>> 2 value conditions for the hardware watchpoints (DVC)
>>>=20
>>> For that, we need to extend ptrace so that GDB can query and set =
these
>>> resources. Since we're extending, we're trying to create an =
interface
>>> that's extendable and that covers both BookE and server processors, =
so
>>> that GDB doesn't need to special-case each of them. We propose the
>>> following 3 new ptrace requests described below.
>>>=20
>>> There have been discussions of a generic hardware debug interface =
for the
>>> kernel which would hopefully contemplate all the functionality below =
and
>>> supersede it.  But we need something that works now, and which =
enables GDB
>>> to be simpler and work with both Server and Embedded processors =
without
>>> special cases.
>>>=20
>>> 1. PTRACE_PPC_GETHWDEBUGINFO
>>>=20
>>> Query for GDB to discover the hardware debug features. The main info =
to
>>> be returned here is the minimum alignment for the hardware =
watchpoints.
>>> BookE processors don't have restrictions here, but server processors =
have
>>> an 8-byte alignment restriction for hardware watchpoints. We'd like =
to avoid
>>> adding special cases to GDB based on what it sees in AUXV.
>>>=20
>>> Since we're at it, we added other useful info that the kernel can =
return to
>>> GDB: this query will return the number of hardware breakpoints, =
hardware
>>> watchpoints and whether it supports a range of addresses and a =
condition.
>>> The query will fill the following structure provided by the =
requesting process:
>>>=20
>>> struct ppc_debug_info {
>>>      unit32_t version;
>>>      unit32_t num_instruction_bps;
>>>      unit32_t num_data_bps;
>>>      unit32_t num_condition_regs;
>>>      unit32_t data_bp_alignment;
>>>      unit32_t sizeof_condition; /* size of the DVC register */
>>>      uint64_t features; /* bitmask of the individual flags */
>>> };
>>>=20
>>> features will have bits indicating whether there is support for:
>>>=20
>>> #define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
>>> #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
>>> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
>>> #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8
>>>=20
>>> 2. PTRACE_SETHWDEBUG
>>>=20
>>> Sets a hardware breakpoint or watchpoint, according to the provided =
structure:
>>>=20
>>> struct ppc_hw_breakpoint {
>>>       uint32_t version;
>>> #define PPC_BREAKPOINT_TRIGGER_EXECUTE  0x1
>>> #define PPC_BREAKPOINT_TRIGGER_READ     0x2
>>> #define PPC_BREAKPOINT_TRIGGER_WRITE    0x4
>>>       uint32_t trigger_type;       /* only some combinations allowed =
*/
>>> #define PPC_BREAKPOINT_MODE_EXACT               0x0
>>> #define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE     0x1
>>> #define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE     0x2
>>> #define PPC_BREAKPOINT_MODE_MASK                0x3
>>>       uint32_t addr_mode;          /* address match mode */
>>>=20
>>> #define PPC_BREAKPOINT_CONDITION_NONE   0x0
>>> #define PPC_BREAKPOINT_CONDITION_AND    0x1
>>> #define PPC_BREAKPOINT_CONDITION_EXACT  0x1	/* different name for =
the same thing as above */
>>> #define PPC_BREAKPOINT_CONDITION_OR     0x2
>>> #define PPC_BREAKPOINT_CONDITION_AND_OR 0x3
>>> #define PPC_BREAKPOINT_CONDITION_BE_ALL 0x00ff0000	/* byte enable =
bits */
>>> #define PPC_BREAKPOINT_CONDITION_BE(n)  (1<<((n)+16))
>>>       uint32_t condition_mode;     /* break/watchpoint condition =
flags */
>>>=20
>>>       uint64_t addr;
>>>       uint64_t addr2;
>>>       uint64_t condition_value;
>>> };
>>>=20
>>> A request specifies one event, not necessarily just one register to =
be set.
>>> For instance, if the request is for a watchpoint with a condition, =
both the
>>> DAC and DVC registers will be set in the same request.
>>>=20
>>> With this GDB can ask for all kinds of hardware breakpoints and =
watchpoints
>>> that the BookE supports. COMEFROM breakpoints available in server =
processors
>>> are not contemplated, but that is out of the scope of this work.
>>>=20
>>> ptrace will return an integer (handle) uniquely identifying the =
breakpoint or
>>> watchpoint just created. This integer will be used in the =
PTRACE_DELHWDEBUG
>>> request to ask for its removal. Return -ENOSPC if the requested =
breakpoint
>>> can't be allocated on the registers.
>>>=20
>>> Some examples of using the structure to:
>>>=20
>>> - set a breakpoint in the first breakpoint register
>>>=20
>>> p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>>> p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_EXECUTE;
>>> p.addr_mode       =3D PPC_BREAKPOINT_MODE_EXACT;
>>> p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_NONE;
>>> p.addr            =3D (uint64_t) address;
>>> p.addr2           =3D 0;
>>> p.condition_value =3D 0;
>>>=20
>>> - set a watchpoint which triggers on reads in the second watchpoint =
register
>>>=20
>>> p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>>> p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_READ;
>>> p.addr_mode       =3D PPC_BREAKPOINT_MODE_EXACT;
>>> p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_NONE;
>>> p.addr            =3D (uint64_t) address;
>>> p.addr2           =3D 0;
>>> p.condition_value =3D 0;
>>>=20
>>> - set a watchpoint which triggers only with a specific value
>>>=20
>>> p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>>> p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_READ;
>>> p.addr_mode       =3D PPC_BREAKPOINT_MODE_EXACT;
>>> p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_AND | =
PPC_BREAKPOINT_CONDITION_BE_ALL;
>>> p.addr            =3D (uint64_t) address;
>>> p.addr2           =3D 0;
>>> p.condition_value =3D (uint64_t) condition;
>>>=20
>>> - set a ranged hardware breakpoint
>>>=20
>>> p.version         =3D PPC_DEBUG_CURRENT_VERSION;
>>> p.trigger_type    =3D PPC_BREAKPOINT_TRIGGER_EXECUTE;
>>> p.addr_mode       =3D PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
>>> p.condition_mode  =3D PPC_BREAKPOINT_CONDITION_NONE;
>>> p.addr            =3D (uint64_t) begin_range;
>>> p.addr2           =3D (uint64_t) end_range;
>>> p.condition_value =3D 0;
>>>=20
>>> 3. PTRACE_DELHWDEBUG
>>>=20
>>> Takes an integer which identifies an existing breakpoint or =
watchpoint
>>> (i.e., the value returned from PTRACE_SETHWDEBUG), and deletes the
>>> corresponding breakpoint or watchpoint..
>>=20
>> This is a good write up.  We should have it as a commit message for =
the first patch.
>=20
> I have to give credit to Thiago for this.
>=20
> Would it be worth adding to Documentation/powerpc/ ?

That would also work.

- k=

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Benjamin Herrenschmidt]

On Thu, 2009-12-10 at 20:24 -0600, Kumar Gala wrote:
> This is a good write up.  We should have it as a commit message for
> the first patch.

No, that should go into Documentation/powerpc/

Cheers,
Ben.

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Kumar Gala]

On Dec 10, 2009, at 9:57 AM, Dave Kleikamp wrote:

> These patches implement an extention to the ptrace interface proposed =
by
> Thiago Bauermann and the the PowerPC gdb team.
>=20
> GDB intends to support the following hardware debug features of BookE
> processors:
>=20
> 4 hardware breakpoints (IAC)
> 2 hardware watchpoints (read, write and read-write) (DAC)
> 2 value conditions for the hardware watchpoints (DVC)
>=20
> For that, we need to extend ptrace so that GDB can query and set these
> resources. Since we're extending, we're trying to create an interface
> that's extendable and that covers both BookE and server processors, so
> that GDB doesn't need to special-case each of them. We propose the
> following 3 new ptrace requests described below.
>=20
> There have been discussions of a generic hardware debug interface for =
the
> kernel which would hopefully contemplate all the functionality below =
and
> supersede it.  But we need something that works now, and which enables =
GDB
> to be simpler and work with both Server and Embedded processors =
without
> special cases.

What do we do in EDM mode?  We need a flag somewhere to determine if HW =
supports conveying DBCR0[EDM] and if it does which of the ptrace calls =
fails?

- k=

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

On Thu, 2009-12-10 at 20:45 -0600, Kumar Gala wrote:

> What do we do in EDM mode?  We need a flag somewhere to determine if
> HW supports conveying DBCR0[EDM] and if it does which of the ptrace
> calls fails?

I really don't have a good answer to this.  I'm open to any and all
advice.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

[RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Dave Kleikamp]

These patches implement an extention to the ptrace interface proposed by
Thiago Bauermann and the the PowerPC gdb team.

This is a resubmission of the patches I first sent on December 9th.
Sorry for the delay.  I have addressed most of the comments, and I'll
follow up on the others.

These patches are based on 2.6.33-rc3.  (Nothing really changed in powerpc
between rc3 & rc4.)  These patches still need some testing, but I want
to get the ball rolling again.

The following has been added as Documentation/powerpc/ptrace.txt:

GDB intends to support the following hardware debug features of BookE
processors:

4 hardware breakpoints (IAC)
2 hardware watchpoints (read, write and read-write) (DAC)
2 value conditions for the hardware watchpoints (DVC)

For that, we need to extend ptrace so that GDB can query and set these
resources. Since we're extending, we're trying to create an interface
that's extendable and that covers both BookE and server processors, so
that GDB doesn't need to special-case each of them. We added the
following 3 new ptrace requests.

1. PTRACE_PPC_GETHWDEBUGINFO

Query for GDB to discover the hardware debug features. The main info to
be returned here is the minimum alignment for the hardware watchpoints.
BookE processors don't have restrictions here, but server processors have
an 8-byte alignment restriction for hardware watchpoints. We'd like to avoid
adding special cases to GDB based on what it sees in AUXV.

Since we're at it, we added other useful info that the kernel can return to
GDB: this query will return the number of hardware breakpoints, hardware
watchpoints and whether it supports a range of addresses and a condition.
The query will fill the following structure provided by the requesting process:

struct ppc_debug_info {
       unit32_t version;
       unit32_t num_instruction_bps;
       unit32_t num_data_bps;
       unit32_t num_condition_regs;
       unit32_t data_bp_alignment;
       unit32_t sizeof_condition; /* size of the DVC register */
       uint64_t features; /* bitmask of the individual flags */
};

features will have bits indicating whether there is support for:

#define PPC_DEBUG_FEATURE_INSN_BP_RANGE		0x1
#define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x2
#define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x4
#define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x8

2. PTRACE_SETHWDEBUG

Sets a hardware breakpoint or watchpoint, according to the provided structure:

struct ppc_hw_breakpoint {
        uint32_t version;
#define PPC_BREAKPOINT_TRIGGER_EXECUTE  0x1
#define PPC_BREAKPOINT_TRIGGER_READ     0x2
#define PPC_BREAKPOINT_TRIGGER_WRITE    0x4
        uint32_t trigger_type;       /* only some combinations allowed */
#define PPC_BREAKPOINT_MODE_EXACT               0x0
#define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE     0x1
#define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE     0x2
#define PPC_BREAKPOINT_MODE_MASK                0x3
        uint32_t addr_mode;          /* address match mode */

#define PPC_BREAKPOINT_CONDITION_MODE   0x3
#define PPC_BREAKPOINT_CONDITION_NONE   0x0
#define PPC_BREAKPOINT_CONDITION_AND    0x1
#define PPC_BREAKPOINT_CONDITION_EXACT  0x1	/* different name for the same thing as above */
#define PPC_BREAKPOINT_CONDITION_OR     0x2
#define PPC_BREAKPOINT_CONDITION_AND_OR 0x3
#define PPC_BREAKPOINT_CONDITION_BE_ALL 0x00ff0000	/* byte enable bits */
#define PPC_BREAKPOINT_CONDITION_BE(n)  (1<<((n)+16))
        uint32_t condition_mode;     /* break/watchpoint condition flags */

        uint64_t addr;
        uint64_t addr2;
        uint64_t condition_value;
};

A request specifies one event, not necessarily just one register to be set.
For instance, if the request is for a watchpoint with a condition, both the
DAC and DVC registers will be set in the same request.

With this GDB can ask for all kinds of hardware breakpoints and watchpoints
that the BookE supports. COMEFROM breakpoints available in server processors
are not contemplated, but that is out of the scope of this work.

ptrace will return an integer (handle) uniquely identifying the breakpoint or
watchpoint just created. This integer will be used in the PTRACE_DELHWDEBUG
request to ask for its removal. Return -ENOSPC if the requested breakpoint
can't be allocated on the registers.

Some examples of using the structure to:

- set a breakpoint in the first breakpoint register

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  p.addr            = (uint64_t) address;
  p.addr2           = 0;
  p.condition_value = 0;

- set a watchpoint which triggers on reads in the second watchpoint register

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_READ;
  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  p.addr            = (uint64_t) address;
  p.addr2           = 0;
  p.condition_value = 0;

- set a watchpoint which triggers only with a specific value

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_READ;
  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_AND | PPC_BREAKPOINT_CONDITION_BE_ALL;
  p.addr            = (uint64_t) address;
  p.addr2           = 0;
  p.condition_value = (uint64_t) condition;

- set a ranged hardware breakpoint

  p.version         = PPC_DEBUG_CURRENT_VERSION;
  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
  p.addr_mode       = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  p.addr            = (uint64_t) begin_range;
  p.addr2           = (uint64_t) end_range;
  p.condition_value = 0;

3. PTRACE_DELHWDEBUG

Takes an integer which identifies an existing breakpoint or watchpoint
(i.e., the value returned from PTRACE_SETHWDEBUG), and deletes the
corresponding breakpoint or watchpoint..

-- 
Dave Kleikamp
IBM Linux Technology Center

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[K.Prasad]

>From shaggy at linux.vnet.ibm.com  Tue Jan 19 08:57:04 2010
From: shaggy at linux.vnet.ibm.com (Dave Kleikamp)
Date: Mon, 18 Jan 2010 14:57:04 -0700
Subject: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through
	extended ptrace interface
Message-ID: <20100118215704.15684.60646.sendpatchset@norville.austin.ibm.com>

> These patches implement an extention to the ptrace interface proposed by
> Thiago Bauermann and the the PowerPC gdb team.

(Using the plain text from mail archive..mail may not be in the usual
mail-reply format...kindly bear).

Hi,
  First of all, thanks for bringing this patch that puts the debug
registers in BookE to good use! I learnt about this patch submission
only recently...please see some of my concerns as under.

Given that there now exists generic kernel interfaces to use
hw-breakpoints - register_user_hw_breakpoint() and
unregister_hw_breakpoint() (available in mainline since 2.6.33-rc1), it
would be prudent to use them for the ptrace requests (that are made in
arch_ptrace()).

This would mean that some of the arch-specific debug register code that
read/write from/to the debug registers of Book-E may have to be in a
form that closely resembles arch/<x86><power>/kernel/hw_breakpoint.c 

A patch that enables arch-specific code for PPC64 is already submitted
(linuxppc-dev ref: 20100121084640.GA3252@in.ibm.com). This converts
ptrace requests for PTRACE_<GET><SET>_DEBUGREG flags to use the
above-mentioned interfaces for PPC64.

If you intend to re-write this patch to use the generic hw-breakpoint
interfaces (which I strongly recommend you to do, for the reasons mentioned
below), I would be more than glad to help you port them.

Some of the benefits of using these generic interfaces include:
- Interoperability with other users of debug register (such as parallel
  kernel requests) i.e. non-exclusive use of debug registers.
- Enables debugging/tracing tools such as perf-events and ftrace to make
  use of debug registers.
- Re-use of common code available in kernel (kernel/hw_breakpoint.c).

Let me know what you think.

Thanks,
K.Prasad

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Benjamin Herrenschmidt]

On Mon, 2010-01-25 at 00:48 +0530, K.Prasad wrote:
> 
> Some of the benefits of using these generic interfaces include:
> - Interoperability with other users of debug register (such as
> parallel
>   kernel requests) i.e. non-exclusive use of debug registers.
> - Enables debugging/tracing tools such as perf-events and ftrace to
> make
>   use of debug registers.
> - Re-use of common code available in kernel (kernel/hw_breakpoint.c).
> 
> Let me know what you think.

This might have changed but last I looked the "generic" breakpoint
interface was still too x86-centric and wasn't capable of expressing
some of the features of the BookE debug register set such as the data
value compare, the ranged breakpoints, etc...

I'd rather have this more dedicated and more complete interface merged
for gdb's sake, and in a second step look at unifying.

I believe that the generic breakpoint infrastructure should not be the
mid-layer. IE. It cannot be made in any clean shape of form, to express
all of the subtle features that a given architecture or platform can
support and as such would always be inferior to a dedicated one.

I can see the interest in exposing some kind of generic API that
implements a common subset of breakpoint or watchpoint facilities to
generic code such as the event tracer. This could be layered on top of
an arch specific mechanism

But having the generic mechanism at the core for everybody is another
attempt of "make everybody look like x86" which I believe in this case
is sub optimal.

Again, I might have missed some evolutions of the latest versions of
your infrastructure that would make it good enough to fully bridge the
gap with our requirements, and I'll let Shaggy decide here what he wants
to do. But I will not block his current patches neither if he thinks
that they are good enough as is.

Cheers,
Ben.

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[K.Prasad]

On Mon, Jan 25, 2010 at 07:32:00AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2010-01-25 at 00:48 +0530, K.Prasad wrote:
> > 
> > Some of the benefits of using these generic interfaces include:
> > - Interoperability with other users of debug register (such as
> > parallel
> >   kernel requests) i.e. non-exclusive use of debug registers.
> > - Enables debugging/tracing tools such as perf-events and ftrace to
> > make
> >   use of debug registers.
> > - Re-use of common code available in kernel (kernel/hw_breakpoint.c).
> > 
> > Let me know what you think.
> 
> This might have changed but last I looked the "generic" breakpoint
> interface was still too x86-centric and wasn't capable of expressing
> some of the features of the BookE debug register set such as the data
> value compare, the ranged breakpoints, etc...
> 
> I'd rather have this more dedicated and more complete interface merged
> for gdb's sake, and in a second step look at unifying.
> 

While the hw-breakpoint infrastructure is more generic (than
what can be termed x86-centric), it cannot support sophisticated
features like DVC register - atleast in its present form.

My idea was to align the Book-E processor's use of debug registers with
the existing breakpoint framework, and in the process make improvements/
changes to the common framework as necessitated.

It could be done in a two-step process to ease the development and review
process (more on this below).

> I believe that the generic breakpoint infrastructure should not be the
> mid-layer. IE. It cannot be made in any clean shape of form, to express
> all of the subtle features that a given architecture or platform can
> support and as such would always be inferior to a dedicated one.
> 
> I can see the interest in exposing some kind of generic API that
> implements a common subset of breakpoint or watchpoint facilities to
> generic code such as the event tracer. This could be layered on top of
> an arch specific mechanism
> 

Given the diversity of debug register implementation (across
processors), concerns about having a poorly-featured mid-layer is
understandable.

But our design goal is to be as flexible as required to accomodate newer
architectures, while providing them the benefit of using the common
framework - such as register arbitration (existing implementation would
assume mutually exclusive access to debug registers), scheduling and
abstraction through generic interfaces.

> But having the generic mechanism at the core for everybody is another
> attempt of "make everybody look like x86" which I believe in this case
> is sub optimal.
> 

No, it is not! In fact patches that port the framework to PPC64 and S390
have already been posted to the respective communities and are under
review.

> Again, I might have missed some evolutions of the latest versions of
> your infrastructure that would make it good enough to fully bridge the
> gap with our requirements, and I'll let Shaggy decide here what he wants
> to do. But I will not block his current patches neither if he thinks
> that they are good enough as is.
> 

As I mentioned above, the generic framework may need to be extended to
support all features required by processors such as Book-E. Any
feature that is very unique to a given processor and exotic to the
hw-breakpoint framework can be kept outside its purview (and implemented
to support only ptrace with exclusive access to those registers).

Sure, let's wait to hear from Shaggy to know how he'd like to proceed. 

Thanks,
K.Prasad

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Frederic Weisbecker]

On Mon, Jan 25, 2010 at 07:32:00AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2010-01-25 at 00:48 +0530, K.Prasad wrote:
> > 
> > Some of the benefits of using these generic interfaces include:
> > - Interoperability with other users of debug register (such as
> > parallel
> >   kernel requests) i.e. non-exclusive use of debug registers.
> > - Enables debugging/tracing tools such as perf-events and ftrace to
> > make
> >   use of debug registers.
> > - Re-use of common code available in kernel (kernel/hw_breakpoint.c).
> > 
> > Let me know what you think.
> 
> This might have changed but last I looked the "generic" breakpoint
> interface was still too x86-centric and wasn't capable of expressing
> some of the features of the BookE debug register set such as the data
> value compare, the ranged breakpoints, etc...



We have one field for addr, one for len and one for the memory access
type.

I think that those three are enough to express breakpoint ranges.
Basically a breakpoint range is a breakpoint that can have a high
len.

I've looked at the G2 PowerPc core breakpoint implementation, just to
face one of such tricky examples.

We have DABR and DABR2 for watchpoint (and respectively IABR and IABR2
for ins breakpoints). Those host the addresses to target, or well,
let's rather call them "address operand" registers. To generalize, I
call these xABR and xABR2 as it seems instruction and data breakpoint
work the same, they just have their own dedicated registers.

Then you have DBCR/IBCR (let's call them xBCR) which control the
breakpoints, with two fields in each that detail the operators to affect
in the address operands registers. You can choose betwen ==, <, or >= .

Another field in the operator is the SIG_TYPE, which describes the
combination, either "matches xABR AND xABR2" or "matches xABR OR xABR2".

If you choose the "OR" SIG_TYPE, it makes no sense to use the < or >=
operators on the addresses operands in practice. Who needs the following
matches?

	addr < xABR || addr >= xABR2
	addr < xABR || addr < xABR2
	addr >= xABR || addr >= xABR2

The only operator that makes sense in a OR type is ==, which
basically provides you two breakpoints:

	addr == xABR || addr == xABR2

Now if you choose the "AND" SIG_TYPE, the following matches
make no sense:

	addr < xABR && addr < xABR2
	addr < xABR && addr >= xABR2 (if xABR < xABR2)
	addr == xABR && addr (>=,<) xABR2

Basically, it's only usable for delimited ranges:

	addr >= xABR && addr < xABR2 (xABR < xABR2)

So the comparison is a trick that can actually only have
a practical use to define two exact matching breakpoints
or a delimited breakpoint range.

Hence, unless I'm missing something obvious, the current
generic interface is sufficient to express that.

I may also miss other kind of implementation that could
have other requirements.

 
> I'd rather have this more dedicated and more complete interface merged
> for gdb's sake, and in a second step look at unifying.


Perhaps. Supporting ptrace breakpoints should be an easy first
step as it's basically the responsibility of the user to fill
the registers, but it's a pretty limited scope work, especially you
won't have perf support.


> I believe that the generic breakpoint infrastructure should not be the
> mid-layer. IE. It cannot be made in any clean shape of form, to express
> all of the subtle features that a given architecture or platform can
> support and as such would always be inferior to a dedicated one.


Actually I think the current interface already does, as I explained
above.

What is broken for any other architectures than x86 is the set
of constraints, but I can easily move it to the arch, unless
I find a good generic solution (or a cool combination between
both).



> I can see the interest in exposing some kind of generic API that
> implements a common subset of breakpoint or watchpoint facilities to
> generic code such as the event tracer. This could be layered on top of
> an arch specific mechanism
> 
> But having the generic mechanism at the core for everybody is another
> attempt of "make everybody look like x86" which I believe in this case
> is sub optimal.


Not at all. It's an attempt to make a generic interface that can
exploit at best _each_ arch specific features. Other than the set
of constraints that I'm going to rework, the generic interface is powerful
enough to host what I've seen in term of cpu breakpoints implementations
for now. But if it's actually not and I'm missing other cases, please
report it to me.

The reason that makes the current generic constraints x86
oriented only is that I've only x86 boxes at home and I needed
to make a first shot without knowing anything about other archs
constraints, but in the long term, our motivations (Prasad's and mines)
are definetely not archX-centric.

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Benjamin Herrenschmidt]

> We have one field for addr, one for len and one for the memory access
> type.
> 
> I think that those three are enough to express breakpoint ranges.
> Basically a breakpoint range is a breakpoint that can have a high
> len.
> 
> I've looked at the G2 PowerPc core breakpoint implementation, just to
> face one of such tricky examples.

BookE has a richer semantic. We have watchpoints with data value compare
for example, we also have instruction value compare for breakpoints, and
a few other niceties. There's also subtle differences between what
processor variants support.

 .../...

> > I'd rather have this more dedicated and more complete interface merged
> > for gdb's sake, and in a second step look at unifying.
> 
> 
> Perhaps. Supporting ptrace breakpoints should be an easy first
> step as it's basically the responsibility of the user to fill
> the registers, but it's a pretty limited scope work, especially you
> won't have perf support.

But we can add it later. 

> > I believe that the generic breakpoint infrastructure should not be the
> > mid-layer. IE. It cannot be made in any clean shape of form, to express
> > all of the subtle features that a given architecture or platform can
> > support and as such would always be inferior to a dedicated one.
> 
> 
> Actually I think the current interface already does, as I explained
> above.
> 
> What is broken for any other architectures than x86 is the set
> of constraints, but I can easily move it to the arch, unless
> I find a good generic solution (or a cool combination between
> both).

Again, this is all "can do" vs. "already done and working". May I point
you to Linus recent rant against magic infrastructures that try to do
everything and do nothing right ? :-) I much prefer starting with
something dedicated that does exactly what is expected, have that
upstream (and in that regard the patches are good for the next merge
window) and -then- maybe look at how some of it could be re-used for
perf.

> Not at all. It's an attempt to make a generic interface that can
> exploit at best _each_ arch specific features. 

That reminds me of the justifications for utrace :-) It might well be
but I very much doubt that is possible. In any case, it doesn't appear
to be there yet. So let's just get that stuff in so we have our
interface finally working, and we can look at doing fancy things with
perf in a second pass.

> Other than the set
> of constraints that I'm going to rework, the generic interface is powerful
> enough to host what I've seen in term of cpu breakpoints implementations
> for now. But if it's actually not and I'm missing other cases, please
> report it to me.
> 
> The reason that makes the current generic constraints x86
> oriented only is that I've only x86 boxes at home and I needed
> to make a first shot without knowing anything about other archs
> constraints, but in the long term, our motivations (Prasad's and mines)
> are definetely not archX-centric.

Cheers,
Ben.

Re: [RFC:PATCH 00/03] powerpc: Expose BookE debug registers through extended ptrace interface

[Frederic Weisbecker]

On Sun, Jan 31, 2010 at 08:33:25AM +1100, Benjamin Herrenschmidt wrote:
> 
> > We have one field for addr, one for len and one for the memory access
> > type.
> > 
> > I think that those three are enough to express breakpoint ranges.
> > Basically a breakpoint range is a breakpoint that can have a high
> > len.
> > 
> > I've looked at the G2 PowerPc core breakpoint implementation, just to
> > face one of such tricky examples.
> 
> BookE has a richer semantic. We have watchpoints with data value compare
> for example, we also have instruction value compare for breakpoints, and
> a few other niceties. There's also subtle differences between what
> processor variants support.
> 
>  .../...



Ah indeed, I missed the data value compare thing. Especially how
it is implemented won't make things easy.

This is basically a comparison against chosen bytes of the data,
with or/and patterns.

Not sure what the "or" can be useful for.

That won't be easy to implement in the generic interface, looking
at how it is done in the BookE.

There is also the address comparison by mask.

Anyway, I think we can add fields in the interface to provide
such features, but we can't support all of them given, as you
said, the subtle differences between different cpu.

For example I think it can be useful to implement support
for data comparison, by mask for example. But I don't imagine
useful usecases to compare byte 4 and byte1 and trigger an event
if one OR other match.

I think we are going to implement what has obvious usecases
(parts of such data comparisons, parts of address mask
comparison) in the generic interface: the fields in perf_attr
that can be filled by perf in userspace.
And the rest can be implemented from the hw_perf_event structure
which contains the arch structure and can then be filled by ptrace at
will.


> 
> > > I'd rather have this more dedicated and more complete interface merged
> > > for gdb's sake, and in a second step look at unifying.
> > 
> > 
> > Perhaps. Supporting ptrace breakpoints should be an easy first
> > step as it's basically the responsibility of the user to fill
> > the registers, but it's a pretty limited scope work, especially you
> > won't have perf support.
> 
> But we can add it later. 



Yeah you're right. Having a raw ptrace support is a first
useful step that won't be a barrier to enhance it further
through the generic API.


> > > I believe that the generic breakpoint infrastructure should not be the
> > > mid-layer. IE. It cannot be made in any clean shape of form, to express
> > > all of the subtle features that a given architecture or platform can
> > > support and as such would always be inferior to a dedicated one.
> > 
> > 
> > Actually I think the current interface already does, as I explained
> > above.
> > 
> > What is broken for any other architectures than x86 is the set
> > of constraints, but I can easily move it to the arch, unless
> > I find a good generic solution (or a cool combination between
> > both).
> 
> Again, this is all "can do" vs. "already done and working". May I point
> you to Linus recent rant against magic infrastructures that try to do
> everything and do nothing right ? :-) I much prefer starting with
> something dedicated that does exactly what is expected, have that
> upstream (and in that regard the patches are good for the next merge
> window) and -then- maybe look at how some of it could be re-used for
> perf.


Sure I'm not against a first raw ptrace support. As I said,
this is not a barrier for what comes next.

Now for the rest, I don't think this is the same story than
utrace.

Trying to have a generic layer for a hardware feature implemented
differently across archs can't be called something that tries to do
everything. Otherwise you can oppose these arguments to everything
that is not in the arch/ directories. No generic irq layer, no
generic timer, etc...

We need this generic layer because we want the breakpoints
to be available for wider uses than ptrace. If there was only
ptrace, I would really agree with you that it's not worth the
generic layer.

It's just that breakpoints are a set of possible features, but each
archs implement its own subset among the possible features. x86
is one of the weakest there, and since this generic layer has been
first x86 oriented, it looks too weak to host the most interesting
possibilities.

Let it grow a bit, it's still young.


> > Not at all. It's an attempt to make a generic interface that can
> > exploit at best _each_ arch specific features. 
> 
> That reminds me of the justifications for utrace :-) It might well be
> but I very much doubt that is possible. In any case, it doesn't appear
> to be there yet.


You are too pessimistic ;-)

I don't think we can express every possibilities through the generic
interface. But we can express the most interesting ones for profiling
uses.

The rest (ptrace) can still be expressed through the arch part of the perf
events.


> So let's just get that stuff in so we have our
> interface finally working, and we can look at doing fancy things with
> perf in a second pass.


Sure.