[Qemu-devel] [PATCH v3 1/3] target-i386:define name of breakpoint bit in dr7

[Qemu-devel] [PATCH v3 1/3] target-i386:define name of breakpoint bit in dr7

[liguang]

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

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 90ef1ff..29245d1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -231,6 +231,13 @@
 #define DR7_TYPE_SHIFT  16
 #define DR7_LEN_SHIFT   18
 #define DR7_FIXED_1     0x00000400
+#define DR7_LOCAL_BP_MASK   0x55
+#define DR7_MAX_BP      4
+#define DR7_TYPE_BP_INST     0x0
+#define DR7_TYPE_DATA_WR     0x1
+#define DR7_TYPE_IO_RW       0x2
+#define DR7_TYPE_DATA_RW     0x3
+
 
 #define PG_PRESENT_BIT	0
 #define PG_RW_BIT	1
-- 
1.7.2.5

[Qemu-devel] [PATCH v3 2/3] target-i386:make hw_breakpoint_enabled return bool type

[liguang]

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

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 29245d1..3646128 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
 #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
 void cpu_x86_set_a20(CPUX86State *env, int a20_state);
 
-static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
+static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
 {
-    return (dr7 >> (index * 2)) & 3;
+    return !(((dr7 >> (index * 2)) ^ 1) & 3);
+}
+
+static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
+{
+    return !!((dr7 >> (index * 2)) & 2);
+}
+
+static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
+{
+    return (hw_global_breakpoint_enabled(dr7, index) ||
+            hw_local_breakpoint_enabled(dr7, index));
 }
 
 static inline int hw_breakpoint_type(unsigned long dr7, int index)
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v3 2/3] target-i386:make hw_breakpoint_enabled return bool type

[Peter Maydell]

On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/cpu.h |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 29245d1..3646128 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
>  #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
>  void cpu_x86_set_a20(CPUX86State *env, int a20_state);
>
> -static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
> +static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
>  {
> -    return (dr7 >> (index * 2)) & 3;
> +    return !(((dr7 >> (index * 2)) ^ 1) & 3);

This is pretty confusing and I'm pretty sure the function is
misnamed too. If we're checking "is local breakpoint enabled"
then we only want to check one of the two enable bits, not both.


> +}
> +
> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
> +{
> +    return !!((dr7 >> (index * 2)) & 2);
> +}
> +
> +static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
> +{
> +    return (hw_global_breakpoint_enabled(dr7, index) ||
> +            hw_local_breakpoint_enabled(dr7, index));
>  }

-- PMM

Re: [Qemu-devel] [PATCH v3 2/3] target-i386:make hw_breakpoint_enabled return bool type

[Jan Kiszka]

On 2012-12-07 11:24, Peter Maydell wrote:
> On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>> ---
>>  target-i386/cpu.h |   15 +++++++++++++--
>>  1 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 29245d1..3646128 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
>>  #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
>>  void cpu_x86_set_a20(CPUX86State *env, int a20_state);
>>
>> -static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
>> +static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
>>  {
>> -    return (dr7 >> (index * 2)) & 3;
>> +    return !(((dr7 >> (index * 2)) ^ 1) & 3);
> 
> This is pretty confusing and I'm pretty sure the function is
> misnamed too. If we're checking "is local breakpoint enabled"
> then we only want to check one of the two enable bits, not both.
> 

Yes, and I already asked to define the proper constants that allow
checking for local vs. global enable bit. They have to be used here
instead of all the magic & 3 or ^ 1 stuff.

BTW, there is no need for "converting" ("!!") the result of the (value &
mask) to bool, the compiler will do this already.

Jan

> 
>> +}
>> +
>> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
>> +{
>> +    return !!((dr7 >> (index * 2)) & 2);
>> +}
>> +
>> +static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
>> +{
>> +    return (hw_global_breakpoint_enabled(dr7, index) ||
>> +            hw_local_breakpoint_enabled(dr7, index));
>>  }
> 
> -- PMM
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v3 2/3] target-i386:make hw_breakpoint_enabled return bool type

[li guang]

在 2012-12-07五的 11:29 +0100，Jan Kiszka写道：
> On 2012-12-07 11:24, Peter Maydell wrote:
> > On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
> >> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >> ---
> >>  target-i386/cpu.h |   15 +++++++++++++--
> >>  1 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> >> index 29245d1..3646128 100644
> >> --- a/target-i386/cpu.h
> >> +++ b/target-i386/cpu.h
> >> @@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
> >>  #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
> >>  void cpu_x86_set_a20(CPUX86State *env, int a20_state);
> >>
> >> -static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
> >> +static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
> >>  {
> >> -    return (dr7 >> (index * 2)) & 3;
> >> +    return !(((dr7 >> (index * 2)) ^ 1) & 3);
> > 
> > This is pretty confusing and I'm pretty sure the function is
> > misnamed too. If we're checking "is local breakpoint enabled"
> > then we only want to check one of the two enable bits, not both.
> > 
> 
> Yes, and I already asked to define the proper constants that allow
> checking for local vs. global enable bit. They have to be used here
> instead of all the magic & 3 or ^ 1 stuff.
> 
> BTW, there is no need for "converting" ("!!") the result of the (value &
> mask) to bool, the compiler will do this already.
> 
> Jan

OK, as both of you commented, I'll fix for them.
Thanks!

> 
> > 
> >> +}
> >> +
> >> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
> >> +{
> >> +    return !!((dr7 >> (index * 2)) & 2);
> >> +}
> >> +
> >> +static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
> >> +{
> >> +    return (hw_global_breakpoint_enabled(dr7, index) ||
> >> +            hw_local_breakpoint_enabled(dr7, index));
> >>  }
> > 
> > -- PMM
> > 
> 

-- 
regards!
li guang

[Qemu-devel] [PATCH v3 3/3] target-i386:slightly refactor dr7 related function

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-i386/helper.c      |   74 +++++++++++++++++++++++++++++---------------
 target-i386/machine.c     |    5 ++-
 target-i386/misc_helper.c |    4 +-
 target-i386/seg_helper.c  |    6 ++--
 4 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf206cf..62746c5 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -966,30 +966,33 @@ hwaddr cpu_get_phys_page_debug(CPUX86State *env, target_ulong addr)
 
 void hw_breakpoint_insert(CPUX86State *env, int index)
 {
-    int type, err = 0;
+    int type = 0, err = 0;
 
     switch (hw_breakpoint_type(env->dr[7], index)) {
-    case 0:
-        if (hw_breakpoint_enabled(env->dr[7], index))
+    case DR7_TYPE_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 DR7_TYPE_DATA_WR:
         type = BP_CPU | BP_MEM_WRITE;
-        goto insert_wp;
-    case 2:
-         /* No support for I/O watchpoints yet */
         break;
-    case 3:
+    case DR7_TYPE_DATA_RW:
         type = BP_CPU | BP_MEM_ACCESS;
-    insert_wp:
+        break;
+	case DR7_TYPE_IO_RW:
+        /* No support for I/O watchpoints yet */
+        break;
+    }
+	if (type) {
         err = cpu_watchpoint_insert(env, env->dr[index],
                                     hw_breakpoint_len(env->dr[7], index),
                                     type, &env->cpu_watchpoint[index]);
-        break;
     }
-    if (err)
+    if (err) {
         env->cpu_breakpoint[index] = NULL;
+    }
 }
 
 void hw_breakpoint_remove(CPUX86State *env, int index)
@@ -997,15 +1000,16 @@ void hw_breakpoint_remove(CPUX86State *env, int index)
     if (!env->cpu_breakpoint[index])
         return;
     switch (hw_breakpoint_type(env->dr[7], index)) {
-    case 0:
-        if (hw_breakpoint_enabled(env->dr[7], index))
+    case DR7_TYPE_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 DR7_TYPE_DATA_RW:
+    case DR7_TYPE_DATA_WR:
         cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
         break;
-    case 2:
+    case DR7_TYPE_IO_RW:
         /* No support for I/O watchpoints yet */
         break;
     }
@@ -1014,22 +1018,42 @@ 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;
+    bool bp_match = false;
+    bool wp_match = false;
 
     dr6 = env->dr[6] & ~0xf;
-    for (reg = 0; reg < 4; 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 < DR7_MAX_BP; index++) {
+        switch (hw_breakpoint_type(env->dr[7], index)) {
+        case DR7_TYPE_BP_INST:
+            if (env->dr[index] == env->eip) {
+                bp_match = true;
+            }
+            break;
+        case DR7_TYPE_DATA_WR:
+        case DR7_TYPE_DATA_RW:
+            if (env->cpu_watchpoint[index] &&
+                env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT) {
+                wp_match = true;
+            }
+			break;
+        case DR7_TYPE_IO_RW:
+            break;
+        }
+        if (bp_match || wp_match) {
+            dr6 |= 1 << index;
+            if (hw_breakpoint_enabled(env->dr[7], index)) {
                 hit_enabled = 1;
+            }
+            bp_match = false;
+            wp_match = false;
         }
     }
-    if (hit_enabled || force_dr6_update)
+    if (hit_enabled || force_dr6_update) {
         env->dr[6] = dr6;
+	}
+
     return hit_enabled;
 }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 4771508..67131a4 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -265,10 +265,11 @@ static int cpu_post_load(void *opaque, int version_id)
 
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
-    for (i = 0; i < 4; i++)
+    for (i = 0; i < DR7_MAX_BP; i++) {
         hw_breakpoint_insert(env, i);
-
+	}
     tlb_flush(env, 1);
+
     return 0;
 }
 
diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c
index a020379..5ee0863 100644
--- a/target-i386/misc_helper.c
+++ b/target-i386/misc_helper.c
@@ -197,11 +197,11 @@ void helper_movl_drN_T0(CPUX86State *env, int reg, target_ulong t0)
         env->dr[reg] = t0;
         hw_breakpoint_insert(env, reg);
     } else if (reg == 7) {
-        for (i = 0; i < 4; i++) {
+        for (i = 0; i < DR7_MAX_BP; i++) {
             hw_breakpoint_remove(env, i);
         }
         env->dr[7] = t0;
-        for (i = 0; i < 4; i++) {
+        for (i = 0; i < DR7_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..16d489a 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -465,9 +465,9 @@ 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 (hw_breakpoint_enabled(env->dr[7], i) == 0x1) {
+    if (env->dr[7] & DR7_LOCAL_BP_MASK) {
+        for (i = 0; i < DR7_MAX_BP; i++) {
+            if (hw_local_breakpoint_enabled(env->dr[7], i)) {
                 hw_breakpoint_remove(env, i);
             }
         }
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v3 3/3] target-i386:slightly refactor dr7 related function

[Peter Maydell]

On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index ff93374..16d489a 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -465,9 +465,9 @@ 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 (hw_breakpoint_enabled(env->dr[7], i) == 0x1) {
> +    if (env->dr[7] & DR7_LOCAL_BP_MASK) {
> +        for (i = 0; i < DR7_MAX_BP; i++) {
> +            if (hw_local_breakpoint_enabled(env->dr[7], i)) {
>                  hw_breakpoint_remove(env, i);
>              }
>          }

No, this is still wrong. The check we are making here is
"is local bp enabled and global bp disabled?", not "is
local bp enabled?", so at a minimum your function is
badly misnamed.

-- PMM