On Thu, Mar 25, 2004 at 03:54:39PM +0530, Amit S. Kale wrote:
> On Thursday 25 Mar 2004 3:10 am, Tom Rini wrote:
> > On Wed, Mar 24, 2004 at 03:24:02PM -0500, Anurekh Saxena wrote:
> > > On Wed, Mar 24, 2004 at 08:05:21PM +0530, Amit S. Kale wrote:
> > > > On Friday 19 Mar 2004 9:33 pm, Tom Rini wrote:
> > > > > Hi. Right now I'm writing up a porting doc that describes the
> > > > > various hook functions we've got. I noticed that nothing is calling
> > > > > kgdb_arch_set/remove_break. Is there some arch we're expecting will
> > > > > need this? I'd like to just go ahead and remove them
> > > >
> > > > I can't remember why that was done. A processor other than PPC, x86 and
> > > > x86_64 needs a special implementation of set and remove breakpoint, I
> > > > guess.
> > > >
> > > > Anurekh, who did initial implementation of arch independent-dependent
> > > > split may have some comments on this.
> > >
> > > *set_break
> > > *remove_break
> > >
> > > These functions should only be defined for architecutes that support
> > > hardware breakpoint. Set KGDB_HW_BREAKPOINT flag.
> >
> > Amit, I think we've got a bug on i386 then. Looking at i386-lite.patch,
> > there's:
> > +void kgdb_correct_hw_break(void)
> > +int kgdb_remove_hw_break(unsigned long addr, int type)
> > +int kgdb_set_hw_break(unsigned long addr, int type)
> > +int remove_hw_break(unsigned breakno)
> > +int set_hw_break(unsigned breakno, unsigned type, unsigned len, unsigned
> > addr)
> >
> > Of these, only kgdb_correct_hw_break is called in core-lite.patch, and
> > set_hw_break/remove_hw_break (for y/Y packets) are called in
> > i386-lite.patch.
> >
> > What I think we need to do is, since y/Y packets are reserved, I'm
> > assuming there's a special version of GDB using these for hw
> > breakpoints, and this needs to be handled in i386-lite.patch.
> > Since core-lite's handling of a z/Z* packet is to assume setting a
> > breakpoint, and hw or sw is controlled by the KGDB_HW_BREAKPOINT flag,
> > we need to make sure this (a) works and (b) is actually calling useful
> > functions.
>
> I had used Y,y packets since Z,z packets weren't supported. Now we support Z,z
> packets. It's time to move Y,y packet functionality to Z,z. That way it'll be
> possible to use gdb commands themselves to place hardware breakpoints.
>
> Anurekh found one more problem. We don't consider hardware breakpoints when
> removing all breakpoints. We should fix that.
The following is an incomplete patch vs core-lite and i386-lite that
_needs_ someone that understand i386 and hardware breakpoints to audit
all of the code related to that in i386/kernel/kgdb.c.
We fix the remove_all_breakpoint issue by adding
kgdb_remove_all_hw_break(), which must do whatever is needed to actually
accomplish that (I don't see how we're even setting a hw break now,
unless there's a call to correct_hw_break later which actually makes
them do something). I've also gone and made the z/Z packet handling
slightly different, and noted where we got the docs on how to handle the
packet that way.
arch/i386/kernel/kgdb.c | 84 +++++++++++++-----------------------------------
include/linux/kgdb.h | 5 +-
kernel/kgdb.c | 67 ++++++++++++++++++++------------------
3 files changed, 62 insertions(+), 94 deletions(-)
diff -u linux-2.6.4/include/linux/kgdb.h linux-2.6.4/include/linux/kgdb.h
--- linux-2.6.4/include/linux/kgdb.h 2004-03-19 08:22:37.143170735 -0700
+++ linux-2.6.4/include/linux/kgdb.h 2004-03-25 12:28:18.370953120 -0700
@@ -84,8 +84,9 @@
extern int kgdb_arch_handle_exception(int vector, int signo, int err_code,
char *InBuffer, char *outBuffer,
struct pt_regs *regs);
-extern int kgdb_arch_set_break(unsigned long addr, int type);
-extern int kgdb_arch_remove_break(unsigned long addr, int type);
+extern int kgdb_set_hw_break(unsigned long addr);
+extern int kgdb_remove_hw_break(unsigned long addr);
+extern void kgdb_remove_all_hw_break(void);
extern void kgdb_correct_hw_break(void);
extern void kgdb_shadowinfo(struct pt_regs *regs, char *buffer,
unsigned threadid);
diff -u linux-2.6.4/kernel/kgdb.c linux-2.6.4/kernel/kgdb.c
--- linux-2.6.4/kernel/kgdb.c 2004-03-19 08:22:37.147169789 -0700
+++ linux-2.6.4/kernel/kgdb.c 2004-03-25 12:27:48.959587058 -0700
@@ -157,13 +157,13 @@
}
int __attribute__ ((weak))
- kgdb_arch_set_break(unsigned long addr, int type)
+ kgdb_set_hw_break(unsigned long addr)
{
return 0;
}
int __attribute__ ((weak))
- kgdb_arch_remove_break(unsigned long addr, int type)
+ kgdb_remove_hw_break(unsigned long addr)
{
return 0;
}
@@ -206,7 +206,6 @@
{
unsigned char checksum;
unsigned char xmitcsum;
- int i;
int count;
char ch;
@@ -540,7 +539,7 @@
return 0;
}
-static int set_break(unsigned long addr)
+static int kgdb_set_sw_break(unsigned long addr)
{
int i, breakno = -1;
int error;
@@ -576,7 +575,7 @@
return 0;
}
-static int remove_break(unsigned long addr)
+static int kgdb_remove_sw_break(unsigned long addr)
{
int i;
int error;
@@ -602,6 +601,8 @@
{
int i;
int error;
+
+ /* Clear memory breakpoints. */
for (i = 0; i < MAX_BREAKPOINTS; i++) {
if (kgdb_break[i].state == bp_enabled) {
unsigned long addr = kgdb_break[i].bpt_addr;
@@ -615,6 +616,10 @@
}
kgdb_break[i].state = bp_disabled;
}
+
+ /* Clear hardware breakpoints. */
+ kgdb_remove_all_hw_break();
+
return 0;
}
@@ -710,7 +715,7 @@
kgdb_usethreadid = shadow_pid(current->pid);
while (1) {
- int bpt_type = 0;
+ char *bpt_type;
error = 0;
/* Clear the out buffer. */
@@ -964,41 +969,39 @@
else
error_packet(remcom_out_buffer, -EINVAL);
break;
+ /* Since GDB-5.3, it's been drafted that '0' is a software
+ * breakpoint, '1' is a hardware breakpoint, so let's do
+ * that.
+ */
case 'z':
case 'Z':
+ bpt_type = &remcom_in_buffer[1];
ptr = &remcom_in_buffer[2];
+
+ if (*bpt_type != '0' && *bpt_type != '1')
+ /* Unsupported. */
+ break;
+ /* Test if this is a hardware breakpoint, and
+ * if we support it. */
+ if (*bpt_type == '1' && !(kgdb_ops->flags &
+ KGDB_HW_BREAKPOINT))
+ /* Unsupported. */
+ break;
+
if (*(ptr++) != ',') {
error_packet(remcom_out_buffer, -EINVAL);
break;
}
kgdb_hex2long(&ptr, &addr);
- bpt_type = remcom_in_buffer[1];
- if (bpt_type != bp_breakpoint) {
- if (bpt_type == bp_hardware_breakpoint &&
- !(kgdb_ops->flags & KGDB_HW_BREAKPOINT))
- break;
-
- /* if set_break is not defined, then
- * remove_break does not matter
- */
- if (!(kgdb_ops->flags & KGDB_HW_BREAKPOINT))
- break;
- }
-
- if (remcom_in_buffer[0] == 'Z') {
- if (bpt_type == bp_breakpoint)
- error = set_break(addr);
- else
- error = kgdb_arch_set_break(addr,
- bpt_type);
- } else {
- if (bpt_type == bp_breakpoint)
- error = remove_break(addr);
- else
- error = kgdb_arch_remove_break(addr,
- bpt_type);
- }
+ if (remcom_in_buffer[0] == 'Z' && *bpt_type == '0')
+ error = kgdb_set_sw_break(addr);
+ else if (remcom_in_buffer[0] == 'Z' && *bpt_type == '1')
+ error = kgdb_set_hw_break(addr);
+ else if (remcom_in_buffer[0] == 'z' && *bpt_type == '0')
+ error = kgdb_remove_sw_break(addr);
+ else if (remcom_in_buffer[0] == 'z' && *bpt_type == '1')
+ error = kgdb_remove_hw_break(addr);
if (error == 0)
strcpy(remcom_out_buffer, "OK");
diff -u linux-2.6.4/arch/i386/kernel/kgdb.c linux-2.6.4/arch/i386/kernel/kgdb.c
--- linux-2.6.4/arch/i386/kernel/kgdb.c 2004-03-19 08:29:47.155575704 -0700
+++ linux-2.6.4/arch/i386/kernel/kgdb.c 2004-03-25 12:28:46.987498490 -0700
@@ -124,11 +124,12 @@
unsigned type;
unsigned len;
unsigned addr;
-} breakinfo[4] = { {
-enabled:0}, {
-enabled:0}, {
-enabled:0}, {
-enabled:0}};
+} breakinfo[4] = {
+ { .enabled = 0 },
+ { .enabled = 0 },
+ { .enabled = 0 },
+ { .enabled = 0 },
+};
void kgdb_correct_hw_break(void)
{
@@ -189,7 +190,7 @@
}
}
-int kgdb_remove_hw_break(unsigned long addr, int type)
+int kgdb_remove_hw_break(unsigned long addr)
{
int i, idx = -1;
for (i = 0; i < 4; i++) {
@@ -205,7 +206,20 @@
return 0;
}
-int kgdb_set_hw_break(unsigned long addr, int type)
+void kgdb_remove_all_hw_break(void)
+{
+ int i;
+
+ for (i = 0; i < 4; i++) {
+ if (breakinfo[i].enabled) {
+ /* Do what? */
+ ;
+ }
+ memset(&breakinfo[i], 0, sizeof(struct hw_breakpoint));
+ }
+}
+
+int kgdb_set_hw_break(unsigned long addr)
{
int i, idx = -1;
for (i = 0; i < 4; i++) {
@@ -218,33 +232,12 @@
return -1;
breakinfo[idx].enabled = 1;
- breakinfo[idx].type = type;
+ breakinfo[idx].type = 1;
breakinfo[idx].len = 1;
breakinfo[idx].addr = addr;
return 0;
}
-int remove_hw_break(unsigned breakno)
-{
- if (!breakinfo[breakno].enabled) {
- return -1;
- }
- breakinfo[breakno].enabled = 0;
- return 0;
-}
-
-int set_hw_break(unsigned breakno, unsigned type, unsigned len, unsigned addr)
-{
- if (breakinfo[breakno].enabled) {
- return -1;
- }
- breakinfo[breakno].enabled = 1;
- breakinfo[breakno].type = type;
- breakinfo[breakno].len = len;
- breakinfo[breakno].addr = addr;
- return 0;
-}
-
void kgdb_printexceptioninfo(int exceptionNo, int errorcode, char *buffer)
{
unsigned dr6;
@@ -293,8 +286,8 @@
char *remcom_out_buffer,
struct pt_regs *linux_regs)
{
- long addr, length;
- long breakno, breaktype;
+ long addr;
+ long breakno;
char *ptr;
int newPC;
int dr6;
@@ -341,35 +334,6 @@
asm volatile ("movl %0, %%db6\n"::"r" (0));
return (0);
-
- case 'Y':
- ptr = &remcom_in_buffer[1];
- kgdb_hex2long(&ptr, &breakno);
- ptr++;
- kgdb_hex2long(&ptr, &breaktype);
- ptr++;
- kgdb_hex2long(&ptr, &length);
- ptr++;
- kgdb_hex2long(&ptr, &addr);
- if (set_hw_break(breakno & 0x3, breaktype & 0x3,
- length & 0x3, addr) == 0) {
- strcpy(remcom_out_buffer, "OK");
- } else {
- strcpy(remcom_out_buffer, "ERROR");
- }
- break;
-
- /* Remove hardware breakpoint */
- case 'y':
- ptr = &remcom_in_buffer[1];
- kgdb_hex2long(&ptr, &breakno);
- if (remove_hw_break(breakno & 0x3) == 0) {
- strcpy(remcom_out_buffer, "OK");
- } else {
- strcpy(remcom_out_buffer, "ERROR");
- }
- break;
-
} /* switch */
return -1; /* this means that we do not want to exit from the handler */
}
--
Tom Rini
http://gate.crashing.org/~trini/