| Message ID | 1354504047-11615-1-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 2012-12-03 04:07, liguang wrote: > 1. define names of breakpoints in dr7 > 2. slightly refactor bits field of breakpoint > related functions. Two topics, (at least) two patches, please. The code is hairy - not your fault, you actually try to improve it. But splitting up makes review easier. Thanks in advance. > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > --- > target-i386/cpu.h | 6 ++++ > target-i386/helper.c | 59 ++++++++++++++++++++++++++++---------------- > target-i386/machine.c | 2 +- > target-i386/misc_helper.c | 4 +- > target-i386/seg_helper.c | 4 +- > 5 files changed, 48 insertions(+), 27 deletions(-) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 90ef1ff..2da6ea0 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_L0_3 0x55 DR7_LOCAL_BP_MASK or so. > +#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 > diff --git a/target-i386/helper.c b/target-i386/helper.c > index bf206cf..54d6712 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -966,27 +966,26 @@ 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: > + 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: > - type = BP_CPU | BP_MEM_ACCESS; > - insert_wp: > + case DR7_DATA_RW: > + if (!type) > + type = BP_CPU | BP_MEM_ACCESS; Coding style. But, to be honest, I find the goto approach cleaner. Alternatively, test for "type" (or "wp_type") != 0 outside the switch, moving the cpu_watchpoint_insert there. > err = cpu_watchpoint_insert(env, env->dr[index], > hw_breakpoint_len(env->dr[7], index), > type, &env->cpu_watchpoint[index]); > break; > + case DR7_IO_RW: > + /* No support for I/O watchpoints yet */ > + break; > } > if (err) > env->cpu_breakpoint[index] = NULL; > @@ -997,15 +996,15 @@ 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 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_WR: > + case DR7_DATA_RW: > 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 +1013,38 @@ 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, type = 0; > int hit_enabled = 0; > > 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) > + break; > + type = 1; "type" is not very telling as it is used here. Either continue to assign it to hw_breakpoint_type and clear it on DR7_IO_RW, or rename it to a boolean like "bp_match". > + break; > + case DR7_DATA_WR: > + case DR7_DATA_RW: > + if (!env->cpu_watchpoint[index]) > + break; > + if (!(env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT)) > + break; Let's fold these two into one. In general: coding style. > + type = 1; > + break; > + case DR7_IO_RW: > + break; > + } > + if (type) { > + dr6 |= 1 << index; > + if (hw_breakpoint_enabled(env->dr[7], index)) > hit_enabled = 1; > + type = 0; If you go for "wp_match", just set to false under DR7_IO_RW. > } > } > 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++) If touching the line, please fix the coding style as well. > 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..317187b 100644 > --- a/target-i386/seg_helper.c > +++ b/target-i386/seg_helper.c > @@ -465,8 +465,8 @@ 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] & DR7_L0_3) { > + for (i = 0; i < DR7_MAX_BP; i++) { > if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { There is a magic value remaining here. You probably want defines for the local vs. global enable bits as well. > hw_breakpoint_remove(env, i); > } > Jan
Am 03.12.2012 10:43, schrieb Jan Kiszka: > On 2012-12-03 04:07, liguang wrote: >> 1. define names of breakpoints in dr7 >> 2. slightly refactor bits field of breakpoint >> related functions. > > Two topics, (at least) two patches, please. The code is hairy - not your > fault, you actually try to improve it. But splitting up makes review > easier. Thanks in advance. I was going to request the same thing: Please first introduce the new constants, then do functional changes on top. When you do that, please remember to include a short cover letter and use git-send-email so that the patches stay together. :) The following link may be helpful for avoiding coding style issues: http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html Regards, Andreas
在 2012-12-03一的 10:43 +0100，Jan Kiszka写道： > On 2012-12-03 04:07, liguang wrote: > > 1. define names of breakpoints in dr7 > > 2. slightly refactor bits field of breakpoint > > related functions. > > Two topics, (at least) two patches, please. The code is hairy - not your > fault, you actually try to improve it. But splitting up makes review > easier. Thanks in advance. OK, I'll split this patch. > > > > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > > --- > > target-i386/cpu.h | 6 ++++ > > target-i386/helper.c | 59 ++++++++++++++++++++++++++++---------------- > > target-i386/machine.c | 2 +- > > target-i386/misc_helper.c | 4 +- > > target-i386/seg_helper.c | 4 +- > > 5 files changed, 48 insertions(+), 27 deletions(-) > > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index 90ef1ff..2da6ea0 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_L0_3 0x55 > > DR7_LOCAL_BP_MASK or so. > > > +#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 > > diff --git a/target-i386/helper.c b/target-i386/helper.c > > index bf206cf..54d6712 100644 > > --- a/target-i386/helper.c > > +++ b/target-i386/helper.c > > @@ -966,27 +966,26 @@ 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: > > + 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: > > - type = BP_CPU | BP_MEM_ACCESS; > > - insert_wp: > > + case DR7_DATA_RW: > > + if (!type) > > + type = BP_CPU | BP_MEM_ACCESS; > > Coding style. > > But, to be honest, I find the goto approach cleaner. Alternatively, test > for "type" (or "wp_type") != 0 outside the switch, moving the > cpu_watchpoint_insert there. okay. > > > err = cpu_watchpoint_insert(env, env->dr[index], > > hw_breakpoint_len(env->dr[7], index), > > type, &env->cpu_watchpoint[index]); > > break; > > + case DR7_IO_RW: > > + /* No support for I/O watchpoints yet */ > > + break; > > } > > if (err) > > env->cpu_breakpoint[index] = NULL; > > @@ -997,15 +996,15 @@ 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 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_WR: > > + case DR7_DATA_RW: > > 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 +1013,38 @@ 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, type = 0; > > int hit_enabled = 0; > > > > 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) > > + break; > > + type = 1; > > "type" is not very telling as it is used here. Either continue to assign > it to hw_breakpoint_type and clear it on DR7_IO_RW, or rename it to a > boolean like "bp_match". bp_match is a nicer name, thanks! > > > + break; > > + case DR7_DATA_WR: > > + case DR7_DATA_RW: > > + if (!env->cpu_watchpoint[index]) > > + break; > > + if (!(env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT)) > > + break; > > Let's fold these two into one. > > In general: coding style. > > > + type = 1; > > + break; > > + case DR7_IO_RW: > > + break; > > + } > > + if (type) { > > + dr6 |= 1 << index; > > + if (hw_breakpoint_enabled(env->dr[7], index)) > > hit_enabled = 1; > > + type = 0; > > If you go for "wp_match", just set to false under DR7_IO_RW. > > > } > > } > > 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++) > > If touching the line, please fix the coding style as well. > > > 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..317187b 100644 > > --- a/target-i386/seg_helper.c > > +++ b/target-i386/seg_helper.c > > @@ -465,8 +465,8 @@ 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] & DR7_L0_3) { > > + for (i = 0; i < DR7_MAX_BP; i++) { > > if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { > > There is a magic value remaining here. You probably want defines for the > local vs. global enable bits as well. okay. Thanks! > > > hw_breakpoint_remove(env, i); > > } > > > > Jan >
在 2012-12-03一的 12:19 +0100，Andreas Färber写道： > Am 03.12.2012 10:43, schrieb Jan Kiszka: > > On 2012-12-03 04:07, liguang wrote: > >> 1. define names of breakpoints in dr7 > >> 2. slightly refactor bits field of breakpoint > >> related functions. > > > > Two topics, (at least) two patches, please. The code is hairy - not your > > fault, you actually try to improve it. But splitting up makes review > > easier. Thanks in advance. > > I was going to request the same thing: Please first introduce the new > constants, then do functional changes on top. > > When you do that, please remember to include a short cover letter and > use git-send-email so that the patches stay together. :) > > The following link may be helpful for avoiding coding style issues: > http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html > > Regards, > Andreas > okay, thanks for your remanding.
diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 90ef1ff..2da6ea0 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_L0_3 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 diff --git a/target-i386/helper.c b/target-i386/helper.c index bf206cf..54d6712 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -966,27 +966,26 @@ 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: + 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: - type = BP_CPU | BP_MEM_ACCESS; - insert_wp: + case DR7_DATA_RW: + if (!type) + type = BP_CPU | BP_MEM_ACCESS; err = cpu_watchpoint_insert(env, env->dr[index], hw_breakpoint_len(env->dr[7], index), type, &env->cpu_watchpoint[index]); break; + case DR7_IO_RW: + /* No support for I/O watchpoints yet */ + break; } if (err) env->cpu_breakpoint[index] = NULL; @@ -997,15 +996,15 @@ 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 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_WR: + case DR7_DATA_RW: 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 +1013,38 @@ 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, type = 0; int hit_enabled = 0; 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) + break; + type = 1; + break; + case DR7_DATA_WR: + case DR7_DATA_RW: + if (!env->cpu_watchpoint[index]) + break; + if (!(env->cpu_watchpoint[index]->flags & BP_WATCHPOINT_HIT)) + break; + type = 1; + break; + case DR7_IO_RW: + break; + } + if (type) { + dr6 |= 1 << index; + if (hw_breakpoint_enabled(env->dr[7], index)) hit_enabled = 1; + type = 0; } } 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..317187b 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -465,8 +465,8 @@ 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] & DR7_L0_3) { + for (i = 0; i < DR7_MAX_BP; i++) { if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { hw_breakpoint_remove(env, i); }
1. define names of breakpoints in dr7 2. slightly refactor bits field of breakpoint related functions. Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- target-i386/cpu.h | 6 ++++ target-i386/helper.c | 59 ++++++++++++++++++++++++++++---------------- target-i386/machine.c | 2 +- target-i386/misc_helper.c | 4 +- target-i386/seg_helper.c | 4 +- 5 files changed, 48 insertions(+), 27 deletions(-)