qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/3] target-i386:slightly refactor dr7 related f


From: li guang
Subject: Re: [Qemu-devel] [PATCH 3/3] target-i386:slightly refactor dr7 related function
Date: Fri, 07 Dec 2012 08:53:05 +0800

在 2012-12-06四的 09:48 +0000,Peter Maydell写道:
> On 6 December 2012 09:36, Andreas Färber <address@hidden> wrote:
> > Am 06.12.2012 10:27, schrieb li guang:
> >> 在 2012-12-06四的 09:23 +0000,Peter Maydell写道:
> >>> On 6 December 2012 09:16, li guang <address@hidden> wrote:
> >>>> 在 2012-12-06四的 08:54 +0000,Peter Maydell写道:
> >>>>> On 6 December 2012 03:03, liguang <address@hidden> wrote:
> >>>>>> Signed-off-by: liguang <address@hidden>
> >>>>>> --- 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);
> >>>>>>              }
> >>>>>>          }
> >>>>>
> >>>>> This is still wrong.
> >>>>
> >>>> do you mean the use of 'hw_breakpoint_enabled'? or others?
> >>>> maybe a mistake, I change it to 'hw_local_breakpoint_enabled'.
> >>>> if it is I'll re-send a corrected patch.
> >>>
> >>> I mean that in the comments on the previous version of this
> >>> patchseet we explained that this check is specifically checking
> >>> for whether the breakpoint is enabled locally, and that your
> >>> change to just returning bool broke this. And in this version
> >>> of the patch there is still exactly the same problem.
> >>
> >> why broke?
> >> this function just ask if breakpoint 'i' was enable,
> >> so we answer enabled or not? 2 simple cases, any problem?
> >
> > The code comment specifically says "reset local breakpoints". IIUC you
> > are also resetting global breakpoints, which you shouldn't.
> >
> > Personally I'd be fine with a hw_local_breakpoint_enabled().
> 
> The check you want is
>  (hw_local_breakpoint_enabled() && !hw_global_breakpoint_enabled())
> 

Yes, it's the right choice.
Thanks!

> if you're going to do it like that. [We don't want to take out the
> bp if it was enabled globally as well as locally.]
> 
> -- PMM
> 

-- 
regards!
li guang




reply via email to

[Prev in Thread] Current Thread [Next in Thread]