* [Qemu-devel] [PATCH] target-i386: clarify logic of breakpoint functions
@ 2013-01-15 2:13 liguang
2013-01-15 3:26 ` Andreas Färber
0 siblings, 1 reply; 3+ messages in thread
From: liguang @ 2013-01-15 2:13 UTC (permalink / raw)
To: afaerber, imammedo, qemu-devel; +Cc: liguang
the breakpoint related functions are a little
hard to read for their implicit dr7 bit filed
and not well organized logic, so try to define
more readable bit filed for dr7 and clarify
the breakpoint logic.
it's just the squashed one for previous slightly
refactor dr7 related functions patches.
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
target-i386/cpu.h | 19 ++++++++++-
target-i386/helper.c | 76 ++++++++++++++++++++++++++++++---------------
target-i386/machine.c | 5 ++-
target-i386/misc_helper.c | 4 +-
target-i386/seg_helper.c | 9 +++--
5 files changed, 79 insertions(+), 34 deletions(-)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1283537..c0c6a9c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -231,6 +231,12 @@
#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
@@ -993,9 +999,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 bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
+{
+ return (dr7 >> (index * 2)) & 1;
+}
+
+static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
+{
+ return (dr7 >> (index * 2)) & 2;
+}
+
static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
{
- return (dr7 >> (index * 2)) & 3;
+ return hw_global_breakpoint_enabled(dr7, index) ||
+ hw_local_breakpoint_enabled(dr7, index);
}
static inline int hw_breakpoint_type(unsigned long dr7, int index)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index dca1360..8d29eb5 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -966,30 +966,34 @@ 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:
+ 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 +1001,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 +1019,43 @@ 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 8354572..8df6a6b 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 db3126b..9b0f7b3 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 c2a99ee..3247dee 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -465,13 +465,14 @@ 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_global_breakpoint_enabled(env->dr[7], i)) {
hw_breakpoint_remove(env, i);
}
}
- env->dr[7] &= ~0x55;
+ env->dr[7] &= ~DR7_LOCAL_BP_MASK;
}
#endif
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: clarify logic of breakpoint functions
2013-01-15 2:13 [Qemu-devel] [PATCH] target-i386: clarify logic of breakpoint functions liguang
@ 2013-01-15 3:26 ` Andreas Färber
2013-01-15 5:08 ` li guang
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Färber @ 2013-01-15 3:26 UTC (permalink / raw)
To: liguang; +Cc: imammedo, qemu-devel
Am 15.01.2013 03:13, schrieb liguang:
> the breakpoint related functions are a little
> hard to read for their implicit dr7 bit filed
> and not well organized logic, so try to define
> more readable bit filed for dr7 and clarify
> the breakpoint logic.
> it's just the squashed one for previous slightly
> refactor dr7 related functions patches.
>
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
> target-i386/cpu.h | 19 ++++++++++-
> target-i386/helper.c | 76 ++++++++++++++++++++++++++++++---------------
> target-i386/machine.c | 5 ++-
> target-i386/misc_helper.c | 4 +-
> target-i386/seg_helper.c | 9 +++--
> 5 files changed, 79 insertions(+), 34 deletions(-)
NACK. This is most definitely not what I requested, I can easily squash
three patches into one myself... :(
What I suggested was to squash specific non-functional changes into the
first one so that I can cherry-pick it and make some progress while
waiting for review feedback; instead you again mix non-functional and
functional changes as originally done, which gains us nothing.
Still missing a proper change log, this being a v5. In particular a
history of how the names were changed and which changes were suggested
by whom (while cc'ing all those people) would spare me the time of
hunting down all four previous versions in the archive on the eve of the
Soft Freeze.
Indentation does look fine now.
Regards,
Andreas
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 1283537..c0c6a9c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -231,6 +231,12 @@
> #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
> @@ -993,9 +999,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 bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
> +{
> + return (dr7 >> (index * 2)) & 1;
> +}
> +
> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
> +{
> + return (dr7 >> (index * 2)) & 2;
> +}
> +
> static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
> {
> - return (dr7 >> (index * 2)) & 3;
> + return hw_global_breakpoint_enabled(dr7, index) ||
> + hw_local_breakpoint_enabled(dr7, index);
> }
>
> static inline int hw_breakpoint_type(unsigned long dr7, int index)
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index dca1360..8d29eb5 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -966,30 +966,34 @@ 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:
> + 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 +1001,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 +1019,43 @@ 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 8354572..8df6a6b 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 db3126b..9b0f7b3 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 c2a99ee..3247dee 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -465,13 +465,14 @@ 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_global_breakpoint_enabled(env->dr[7], i)) {
> hw_breakpoint_remove(env, i);
> }
> }
> - env->dr[7] &= ~0x55;
> + env->dr[7] &= ~DR7_LOCAL_BP_MASK;
> }
> #endif
> }
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: clarify logic of breakpoint functions
2013-01-15 3:26 ` Andreas Färber
@ 2013-01-15 5:08 ` li guang
0 siblings, 0 replies; 3+ messages in thread
From: li guang @ 2013-01-15 5:08 UTC (permalink / raw)
To: Andreas Färber; +Cc: imammedo, qemu-devel
在 2013-01-15二的 04:26 +0100，Andreas Färber写道：
> Am 15.01.2013 03:13, schrieb liguang:
> > the breakpoint related functions are a little
> > hard to read for their implicit dr7 bit filed
> > and not well organized logic, so try to define
> > more readable bit filed for dr7 and clarify
> > the breakpoint logic.
> > it's just the squashed one for previous slightly
> > refactor dr7 related functions patches.
> >
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> > target-i386/cpu.h | 19 ++++++++++-
> > target-i386/helper.c | 76 ++++++++++++++++++++++++++++++---------------
> > target-i386/machine.c | 5 ++-
> > target-i386/misc_helper.c | 4 +-
> > target-i386/seg_helper.c | 9 +++--
> > 5 files changed, 79 insertions(+), 34 deletions(-)
>
> NACK. This is most definitely not what I requested, I can easily squash
> three patches into one myself... :(
> What I suggested was to squash specific non-functional changes into the
> first one so that I can cherry-pick it and make some progress while
> waiting for review feedback; instead you again mix non-functional and
> functional changes as originally done, which gains us nothing.
>
> Still missing a proper change log, this being a v5. In particular a
> history of how the names were changed and which changes were suggested
> by whom (while cc'ing all those people) would spare me the time of
> hunting down all four previous versions in the archive on the eve of the
> Soft Freeze.
>
> Indentation does look fine now.
>
> Regards,
> Andreas
>
OK, I will split functional and non-functional changes.
> >
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 1283537..c0c6a9c 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -231,6 +231,12 @@
> > #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
> > @@ -993,9 +999,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 bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
> > +{
> > + return (dr7 >> (index * 2)) & 1;
> > +}
> > +
> > +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
> > +{
> > + return (dr7 >> (index * 2)) & 2;
> > +}
> > +
> > static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
> > {
> > - return (dr7 >> (index * 2)) & 3;
> > + return hw_global_breakpoint_enabled(dr7, index) ||
> > + hw_local_breakpoint_enabled(dr7, index);
> > }
> >
> > static inline int hw_breakpoint_type(unsigned long dr7, int index)
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index dca1360..8d29eb5 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -966,30 +966,34 @@ 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:
> > + 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 +1001,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 +1019,43 @@ 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 8354572..8df6a6b 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 db3126b..9b0f7b3 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 c2a99ee..3247dee 100644
> > --- a/target-i386/seg_helper.c
> > +++ b/target-i386/seg_helper.c
> > @@ -465,13 +465,14 @@ 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_global_breakpoint_enabled(env->dr[7], i)) {
> > hw_breakpoint_remove(env, i);
> > }
> > }
> > - env->dr[7] &= ~0x55;
> > + env->dr[7] &= ~DR7_LOCAL_BP_MASK;
> > }
> > #endif
> > }
> >
>
>
--
regards!
li guang
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-01-15 5:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-15 2:13 [Qemu-devel] [PATCH] target-i386: clarify logic of breakpoint functions liguang
2013-01-15 3:26 ` Andreas Färber
2013-01-15 5:08 ` li guang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).