The DR registers are rarely useful when decoding oopses.
With screen real estate during oopses at a premium, we can save two lines
by only printing out these registers when they are set to something other
than they power-on state.
Signed-off-by: Dave Jones <[email protected]>
diff -durpN '--exclude-from=/home/davej/.exclude' /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c linux-dj/arch/x86/kernel/process_64.c
--- /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c 2013-05-01 10:02:52.064151923 -0400
+++ linux-dj/arch/x86/kernel/process_64.c 2013-05-06 20:35:09.219868881 -0400
@@ -105,11 +105,18 @@ void __show_regs(struct pt_regs *regs, i
get_debugreg(d0, 0);
get_debugreg(d1, 1);
get_debugreg(d2, 2);
- printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
get_debugreg(d3, 3);
get_debugreg(d6, 6);
get_debugreg(d7, 7);
+
+ /* Only print out debug registers if they are in their non-default state. */
+ if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
+ (d6 & ~DR6_RESERVED) && (d7 == 0x400))
+ return;
+
+ printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
printk(KERN_DEFAULT "DR3: %016lx DR6: %016lx DR7: %016lx\n", d3, d6, d7);
+
}
void release_thread(struct task_struct *dead_task)
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 7305f7d..5905dc5 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -110,11 +110,16 @@ void __show_regs(struct pt_regs *regs, int all)
get_debugreg(d1, 1);
get_debugreg(d2, 2);
get_debugreg(d3, 3);
- printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
- d0, d1, d2, d3);
-
get_debugreg(d6, 6);
get_debugreg(d7, 7);
+
+ /* Only print out debug registers if they are in their non-default state. */
+ if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
+ (d6 & ~DR6_RESERVED) && (d7 == 0x400))
+ return;
+
+ printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
+ d0, d1, d2, d3);
printk(KERN_DEFAULT "DR6: %08lx DR7: %08lx\n",
d6, d7);
}
On Tue, Jun 18, 2013 at 12:11:32AM -0400, Dave Jones wrote:
> The DR registers are rarely useful when decoding oopses.
> With screen real estate during oopses at a premium, we can save two lines
> by only printing out these registers when they are set to something other
> than they power-on state.
Makes sense, except...
>
> Signed-off-by: Dave Jones <[email protected]>
>
> diff -durpN '--exclude-from=/home/davej/.exclude' /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c linux-dj/arch/x86/kernel/process_64.c
> --- /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c 2013-05-01 10:02:52.064151923 -0400
> +++ linux-dj/arch/x86/kernel/process_64.c 2013-05-06 20:35:09.219868881 -0400
> @@ -105,11 +105,18 @@ void __show_regs(struct pt_regs *regs, i
> get_debugreg(d0, 0);
> get_debugreg(d1, 1);
> get_debugreg(d2, 2);
> - printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
> get_debugreg(d3, 3);
> get_debugreg(d6, 6);
> get_debugreg(d7, 7);
> +
> + /* Only print out debug registers if they are in their non-default state. */
> + if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
> + (d6 & ~DR6_RESERVED) && (d7 == 0x400))
... I'm not sure about %dr6. So we're not dumping %dr6 when, a.o.
any of the bits in the bit slices [3:0], [15:13] are set (bit 12
is Read-As-Zero). Now those mean stuff like Breakpoint-Condition
Detected or Debug-Register-Access Detected and so on and I think this is
meaningful information.
So actually, we wouldn't want to dump %dr6 when its contents are its
default contents, i.e, 0xffff0ff0, i.e. the test should be:
(d6 == DR6_RESERVED)
no?
> + return;
> +
> + printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
> printk(KERN_DEFAULT "DR3: %016lx DR6: %016lx DR7: %016lx\n", d3, d6, d7);
> +
> }
>
> void release_thread(struct task_struct *dead_task)
>
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 7305f7d..5905dc5 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -110,11 +110,16 @@ void __show_regs(struct pt_regs *regs, int all)
> get_debugreg(d1, 1);
> get_debugreg(d2, 2);
> get_debugreg(d3, 3);
> - printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
> - d0, d1, d2, d3);
> -
> get_debugreg(d6, 6);
> get_debugreg(d7, 7);
> +
> + /* Only print out debug registers if they are in their non-default state. */
> + if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
> + (d6 & ~DR6_RESERVED) && (d7 == 0x400))
ditto.
> + return;
> +
> + printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
> + d0, d1, d2, d3);
> printk(KERN_DEFAULT "DR6: %08lx DR7: %08lx\n",
> d6, d7);
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Tue, Jun 18, 2013 at 10:43:56AM +0200, Borislav Petkov wrote:
> On Tue, Jun 18, 2013 at 12:11:32AM -0400, Dave Jones wrote:
> > The DR registers are rarely useful when decoding oopses.
> > With screen real estate during oopses at a premium, we can save two lines
> > by only printing out these registers when they are set to something other
> > than they power-on state.
>
> Makes sense, except...
>
> >
> > Signed-off-by: Dave Jones <[email protected]>
> >
> > diff -durpN '--exclude-from=/home/davej/.exclude' /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c linux-dj/arch/x86/kernel/process_64.c
> > --- /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c 2013-05-01 10:02:52.064151923 -0400
> > +++ linux-dj/arch/x86/kernel/process_64.c 2013-05-06 20:35:09.219868881 -0400
> > @@ -105,11 +105,18 @@ void __show_regs(struct pt_regs *regs, i
> > get_debugreg(d0, 0);
> > get_debugreg(d1, 1);
> > get_debugreg(d2, 2);
> > - printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
> > get_debugreg(d3, 3);
> > get_debugreg(d6, 6);
> > get_debugreg(d7, 7);
> > +
> > + /* Only print out debug registers if they are in their non-default state. */
> > + if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
> > + (d6 & ~DR6_RESERVED) && (d7 == 0x400))
>
> ... I'm not sure about %dr6. So we're not dumping %dr6 when, a.o.
> any of the bits in the bit slices [3:0], [15:13] are set (bit 12
> is Read-As-Zero). Now those mean stuff like Breakpoint-Condition
> Detected or Debug-Register-Access Detected and so on and I think this is
> meaningful information.
>
> So actually, we wouldn't want to dump %dr6 when its contents are its
> default contents, i.e, 0xffff0ff0, i.e. the test should be:
>
> (d6 == DR6_RESERVED)
>
> no?
My intent here was to ignore cases where the reserved bits haven't been set.
I occasionally see DR6: 00000000fffe0ff0 for eg.
But maybe you're right, and that is a clue and is worth printing ?
I can't personally recall ever diagnosing a bug using those register dumps
in the last 15 years.
Dave
On Tue, Jun 18, 2013 at 10:07:30AM -0400, Dave Jones wrote:
> My intent here was to ignore cases where the reserved bits haven't
> been set. I occasionally see DR6: 00000000fffe0ff0 for eg.
That's bit 16 which, according to the docs is read-as-1:
"All remaining bits in the DR6 register are reserved. Reserved bits
31:16 and 11:4 must all be set to 1, while reserved bit 12 must be
cleared to 0. In 64-bit mode, the upper 32 bits of DR6 are reserved and
must be written with zeros. Writing a 1 to any of the upper 32 bits
results in a general-protection exception, #GP(0)."
This above if from AMD APM and Intel's SDM has a graphic showing the
exact same thing:
[31:16] = set to 1; [12] = 0b, [11:4] = 1b
So if you see bit 16 cleared, then some BIOS or even hardware is doing
funky things. I wouldn't wonder at all if BIOS dudes used reserved bits
in registers as scratch space.
> But maybe you're right, and that is a clue and is worth printing ? I
> can't personally recall ever diagnosing a bug using those register
> dumps in the last 15 years.
Right, I don't know whether it would always help but if you have an
oops and see, say bit 0 in DR6 set, i.e. a debug exception was caused
by address breakpoint condition in DR0, then that could be useful info,
methinks.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
The DR registers are rarely useful when decoding oopses.
With screen real estate during oopses at a premium, we can save two lines
by only printing out these registers when they are set to something other
than they power-on state.
[v2: incorporate D6 change suggested by Borislav Petkov <[email protected]>]
Signed-off-by: Dave Jones <[email protected]>
diff -durpN '--exclude-from=/home/davej/.exclude' /home/davej/src/kernel/git-trees/linux/arch/x86/kernel/process_64.c linux-dj/arch/x86/kernel/process_64.c
--- linux/arch/x86/kernel/process_64.c 2013-05-01 10:02:52.064151923 -0400
+++ linux-dj/arch/x86/kernel/process_64.c 2013-05-06 20:35:09.219868881 -0400
@@ -105,11 +105,18 @@ void __show_regs(struct pt_regs *regs, i
get_debugreg(d0, 0);
get_debugreg(d1, 1);
get_debugreg(d2, 2);
- printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
get_debugreg(d3, 3);
get_debugreg(d6, 6);
get_debugreg(d7, 7);
+
+ /* Only print out debug registers if they are in their non-default state. */
+ if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
+ (d6 == DR6_RESERVED) && (d7 == 0x400))
+ return;
+
+ printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
printk(KERN_DEFAULT "DR3: %016lx DR6: %016lx DR7: %016lx\n", d3, d6, d7);
+
}
void release_thread(struct task_struct *dead_task)
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 7305f7d..5905dc5 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -110,11 +110,16 @@ void __show_regs(struct pt_regs *regs, int all)
get_debugreg(d1, 1);
get_debugreg(d2, 2);
get_debugreg(d3, 3);
- printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
- d0, d1, d2, d3);
-
get_debugreg(d6, 6);
get_debugreg(d7, 7);
+
+ /* Only print out debug registers if they are in their non-default state. */
+ if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
+ (d6 == DR6_RESERVED) && (d7 == 0x400))
+ return;
+
+ printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
+ d0, d1, d2, d3);
printk(KERN_DEFAULT "DR6: %08lx DR7: %08lx\n",
d6, d7);
}
On Tue, Jun 18, 2013 at 12:09:11PM -0400, Dave Jones wrote:
> The DR registers are rarely useful when decoding oopses.
> With screen real estate during oopses at a premium, we can save two lines
> by only printing out these registers when they are set to something other
> than they power-on state.
>
> [v2: incorporate D6 change suggested by Borislav Petkov <[email protected]>]
>
> Signed-off-by: Dave Jones <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
Commit-ID: 4338774cd41a6abf72aa76585ce2184cea8ff8a2
Gitweb: http://git.kernel.org/tip/4338774cd41a6abf72aa76585ce2184cea8ff8a2
Author: Dave Jones <[email protected]>
AuthorDate: Tue, 18 Jun 2013 12:09:11 -0400
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 19 Jun 2013 14:33:59 +0200
x86/debug: Only print out DR registers if they are not power-on defaults
The DR registers are rarely useful when decoding oopses.
With screen real estate during oopses at a premium, we can save
two lines by only printing out these registers when they are set
to something other than they power-on state.
Signed-off-by: Dave Jones <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/process_32.c | 11 ++++++++---
arch/x86/kernel/process_64.c | 9 ++++++++-
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 7305f7d..f84cfd1 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -110,11 +110,16 @@ void __show_regs(struct pt_regs *regs, int all)
get_debugreg(d1, 1);
get_debugreg(d2, 2);
get_debugreg(d3, 3);
- printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
- d0, d1, d2, d3);
-
get_debugreg(d6, 6);
get_debugreg(d7, 7);
+
+ /* Only print out debug registers if they are in their non-default state. */
+ if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
+ (d6 == DR6_RESERVED) && (d7 == 0x400))
+ return;
+
+ printk(KERN_DEFAULT "DR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
+ d0, d1, d2, d3);
printk(KERN_DEFAULT "DR6: %08lx DR7: %08lx\n",
d6, d7);
}
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 355ae06..a8b9abc 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -105,11 +105,18 @@ void __show_regs(struct pt_regs *regs, int all)
get_debugreg(d0, 0);
get_debugreg(d1, 1);
get_debugreg(d2, 2);
- printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
get_debugreg(d3, 3);
get_debugreg(d6, 6);
get_debugreg(d7, 7);
+
+ /* Only print out debug registers if they are in their non-default state. */
+ if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
+ (d6 == DR6_RESERVED) && (d7 == 0x400))
+ return;
+
+ printk(KERN_DEFAULT "DR0: %016lx DR1: %016lx DR2: %016lx\n", d0, d1, d2);
printk(KERN_DEFAULT "DR3: %016lx DR6: %016lx DR7: %016lx\n", d3, d6, d7);
+
}
void release_thread(struct task_struct *dead_task)
On 06/18/2013 08:59 AM, Borislav Petkov wrote:
> On Tue, Jun 18, 2013 at 10:07:30AM -0400, Dave Jones wrote:
>> My intent here was to ignore cases where the reserved bits haven't
>> been set. I occasionally see DR6: 00000000fffe0ff0 for eg.
>
> That's bit 16 which, according to the docs is read-as-1:
>
> "All remaining bits in the DR6 register are reserved. Reserved bits
> 31:16 and 11:4 must all be set to 1, while reserved bit 12 must be
> cleared to 0. In 64-bit mode, the upper 32 bits of DR6 are reserved and
> must be written with zeros. Writing a 1 to any of the upper 32 bits
> results in a general-protection exception, #GP(0)."
>
> This above if from AMD APM and Intel's SDM has a graphic showing the
> exact same thing:
>
> [31:16] = set to 1; [12] = 0b, [11:4] = 1b
>
> So if you see bit 16 cleared, then some BIOS or even hardware is doing
> funky things. I wouldn't wonder at all if BIOS dudes used reserved bits
> in registers as scratch space.
>
>> But maybe you're right, and that is a clue and is worth printing ? I
>> can't personally recall ever diagnosing a bug using those register
>> dumps in the last 15 years.
>
> Right, I don't know whether it would always help but if you have an
> oops and see, say bit 0 in DR6 set, i.e. a debug exception was caused
> by address breakpoint condition in DR0, then that could be useful info,
> methinks.
>
There is serious confusion with regards to DR6 about the bits which are
*fixed* (forced to 1) and the ones which are *reserved* (should always
have a fixed value.)
There are some bits in DR6 which are used by hardware probes.
-hpa
On Wed, Jun 19, 2013 at 02:42:49PM -0700, H. Peter Anvin wrote:
> There is serious confusion with regards to DR6 about the bits which
> are *fixed* (forced to 1) and the ones which are *reserved* (should
> always have a fixed value.)
>
> There are some bits in DR6 which are used by hardware probes.
Haha, I knew it! HW guys don't care about the architectural documents.
Maybe DR6 should probably be dropped altogether from the check before
exiting __show_regs early?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 06/19/2013 02:52 PM, Borislav Petkov wrote:
> On Wed, Jun 19, 2013 at 02:42:49PM -0700, H. Peter Anvin wrote:
>> There is serious confusion with regards to DR6 about the bits which
>> are *fixed* (forced to 1) and the ones which are *reserved* (should
>> always have a fixed value.)
>>
>> There are some bits in DR6 which are used by hardware probes.
>
> Haha, I knew it! HW guys don't care about the architectural documents.
>
> Maybe DR6 should probably be dropped altogether from the check before
> exiting __show_regs early?
>
Uhm... I think you're missing the point of architectural documents.
The architectural documents describe the contract between hardware and
software. It doesn't mean that hardware can't go beyond it... it is in
fact the norm. The architectural documentation states what software may
rely on and within what bounds it can assume safe behavior to be
compatible both now and with any future extensions.
-hpa
On Wed, Jun 19, 2013 at 03:02:19PM -0700, H. Peter Anvin wrote:
> On 06/19/2013 02:52 PM, Borislav Petkov wrote:
> > On Wed, Jun 19, 2013 at 02:42:49PM -0700, H. Peter Anvin wrote:
> >> There is serious confusion with regards to DR6 about the bits which
> >> are *fixed* (forced to 1) and the ones which are *reserved* (should
> >> always have a fixed value.)
> >>
> >> There are some bits in DR6 which are used by hardware probes.
> >
> > Haha, I knew it! HW guys don't care about the architectural documents.
> >
> > Maybe DR6 should probably be dropped altogether from the check before
> > exiting __show_regs early?
>
> Uhm... I think you're missing the point of architectural documents.
>
> The architectural documents describe the contract between hardware and
> software. It doesn't mean that hardware can't go beyond it... it is in
> fact the norm. The architectural documentation states what software may
> rely on and within what bounds it can assume safe behavior to be
> compatible both now and with any future extensions.
Well, that I know :).
>From your comment above it sounded like we can't really rely on some of
the bits in DR6_RESERVED to be really always 1 as they cannot only be
*fixed* (forced to 1) but also be reserved with a fixed value which is
not necessarily 1.
And there's a section in the SDM "19.23.1 Differences in Debug Register
DR6" which says that some families allow writes to 1b of bit 12 while
others don't.
Now Dave stated earlier "I occasionally see DR6: 00000000fffe0ff0 for
eg." which says that bit 16 can be 0b but the AMD spec says "read as 1s"
and Intel's spec says "Reserved (set to 1)" which I only assume is the
same thing: the default state of those is 1.
So maybe the DR6 check should be reversed to actually look at only the
defined bits and ignore "volatile" reserved bits, whichever they might
be :-):
if (!(dr6 & 0xe00f))
I.e., bit slices [15:13] and [3:0] should be 0.
Then we can assume no breakpoint condition has been detected nor the
debug registers are being used in some way.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--