| Message ID | 1354159971-6720-2-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 29 November 2012 03:32, liguang <lig.fnst@cn.fujitsu.com> wrote: > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > --- > target-i386/cpu.h | 2 ++ > target-i386/helper.c | 24 +++++++++++------------- > target-i386/misc_helper.c | 6 +++--- > target-i386/seg_helper.c | 6 +++--- > 4 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 7f292e6..7ecfe21 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -561,6 +561,8 @@ > /* dr7 fields */ > /* max breakpoints*/ > #define MAX_BP 4 > +/*enable local breakpoint bit 0,2,4,6*/ > +#define BP_LOCAL 0x55 This needs a better name, to make it clear that it's not just a single enable bit but actually a mask of all the local enable bits. Also needs DR7_ prefix. You've split these changes up between patches inconsistently; either have one patch which adds all the constants and then patches which just use them, or have patches which both add and use the constants, but don't mix the two. I'd recommend that each patch should both add and use a related set of constants, so it's self contained and easy to review. -- PMM
在 2012-11-29四的 11:28 +0000，Peter Maydell写道： > On 29 November 2012 03:32, liguang <lig.fnst@cn.fujitsu.com> wrote: > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > > --- > > target-i386/cpu.h | 2 ++ > > target-i386/helper.c | 24 +++++++++++------------- > > target-i386/misc_helper.c | 6 +++--- > > target-i386/seg_helper.c | 6 +++--- > > 4 files changed, 19 insertions(+), 19 deletions(-) > > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index 7f292e6..7ecfe21 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -561,6 +561,8 @@ > > /* dr7 fields */ > > /* max breakpoints*/ > > #define MAX_BP 4 > > +/*enable local breakpoint bit 0,2,4,6*/ > > +#define BP_LOCAL 0x55 > > This needs a better name, to make it clear that it's not > just a single enable bit but actually a mask of all the > local enable bits. Also needs DR7_ prefix. > > You've split these changes up between patches inconsistently; > either have one patch which adds all the constants and > then patches which just use them, or have patches which > both add and use the constants, but don't mix the two. > > I'd recommend that each patch should both add and use a > related set of constants, so it's self contained and > easy to review. you're right, thanks! > > -- PMM >
diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 7f292e6..7ecfe21 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -561,6 +561,8 @@ /* dr7 fields */ /* max breakpoints*/ #define MAX_BP 4 +/*enable local breakpoint bit 0,2,4,6*/ +#define BP_LOCAL 0x55 /* Break on instruction execution only */ #define BP_INST 0x0 /* Break on data writes only */ diff --git a/target-i386/helper.c b/target-i386/helper.c index 5686130..9ca52a7 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -969,24 +969,23 @@ void hw_breakpoint_insert(CPUX86State *env, int index) int type, err = 0; switch (hw_breakpoint_type(env->dr[7], index)) { - case 0: + case BP_INST: if (hw_breakpoint_enabled(env->dr[7], index)) err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, &env->cpu_breakpoint[index]); break; - case 1: + case BP_DATA_WR: type = BP_CPU | BP_MEM_WRITE; goto insert_wp; - case 2: - /* No support for I/O watchpoints yet */ - break; - case 3: + case BP_DATA_RW: type = BP_CPU | BP_MEM_ACCESS; insert_wp: err = cpu_watchpoint_insert(env, env->dr[index], hw_breakpoint_len(env->dr[7], index), type, &env->cpu_watchpoint[index]); - break; + case BP_IO_RW: + /* No support for I/O watchpoints yet */ + break; } if (err) env->cpu_breakpoint[index] = NULL; @@ -997,15 +996,14 @@ void hw_breakpoint_remove(CPUX86State *env, int index) if (!env->cpu_breakpoint[index]) return; switch (hw_breakpoint_type(env->dr[7], index)) { - case 0: + case BP_INST: if (hw_breakpoint_enabled(env->dr[7], index)) cpu_breakpoint_remove_by_ref(env, env->cpu_breakpoint[index]); break; - case 1: - case 3: + case BP_DATA_WR: + case BP_DATA_RW: cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]); - break; - case 2: + case BP_IO_RW: /* No support for I/O watchpoints yet */ break; } @@ -1018,7 +1016,7 @@ int check_hw_breakpoints(CPUX86State *env, int force_dr6_update) int hit_enabled = 0; dr6 = env->dr[6] & ~0xf; - for (reg = 0; reg < 4; reg++) { + for (reg = 0; reg < MAX_BP; reg++) { type = hw_breakpoint_type(env->dr[7], reg); if ((type == 0 && env->dr[reg] == env->eip) || ((type & 1) && env->cpu_watchpoint[reg] && diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c index a020379..9d1187a 100644 --- a/target-i386/misc_helper.c +++ b/target-i386/misc_helper.c @@ -192,16 +192,16 @@ void helper_movl_drN_T0(CPUX86State *env, int reg, target_ulong t0) { int i; - if (reg < 4) { + if (reg < MAX_BP) { hw_breakpoint_remove(env, reg); env->dr[reg] = t0; hw_breakpoint_insert(env, reg); } else if (reg == 7) { - for (i = 0; i < 4; i++) { + for (i = 0; i < MAX_BP; i++) { hw_breakpoint_remove(env, i); } env->dr[7] = t0; - for (i = 0; i < 4; i++) { + for (i = 0; i < MAX_BP; i++) { hw_breakpoint_insert(env, i); } } else { diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c index ff93374..e99b287 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -465,13 +465,13 @@ static void switch_tss(CPUX86State *env, int tss_selector, #ifndef CONFIG_USER_ONLY /* reset local breakpoints */ - if (env->dr[7] & 0x55) { - for (i = 0; i < 4; i++) { + if (env->dr[7] & BP_LOCAL) { + for (i = 0; i < MAX_BP; i++) { if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { hw_breakpoint_remove(env, i); } } - env->dr[7] &= ~0x55; + env->dr[7] &= ~BP_LOCAL; } #endif }
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- target-i386/cpu.h | 2 ++ target-i386/helper.c | 24 +++++++++++------------- target-i386/misc_helper.c | 6 +++--- target-i386/seg_helper.c | 6 +++--- 4 files changed, 19 insertions(+), 19 deletions(-)