Hi All,
Please find a set of two patches that introduce generic
hw-breakpoint interfaces for a couple of ptrace flags used by server-class
processors and another change to advertise a feature to user-space hitherto
available and used, but not claimed as supported.
GDB, which recently began using the
PPC_PTRACE_GETHWDBGINFO/PPC_PTRACE_SETHWDEBUG/PPC_PTRACE_DELHWDEBUG
flags on BookS processors is presently unable to set watchpoints. The
changes in Patch1, will fix that issue and help it use a common set of code
across BookE and BookS.
K.Prasad (2):
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC
ptrace flags
[PowerPC Book3E] Introduce new ptrace debug feature flag
Documentation/powerpc/ptrace.txt | 16 ++++++
arch/powerpc/include/asm/ptrace.h | 1 +
arch/powerpc/kernel/ptrace.c | 105
++++++++++++++++++++++++++++++++++---
3 files changed, 114 insertions(+), 8 deletions(-)
Thanks,
K.Prasad
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
Tested-by: Edjunior Barbosa Machado <redacted>
Signed-off-by: K.Prasad <redacted>
---
Documentation/powerpc/ptrace.txt | 16 ++++++
arch/powerpc/kernel/ptrace.c | 104 +++++++++++++++++++++++++++++++++++---
2 files changed, 112 insertions(+), 8 deletions(-)
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS) using version 2++ p.version = 2;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where+ * addr2 - addr <= 8 Bytes.+ */+ p.addr2 = (uint64_t) end_range;+ p.condition_value = 0;+ 3. PTRACE_DELHWDEBUG Takes an integer which identifies an existing breakpoint or watchpoint
@@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,*/if((bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_RW)==0||(bp_info->trigger_type&~PPC_BREAKPOINT_TRIGGER_RW)!=0||-bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT||bp_info->condition_mode!=PPC_BREAKPOINT_CONDITION_NONE)return-EINVAL;-if(child->thread.dabr)-return-ENOSPC;-if((unsignedlong)bp_info->addr>=TASK_SIZE)return-EIO;
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child,dabr|=DABR_DATA_READ;if(bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_WRITE)dabr|=DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+if(bp_info->version==1)+gotoversion_one;+if(ptrace_get_breakpoints(child)<0)+return-ESRCH;-child->thread.dabr=dabr;+bp=thread->ptrace_bps[0];+if(!bp_info->addr){+if(bp){+unregister_hw_breakpoint(bp);+thread->ptrace_bps[0]=NULL;+}+ptrace_put_breakpoints(child);+return0;+}+/*+*Checkiftherequestisfor'range'breakpoints.Wecan+*supportitifrange<8bytes.+*/+if(bp_info->addr_mode==PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)+len=bp_info->addr2-bp_info->addr;+elseif(bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT){+ptrace_put_breakpoints(child);+return-EINVAL;+}+if(bp){+attr=bp->attr;+attr.bp_addr=(unsignedlong)bp_info->addr&~HW_BREAKPOINT_ALIGN;+arch_bp_generic_fields(dabr&+(DABR_DATA_WRITE|DABR_DATA_READ),+&attr.bp_type);+attr.bp_len=len;+ret=modify_user_hw_breakpoint(bp,&attr);+if(ret){+ptrace_put_breakpoints(child);+returnret;+}+thread->ptrace_bps[0]=bp;+ptrace_put_breakpoints(child);+thread->dabr=dabr;+return0;+}+/* Create a new breakpoint request if one doesn't exist already */+hw_breakpoint_init(&attr);+attr.bp_addr=(unsignedlong)bp_info->addr&~HW_BREAKPOINT_ALIGN;+attr.bp_len=len;+arch_bp_generic_fields(dabr&(DABR_DATA_WRITE|DABR_DATA_READ),+&attr.bp_type);++thread->ptrace_bps[0]=bp=register_user_hw_breakpoint(&attr,+ptrace_triggered,NULL,child);+if(IS_ERR(bp)){+thread->ptrace_bps[0]=NULL;+ptrace_put_breakpoints(child);+returnPTR_ERR(bp);+}++ptrace_put_breakpoints(child);+return1;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */++version_one:+if(bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT)+return-EINVAL;++if(child->thread.dabr)+return-ENOSPC;++child->thread.dabr=dabr;return1;#endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */}staticlongppc_del_hwdebug(structtask_struct*child,longaddr,longdata){+#ifdef CONFIG_HAVE_HW_BREAKPOINT+structthread_struct*thread=&(child->thread);+structperf_event*bp;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */#ifdef CONFIG_PPC_ADV_DEBUG_REGSintrc;
@@ -1426,10 +1499,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)#elseif(data!=1)return-EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+if(ptrace_get_breakpoints(child)<0)+return-ESRCH;++bp=thread->ptrace_bps[0];+if(bp){+unregister_hw_breakpoint(bp);+thread->ptrace_bps[0]=NULL;+}+ptrace_put_breakpoints(child);+return0;+#else /* CONFIG_HAVE_HW_BREAKPOINT */if(child->thread.dabr==0)return-ENOENT;child->thread.dabr=0;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */return0;#endif
@@ -1536,7 +1623,8 @@ long arch_ptrace(struct task_struct *child, long request,casePPC_PTRACE_GETHWDBGINFO:{structppc_debug_infodbginfo;-dbginfo.version=1;+/* We return the highest version number supported */+dbginfo.version=2;#ifdef CONFIG_PPC_ADV_DEBUG_REGSdbginfo.num_instruction_bps=CONFIG_PPC_ADV_DEBUG_IACS;dbginfo.num_data_bps=CONFIG_PPC_ADV_DEBUG_DACS;
@@ -1560,7 +1648,7 @@ long arch_ptrace(struct task_struct *child, long request,dbginfo.data_bp_alignment=4;#endifdbginfo.sizeof_condition=0;-dbginfo.features=0;+dbginfo.features=PPC_DEBUG_FEATURE_DATA_BP_RANGE;#endif /* CONFIG_PPC_ADV_DEBUG_REGS */if(!access_ok(VERIFY_WRITE,datavp,
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
Signed-off-by: K.Prasad <redacted>
---
arch/powerpc/include/asm/ptrace.h | 1 +
arch/powerpc/kernel/ptrace.c | 1 +
2 files changed, 2 insertions(+), 0 deletions(-)
From: David Gibson <hidden> Date: 2011-08-23 05:09:02
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
So, the structure itself doesn't seem to have been extended. I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
What is the mechanism for implementing the range breakpoint on book3s?
quoted hunk
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
Tested-by: Edjunior Barbosa Machado <redacted>
Signed-off-by: K.Prasad <redacted>
---
Documentation/powerpc/ptrace.txt | 16 ++++++
arch/powerpc/kernel/ptrace.c | 104 +++++++++++++++++++++++++++++++++++---
2 files changed, 112 insertions(+), 8 deletions(-)
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS) using version 2++ p.version = 2;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where+ * addr2 - addr <= 8 Bytes.+ */+ p.addr2 = (uint64_t) end_range;+ p.condition_value = 0;+ 3. PTRACE_DELHWDEBUG Takes an integer which identifies an existing breakpoint or watchpoint
I'm confused. This compiled before on book3s, and I don't see any
changes to Makefile or Kconfig in the patch that will result in this
code compiling when it previously didn't Why are these new guards
added?
quoted hunk
#ifndef CONFIG_PPC_ADV_DEBUG_REGS
unsigned long dabr;
#endif
- if (bp_info->version != 1)
+ if ((bp_info->version != 1) && (bp_info->version != 2))
return -ENOTSUPP;
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
/*
You remove this test to see if the single watchpoint slot is already
in use, but I don't see another test replacing it.
quoted hunk
if ((unsigned long)bp_info->addr >= TASK_SIZE)
return -EIO;
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child, dabr |= DABR_DATA_READ; if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE) dabr |= DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (bp_info->version == 1)+ goto version_one;
There are several legitimate uses of goto in the kernel, but this is
definitely not one of them. You're essentially using it to put the
old and new versions of the same function in one block. Nasty.
+ if (ptrace_get_breakpoints(child) < 0)
+ return -ESRCH;
- child->thread.dabr = dabr;
+ bp = thread->ptrace_bps[0];
+ if (!bp_info->addr) {
+ if (bp) {
+ unregister_hw_breakpoint(bp);
+ thread->ptrace_bps[0] = NULL;
+ }
+ ptrace_put_breakpoints(child);
+ return 0;
Why are you making setting a 0 watchpoint remove the existing one (I
think that's what this does). I thought there was an explicit del
breakpoint operation instead.
+ }
+ /*
+ * Check if the request is for 'range' breakpoints. We can
+ * support it if range < 8 bytes.
+ */
+ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+ len = bp_info->addr2 - bp_info->addr;
So you compute the length here, but I don't see you ever test if it is
< 8 and return an error.
@@ -1426,10 +1499,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) #else if (data != 1) return -EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (ptrace_get_breakpoints(child) < 0)+ return -ESRCH;++ bp = thread->ptrace_bps[0];+ if (bp) {+ unregister_hw_breakpoint(bp);+ thread->ptrace_bps[0] = NULL;+ }+ ptrace_put_breakpoints(child);+ return 0;+#else /* CONFIG_HAVE_HW_BREAKPOINT */ if (child->thread.dabr == 0) return -ENOENT; child->thread.dabr = 0;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */ return 0; #endif
@@ -1536,7 +1623,8 @@ long arch_ptrace(struct task_struct *child, long request, case PPC_PTRACE_GETHWDBGINFO: { struct ppc_debug_info dbginfo;- dbginfo.version = 1;+ /* We return the highest version number supported */+ dbginfo.version = 2; #ifdef CONFIG_PPC_ADV_DEBUG_REGS dbginfo.num_instruction_bps = CONFIG_PPC_ADV_DEBUG_IACS; dbginfo.num_data_bps = CONFIG_PPC_ADV_DEBUG_DACS;
@@ -1560,7 +1648,7 @@ long arch_ptrace(struct task_struct *child, long request, dbginfo.data_bp_alignment = 4; #endif dbginfo.sizeof_condition = 0;- dbginfo.features = 0;+ dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE; #endif /* CONFIG_PPC_ADV_DEBUG_REGS */ if (!access_ok(VERIFY_WRITE, datavp,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
From: David Gibson <hidden> Date: 2011-08-23 05:09:36
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
@@ -1636,6 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request,#ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGEdbginfo.features|=PPC_DEBUG_FEATURE_DATA_BP_RANGE|+PPC_DEBUG_FEATURE_DATA_BP_EXACT|PPC_DEBUG_FEATURE_DATA_BP_MASK;#endif#else /* !CONFIG_PPC_ADV_DEBUG_REGS */
--
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
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
quoted
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
So, the structure itself doesn't seem to have been extended. I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
I'll add a comment w.r.t change in semantics - such as the ability to
accept 'range' breakpoints in BookS.
quoted
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
What is the mechanism for implementing the range breakpoint on book3s?
The hw-breakpoint interface, accepts length as an argument in BookS (any
value <= 8 Bytes) and would filter out extraneous interrupts arising out
of accesses outside the range comprising <addr, addr + len> inside
hw_breakpoint_handler function.
We put that ability to use here.
quoted
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
Tested-by: Edjunior Barbosa Machado <redacted>
Signed-off-by: K.Prasad <redacted>
---
Documentation/powerpc/ptrace.txt | 16 ++++++
arch/powerpc/kernel/ptrace.c | 104 +++++++++++++++++++++++++++++++++++---
2 files changed, 112 insertions(+), 8 deletions(-)
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS) using version 2++ p.version = 2;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where+ * addr2 - addr <= 8 Bytes.+ */+ p.addr2 = (uint64_t) end_range;+ p.condition_value = 0;+ 3. PTRACE_DELHWDEBUG Takes an integer which identifies an existing breakpoint or watchpoint
I'm confused. This compiled before on book3s, and I don't see any
changes to Makefile or Kconfig in the patch that will result in this
code compiling when it previously didn't Why are these new guards
added?
The code is guarded using the CONFIG_ flags for two reasons.
a) We don't want the code to be included for BookE and other
architectures.
b) In BookS, we're now adding a new ability based on whether
CONFIG_HAVE_HW_BREAKPOINT is defined. Presently this config option is
kept on by default, however there are plans to make this a config-time
option.
quoted
#ifndef CONFIG_PPC_ADV_DEBUG_REGS
unsigned long dabr;
#endif
- if (bp_info->version != 1)
+ if ((bp_info->version != 1) && (bp_info->version != 2))
return -ENOTSUPP;
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
/*
You remove this test to see if the single watchpoint slot is already
in use, but I don't see another test replacing it.
This test is retained for !CONFIG_HAVE_HW_BREAKPOINT case. In case of
using hw-breakpoint interfaces, we have a double check through
thread->ptrace_bps[0] and using register_user_hw_breakpoint function
(which would error out if not enough free slots are available).
quoted
if ((unsigned long)bp_info->addr >= TASK_SIZE)
return -EIO;
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child, dabr |= DABR_DATA_READ; if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE) dabr |= DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (bp_info->version == 1)+ goto version_one;
There are several legitimate uses of goto in the kernel, but this is
definitely not one of them. You're essentially using it to put the
old and new versions of the same function in one block. Nasty.
Maybe it's the label that's causing bother here. It might look elegant
if it was called something like exit_* or error_* :-)
The goto here helps reduce code, is similar to the error exits we use
everywhere.
quoted
+ if (ptrace_get_breakpoints(child) < 0)
+ return -ESRCH;
- child->thread.dabr = dabr;
+ bp = thread->ptrace_bps[0];
+ if (!bp_info->addr) {
+ if (bp) {
+ unregister_hw_breakpoint(bp);
+ thread->ptrace_bps[0] = NULL;
+ }
+ ptrace_put_breakpoints(child);
+ return 0;
Why are you making setting a 0 watchpoint remove the existing one (I
think that's what this does). I thought there was an explicit del
breakpoint operation instead.
We had to define the semantics for what writing a 0 to DABR could mean,
and I think it is intuitive to consider it as deletion
request...couldn't think of a case where DABR with addr=0 and RW=1 would
be required.
quoted
+ }
+ /*
+ * Check if the request is for 'range' breakpoints. We can
+ * support it if range < 8 bytes.
+ */
+ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+ len = bp_info->addr2 - bp_info->addr;
So you compute the length here, but I don't see you ever test if it is
< 8 and return an error.
The hw-breakpoint interfaces would fail if the length was > 8.
You seem to be silently masking the given address, which seems
completely wrong.
We have two ways of looking at the input address.
a) Assume that the input address is not multiplexed with the read/write
bits and return -EINVAL (for not confirming to the 8-byte alignment
requirement).
b) Consider the input address to be encoded with the read/write
watchpoint type request and align the address by default. This is how
the code behaves presently for the !CONFIG_HAVE_HW_BREAKPOINT case.
I chose to go with b) and discard the last 3-bits from the address.
Thanks for the detailed review. Looking forward for your comments.
Thanks,
K.Prasad
On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
quoted
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.
Thanks,
K.Prasad
From: David Gibson <hidden> Date: 2011-08-24 04:00:15
On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
quoted
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.
I can see that. But you haven't answered why.
--
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
From: David Gibson <hidden> Date: 2011-08-24 04:00:15
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
quoted
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
So, the structure itself doesn't seem to have been extended. I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
I still don't follow you.
I'll add a comment w.r.t change in semantics - such as the ability to
accept 'range' breakpoints in BookS.
quoted
quoted
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
What is the mechanism for implementing the range breakpoint on book3s?
The hw-breakpoint interface, accepts length as an argument in BookS (any
value <= 8 Bytes) and would filter out extraneous interrupts arising out
of accesses outside the range comprising <addr, addr + len> inside
hw_breakpoint_handler function.
We put that ability to use here.
Ah, so in hardware the breakpoints are always 8 bytes long, but you
filter out false hits on a shorter range? Of course, the utility of
range breakpoints is questionable when length <=8, but the start must
be aligned on an 8-byte boundary.
[snip]
quoted
quoted
if ((unsigned long)bp_info->addr >= TASK_SIZE)
return -EIO;
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child, dabr |= DABR_DATA_READ; if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE) dabr |= DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (bp_info->version == 1)+ goto version_one;
There are several legitimate uses of goto in the kernel, but this is
definitely not one of them. You're essentially using it to put the
old and new versions of the same function in one block. Nasty.
Maybe it's the label that's causing bother here. It might look elegant
if it was called something like exit_* or error_* :-)
The goto here helps reduce code, is similar to the error exits we use
everywhere.
Rubbish, it is not an exception exit at all, it is two separate code
paths for the different versions which would be much clearer as two
different functions.
quoted
quoted
+ if (ptrace_get_breakpoints(child) < 0)
+ return -ESRCH;
- child->thread.dabr = dabr;
+ bp = thread->ptrace_bps[0];
+ if (!bp_info->addr) {
+ if (bp) {
+ unregister_hw_breakpoint(bp);
+ thread->ptrace_bps[0] = NULL;
+ }
+ ptrace_put_breakpoints(child);
+ return 0;
Why are you making setting a 0 watchpoint remove the existing one (I
think that's what this does). I thought there was an explicit del
breakpoint operation instead.
We had to define the semantics for what writing a 0 to DABR could mean,
and I think it is intuitive to consider it as deletion
request...couldn't think of a case where DABR with addr=0 and RW=1 would
be required.
When a user space program maps pages at virtual address 0, which it
can do.
quoted
quoted
+ }
+ /*
+ * Check if the request is for 'range' breakpoints. We can
+ * support it if range < 8 bytes.
+ */
+ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+ len = bp_info->addr2 - bp_info->addr;
So you compute the length here, but I don't see you ever test if it is
< 8 and return an error.
The hw-breakpoint interfaces would fail if the length was > 8.
You seem to be silently masking the given address, which seems
completely wrong.
We have two ways of looking at the input address.
a) Assume that the input address is not multiplexed with the read/write
bits and return -EINVAL (for not confirming to the 8-byte alignment
requirement).
b) Consider the input address to be encoded with the read/write
watchpoint type request and align the address by default. This is how
the code behaves presently for the !CONFIG_HAVE_HW_BREAKPOINT case.
Hrm, ok, but this needs commenting.
I chose to go with b) and discard the last 3-bits from the address.
Thanks for the detailed review. Looking forward for your comments.
Thanks,
K.Prasad
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
--
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
On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
quoted
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.
I can see that. But you haven't answered why.
BookS doesn't support BP_EXACT, that's why I suggested this flag.
A BP_EXACT watchpoint triggers only when there's a memory access exactly
at the given address. It doesn't trigger when there's (for example) a
4-byte write at an address immediately before which also changes the
memory contents of the byte watched by the BP_EXACT watchpoint. a ranged
watchpoint would trigger, so the semantics are different.
As a general rule, GDB only sets ranged watchpoints and only uses
BP_EXACT ones when the user sets a flag. I want GDB to fail when the
user sets the flag on BookS since it can't provide the feature.
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center
From: David Gibson <hidden> Date: 2011-08-26 04:45:51
On Wed, Aug 24, 2011 at 09:41:43PM -0300, Thiago Jung Bauermann wrote:
On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
quoted
On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
quoted
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.
I can see that. But you haven't answered why.
BookS doesn't support BP_EXACT, that's why I suggested this flag.
Surely you can support it with exactly the same sort of filtering
you're using for the 8-byte ranges now?
A BP_EXACT watchpoint triggers only when there's a memory access exactly
at the given address. It doesn't trigger when there's (for example) a
4-byte write at an address immediately before which also changes the
memory contents of the byte watched by the BP_EXACT watchpoint. a ranged
watchpoint would trigger, so the semantics are different.
As a general rule, GDB only sets ranged watchpoints and only uses
BP_EXACT ones when the user sets a flag. I want GDB to fail when the
user sets the flag on BookS since it can't provide the feature.
--
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
On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
quoted
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
So, the structure itself doesn't seem to have been extended. I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
I still don't follow you.
Two things here.
One, the change of semantics warranted an increment of the version
number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
BookS, while the old version number did not. I've added a small comment
in the code to this effect.
Two, regarding changes in the "ppc_hw_breakpoint" and "ppc_debug_info"
structures - we would like to add more members to it if we can (GDB has a
pending request to add more members to it). However the problem foreseen
is that there could be a mismatch between the versions of the structure
used by the user vs kernel-space i.e. if a new version of the structure,
known to the kernel, had an extra member while the user-space still had
the old version, then it becomes dangerous because the __copy_to_user
function would overflow the buffer size in user-space.
This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
designed to accept a version number (and provide corresponding
"struct ppc_debug_info") rather than send a populated "ppc_debug_info"
structure along with the version number.
quoted
I'll add a comment w.r.t change in semantics - such as the ability to
accept 'range' breakpoints in BookS.
quoted
quoted
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
What is the mechanism for implementing the range breakpoint on book3s?
The hw-breakpoint interface, accepts length as an argument in BookS (any
value <= 8 Bytes) and would filter out extraneous interrupts arising out
of accesses outside the range comprising <addr, addr + len> inside
hw_breakpoint_handler function.
We put that ability to use here.
Ah, so in hardware the breakpoints are always 8 bytes long, but you
filter out false hits on a shorter range? Of course, the utility of
range breakpoints is questionable when length <=8, but the start must
be aligned on an 8-byte boundary.
Yes, we ensure that through
+ attr.bp_addr = (unsigned long)bp_info->addr & ~HW_BREAKPOINT_ALIGN;
[snip]
quoted
quoted
quoted
if ((unsigned long)bp_info->addr >= TASK_SIZE)
return -EIO;
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child, dabr |= DABR_DATA_READ; if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE) dabr |= DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (bp_info->version == 1)+ goto version_one;
There are several legitimate uses of goto in the kernel, but this is
definitely not one of them. You're essentially using it to put the
old and new versions of the same function in one block. Nasty.
Maybe it's the label that's causing bother here. It might look elegant
if it was called something like exit_* or error_* :-)
The goto here helps reduce code, is similar to the error exits we use
everywhere.
Rubbish, it is not an exception exit at all, it is two separate code
paths for the different versions which would be much clearer as two
different functions.
I've re-written this part of the code to avoid a goto statement.
quoted
quoted
quoted
+ if (ptrace_get_breakpoints(child) < 0)
+ return -ESRCH;
- child->thread.dabr = dabr;
+ bp = thread->ptrace_bps[0];
+ if (!bp_info->addr) {
+ if (bp) {
+ unregister_hw_breakpoint(bp);
+ thread->ptrace_bps[0] = NULL;
+ }
+ ptrace_put_breakpoints(child);
+ return 0;
Why are you making setting a 0 watchpoint remove the existing one (I
think that's what this does). I thought there was an explicit del
breakpoint operation instead.
We had to define the semantics for what writing a 0 to DABR could mean,
and I think it is intuitive to consider it as deletion
request...couldn't think of a case where DABR with addr=0 and RW=1 would
be required.
When a user space program maps pages at virtual address 0, which it
can do.
Agreed. I've removed the code under if (!bp_info->addr) branch.
quoted
quoted
quoted
+ }
+ /*
+ * Check if the request is for 'range' breakpoints. We can
+ * support it if range < 8 bytes.
+ */
+ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+ len = bp_info->addr2 - bp_info->addr;
So you compute the length here, but I don't see you ever test if it is
< 8 and return an error.
The hw-breakpoint interfaces would fail if the length was > 8.
You seem to be silently masking the given address, which seems
completely wrong.
We have two ways of looking at the input address.
a) Assume that the input address is not multiplexed with the read/write
bits and return -EINVAL (for not confirming to the 8-byte alignment
requirement).
b) Consider the input address to be encoded with the read/write
watchpoint type request and align the address by default. This is how
the code behaves presently for the !CONFIG_HAVE_HW_BREAKPOINT case.
Hrm, ok, but this needs commenting.
Added a comment to this effect.
I'm pasting the modified patch below. Kindly let me know your comments.
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS) using version 2++ p.version = 2;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where+ * addr2 - addr <= 8 Bytes.+ */+ p.addr2 = (uint64_t) end_range;+ p.condition_value = 0;+ 3. PTRACE_DELHWDEBUG Takes an integer which identifies an existing breakpoint or watchpoint
@@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,*/if((bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_RW)==0||(bp_info->trigger_type&~PPC_BREAKPOINT_TRIGGER_RW)!=0||-bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT||bp_info->condition_mode!=PPC_BREAKPOINT_CONDITION_NONE)return-EINVAL;-if(child->thread.dabr)-return-ENOSPC;-if((unsignedlong)bp_info->addr>=TASK_SIZE)return-EIO;
@@ -1399,14 +1401,84 @@ static long ppc_set_hwdebug(struct task_struct *child,if(bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_WRITE)dabr|=DABR_DATA_WRITE;-child->thread.dabr=dabr;+if(bp_info->version==1){+if(bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT)+return-EINVAL;+if(child->thread.dabr)+return-ENOSPC;+child->thread.dabr=dabr;+return1;+}+#ifdef CONFIG_HAVE_HW_BREAKPOINT+/*+*Wewilluseversion=2,todenotetheuseof+*PPC_BREAKPOINT_MODE_RANGE_INCLUSIVEmodeofwatchpoints.+*/+if(bp_info->version!=2)+return-EINVAL;+if(ptrace_get_breakpoints(child)<0)+return-ESRCH;+bp=thread->ptrace_bps[0];+/*+*Checkiftherequestisfor'range'breakpoints.Wecan+*supportitifrange<8bytes.+*/+if(bp_info->addr_mode==PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)+len=bp_info->addr2-bp_info->addr;+elseif(bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT){+ptrace_put_breakpoints(child);+return-EINVAL;+}+if(bp){+attr=bp->attr;+/*+*Considertheinputaddresstobeencodedwiththeread/write+*watchpointtyperequestandaligntheaddressbydefault.+*/+attr.bp_addr=(unsignedlong)bp_info->addr&~HW_BREAKPOINT_ALIGN;+arch_bp_generic_fields(dabr&+(DABR_DATA_WRITE|DABR_DATA_READ),+&attr.bp_type);+attr.bp_len=len;+ret=modify_user_hw_breakpoint(bp,&attr);+if(ret){+ptrace_put_breakpoints(child);+returnret;+}+thread->ptrace_bps[0]=bp;+ptrace_put_breakpoints(child);+thread->dabr=dabr;+return0;+}++/* Create a new breakpoint request if one doesn't exist already */+hw_breakpoint_init(&attr);+attr.bp_addr=(unsignedlong)bp_info->addr&~HW_BREAKPOINT_ALIGN;+attr.bp_len=len;+arch_bp_generic_fields(dabr&(DABR_DATA_WRITE|DABR_DATA_READ),+&attr.bp_type);++thread->ptrace_bps[0]=bp=register_user_hw_breakpoint(&attr,+ptrace_triggered,NULL,child);+if(IS_ERR(bp)){+thread->ptrace_bps[0]=NULL;+ptrace_put_breakpoints(child);+returnPTR_ERR(bp);+}++ptrace_put_breakpoints(child);return1;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */#endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */}staticlongppc_del_hwdebug(structtask_struct*child,longaddr,longdata){+#ifdef CONFIG_HAVE_HW_BREAKPOINT+structthread_struct*thread=&(child->thread);+structperf_event*bp;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */#ifdef CONFIG_PPC_ADV_DEBUG_REGSintrc;
@@ -1426,10 +1498,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)#elseif(data!=1)return-EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+if(ptrace_get_breakpoints(child)<0)+return-ESRCH;++bp=thread->ptrace_bps[0];+if(bp){+unregister_hw_breakpoint(bp);+thread->ptrace_bps[0]=NULL;+}+ptrace_put_breakpoints(child);+return0;+#else /* CONFIG_HAVE_HW_BREAKPOINT */if(child->thread.dabr==0)return-ENOENT;child->thread.dabr=0;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */return0;#endif
@@ -1536,7 +1622,8 @@ long arch_ptrace(struct task_struct *child, long request,casePPC_PTRACE_GETHWDBGINFO:{structppc_debug_infodbginfo;-dbginfo.version=1;+/* We return the highest version number supported */+dbginfo.version=2;#ifdef CONFIG_PPC_ADV_DEBUG_REGSdbginfo.num_instruction_bps=CONFIG_PPC_ADV_DEBUG_IACS;dbginfo.num_data_bps=CONFIG_PPC_ADV_DEBUG_DACS;
@@ -1560,7 +1647,7 @@ long arch_ptrace(struct task_struct *child, long request,dbginfo.data_bp_alignment=4;#endifdbginfo.sizeof_condition=0;-dbginfo.features=0;+dbginfo.features=PPC_DEBUG_FEATURE_DATA_BP_RANGE;#endif /* CONFIG_PPC_ADV_DEBUG_REGS */if(!access_ok(VERIFY_WRITE,datavp,
On Fri, 2011-08-26 at 14:41 +1000, David Gibson wrote:
On Wed, Aug 24, 2011 at 09:41:43PM -0300, Thiago Jung Bauermann wrote:
quoted
On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
quoted
On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
quoted
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.
I can see that. But you haven't answered why.
BookS doesn't support BP_EXACT, that's why I suggested this flag.
Surely you can support it with exactly the same sort of filtering
you're using for the 8-byte ranges now?
Yes, but to detect that the processor doesn't support BP_EXACT in
hardware I'd have to send a ptrace request, and have it rejected. Only
then I'd step back and simulate one with ranges. Considering that it's
easy and backwards compatible to add a new flag to signal that BP_EXACT
is not supported, I don't know why it would be better to go with the
more convoluted process.
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center
On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
quoted
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
quoted
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
So, the structure itself doesn't seem to have been extended. I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
I still don't follow you.
Two things here.
One, the change of semantics warranted an increment of the version
number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
BookS, while the old version number did not. I've added a small comment
in the code to this effect.
Two, regarding changes in the "ppc_hw_breakpoint" and "ppc_debug_info"
structures - we would like to add more members to it if we can (GDB has a
pending request to add more members to it). However the problem foreseen
is that there could be a mismatch between the versions of the structure
used by the user vs kernel-space i.e. if a new version of the structure,
known to the kernel, had an extra member while the user-space still had
the old version, then it becomes dangerous because the __copy_to_user
function would overflow the buffer size in user-space.
This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
designed to accept a version number (and provide corresponding
"struct ppc_debug_info") rather than send a populated "ppc_debug_info"
structure along with the version number.
Based on further discussions with the code-reviewer (David Gibson
[off-list ref]), it was decided that incrementing the version number
for the proposed changes is unnecessary as the patch only introduces new
features but not a change in semantics.
Please find a new version of the patch where the version number is
retained as 1, along with the other planned changes.
Thanks,
K.Prasad
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
Tested-by: Edjunior Barbosa Machado [off-list ref]
Signed-off-by: K.Prasad [off-list ref]
From: David Gibson <hidden> Date: 2011-09-19 02:46:03
On Tue, Aug 30, 2011 at 09:27:41PM -0300, Thiago Jung Bauermann wrote:
On Fri, 2011-08-26 at 14:41 +1000, David Gibson wrote:
quoted
On Wed, Aug 24, 2011 at 09:41:43PM -0300, Thiago Jung Bauermann wrote:
quoted
On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
quoted
On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
quoted
While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.
I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported. So, why do you need a new flag?
Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.
I can see that. But you haven't answered why.
BookS doesn't support BP_EXACT, that's why I suggested this flag.
Surely you can support it with exactly the same sort of filtering
you're using for the 8-byte ranges now?
Yes, but to detect that the processor doesn't support BP_EXACT in
hardware I'd have to send a ptrace request, and have it rejected. Only
then I'd step back and simulate one with ranges. Considering that it's
easy and backwards compatible to add a new flag to signal that BP_EXACT
is not supported, I don't know why it would be better to go with the
more convoluted process.
No, I'm saying why not implement BP_EXACT on server.
--
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
From: David Gibson <hidden> Date: 2011-10-12 03:34:11
On Fri, Sep 16, 2011 at 12:57:10PM +0530, K.Prasad wrote:
On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
quoted
On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
quoted
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
quoted
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
So, the structure itself doesn't seem to have been extended. I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
I still don't follow you.
Two things here.
One, the change of semantics warranted an increment of the version
number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
BookS, while the old version number did not. I've added a small comment
in the code to this effect.
Two, regarding changes in the "ppc_hw_breakpoint" and "ppc_debug_info"
structures - we would like to add more members to it if we can (GDB has a
pending request to add more members to it). However the problem foreseen
is that there could be a mismatch between the versions of the structure
used by the user vs kernel-space i.e. if a new version of the structure,
known to the kernel, had an extra member while the user-space still had
the old version, then it becomes dangerous because the __copy_to_user
function would overflow the buffer size in user-space.
This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
designed to accept a version number (and provide corresponding
"struct ppc_debug_info") rather than send a populated "ppc_debug_info"
structure along with the version number.
Based on further discussions with the code-reviewer (David Gibson
[off-list ref]), it was decided that incrementing the version number
for the proposed changes is unnecessary as the patch only introduces new
features but not a change in semantics.
Please find a new version of the patch where the version number is
retained as 1, along with the other planned changes.
Thanks,
K.Prasad
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
Above pargraph needs revision.
quoted hunk
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
Tested-by: Edjunior Barbosa Machado [off-list ref]
Signed-off-by: K.Prasad [off-list ref]
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS)++ p.version = 1;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT;
MODE_RANGE_EXACT? Shouldn't that just be MODE_EXACT?
quoted hunk
+
+ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+ p.addr = (uint64_t) begin_range;
+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+ * addr2 - addr <= 8 Bytes.
+ */
+ p.addr2 = (uint64_t) end_range;
+ p.condition_value = 0;
+
3. PTRACE_DELHWDEBUG
Takes an integer which identifies an existing breakpoint or watchpoint
@@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,*/if((bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_RW)==0||(bp_info->trigger_type&~PPC_BREAKPOINT_TRIGGER_RW)!=0||-bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT||bp_info->condition_mode!=PPC_BREAKPOINT_CONDITION_NONE)return-EINVAL;-if(child->thread.dabr)-return-ENOSPC;-if((unsignedlong)bp_info->addr>=TASK_SIZE)return-EIO;
@@ -1398,15 +1400,83 @@ static long ppc_set_hwdebug(struct task_struct *child,dabr|=DABR_DATA_READ;if(bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_WRITE)dabr|=DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+if(ptrace_get_breakpoints(child)<0)+return-ESRCH;-child->thread.dabr=dabr;+bp=thread->ptrace_bps[0];+if(!bp_info->addr){
I think this is treating a hardware breakpoint at address 0 as if it
didn't exist. That seems wrong.
+ if (bp) {
+ unregister_hw_breakpoint(bp);
+ thread->ptrace_bps[0] = NULL;
+ }
+ ptrace_put_breakpoints(child);
+ return 0;
+ }
+ /*
+ * Check if the request is for 'range' breakpoints. We can
+ * support it if range < 8 bytes.
+ */
+ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+ len = bp_info->addr2 - bp_info->addr;
+ else if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
+ ptrace_put_breakpoints(child);
+ return -EINVAL;
+ }
Misindent here. This statement would be clearer if you had {} on all
of the if branches.
@@ -1426,10 +1496,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) #else if (data != 1) return -EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (ptrace_get_breakpoints(child) < 0)+ return -ESRCH;++ bp = thread->ptrace_bps[0];+ if (bp) {+ unregister_hw_breakpoint(bp);+ thread->ptrace_bps[0] = NULL;+ }+ ptrace_put_breakpoints(child);+ return 0;+#else /* CONFIG_HAVE_HW_BREAKPOINT */ if (child->thread.dabr == 0) return -ENOENT; child->thread.dabr = 0;+#endif /* CONFIG_HAVE_HW_BREAKPOINT */ return 0; #endif
@@ -1560,7 +1644,7 @@ long arch_ptrace(struct task_struct *child, long request, dbginfo.data_bp_alignment = 4; #endif dbginfo.sizeof_condition = 0;- dbginfo.features = 0;+ dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE; #endif /* CONFIG_PPC_ADV_DEBUG_REGS */ if (!access_ok(VERIFY_WRITE, datavp,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Wed, Oct 12, 2011 at 02:33:59PM +1100, David Gibson wrote:
On Fri, Sep 16, 2011 at 12:57:10PM +0530, K.Prasad wrote:
quoted
On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
quoted
On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
quoted
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
quoted
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
quoted
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
[snipped]
quoted
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.
This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
"ppc_hw_breakpoint" and "ppc_debug_info" is incremented to denote new semantics.
Above pargraph needs revision.
Sure, done.
quoted
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]
Tested-by: Edjunior Barbosa Machado [off-list ref]
Signed-off-by: K.Prasad [off-list ref]
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS)++ p.version = 1;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT;
MODE_RANGE_EXACT? Shouldn't that just be MODE_EXACT?
That was silly. Thanks for pointing it out.
quoted
+
+ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+ p.addr = (uint64_t) begin_range;
+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+ * addr2 - addr <= 8 Bytes.
+ */
+ p.addr2 = (uint64_t) end_range;
+ p.condition_value = 0;
+
3. PTRACE_DELHWDEBUG
Takes an integer which identifies an existing breakpoint or watchpoint
@@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,*/if((bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_RW)==0||(bp_info->trigger_type&~PPC_BREAKPOINT_TRIGGER_RW)!=0||-bp_info->addr_mode!=PPC_BREAKPOINT_MODE_EXACT||bp_info->condition_mode!=PPC_BREAKPOINT_CONDITION_NONE)return-EINVAL;-if(child->thread.dabr)-return-ENOSPC;-if((unsignedlong)bp_info->addr>=TASK_SIZE)return-EIO;
@@ -1398,15 +1400,83 @@ static long ppc_set_hwdebug(struct task_struct *child,dabr|=DABR_DATA_READ;if(bp_info->trigger_type&PPC_BREAKPOINT_TRIGGER_WRITE)dabr|=DABR_DATA_WRITE;+#ifdef CONFIG_HAVE_HW_BREAKPOINT+if(ptrace_get_breakpoints(child)<0)+return-ESRCH;-child->thread.dabr=dabr;+bp=thread->ptrace_bps[0];+if(!bp_info->addr){
I think this is treating a hardware breakpoint at address 0 as if it
didn't exist. That seems wrong.
Yes, I think the above the condition need not exist after we've agreed
that 0 is a valid address to set breakpoint upon. One needs to use
PPC_PTRACE_DELHWDEBUG to delete a breakpoint and not by writing 0 (as
done with PTRACE_SET_DEBUGREG).
I'll remove that.
quoted
+ if (bp) {
+ unregister_hw_breakpoint(bp);
+ thread->ptrace_bps[0] = NULL;
+ }
+ ptrace_put_breakpoints(child);
+ return 0;
+ }
+ /*
+ * Check if the request is for 'range' breakpoints. We can
+ * support it if range < 8 bytes.
+ */
+ if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+ len = bp_info->addr2 - bp_info->addr;
+ else if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
+ ptrace_put_breakpoints(child);
+ return -EINVAL;
+ }
Misindent here. This statement would be clearer if you had {} on all
of the if branches.
If gdb is using the new breakpoint interface, surely it should just
use it, rather than doing this bit frobbing as in the old SET_DABR
call.
I understand that you wanted to avoid this duplication of effort in terms
of encoding and decoding the breakpoint type from
PPC_BREAKPOINT_TRIGGER_READ to DABR_DATA_READ to HW_BREAKPOINT_R.
However HW_BREAKPOINT_R is a generic definition used across
architectures, DABR_DATA_READ is used in the !CONFIG_HAVE_HW_BREAKPOINT
case while PPC_BREAKPOINT_TRIGGER_READ is used in
CONFIG_PPC_ADV_DEBUG_REGS case.
While we could define PPC_BREAKPOINT_TRIGGER_READ and DABR_DATA_READ to
the same value it may not result in any code savings (since the bit
translation is done for !CONFIG_HAVE_HW_BREAKPOINT case anyway). So, I
think it is best left the way it is.
I'm attaching the revised patch (after incorporating your comments).
Kindly let me know your comments.
Thanks,
K.Prasad
---------
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too. This patch enables the use of generic
hardware breakpoint interfaces for these new flags.
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
Tested-by: Edjunior Barbosa Machado <redacted>
Signed-off-by: K.Prasad <redacted>
---
Documentation/powerpc/ptrace.txt | 16 +++++++
arch/powerpc/kernel/ptrace.c | 88 +++++++++++++++++++++++++++++++++++---
2 files changed, 98 insertions(+), 6 deletions(-)
If gdb is using the new breakpoint interface, surely it should just
use it, rather than doing this bit frobbing as in the old SET_DABR
call.
I understand that you wanted to avoid this duplication of effort in terms
of encoding and decoding the breakpoint type from
PPC_BREAKPOINT_TRIGGER_READ to DABR_DATA_READ to HW_BREAKPOINT_R.
However HW_BREAKPOINT_R is a generic definition used across
architectures, DABR_DATA_READ is used in the !CONFIG_HAVE_HW_BREAKPOINT
case while PPC_BREAKPOINT_TRIGGER_READ is used in
CONFIG_PPC_ADV_DEBUG_REGS case.
While we could define PPC_BREAKPOINT_TRIGGER_READ and DABR_DATA_READ to
the same value it may not result in any code savings (since the bit
translation is done for !CONFIG_HAVE_HW_BREAKPOINT case anyway). So, I
think it is best left the way it is.
That's not what I'm suggesting. What I'm saying is that ig userspace
is using the new generic interface, then it should just set the
bp_type field, and it should not use the DABR_DATA_{READ,WRITE} bits.
The DABR_DATA bits should *only* be processed in the legacy interface,
never in the generic interface.
quoted hunk
I'm attaching the revised patch (after incorporating your comments).
Kindly let me know your comments.
Thanks,
K.Prasad
---------
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too. This patch enables the use of generic
hardware breakpoint interfaces for these new flags.
Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).
Tested-by: Edjunior Barbosa Machado <redacted>
Signed-off-by: K.Prasad <redacted>
---
Documentation/powerpc/ptrace.txt | 16 +++++++
arch/powerpc/kernel/ptrace.c | 88 +++++++++++++++++++++++++++++++++++---
2 files changed, 98 insertions(+), 6 deletions(-)
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS)++ p.version = 1;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;
You should probably document the alignment constraint on the address
here, too.
quoted hunk
+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+ * addr2 - addr <= 8 Bytes.
+ */
+ p.addr2 = (uint64_t) end_range;
+ p.condition_value = 0;
+
3. PTRACE_DELHWDEBUG
Takes an integer which identifies an existing breakpoint or watchpoint
You still have this code which has no business in the generic
interface path.
+ attr.bp_len = len;
+ ret = modify_user_hw_breakpoint(bp, &attr);
+ if (ret) {
+ ptrace_put_breakpoints(child);
+ return ret;
+ }
If a bp already exists, you're modifying it. I thought the semantics
of the new interface meant that you shoul return ENOSPC in this case,
and a DEL would be necessary before adding another breakpoint.
@@ -1560,7 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request, dbginfo.data_bp_alignment = 4; #endif dbginfo.sizeof_condition = 0;- dbginfo.features = 0;+ dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE; #endif /* CONFIG_PPC_ADV_DEBUG_REGS */ if (!access_ok(VERIFY_WRITE, datavp,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
If gdb is using the new breakpoint interface, surely it should just
use it, rather than doing this bit frobbing as in the old SET_DABR
call.
I understand that you wanted to avoid this duplication of effort in terms
of encoding and decoding the breakpoint type from
PPC_BREAKPOINT_TRIGGER_READ to DABR_DATA_READ to HW_BREAKPOINT_R.
However HW_BREAKPOINT_R is a generic definition used across
architectures, DABR_DATA_READ is used in the !CONFIG_HAVE_HW_BREAKPOINT
case while PPC_BREAKPOINT_TRIGGER_READ is used in
CONFIG_PPC_ADV_DEBUG_REGS case.
While we could define PPC_BREAKPOINT_TRIGGER_READ and DABR_DATA_READ to
the same value it may not result in any code savings (since the bit
translation is done for !CONFIG_HAVE_HW_BREAKPOINT case anyway). So, I
think it is best left the way it is.
That's not what I'm suggesting. What I'm saying is that ig userspace
is using the new generic interface, then it should just set the
bp_type field, and it should not use the DABR_DATA_{READ,WRITE} bits.
The DABR_DATA bits should *only* be processed in the legacy interface,
never in the generic interface.
The DABR_DATA_{READ,WRITE} bits are neither set by the user, nor
expected by the hw-breakpoint interface. It is an intermediate code used
to re-use the arch_bp_generic_fields function. We could convert directly
from PPC_BREAKPOINT_TRIGGER_READ to HW_BREAKPOINT_R (using a switch-case)
but that may not result in any code savings.
DABR_DATA_{READ,WRITE} is indeed legacy and cannot be set by user-space
for a PPC_PTRACE_SETHWDEBUG + CONFIG_HAVE_HW_BREAKPOINT combination.
[snipped]
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS)++ p.version = 1;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;
You should probably document the alignment constraint on the address
here, too.
Alignment constraints will be learnt by the user-space during runtime.
We provide that as part of 'struct ppc_debug_info' in
'data_bp_alignment' field.
While the alignment is always 8-bytes for BookS, I think userspace
should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.
quoted
+ /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+ * addr2 - addr <= 8 Bytes.
+ */
+ p.addr2 = (uint64_t) end_range;
+ p.condition_value = 0;
+
3. PTRACE_DELHWDEBUG
Takes an integer which identifies an existing breakpoint or watchpoint
You still have this code which has no business in the generic
interface path.
Same explanation as above. If I have to avoid the call to
arch_bp_generic_fields() then, it should be replaced with
switch(bp_info->trigger_type) {
case PPC_BREAKPOINT_TRIGGER_READ:
attr.bp_type = HW_BREAKPOINT_R;
case PPC_BREAKPOINT_TRIGGER_WRITE:
attr.bp_type = HW_BREAKPOINT_W;
case PPC_BREAKPOINT_TRIGGER_RW:
attr.bp_type = (HW_BREAKPOINT_W | HW_BREAKPOINT_R);
}
All these additional lines for no extra benefit (or I haven't
still understood your comments fully).
quoted
+ attr.bp_len = len;
+ ret = modify_user_hw_breakpoint(bp, &attr);
+ if (ret) {
+ ptrace_put_breakpoints(child);
+ return ret;
+ }
If a bp already exists, you're modifying it. I thought the semantics
of the new interface meant that you shoul return ENOSPC in this case,
and a DEL would be necessary before adding another breakpoint.
I'm not too sure what would be the desired behaviour for this interface,
either way is fine with me. I'd like to hear from the GDB folks (copied
in this email) to know what would please them.
@@ -1426,10 +1488,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) #else if (data != 1) return -EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (ptrace_get_breakpoints(child) < 0)+ return -ESRCH;++ bp = thread->ptrace_bps[0];+ if (bp) {+ unregister_hw_breakpoint(bp);+ thread->ptrace_bps[0] = NULL;+ }+ ptrace_put_breakpoints(child);+ return 0;
Shouldn't DEL return an error if there is no existing bp.
Same comment as above. We'd like to know what behaviour would help the
GDB use this interface better as there's no right or wrong way here.
Thanks again for your patient review. I will post the modified patch
after hearing comments from all.
Thanks.
K.Prasad
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS)++ p.version = 1;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;
You should probably document the alignment constraint on the address
here, too.
Alignment constraints will be learnt by the user-space during runtime.
We provide that as part of 'struct ppc_debug_info' in
'data_bp_alignment' field.
While the alignment is always 8-bytes for BookS, I think userspace
should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.
Right. In particular, BookE doesn't have alignment constraints.
quoted
quoted
+ attr.bp_len = len;
+ ret = modify_user_hw_breakpoint(bp, &attr);
+ if (ret) {
+ ptrace_put_breakpoints(child);
+ return ret;
+ }
If a bp already exists, you're modifying it. I thought the semantics
of the new interface meant that you shoul return ENOSPC in this case,
and a DEL would be necessary before adding another breakpoint.
I'm not too sure what would be the desired behaviour for this interface,
either way is fine with me. I'd like to hear from the GDB folks (copied
in this email) to know what would please them.
ENOSPC should be returned. The interface doesn't have provisions for
modifying breakpoints. The client should delete/create instead of trying
to modify.
Since PTRACE_PPC_GETHWDEBUGINFO returns the number of available
breakpoint registers, the client shouldn't (and GDB doesn't) try to set
more breakpoints than possible.
quoted
quoted
@@ -1426,10 +1488,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) #else if (data != 1) return -EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (ptrace_get_breakpoints(child) < 0)+ return -ESRCH;++ bp = thread->ptrace_bps[0];+ if (bp) {+ unregister_hw_breakpoint(bp);+ thread->ptrace_bps[0] = NULL;+ }+ ptrace_put_breakpoints(child);+ return 0;
Shouldn't DEL return an error if there is no existing bp.
Same comment as above. We'd like to know what behaviour would help the
GDB use this interface better as there's no right or wrong way here.
GDB expects DEL to return ENOENT is there's no existing bp.
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center
@@ -127,6 +127,22 @@ Some examples of using the structure to: p.addr2 = (uint64_t) end_range; p.condition_value = 0;+- set a watchpoint in server processors (BookS)++ p.version = 1;+ p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW;+ p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;+ or+ p.addr_mode = PPC_BREAKPOINT_MODE_EXACT;++ p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;+ p.addr = (uint64_t) begin_range;
You should probably document the alignment constraint on the address
here, too.
Alignment constraints will be learnt by the user-space during runtime.
We provide that as part of 'struct ppc_debug_info' in
'data_bp_alignment' field.
While the alignment is always 8-bytes for BookS, I think userspace
should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.
Right. In particular, BookE doesn't have alignment constraints.
Okay.
quoted
quoted
quoted
+ attr.bp_len = len;
+ ret = modify_user_hw_breakpoint(bp, &attr);
+ if (ret) {
+ ptrace_put_breakpoints(child);
+ return ret;
+ }
If a bp already exists, you're modifying it. I thought the semantics
of the new interface meant that you shoul return ENOSPC in this case,
and a DEL would be necessary before adding another breakpoint.
I'm not too sure what would be the desired behaviour for this interface,
either way is fine with me. I'd like to hear from the GDB folks (copied
in this email) to know what would please them.
ENOSPC should be returned. The interface doesn't have provisions for
modifying breakpoints. The client should delete/create instead of trying
to modify.
Since PTRACE_PPC_GETHWDEBUGINFO returns the number of available
breakpoint registers, the client shouldn't (and GDB doesn't) try to set
more breakpoints than possible.
Okay, I will modify the code accordingly.
quoted
quoted
quoted
@@ -1426,10 +1488,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) #else if (data != 1) return -EINVAL;++#ifdef CONFIG_HAVE_HW_BREAKPOINT+ if (ptrace_get_breakpoints(child) < 0)+ return -ESRCH;++ bp = thread->ptrace_bps[0];+ if (bp) {+ unregister_hw_breakpoint(bp);+ thread->ptrace_bps[0] = NULL;+ }+ ptrace_put_breakpoints(child);+ return 0;
Shouldn't DEL return an error if there is no existing bp.
Same comment as above. We'd like to know what behaviour would help the
GDB use this interface better as there's no right or wrong way here.
GDB expects DEL to return ENOENT is there's no existing bp.
Fine, here too. We'll return a -ENOENT here.
Thanks for your comments.
-- K.Prasad