Message ID | 20130427144537.GA24256@redhat.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
I don't know why this is uapi... finest make a lot of sense to me. Oleg Nesterov <oleg@redhat.com> wrote: >On 04/26, Oleg Nesterov wrote: > >> On 04/26, H. Peter Anvin wrote: >> > >> > On 04/26/2013 09:38 AM, Oleg Nesterov wrote: >> > > >> > > - do_debug: >> > > >> > > dr6 &= ~DR6_RESERVED; >> > > >> > > this also wrongly clears 32-63 bits. Fortunately these >> > > bits are reserved and must be zero. >> > >> > I don't think this is wrongly at all. >> >> OK, I meant that it also clears the bits that are not specified in >> DR6_RESERVED mask. >> >> > The whole point is to mask out >> > the bits that the handler doesn't want to deal with, so masking out >the >> > reserved bits [63:32] seems reasonable to me. >> >> Then we should do >> >> - #define DR6_RESERVED 0xFFFF0FF0 >> + #define DR6_RESERVED 0xFFFFFFFFFFFF0FF0 >> >> ? >> >> or what? (just in case, I will happily agree with "do nothing" ;) > >Or we can do the s/reserved/mask/ change and avoid any "unexpected" >effect of "long &= ~int". This allso allows to kill ifdef(__i386__). > >But this is include/uapi, I do not know if I can simply remove the >old define's. > >In short: whatever you prefer, including "leave it alone". > >Oleg. > >diff --git a/arch/x86/include/uapi/asm/debugreg.h >b/arch/x86/include/uapi/asm/debugreg.h >index 3c0874d..2678b23 100644 >--- a/arch/x86/include/uapi/asm/debugreg.h >+++ b/arch/x86/include/uapi/asm/debugreg.h >@@ -14,8 +14,7 @@ > which debugging register was responsible for the trap. The other bits > are either reserved or not of interest to us. */ > >-/* Define reserved bits in DR6 which are always set to 1 */ >-#define DR6_RESERVED (0xFFFF0FF0) >+#define DR6_MASK (0xF00FU) /* Everything else is reserved */ > > #define DR_TRAP0 (0x1) /* db0 */ > #define DR_TRAP1 (0x2) /* db1 */ >@@ -32,6 +31,8 @@ > and indicates what types of access we trap on, and how large the data > field is that we are looking at */ > >+#define DR_CONTROL_MASK (0xFFFF03FFU) /* Everything else is reserved >*/ >+ > #define DR_CONTROL_SHIFT 16 /* Skip this many bits in ctl register */ > #define DR_CONTROL_SIZE 4 /* 4 control bits per register */ > >@@ -64,12 +65,6 @@ > We can slow the instruction pipeline for instructions coming via the >gdt or the ldt if we want to. I am not sure why this is an advantage >*/ > >-#ifdef __i386__ >-#define DR_CONTROL_RESERVED (0xFC00) /* Reserved by Intel */ >-#else >-#define DR_CONTROL_RESERVED (0xFFFFFFFF0000FC00UL) /* Reserved */ >-#endif >- > #define DR_LOCAL_SLOWDOWN (0x100) /* Local slow the pipeline */ > #define DR_GLOBAL_SLOWDOWN (0x200) /* Global slow the pipeline */ > >diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c >index 7461f50..bc5fb98 100644 >--- a/arch/x86/kernel/ptrace.c >+++ b/arch/x86/kernel/ptrace.c >@@ -657,7 +657,7 @@ static int ptrace_write_dr7(struct task_struct >*tsk, unsigned long data) > bool second_pass = false; > int i, rc, ret = 0; > >- data &= ~DR_CONTROL_RESERVED; >+ data &= DR_CONTROL_MASK; > old_dr7 = ptrace_get_dr7(thread->ptrace_bps); > > restore: >diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c >index 68bda7a..42a635f 100644 >--- a/arch/x86/kernel/traps.c >+++ b/arch/x86/kernel/traps.c >@@ -402,7 +402,7 @@ dotraplinkage void __kprobes do_debug(struct >pt_regs *regs, long error_code) > get_debugreg(dr6, 6); > > /* Filter out all the reserved bits which are preset to 1 */ >- dr6 &= ~DR6_RESERVED; >+ dr6 &= DR6_MASK; > > /* > * If dr6 has no reason to give us about the origin of this trap,
On Sat, Apr 27, 2013 at 04:45:37PM +0200, Oleg Nesterov wrote: > On 04/26, Oleg Nesterov wrote: > > > On 04/26, H. Peter Anvin wrote: > > > > > > On 04/26/2013 09:38 AM, Oleg Nesterov wrote: > > > > > > > > - do_debug: > > > > > > > > dr6 &= ~DR6_RESERVED; > > > > > > > > this also wrongly clears 32-63 bits. Fortunately these > > > > bits are reserved and must be zero. > > > > > > I don't think this is wrongly at all. > > > > OK, I meant that it also clears the bits that are not specified in > > DR6_RESERVED mask. > > > > > The whole point is to mask out > > > the bits that the handler doesn't want to deal with, so masking out the > > > reserved bits [63:32] seems reasonable to me. > > > > Then we should do > > > > - #define DR6_RESERVED 0xFFFF0FF0 > > + #define DR6_RESERVED 0xFFFFFFFFFFFF0FF0 > > > > ? > > > > or what? (just in case, I will happily agree with "do nothing" ;) > > Or we can do the s/reserved/mask/ change and avoid any "unexpected" > effect of "long &= ~int". This allso allows to kill ifdef(__i386__). > > But this is include/uapi, I do not know if I can simply remove the > old define's. > > In short: whatever you prefer, including "leave it alone". > > Oleg. > > diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h > index 3c0874d..2678b23 100644 > --- a/arch/x86/include/uapi/asm/debugreg.h > +++ b/arch/x86/include/uapi/asm/debugreg.h > @@ -14,8 +14,7 @@ > which debugging register was responsible for the trap. The other bits > are either reserved or not of interest to us. */ > > -/* Define reserved bits in DR6 which are always set to 1 */ > -#define DR6_RESERVED (0xFFFF0FF0) > +#define DR6_MASK (0xF00FU) /* Everything else is reserved */ I'm personally fine either with that or with Peter's suggestion to do: -#define DR6_RESERVED (0xFFFF0FF0) +#define DR6_RESERVED (~0xF00FUL) If this should stay stable UAPI, we probably want Peter's solution. Otherwise I really don't mind. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h index 3c0874d..2678b23 100644 --- a/arch/x86/include/uapi/asm/debugreg.h +++ b/arch/x86/include/uapi/asm/debugreg.h @@ -14,8 +14,7 @@ which debugging register was responsible for the trap. The other bits are either reserved or not of interest to us. */ -/* Define reserved bits in DR6 which are always set to 1 */ -#define DR6_RESERVED (0xFFFF0FF0) +#define DR6_MASK (0xF00FU) /* Everything else is reserved */ #define DR_TRAP0 (0x1) /* db0 */ #define DR_TRAP1 (0x2) /* db1 */ @@ -32,6 +31,8 @@ and indicates what types of access we trap on, and how large the data field is that we are looking at */ +#define DR_CONTROL_MASK (0xFFFF03FFU) /* Everything else is reserved */ + #define DR_CONTROL_SHIFT 16 /* Skip this many bits in ctl register */ #define DR_CONTROL_SIZE 4 /* 4 control bits per register */ @@ -64,12 +65,6 @@ We can slow the instruction pipeline for instructions coming via the gdt or the ldt if we want to. I am not sure why this is an advantage */ -#ifdef __i386__ -#define DR_CONTROL_RESERVED (0xFC00) /* Reserved by Intel */ -#else -#define DR_CONTROL_RESERVED (0xFFFFFFFF0000FC00UL) /* Reserved */ -#endif - #define DR_LOCAL_SLOWDOWN (0x100) /* Local slow the pipeline */ #define DR_GLOBAL_SLOWDOWN (0x200) /* Global slow the pipeline */ diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index 7461f50..bc5fb98 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -657,7 +657,7 @@ static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data) bool second_pass = false; int i, rc, ret = 0; - data &= ~DR_CONTROL_RESERVED; + data &= DR_CONTROL_MASK; old_dr7 = ptrace_get_dr7(thread->ptrace_bps); restore: diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 68bda7a..42a635f 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -402,7 +402,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code) get_debugreg(dr6, 6); /* Filter out all the reserved bits which are preset to 1 */ - dr6 &= ~DR6_RESERVED; + dr6 &= DR6_MASK; /* * If dr6 has no reason to give us about the origin of this trap,