Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

2010/2/19 K.Prasad <prasad@xxxxxxxxxxxxxxxxxx>:
> On Thu, Feb 18, 2010 at 07:00:01PM +0100, Frederic Weisbecker wrote:
>
>> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
>> index 017d937..0c1033d 100644
>> --- a/arch/x86/kernel/ptrace.c
>> +++ b/arch/x86/kernel/ptrace.c
>> @@ -702,7 +702,7 @@ static unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)
>>       } else if (n == 6) {
>>               val = thread->debugreg6;
>>        } else if (n == 7) {
>> -             val = ptrace_get_dr7(thread->ptrace_bps);
>> +             val = thread->ptrace_dr7;
>
> Some more comments that I missed out in the previous mail...
>
> Shouldn't ptrace_get_dr7() function be entirely removed now, given that
> its only caller no longer invokes it?


Not in this set. This is an urgent fix, we shouldn't take corner risks
so late in the process.

Also, this thread->ptrace_dr7 is a temporary thing, a hack to fix a
regression without
taking too much risk. But we need something more proper in the longer term, like
storing this information in the arch_hw_breakpoint (we shouldn't bloat
the thread
structure with such mostly unused field).

I have some ideas to get the arch_hw_breakpoint more "arch", to
register breakpoints
using arch informations only, so that we don't depend on a generic
breakpoint attribute
conversion for ptrace, which may be too limited for tricky arch
breakpoint implementations,
and an unecessary midlayer there.
I'll tell you more while answering to your ppc breakpoint implementation.



>>       }
>>       return val;
>>  }
>> @@ -778,8 +778,11 @@ int ptrace_set_debugreg(struct task_struct *tsk, int n, unsigned long val)
>>                       return rc;
>>       }
>>       /* All that's left is DR7 */
>> -     if (n == 7)
>> +     if (n == 7) {
>>               rc = ptrace_write_dr7(tsk, val);
>
> And ptrace_write_dr7() should be made to populate thread->ptrace_dr7 if
> it is going to return a success.


Yeah we can do that from ptrace_write_dr7(). Either way.
It's just more quick to do that here. I did not think much
about it as I don't think about ptrace_dr7 as a long term field.

Thanks.


>> +             if (!rc)
>> +                     thread->ptrace_dr7 = val;
>> +     }
>>
>>  ret_path:
>>       return rc;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/