| Message ID | 1354608695-3232-1-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 4 December 2012 08:11, liguang <lig.fnst@cn.fujitsu.com> wrote: > 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 I still think these last four should be DR7_TYPE_BP_INST etc to indicate that they're values for the TYPE field, not direct specifications of bits in DR7. -- PMM
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 >
在 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 > > >
在 2012-12-04二的 12:49 +0000，Peter Maydell写道： > On 4 December 2012 08:11, liguang <lig.fnst@cn.fujitsu.com> wrote: > > 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 > > I still think these last four should be DR7_TYPE_BP_INST etc to > indicate that they're values for the TYPE field, not direct > specifications of bits in DR7. hmm, is it necessary? you know, the use of these names is after calling 'hw_breakpoint_type' function, so, it's obvious for dr7's type field. > > -- PMM
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
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
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- target-i386/cpu.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)