Message ID | 1354608695-3232-2-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 | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 9abec3e..8ca25c8 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -996,9 +996,9 @@ 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_breakpoint_enabled(unsigned long dr7, int index) > { > - return (dr7 >> (index * 2)) & 3; > + return !!((dr7 >> (index * 2)) & 3); > } > > static inline int hw_breakpoint_type(unsigned long dr7, int index) Doesn't this break the use of this function in target-i386/seg_helper.c: if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { which specifically wants to determine whether the breakpoint is enabled only locally? -- PMM
On 2012-12-04 11:23, Peter Maydell wrote: > 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 | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >> index 9abec3e..8ca25c8 100644 >> --- a/target-i386/cpu.h >> +++ b/target-i386/cpu.h >> @@ -996,9 +996,9 @@ 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_breakpoint_enabled(unsigned long dr7, int index) >> { >> - return (dr7 >> (index * 2)) & 3; >> + return !!((dr7 >> (index * 2)) & 3); >> } >> >> static inline int hw_breakpoint_type(unsigned long dr7, int index) > > Doesn't this break the use of this function in target-i386/seg_helper.c: > > if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { > > which specifically wants to determine whether the breakpoint is > enabled only locally? It does. And that also indicates the function is misnamed. Something like hw_breakpoint_state might be better. Jan
On 4 December 2012 11:11, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-12-04 11:23, Peter Maydell wrote: >> Doesn't this break the use of this function in target-i386/seg_helper.c: >> >> if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { >> >> which specifically wants to determine whether the breakpoint is >> enabled only locally? > > It does. And that also indicates the function is misnamed. Something > like hw_breakpoint_state might be better. And/or we could just refactor the code in seg_helper.c, which is a nasty mix of direct access to dr[7] and using the hw_breakpoint_* functions. -- PMM
在 2012-12-04二的 11:26 +0000,Peter Maydell写道: > On 4 December 2012 11:11, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 2012-12-04 11:23, Peter Maydell wrote: > >> Doesn't this break the use of this function in target-i386/seg_helper.c: > >> > >> if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { > >> > >> which specifically wants to determine whether the breakpoint is > >> enabled only locally? It was changed to 'if (hw_breakpoint_enabled(env->dr[7], i)) {' in patch 3/3 > > > > It does. And that also indicates the function is misnamed. Something > > like hw_breakpoint_state might be better. > misnamed? I think hw_breakpoint_enabled is ask whether breakpoint ^^^^^^^^ is enabled or not, so it's almost suitable. > And/or we could just refactor the code in seg_helper.c, which is > a nasty mix of direct access to dr[7] and using the hw_breakpoint_* > functions. > > -- PMM
On 2012-12-05 01:51, li guang wrote: > 在 2012-12-04二的 11:26 +0000,Peter Maydell写道: >> On 4 December 2012 11:11, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> On 2012-12-04 11:23, Peter Maydell wrote: >>>> Doesn't this break the use of this function in target-i386/seg_helper.c: >>>> >>>> if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { >>>> >>>> which specifically wants to determine whether the breakpoint is >>>> enabled only locally? > > It was changed to 'if (hw_breakpoint_enabled(env->dr[7], i)) {' > in patch 3/3 Which is broken as it neglects the different types of "enabled". > >>> >>> It does. And that also indicates the function is misnamed. Something >>> like hw_breakpoint_state might be better. >> > > misnamed? I think hw_breakpoint_enabled is ask whether breakpoint > ^^^^^^^^ > is enabled or not, so it's almost suitable. There are two types of enabled breakpoints: task-local and global. The current hw_breakpoint_enabled returns both as a bitmask, and that is causing the confusing and regression in your patches. Jan
在 2012-12-05三的 09:53 +0100,Jan Kiszka写道: > On 2012-12-05 01:51, li guang wrote: > > 在 2012-12-04二的 11:26 +0000,Peter Maydell写道: > >> On 4 December 2012 11:11, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >>> On 2012-12-04 11:23, Peter Maydell wrote: > >>>> Doesn't this break the use of this function in target-i386/seg_helper.c: > >>>> > >>>> if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { > >>>> > >>>> which specifically wants to determine whether the breakpoint is > >>>> enabled only locally? > > > > It was changed to 'if (hw_breakpoint_enabled(env->dr[7], i)) {' > > in patch 3/3 > > Which is broken as it neglects the different types of "enabled". > > > > >>> > >>> It does. And that also indicates the function is misnamed. Something > >>> like hw_breakpoint_state might be better. > >> > > > > misnamed? I think hw_breakpoint_enabled is ask whether breakpoint > > ^^^^^^^^ > > is enabled or not, so it's almost suitable. > > There are two types of enabled breakpoints: task-local and global. The > current hw_breakpoint_enabled returns both as a bitmask, and that is > causing the confusing and regression in your patches. > It is no doubt that 'hw_breakpoint_enabled' is only check local breakpoint, you know, so, do we really have to handle all the cases since seems there's nowhere uses global breapoints at present? > Jan >
在 2012-12-06四的 10:08 +0800,li guang写道: > 在 2012-12-05三的 09:53 +0100,Jan Kiszka写道: > > On 2012-12-05 01:51, li guang wrote: > > > 在 2012-12-04二的 11:26 +0000,Peter Maydell写道: > > >> On 4 December 2012 11:11, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > >>> On 2012-12-04 11:23, Peter Maydell wrote: > > >>>> Doesn't this break the use of this function in target-i386/seg_helper.c: > > >>>> > > >>>> if (hw_breakpoint_enabled(env->dr[7], i) == 0x1) { > > >>>> > > >>>> which specifically wants to determine whether the breakpoint is > > >>>> enabled only locally? > > > > > > It was changed to 'if (hw_breakpoint_enabled(env->dr[7], i)) {' > > > in patch 3/3 > > > > Which is broken as it neglects the different types of "enabled". > > > > > > > >>> > > >>> It does. And that also indicates the function is misnamed. Something > > >>> like hw_breakpoint_state might be better. > > >> > > > > > > misnamed? I think hw_breakpoint_enabled is ask whether breakpoint > > > ^^^^^^^^ > > > is enabled or not, so it's almost suitable. > > > > There are two types of enabled breakpoints: task-local and global. The > > current hw_breakpoint_enabled returns both as a bitmask, and that is > > causing the confusing and regression in your patches. > > > > It is no doubt that 'hw_breakpoint_enabled' is only check local > breakpoint, you know, so, do we really have to handle all the cases > since seems there's nowhere uses global breapoints at present? Oh, you're right, I'll fix it. Thanks! > > > Jan > > >
diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 9abec3e..8ca25c8 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -996,9 +996,9 @@ 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_breakpoint_enabled(unsigned long dr7, int index) { - return (dr7 >> (index * 2)) & 3; + return !!((dr7 >> (index * 2)) & 3); } static inline int hw_breakpoint_type(unsigned long dr7, int index)
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- target-i386/cpu.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)