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

[Qemu-devel] [PATCH 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 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);
+}
+
+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

[Qemu-devel] [PATCH 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_breakpoint_enabled(env->dr[7], i)) {
                 hw_breakpoint_remove(env, i);
             }
         }
-- 
1.7.2.5

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

[Peter Maydell]

On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> --- 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_breakpoint_enabled(env->dr[7], i)) {
>                  hw_breakpoint_remove(env, i);
>              }
>          }

This is still wrong.

PS: if you could put the 'v2'/'v3' etc marking in the [PATCH]
tags for the subjects of all the patch emails and not just the
cover letter that would be helpful. git format-patch's
--subject-prefix='PATCH v3' option should do this for you.

-- PMM

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

[li guang]

在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > --- 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_breakpoint_enabled(env->dr[7], i)) {
> >                  hw_breakpoint_remove(env, i);
> >              }
> >          }
> 
> This is still wrong.

do you mean the use of 'hw_breakpoint_enabled'? or others?
maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
if it is I'll re-send a corrected patch.
Thanks!

> PS: if you could put the 'v2'/'v3' etc marking in the [PATCH]
> tags for the subjects of all the patch emails and not just the
> cover letter that would be helpful. git format-patch's
> --subject-prefix='PATCH v3' option should do this for you.
> 
> -- PMM

-- 
regards!
li guang

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

[Peter Maydell]

On 6 December 2012 09:16, li guang <lig.fnst@cn.fujitsu.com> wrote:
> 在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
>> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
>> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>> > --- 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_breakpoint_enabled(env->dr[7], i)) {
>> >                  hw_breakpoint_remove(env, i);
>> >              }
>> >          }
>>
>> This is still wrong.
>
> do you mean the use of 'hw_breakpoint_enabled'? or others?
> maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
> if it is I'll re-send a corrected patch.

I mean that in the comments on the previous version of this
patchseet we explained that this check is specifically checking
for whether the breakpoint is enabled locally, and that your
change to just returning bool broke this. And in this version
of the patch there is still exactly the same problem.

-- PMM

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

[li guang]

在 2012-12-06四的 09:23 +0000，Peter Maydell写道：
> On 6 December 2012 09:16, li guang <lig.fnst@cn.fujitsu.com> wrote:
> > 在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
> >> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
> >> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >> > --- 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_breakpoint_enabled(env->dr[7], i)) {
> >> >                  hw_breakpoint_remove(env, i);
> >> >              }
> >> >          }
> >>
> >> This is still wrong.
> >
> > do you mean the use of 'hw_breakpoint_enabled'? or others?
> > maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
> > if it is I'll re-send a corrected patch.
> 
> I mean that in the comments on the previous version of this
> patchseet we explained that this check is specifically checking
> for whether the breakpoint is enabled locally, and that your
> change to just returning bool broke this. And in this version
> of the patch there is still exactly the same problem.

why broke?
this function just ask if breakpoint 'i' was enable,
so we answer enabled or not? 2 simple cases, any problem?

> 
> -- PMM

-- 
regards!
li guang

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

[陳韋任 (Wei-Ren Chen)]

On Thu, Dec 06, 2012 at 05:27:44PM +0800, li guang wrote:
> 在 2012-12-06四的 09:23 +0000，Peter Maydell写道：
> > On 6 December 2012 09:16, li guang <lig.fnst@cn.fujitsu.com> wrote:
> > > 在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
> > >> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
> > >> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > >> > --- 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_breakpoint_enabled(env->dr[7], i)) {
> > >> >                  hw_breakpoint_remove(env, i);
> > >> >              }
> > >> >          }
> > >>
> > >> This is still wrong.
> > >
> > > do you mean the use of 'hw_breakpoint_enabled'? or others?
> > > maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
> > > if it is I'll re-send a corrected patch.
> > 
> > I mean that in the comments on the previous version of this
> > patchseet we explained that this check is specifically checking
> > for whether the breakpoint is enabled locally, and that your
> > change to just returning bool broke this. And in this version
> > of the patch there is still exactly the same problem.
> 
> why broke?
> this function just ask if breakpoint 'i' was enable,
> so we answer enabled or not? 2 simple cases, any problem?

  I don't read this patch from the starting. But Peter, do you mean
the return value matters here? I see the original version compares
the return value with 0x1, do you mean we *need* this comparsion here?

Regards,
chenwj

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj

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

[Andreas Färber]

Am 06.12.2012 10:27, schrieb li guang:
> 在 2012-12-06四的 09:23 +0000，Peter Maydell写道：
>> On 6 December 2012 09:16, li guang <lig.fnst@cn.fujitsu.com> wrote:
>>> 在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
>>>> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
>>>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>>>> --- 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_breakpoint_enabled(env->dr[7], i)) {
>>>>>                  hw_breakpoint_remove(env, i);
>>>>>              }
>>>>>          }
>>>>
>>>> This is still wrong.
>>>
>>> do you mean the use of 'hw_breakpoint_enabled'? or others?
>>> maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
>>> if it is I'll re-send a corrected patch.
>>
>> I mean that in the comments on the previous version of this
>> patchseet we explained that this check is specifically checking
>> for whether the breakpoint is enabled locally, and that your
>> change to just returning bool broke this. And in this version
>> of the patch there is still exactly the same problem.
> 
> why broke?
> this function just ask if breakpoint 'i' was enable,
> so we answer enabled or not? 2 simple cases, any problem?

The code comment specifically says "reset local breakpoints". IIUC you
are also resetting global breakpoints, which you shouldn't.

Personally I'd be fine with a hw_local_breakpoint_enabled().

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Peter Maydell]

On 6 December 2012 09:36, Andreas Färber <afaerber@suse.de> wrote:
> Am 06.12.2012 10:27, schrieb li guang:
>> 在 2012-12-06四的 09:23 +0000，Peter Maydell写道：
>>> On 6 December 2012 09:16, li guang <lig.fnst@cn.fujitsu.com> wrote:
>>>> 在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
>>>>> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
>>>>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>>>>> --- 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_breakpoint_enabled(env->dr[7], i)) {
>>>>>>                  hw_breakpoint_remove(env, i);
>>>>>>              }
>>>>>>          }
>>>>>
>>>>> This is still wrong.
>>>>
>>>> do you mean the use of 'hw_breakpoint_enabled'? or others?
>>>> maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
>>>> if it is I'll re-send a corrected patch.
>>>
>>> I mean that in the comments on the previous version of this
>>> patchseet we explained that this check is specifically checking
>>> for whether the breakpoint is enabled locally, and that your
>>> change to just returning bool broke this. And in this version
>>> of the patch there is still exactly the same problem.
>>
>> why broke?
>> this function just ask if breakpoint 'i' was enable,
>> so we answer enabled or not? 2 simple cases, any problem?
>
> The code comment specifically says "reset local breakpoints". IIUC you
> are also resetting global breakpoints, which you shouldn't.
>
> Personally I'd be fine with a hw_local_breakpoint_enabled().

The check you want is
 (hw_local_breakpoint_enabled() && !hw_global_breakpoint_enabled())

if you're going to do it like that. [We don't want to take out the
bp if it was enabled globally as well as locally.]

-- PMM

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

[Andreas Färber]

Am 06.12.2012 10:48, schrieb Peter Maydell:
> On 6 December 2012 09:36, Andreas Färber <afaerber@suse.de> wrote:
>> The code comment specifically says "reset local breakpoints". IIUC you
>> are also resetting global breakpoints, which you shouldn't.
>>
>> Personally I'd be fine with a hw_local_breakpoint_enabled().
> 
> The check you want is
>  (hw_local_breakpoint_enabled() && !hw_global_breakpoint_enabled())
> 
> if you're going to do it like that. [We don't want to take out the
> bp if it was enabled globally as well as locally.]

If it doesn't exist already (I'm assuming not, otherwise it could've
been used) we could simply define the "local" function to return false
if global is enabled. :) But maybe that's too clever.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[li guang]

在 2012-12-06四的 09:48 +0000，Peter Maydell写道：
> On 6 December 2012 09:36, Andreas Färber <afaerber@suse.de> wrote:
> > Am 06.12.2012 10:27, schrieb li guang:
> >> 在 2012-12-06四的 09:23 +0000，Peter Maydell写道：
> >>> On 6 December 2012 09:16, li guang <lig.fnst@cn.fujitsu.com> wrote:
> >>>> 在 2012-12-06四的 08:54 +0000，Peter Maydell写道：
> >>>>> On 6 December 2012 03:03, liguang <lig.fnst@cn.fujitsu.com> wrote:
> >>>>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >>>>>> --- 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_breakpoint_enabled(env->dr[7], i)) {
> >>>>>>                  hw_breakpoint_remove(env, i);
> >>>>>>              }
> >>>>>>          }
> >>>>>
> >>>>> This is still wrong.
> >>>>
> >>>> do you mean the use of 'hw_breakpoint_enabled'? or others?
> >>>> maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
> >>>> if it is I'll re-send a corrected patch.
> >>>
> >>> I mean that in the comments on the previous version of this
> >>> patchseet we explained that this check is specifically checking
> >>> for whether the breakpoint is enabled locally, and that your
> >>> change to just returning bool broke this. And in this version
> >>> of the patch there is still exactly the same problem.
> >>
> >> why broke?
> >> this function just ask if breakpoint 'i' was enable,
> >> so we answer enabled or not? 2 simple cases, any problem?
> >
> > The code comment specifically says "reset local breakpoints". IIUC you
> > are also resetting global breakpoints, which you shouldn't.
> >
> > Personally I'd be fine with a hw_local_breakpoint_enabled().
> 
> The check you want is
>  (hw_local_breakpoint_enabled() && !hw_global_breakpoint_enabled())
> 

Yes, it's the right choice.
Thanks!

> if you're going to do it like that. [We don't want to take out the
> bp if it was enabled globally as well as locally.]
> 
> -- PMM
> 

-- 
regards!
li guang

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

[Peter Maydell]

On 6 December 2012 09:27, li guang <lig.fnst@cn.fujitsu.com> wrote:
> 在 2012-12-06四的 09:23 +0000，Peter Maydell写道：
>> I mean that in the comments on the previous version of this
>> patchseet we explained that this check is specifically checking
>> for whether the breakpoint is enabled locally, and that your
>> change to just returning bool broke this. And in this version
>> of the patch there is still exactly the same problem.
>
> why broke?
> this function just ask if breakpoint 'i' was enable,
> so we answer enabled or not? 2 simple cases, any problem?

See what we said last time around:
 http://lists.nongnu.org/archive/html/qemu-devel/2012-12/msg00456.html

Most of the callers to hw_breakpoint_enabled() are only
asking "is this enabled at all?". This callsite is asking
"is this breakpoint enabled as a local breakpoint and only
as a local breakpoint?".

-- PMM

[Qemu-devel] [PATCH 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..9abec3e 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_BP_INST     0x0
+#define DR7_DATA_WR     0x1
+#define DR7_IO_RW       0x2
+#define DR7_DATA_RW     0x3
+
 
 #define PG_PRESENT_BIT	0
 #define PG_RW_BIT	1
-- 
1.7.2.5

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

[liguang]

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

diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf206cf..28307a1 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -966,30 +966,31 @@ 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_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_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_DATA_RW:
         type = BP_CPU | BP_MEM_ACCESS;
-    insert_wp:
+	case DR7_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 +998,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_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_DATA_RW:
+    case DR7_DATA_WR:
         cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
         break;
-    case 2:
+    case DR7_IO_RW:
         /* No support for I/O watchpoints yet */
         break;
     }
@@ -1014,22 +1016,40 @@ 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_BP_INST:
+            if (env->dr[index] == env->eip) {
+                bp_match = true;
+            }
+            break;
+        case DR7_DATA_WR:
+        case DR7_DATA_RW:
+            if (env->cpu_watchpoint[index] &&
+                env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT) {
+                wp_match = true;
+            }
+        case DR7_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)
         env->dr[6] = dr6;
+
     return hit_enabled;
 }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 4771508..a4b1a1e 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -265,7 +265,7 @@ 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);
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_breakpoint_enabled(env->dr[7], i)) {
                 hw_breakpoint_remove(env, i);
             }
         }
-- 
1.7.2.5

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

[Blue Swirl]

On Tue, Dec 4, 2012 at 8:11 AM, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/helper.c      |   70 +++++++++++++++++++++++++++++----------------
>  target-i386/machine.c     |    2 +-
>  target-i386/misc_helper.c |    4 +-
>  target-i386/seg_helper.c  |    6 ++--
>  4 files changed, 51 insertions(+), 31 deletions(-)
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf206cf..28307a1 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -966,30 +966,31 @@ 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_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_DATA_WR:
>          type = BP_CPU | BP_MEM_WRITE;
> -        goto insert_wp;
> -    case 2:
> -         /* No support for I/O watchpoints yet */
> -        break;
> -    case 3:

Missing 'break'.

> +    case DR7_DATA_RW:
>          type = BP_CPU | BP_MEM_ACCESS;
> -    insert_wp:
> +       case DR7_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 +998,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_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_DATA_RW:
> +    case DR7_DATA_WR:
>          cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
>          break;
> -    case 2:
> +    case DR7_IO_RW:
>          /* No support for I/O watchpoints yet */
>          break;
>      }
> @@ -1014,22 +1016,40 @@ 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_BP_INST:
> +            if (env->dr[index] == env->eip) {
> +                bp_match = true;
> +            }
> +            break;
> +        case DR7_DATA_WR:
> +        case DR7_DATA_RW:
> +            if (env->cpu_watchpoint[index] &&
> +                env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT) {
> +                wp_match = true;
> +            }

Also here.

> +        case DR7_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)
>          env->dr[6] = dr6;
> +
>      return hit_enabled;
>  }
>
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 4771508..a4b1a1e 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -265,7 +265,7 @@ 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++)

Please add braces and check your patches with checkpatch.pl.

>          hw_breakpoint_insert(env, i);
>
>      tlb_flush(env, 1);
> 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_breakpoint_enabled(env->dr[7], i)) {
>                  hw_breakpoint_remove(env, i);
>              }
>          }
> --
> 1.7.2.5
>

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

[li guang]

在 2012-12-04二的 18:51 +0000，Blue Swirl写道：
> On Tue, Dec 4, 2012 at 8:11 AM, liguang <lig.fnst@cn.fujitsu.com> wrote:
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  target-i386/helper.c      |   70 +++++++++++++++++++++++++++++----------------
> >  target-i386/machine.c     |    2 +-
> >  target-i386/misc_helper.c |    4 +-
> >  target-i386/seg_helper.c  |    6 ++--
> >  4 files changed, 51 insertions(+), 31 deletions(-)
> >
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index bf206cf..28307a1 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -966,30 +966,31 @@ 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_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_DATA_WR:
> >          type = BP_CPU | BP_MEM_WRITE;
> > -        goto insert_wp;
> > -    case 2:
> > -         /* No support for I/O watchpoints yet */
> > -        break;
> > -    case 3:
> 
> Missing 'break'.

yes, will fix, thanks!

> 
> > +    case DR7_DATA_RW:
> >          type = BP_CPU | BP_MEM_ACCESS;
> > -    insert_wp:
> > +       case DR7_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 +998,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_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_DATA_RW:
> > +    case DR7_DATA_WR:
> >          cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
> >          break;
> > -    case 2:
> > +    case DR7_IO_RW:
> >          /* No support for I/O watchpoints yet */
> >          break;
> >      }
> > @@ -1014,22 +1016,40 @@ 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_BP_INST:
> > +            if (env->dr[index] == env->eip) {
> > +                bp_match = true;
> > +            }
> > +            break;
> > +        case DR7_DATA_WR:
> > +        case DR7_DATA_RW:
> > +            if (env->cpu_watchpoint[index] &&
> > +                env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT) {
> > +                wp_match = true;
> > +            }
> 
> Also here.
> 

No, just fall through.

> > +        case DR7_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)
> >          env->dr[6] = dr6;
> > +
> >      return hit_enabled;
> >  }
> >
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index 4771508..a4b1a1e 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -265,7 +265,7 @@ 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++)
> 
> Please add braces and check your patches with checkpatch.pl.

hmm, OK.

> 
> >          hw_breakpoint_insert(env, i);
> >
> >      tlb_flush(env, 1);
> > 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_breakpoint_enabled(env->dr[7], i)) {
> >                  hw_breakpoint_remove(env, i);
> >              }
> >          }
> > --
> > 1.7.2.5
> >
> 

-- 
regards!
li guang

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

[Jan Kiszka]

On 2012-12-05 01:56, li guang wrote:
>>> @@ -1014,22 +1016,40 @@ 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_BP_INST:
>>> +            if (env->dr[index] == env->eip) {
>>> +                bp_match = true;
>>> +            }
>>> +            break;
>>> +        case DR7_DATA_WR:
>>> +        case DR7_DATA_RW:
>>> +            if (env->cpu_watchpoint[index] &&
>>> +                env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT) {
>>> +                wp_match = true;
>>> +            }
>>
>> Also here.
>>
> 
> No, just fall through.

I told you how to clearly mark such cases.

Jan

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