[Qemu-devel] [PATCH 1/3] define name for some fields of dr7

[Qemu-devel] [PATCH 1/3] define name for some fields of dr7

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-i386/cpu.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 90ef1ff..7f292e6 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -558,6 +558,19 @@
 #define CPU_INTERRUPT_TPR       CPU_INTERRUPT_TGT_INT_3
 
 
+/* dr7 fields */
+/* max breakpoints*/
+#define MAX_BP      4
+/* Break on instruction execution only */
+#define BP_INST     0x0
+/* Break on data writes only */
+#define BP_DATA_WR  0x1
+/* Break on I/O reads or writes */
+#define BP_IO_RW    0x10
+/* Break on data reads or writes but not instruction fetches */
+#define BP_DATA_RW  0x11
+
+
 enum {
     CC_OP_DYNAMIC, /* must use dynamic code to get cc_op */
     CC_OP_EFLAGS,  /* all cc are explicitly computed, CC_SRC = flags */
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/3] use dr7's bit name for breakpoint

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-i386/cpu.h         |    2 ++
 target-i386/helper.c      |   24 +++++++++++-------------
 target-i386/misc_helper.c |    6 +++---
 target-i386/seg_helper.c  |    6 +++---
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 7f292e6..7ecfe21 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -561,6 +561,8 @@
 /* dr7 fields */
 /* max breakpoints*/
 #define MAX_BP      4
+/*enable local breakpoint bit 0,2,4,6*/
+#define BP_LOCAL    0x55
 /* Break on instruction execution only */
 #define BP_INST     0x0
 /* Break on data writes only */
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 5686130..9ca52a7 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -969,24 +969,23 @@ void hw_breakpoint_insert(CPUX86State *env, int index)
     int type, err = 0;
 
     switch (hw_breakpoint_type(env->dr[7], index)) {
-    case 0:
+    case BP_INST:
         if (hw_breakpoint_enabled(env->dr[7], index))
             err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU,
                                         &env->cpu_breakpoint[index]);
         break;
-    case 1:
+    case BP_DATA_WR:
         type = BP_CPU | BP_MEM_WRITE;
         goto insert_wp;
-    case 2:
-         /* No support for I/O watchpoints yet */
-        break;
-    case 3:
+    case BP_DATA_RW:
         type = BP_CPU | BP_MEM_ACCESS;
     insert_wp:
         err = cpu_watchpoint_insert(env, env->dr[index],
                                     hw_breakpoint_len(env->dr[7], index),
                                     type, &env->cpu_watchpoint[index]);
-        break;
+	case BP_IO_RW:
+	  /* No support for I/O watchpoints yet */
+	  break;
     }
     if (err)
         env->cpu_breakpoint[index] = NULL;
@@ -997,15 +996,14 @@ void hw_breakpoint_remove(CPUX86State *env, int index)
     if (!env->cpu_breakpoint[index])
         return;
     switch (hw_breakpoint_type(env->dr[7], index)) {
-    case 0:
+    case BP_INST:
         if (hw_breakpoint_enabled(env->dr[7], index))
             cpu_breakpoint_remove_by_ref(env, env->cpu_breakpoint[index]);
         break;
-    case 1:
-    case 3:
+    case BP_DATA_WR:
+    case BP_DATA_RW:
         cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
-        break;
-    case 2:
+    case BP_IO_RW:
         /* No support for I/O watchpoints yet */
         break;
     }
@@ -1018,7 +1016,7 @@ int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
     int hit_enabled = 0;
 
     dr6 = env->dr[6] & ~0xf;
-    for (reg = 0; reg < 4; reg++) {
+    for (reg = 0; reg < MAX_BP; reg++) {
         type = hw_breakpoint_type(env->dr[7], reg);
         if ((type == 0 && env->dr[reg] == env->eip) ||
             ((type & 1) && env->cpu_watchpoint[reg] &&
diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c
index a020379..9d1187a 100644
--- a/target-i386/misc_helper.c
+++ b/target-i386/misc_helper.c
@@ -192,16 +192,16 @@ void helper_movl_drN_T0(CPUX86State *env, int reg, target_ulong t0)
 {
     int i;
 
-    if (reg < 4) {
+    if (reg < MAX_BP) {
         hw_breakpoint_remove(env, reg);
         env->dr[reg] = t0;
         hw_breakpoint_insert(env, reg);
     } else if (reg == 7) {
-        for (i = 0; i < 4; i++) {
+        for (i = 0; i < MAX_BP; i++) {
             hw_breakpoint_remove(env, i);
         }
         env->dr[7] = t0;
-        for (i = 0; i < 4; i++) {
+        for (i = 0; i < MAX_BP; i++) {
             hw_breakpoint_insert(env, i);
         }
     } else {
diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index ff93374..e99b287 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -465,13 +465,13 @@ static void switch_tss(CPUX86State *env, int tss_selector,
 
 #ifndef CONFIG_USER_ONLY
     /* reset local breakpoints */
-    if (env->dr[7] & 0x55) {
-        for (i = 0; i < 4; i++) {
+    if (env->dr[7] & BP_LOCAL) {
+        for (i = 0; i < MAX_BP; i++) {
             if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) {
                 hw_breakpoint_remove(env, i);
             }
         }
-        env->dr[7] &= ~0x55;
+        env->dr[7] &= ~BP_LOCAL;
     }
 #endif
 }
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 2/3] use dr7's bit name for breakpoint

[Peter Maydell]

On 29 November 2012 03:32, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/cpu.h         |    2 ++
>  target-i386/helper.c      |   24 +++++++++++-------------
>  target-i386/misc_helper.c |    6 +++---
>  target-i386/seg_helper.c  |    6 +++---
>  4 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 7f292e6..7ecfe21 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -561,6 +561,8 @@
>  /* dr7 fields */
>  /* max breakpoints*/
>  #define MAX_BP      4
> +/*enable local breakpoint bit 0,2,4,6*/
> +#define BP_LOCAL    0x55

This needs a better name, to make it clear that it's not
just a single enable bit but actually a mask of all the
local enable bits. Also needs DR7_ prefix.

You've split these changes up between patches inconsistently;
either have one patch which adds all the constants and
then patches which just use them, or have patches which
both add and use the constants, but don't mix the two.

I'd recommend that each patch should both add and use a
related set of constants, so it's self contained and
easy to review.

-- PMM

Re: [Qemu-devel] [PATCH 2/3] use dr7's bit name for breakpoint

[li guang]

在 2012-11-29四的 11:28 +0000，Peter Maydell写道：
> On 29 November 2012 03:32, liguang <lig.fnst@cn.fujitsu.com> wrote:
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  target-i386/cpu.h         |    2 ++
> >  target-i386/helper.c      |   24 +++++++++++-------------
> >  target-i386/misc_helper.c |    6 +++---
> >  target-i386/seg_helper.c  |    6 +++---
> >  4 files changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 7f292e6..7ecfe21 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -561,6 +561,8 @@
> >  /* dr7 fields */
> >  /* max breakpoints*/
> >  #define MAX_BP      4
> > +/*enable local breakpoint bit 0,2,4,6*/
> > +#define BP_LOCAL    0x55
> 
> This needs a better name, to make it clear that it's not
> just a single enable bit but actually a mask of all the
> local enable bits. Also needs DR7_ prefix.
> 
> You've split these changes up between patches inconsistently;
> either have one patch which adds all the constants and
> then patches which just use them, or have patches which
> both add and use the constants, but don't mix the two.
> 
> I'd recommend that each patch should both add and use a
> related set of constants, so it's self contained and
> easy to review.

you're right, thanks!

> 
> -- PMM
> 

-- 
regards!
li guang                  
linux kernel team at FNST, china

thinking with brain but heart
living with heart but brain

[Qemu-devel] [PATCH 3/3] target-i386:refactor check_hw_breakpoints function

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-i386/helper.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 9ca52a7..a506df0 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1012,22 +1012,34 @@ void hw_breakpoint_remove(CPUX86State *env, int index)
 int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
 {
     target_ulong dr6;
-    int reg, type;
+    int index;
     int hit_enabled = 0;
 
     dr6 = env->dr[6] & ~0xf;
-    for (reg = 0; reg < MAX_BP; reg++) {
-        type = hw_breakpoint_type(env->dr[7], reg);
-        if ((type == 0 && env->dr[reg] == env->eip) ||
-            ((type & 1) && env->cpu_watchpoint[reg] &&
-             (env->cpu_watchpoint[reg]->flags & BP_WATCHPOINT_HIT))) {
-            dr6 |= 1 << reg;
-            if (hw_breakpoint_enabled(env->dr[7], reg))
+    for (index = 0; index < MAX_BP; index++) {
+        switch (hw_breakpoint_type(env->dr[7], index)){
+        case BP_INST:
+            if (env->dr[index] != env->eip)
+                break;
+            goto enable_hit;
+        case BP_DATA_WR:
+        case BP_DATA_RW:
+            if (!env->cpu_watchpoint[index])
+                break;
+            if (!(env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT))
+                break;
+        enable_hit:
+            dr6 |= 1 << index;
+            if (hw_breakpoint_enabled(env->dr[7], index))
                 hit_enabled = 1;
+            break;
+        case BP_IO_RW:
+            break;
         }
     }
     if (hit_enabled || force_dr6_update)
         env->dr[6] = dr6;
+
     return hit_enabled;
 }
 
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 3/3] target-i386:refactor check_hw_breakpoints function

[Peter Maydell]

On 29 November 2012 03:32, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/helper.c |   28 ++++++++++++++++++++--------
>  1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 9ca52a7..a506df0 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1012,22 +1012,34 @@ void hw_breakpoint_remove(CPUX86State *env, int index)
>  int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
>  {
>      target_ulong dr6;
> -    int reg, type;
> +    int index;
>      int hit_enabled = 0;
>
>      dr6 = env->dr[6] & ~0xf;
> -    for (reg = 0; reg < MAX_BP; reg++) {
> -        type = hw_breakpoint_type(env->dr[7], reg);
> -        if ((type == 0 && env->dr[reg] == env->eip) ||
> -            ((type & 1) && env->cpu_watchpoint[reg] &&
> -             (env->cpu_watchpoint[reg]->flags & BP_WATCHPOINT_HIT))) {
> -            dr6 |= 1 << reg;
> -            if (hw_breakpoint_enabled(env->dr[7], reg))
> +    for (index = 0; index < MAX_BP; index++) {
> +        switch (hw_breakpoint_type(env->dr[7], index)){
> +        case BP_INST:
> +            if (env->dr[index] != env->eip)
> +                break;
> +            goto enable_hit;
> +        case BP_DATA_WR:
> +        case BP_DATA_RW:
> +            if (!env->cpu_watchpoint[index])
> +                break;
> +            if (!(env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT))
> +                break;
> +        enable_hit:
> +            dr6 |= 1 << index;
> +            if (hw_breakpoint_enabled(env->dr[7], index))
>                  hit_enabled = 1;
> +            break;
> +        case BP_IO_RW:
> +            break;
>          }

If you have to resort to gotos and fallthroughs in
a switch statement, I don't think this is actually
clearer than the existing code...

(Also it has coding style issues, use scripts/checkpatch.pl.)

-- PMM

Re: [Qemu-devel] [PATCH 1/3] define name for some fields of dr7

[li guang]

sorry, need a fix like following.

在 2012-11-29四的 11:32 +0800，liguang写道：
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/cpu.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 90ef1ff..7f292e6 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -558,6 +558,19 @@
>  #define CPU_INTERRUPT_TPR       CPU_INTERRUPT_TGT_INT_3
>  
> 
> +/* dr7 fields */
> +/* max breakpoints*/
> +#define MAX_BP      4
> +/* Break on instruction execution only */
> +#define BP_INST     0x0
> +/* Break on data writes only */
> +#define BP_DATA_WR  0x1
> +/* Break on I/O reads or writes */
> +#define BP_IO_RW    0x10
> +/* Break on data reads or writes but not instruction fetches */
> +#define BP_DATA_RW  0x11
> +
> +

@@ -568,9 +568,9 @@
 /* Break on data writes only */
 #define BP_DATA_WR  0x1
 /* Break on I/O reads or writes */
-#define BP_IO_RW    0x10
+#define BP_IO_RW    0x2
 /* Break on data reads or writes but not instruction fetches */
-#define BP_DATA_RW  0x11
+#define BP_DATA_RW  0x3
 


>  enum {
>      CC_OP_DYNAMIC, /* must use dynamic code to get cc_op */
>      CC_OP_EFLAGS,  /* all cc are explicitly computed, CC_SRC = flags */

-- 
regards!
li guang                  
linux kernel team at FNST, china

thinking with brain but heart
living with heart but brain

Re: [Qemu-devel] [PATCH 1/3] define name for some fields of dr7

[Peter Maydell]

On 29 November 2012 03:32, liguang <lig.fnst@cn.fujitsu.com> wrote:

Your Subject: line is missing the "target-i386:" prefix.
(also, should be "names".)

> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/cpu.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 90ef1ff..7f292e6 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -558,6 +558,19 @@
>  #define CPU_INTERRUPT_TPR       CPU_INTERRUPT_TGT_INT_3
>
>
> +/* dr7 fields */
> +/* max breakpoints*/
> +#define MAX_BP      4
> +/* Break on instruction execution only */
> +#define BP_INST     0x0
> +/* Break on data writes only */
> +#define BP_DATA_WR  0x1
> +/* Break on I/O reads or writes */

... or undefined for 386, 486 or if CR4 DE flag is clear.

> +#define BP_IO_RW    0x10
> +/* Break on data reads or writes but not instruction fetches */
> +#define BP_DATA_RW  0x11

These should all go next to the existing definitions in this
file for some DR7 fields, and they should follow the existing
naming conventions, ie DR7_something. I suggest naming them
DR7_TYPE_INSN, DR7_TYPE_DATA_WR, etc. Could also use a comment
that the DR7_TYPE_ constants are the values for the TYPE field
of DR7, and are what is returned by hw_breakpoint_type().

-- PMM