Re: Fix amd64 ddb hardware watchpoints for SMP

2013-05-20 Thread John Baldwin
On Thursday, May 16, 2013 2:41:27 am Konstantin Belousov wrote:
 The ddb use of hardware watchpoints on the x86 architectures is known to
 be lacking. There are at least two known problems. One is the improper
 interaction with the user-mode debuggers which use debug registers.
 Another is that ddb only loads the debug registers for the watchpoint
 into the CPU which is executing ddb code, not setting up non-current
 processors.
 
 Not touching the first problem for now, I want to fix the second issue,
 since as is, hardware watchpoints are useless on SMP. Patch below makes
 the stopped processors to load the debug registers after resuming from
 the stop handler, if directed by ddb.
 
 Also the patch contains two other commands for ddb which made my
 exprerience with debugger on amd64 better. The 'show pginfo[/p] addr'
 command dumps the vm_page_t information, either by vm_page address, or,
 if the /p modifier is specified, by the physical page address. The 'show
 phys2dmap addr' command translates physical address into the directly
 mapped address, which can be accessed from ddb then.

This looks fine to me.  It would be nice to fix i386 as well to be consistent.
I would commit the new DDB commands as a separate patch from the watchpoint
fixes.

-- 
John Baldwin
___
freebsd-amd64@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-amd64
To unsubscribe, send any mail to freebsd-amd64-unsubscr...@freebsd.org


Fix amd64 ddb hardware watchpoints for SMP

2013-05-16 Thread Konstantin Belousov
The ddb use of hardware watchpoints on the x86 architectures is known to
be lacking. There are at least two known problems. One is the improper
interaction with the user-mode debuggers which use debug registers.
Another is that ddb only loads the debug registers for the watchpoint
into the CPU which is executing ddb code, not setting up non-current
processors.

Not touching the first problem for now, I want to fix the second issue,
since as is, hardware watchpoints are useless on SMP. Patch below makes
the stopped processors to load the debug registers after resuming from
the stop handler, if directed by ddb.

Also the patch contains two other commands for ddb which made my
exprerience with debugger on amd64 better. The 'show pginfo[/p] addr'
command dumps the vm_page_t information, either by vm_page address, or,
if the /p modifier is specified, by the physical page address. The 'show
phys2dmap addr' command translates physical address into the directly
mapped address, which can be accessed from ddb then.

diff --git a/sys/amd64/amd64/db_trace.c b/sys/amd64/amd64/db_trace.c
index 2c81f87..39297d9 100644
--- a/sys/amd64/amd64/db_trace.c
+++ b/sys/amd64/amd64/db_trace.c
@@ -33,6 +33,7 @@ __FBSDID($FreeBSD$);
 #include sys/systm.h
 #include sys/kdb.h
 #include sys/proc.h
+#include sys/smp.h
 #include sys/stack.h
 #include sys/sysent.h
 
@@ -63,6 +64,8 @@ static db_varfcn_t db_frame;
 static db_varfcn_t db_rsp;
 static db_varfcn_t db_ss;
 
+CTASSERT(sizeof(struct dbreg) == sizeof(((struct pcpu *)NULL)-pc_dbreg));
+
 /*
  * Machine register set.
  */
@@ -591,64 +594,82 @@ db_md_set_watchpoint(addr, size)
db_expr_t addr;
db_expr_t size;
 {
-   struct dbreg d;
-   int avail, i, wsize;
+   struct dbreg *d;
+   struct pcpu *pc;
+   int avail, c, cpu, i, wsize;
 
-   fill_dbregs(NULL, d);
+   d = (struct dbreg *)PCPU_PTR(dbreg);
+   cpu = PCPU_GET(cpuid);
+   fill_dbregs(NULL, d);
 
avail = 0;
-   for(i = 0; i  4; i++) {
-   if (!DBREG_DR7_ENABLED(d.dr[7], i))
+   for (i = 0; i  4; i++) {
+   if (!DBREG_DR7_ENABLED(d-dr[7], i))
avail++;
}
 
if (avail * 8  size)
return (-1);
 
-   for (i = 0; i  4  (size  0); i++) {
-   if (!DBREG_DR7_ENABLED(d.dr[7], i)) {
+   for (i = 0; i  4  size  0; i++) {
+   if (!DBREG_DR7_ENABLED(d-dr[7], i)) {
if (size = 8 || (avail == 1  size  4))
wsize = 8;
else if (size  2)
wsize = 4;
else
wsize = size;
-   amd64_set_watch(i, addr, wsize,
-  DBREG_DR7_WRONLY, d);
+   amd64_set_watch(i, addr, wsize, DBREG_DR7_WRONLY, d);
addr += wsize;
size -= wsize;
avail--;
}
}
 
-   set_dbregs(NULL, d);
+   set_dbregs(NULL, d);
+   CPU_FOREACH(c) {
+   if (c == cpu)
+   continue;
+   pc = pcpu_find(c);
+   memcpy(pc-pc_dbreg, d, sizeof(*d));
+   pc-pc_dbreg_cmd = PC_DBREG_CMD_LOAD;
+   }
 
-   return(0);
+   return (0);
 }
 
-
 int
 db_md_clr_watchpoint(addr, size)
db_expr_t addr;
db_expr_t size;
 {
-   struct dbreg d;
-   int i;
+   struct dbreg *d;
+   struct pcpu *pc;
+   int i, c, cpu;
 
-   fill_dbregs(NULL, d);
+   d = (struct dbreg *)PCPU_PTR(dbreg);
+   cpu = PCPU_GET(cpuid);
+   fill_dbregs(NULL, d);
 
-   for(i = 0; i  4; i++) {
-   if (DBREG_DR7_ENABLED(d.dr[7], i)) {
-   if ((DBREG_DRX((d), i) = addr) 
-   (DBREG_DRX((d), i)  addr+size))
-   amd64_clr_watch(i, d);
+   for (i = 0; i  4; i++) {
+   if (DBREG_DR7_ENABLED(d-dr[7], i)) {
+   if (DBREG_DRX((d), i) = addr 
+   DBREG_DRX((d), i)  addr + size)
+   amd64_clr_watch(i, d);
 
}
}
 
-   set_dbregs(NULL, d);
+   set_dbregs(NULL, d);
+   CPU_FOREACH(c) {
+   if (c == cpu)
+   continue;
+   pc = pcpu_find(c);
+   memcpy(pc-pc_dbreg, d, sizeof(*d));
+   pc-pc_dbreg_cmd = PC_DBREG_CMD_LOAD;
+   }
 
-   return(0);
+   return (0);
 }
 
 
@@ -699,3 +720,17 @@ db_md_list_watchpoints()
}
db_printf(\n);
 }
+
+void
+amd64_db_resume_dbreg(void)
+{
+   struct dbreg *d;
+
+   switch (PCPU_GET(dbreg_cmd)) {
+   case PC_DBREG_CMD_LOAD:
+   d = (struct dbreg *)PCPU_PTR(dbreg);
+   set_dbregs(NULL, d);
+   PCPU_SET(dbreg_cmd, PC_DBREG_CMD_NONE);
+