[U-Boot] [PATCH] 8313erdb: Set guarded bit on BAT that covers the end of the address space.
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com --- Note that there are likely other board with the same issue.
include/configs/MPC8313ERDB.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h index 0ef4eba..21aedee 100644 --- a/include/configs/MPC8313ERDB.h +++ b/include/configs/MPC8313ERDB.h @@ -544,7 +544,7 @@ #define CONFIG_SYS_IBAT5U (CONFIG_SYS_IMMR | BATU_BL_256M | BATU_VS | BATU_VP)
/* SDRAM @ 0xF0000000, stack in DCACHE 0xFDF00000 & FLASH @ 0xFE000000 */ -#define CONFIG_SYS_IBAT6L (0xF0000000 | BATL_PP_10) +#define CONFIG_SYS_IBAT6L (0xF0000000 | BATL_PP_10 | BATL_GUARDEDSTORAGE) #define CONFIG_SYS_IBAT6U (0xF0000000 | BATU_BL_256M | BATU_VS | BATU_VP)
#define CONFIG_SYS_IBAT7L (0)
On Tue, Mar 17, 2009 at 12:09:31PM -0500, Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com
Note that there are likely other board with the same issue.
Wow, I was actually chasing this (I think) bug for some time.
The effect of this bug was quite weird: some kernels didn't boot, and the only difference in the kernel image was.. the build date (i.e. data in linux_banner and init_uts_ns symbols).
I suspected the decompression code (what else could it be?), but I didn't manage to track it down to a failing instruction, as the failing kernel was booting *OK* with BDI-2000 attached. Heh.
I wonder how you tracked it down to zlib code and a particular loop, please share the technique. ;-)
Thanks!
Anton Vorontsov wrote:
On Tue, Mar 17, 2009 at 12:09:31PM -0500, Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com
Note that there are likely other board with the same issue.
Wow, I was actually chasing this (I think) bug for some time.
The effect of this bug was quite weird: some kernels didn't boot, and the only difference in the kernel image was.. the build date (i.e. data in linux_banner and init_uts_ns symbols).
I suspected the decompression code (what else could it be?), but I didn't manage to track it down to a failing instruction, as the failing kernel was booting *OK* with BDI-2000 attached. Heh.
I wonder how you tracked it down to zlib code and a particular loop, please share the technique. ;-)
I changed the kernel's decompression address to 0x1000 so that the exception vectors don't get overwritten, and looked at the machine check dump. That pointed to the relevant part of the zlib code, at which point I used print statements to figure out what was going on, combined with arbiter registers after reboot which pointed out 0xfffffff8 as the offending address.
-Scott
On Tue, Mar 17, 2009 at 02:49:17PM -0500, Scott Wood wrote:
Anton Vorontsov wrote:
On Tue, Mar 17, 2009 at 12:09:31PM -0500, Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com
Note that there are likely other board with the same issue.
Wow, I was actually chasing this (I think) bug for some time.
The effect of this bug was quite weird: some kernels didn't boot, and the only difference in the kernel image was.. the build date (i.e. data in linux_banner and init_uts_ns symbols).
I suspected the decompression code (what else could it be?), but I didn't manage to track it down to a failing instruction, as the failing kernel was booting *OK* with BDI-2000 attached. Heh.
I wonder how you tracked it down to zlib code and a particular loop, please share the technique. ;-)
I changed the kernel's decompression address to 0x1000 so that the exception vectors don't get overwritten,
Ah. That's the key part, and no need for jtag/cop debuggers at all, which, it appears, pampered me. Neat.
Thanks Scott.
On Tuesday 17 March 2009 13:09:31 Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
isnt that a compiler generating bad code then ? if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively. -mike
On Tue, Mar 17, 2009 at 01:47:04PM -0400, Mike Frysinger wrote:
On Tuesday 17 March 2009 13:09:31 Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
isnt that a compiler generating bad code then ?
No. The dereference was on a not-taken side of a conditional branch.
if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively.
There is no reasonable way for the compiler to prevent such speculative accesses. Non-memory-like mappings must have the guarded bit set. That is what the bit is there for.
-Scott
On Tuesday 17 March 2009 13:52:27 Scott Wood wrote:
On Tue, Mar 17, 2009 at 01:47:04PM -0400, Mike Frysinger wrote:
On Tuesday 17 March 2009 13:09:31 Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
isnt that a compiler generating bad code then ?
No. The dereference was on a not-taken side of a conditional branch.
you mean in the shadow ? so something like: p = NULL; if (p != NULL) { /* this is the shadow region */ }
if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively.
There is no reasonable way for the compiler to prevent such speculative accesses. Non-memory-like mappings must have the guarded bit set. That is what the bit is there for.
if the hardware doesnt have a way of preventing it, then the compiler must nop bad accesses that are unknown.
i'm not sure your example proves your position. if you have a region that cannot stand speculative access, how do you handle bad pointer checking if the compiler may generate code that'll speculatively hit it at any time ? -mike
if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively.
There is no reasonable way for the compiler to prevent such speculative accesses. Non-memory-like mappings must have the guarded bit set. That is what the bit is there for.
if the hardware doesnt have a way of preventing it, then the compiler must nop bad accesses that are unknown.
i'm not sure your example proves your position. if you have a region that cannot stand speculative access, how do you handle bad pointer checking if the compiler may generate code that'll speculatively hit it at any time ?
This has nothing to do w/the compiler but how HW speculation actually works.
If you don't understand that say so and ask about it.. Dont try and claim Scott's example and patch aren't valid because they are.
- k
On Tuesday 17 March 2009 14:13:39 Kumar Gala wrote:
if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively.
There is no reasonable way for the compiler to prevent such speculative accesses. Non-memory-like mappings must have the guarded bit set. That is what the bit is there for.
if the hardware doesnt have a way of preventing it, then the compiler must nop bad accesses that are unknown.
i'm not sure your example proves your position. if you have a region that cannot stand speculative access, how do you handle bad pointer checking if the compiler may generate code that'll speculatively hit it at any time ?
This has nothing to do w/the compiler but how HW speculation actually works.
If you don't understand that say so and ask about it.. Dont try and claim Scott's example and patch aren't valid because they are.
i never said Scott's proposed change was wrong -mike
Mike Frysinger wrote:
No. The dereference was on a not-taken side of a conditional branch.
you mean in the shadow ? so something like: p = NULL; if (p != NULL) { /* this is the shadow region */ }
Yes.
if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively.
There is no reasonable way for the compiler to prevent such speculative accesses. Non-memory-like mappings must have the guarded bit set. That is what the bit is there for.
if the hardware doesnt have a way of preventing it, then the compiler must nop bad accesses that are unknown.
The hardware *does* have a way of preventing it. It's the guarded bit. :-)
I don't know of any amount of "nop" instructions that would be architecturally guaranteed to avoid this -- they're no-ops, not syncs (despite how some other architectures use them). They can be discarded as fast as the chip can decode them.
Adding an isync instruction should avoid it, but that's a heavy performance penalty. Why pay it for all accesses rather than just those which are not memory-like (and can be flagged as such in the TLB)?
i'm not sure your example proves your position. if you have a region that cannot stand speculative access, how do you handle bad pointer checking if the compiler may generate code that'll speculatively hit it at any time ?
The compiler is not speculatively doing anything. It is the CPU -- and the guarded bit tells it to not do that.
-Scott
On Tuesday 17 March 2009 14:18:13 Scott Wood wrote:
Mike Frysinger wrote:
if C code is doing ptr checks, the compiler should make sure that pointer is not dereferenced at all if the hardware cannot suffer the consequences, even speculatively.
There is no reasonable way for the compiler to prevent such speculative accesses. Non-memory-like mappings must have the guarded bit set. That is what the bit is there for.
if the hardware doesnt have a way of preventing it, then the compiler must nop bad accesses that are unknown.
The hardware *does* have a way of preventing it. It's the guarded bit. :-)
I don't know of any amount of "nop" instructions that would be architecturally guaranteed to avoid this -- they're no-ops, not syncs (despite how some other architectures use them). They can be discarded as fast as the chip can decode them.
right, it depends on the pipeline. some let the nops force the address decode stages to get filled so they dont get speculatively filled and then fetched. sounds like the ppc pipeline doesnt operate that way.
Adding an isync instruction should avoid it, but that's a heavy performance penalty. Why pay it for all accesses rather than just those which are not memory-like (and can be flagged as such in the TLB)?
right, if the mappings allow speculative fetches to be turned off for certain regions, that's the way to go
i'm not sure your example proves your position. if you have a region that cannot stand speculative access, how do you handle bad pointer checking if the compiler may generate code that'll speculatively hit it at any time ?
The compiler is not speculatively doing anything. It is the CPU -- and the guarded bit tells it to not do that.
i didnt mean the compiler was doing it. i meant the compiler generates dense code that the hardware turns around and speculatively loads.
the change sounded like you were addressing a specific issue that was noticed, but other regions could still (in general) cause problems. but i guess that isnt the case at all.
thanks for the info -mike
Mike Frysinger wrote:
the change sounded like you were addressing a specific issue that was noticed, but other regions could still (in general) cause problems. but i guess that isnt the case at all.
I mentioned the specific case mainly to show that it was a problem that I was actually running into (rather than a theoretical problem), and thus is a candidate for the imminent release.
For "next", a more comprehensive review of mappings is in order. For example, the flash mapping shouldn't be cacheable -- it's fine for normal data access, but it could cause problems when programming the flash.
-Scott
Mike Frysinger wrote:
On Tuesday 17 March 2009 14:18:13 Scott Wood wrote:
Mike Frysinger wrote:
[snip]
I don't know of any amount of "nop" instructions that would be architecturally guaranteed to avoid this -- they're no-ops, not syncs (despite how some other architectures use them). They can be discarded as fast as the chip can decode them.
right, it depends on the pipeline. some let the nops force the address decode stages to get filled so they dont get speculatively filled and then fetched. sounds like the ppc pipeline doesnt operate that way.
For the record, using NOPs to fill the pipeline is a poor solution because it is very implementation specific: you have to assume a certain implementation (how NOPs are processed and the depth of the pipeline) for this to work.
If you switched PowerPC processors (been known to happen) or, if Freescale ever made a Super Deluxe 100% Backward Compatible Version of your selected CPU with a deeper pipeline (never have that I know about, but we can dream...), the NOP method would mysteriously break again. Not only that, but it would mysteriously break 5 years or more from now when we've all forgotten what the fix was. The result would be that we would have to spend lots of energy resurrecting old boards, old compilers, and old code only to discover we were done in by assumptions.
As Scott so eloquently pointed out, /that's/ why there is a "guarded" bit. (OK ok, he actually pointed out "That's /why/ there is a guarded bit." ;-)
[snip]
thanks for the info -mike
Best regards, gvb
Aha! It seems like the root cause of this problem: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/53025/match=nasty+gu...
As a MPC8313E(-RDB) user I'm happy no more checkstops will occur for some kernels. I'm even more happy I took over the BAT settings from MPC8315ERDB.h for our custom MPC8313 board (MPC8315ERDB.h doesn't have this problem as far as i can tell)
Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com
Note that there are likely other board with the same issue.
include/configs/MPC8313ERDB.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h index 0ef4eba..21aedee 100644 --- a/include/configs/MPC8313ERDB.h +++ b/include/configs/MPC8313ERDB.h @@ -544,7 +544,7 @@ #define CONFIG_SYS_IBAT5U (CONFIG_SYS_IMMR | BATU_BL_256M | BATU_VS | BATU_VP)
/* SDRAM @ 0xF0000000, stack in DCACHE 0xFDF00000 & FLASH @ 0xFE000000 */ -#define CONFIG_SYS_IBAT6L (0xF0000000 | BATL_PP_10) +#define CONFIG_SYS_IBAT6L (0xF0000000 | BATL_PP_10 | BATL_GUARDEDSTORAGE) #define CONFIG_SYS_IBAT6U (0xF0000000 | BATU_BL_256M | BATU_VS | BATU_VP)
#define CONFIG_SYS_IBAT7L (0)
It seems like the root cause of this problem: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/53025/ match=nasty+gunzip+problem+mpc8313e+rdb
As a MPC8313E(-RDB) user I'm happy no more checkstops will occur for some kernels. I'm even more happy I took over the BAT settings from MPC8315ERDB.h for our custom MPC8313 board (MPC8315ERDB.h doesn't have this problem as far as i can tell)
[snip]
Scott Wood wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by
a device.
In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU
speculatively loading
from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com
Note that there are likely other board with the same issue.
include/configs/MPC8313ERDB.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h index 0ef4eba..21aedee 100644 --- a/include/configs/MPC8313ERDB.h +++ b/include/configs/MPC8313ERDB.h @@ -544,7 +544,7 @@ #define CONFIG_SYS_IBAT5U (CONFIG_SYS_IMMR | BATU_BL_256M
| BATU_VS | BATU_VP)
/* SDRAM @ 0xF0000000, stack in DCACHE 0xFDF00000 & FLASH
@ 0xFE000000 */
-#define CONFIG_SYS_IBAT6L (0xF0000000 | BATL_PP_10) +#define CONFIG_SYS_IBAT6L (0xF0000000 | BATL_PP_10 |
BATL_GUARDEDSTORAGE)
#define CONFIG_SYS_IBAT6U (0xF0000000 | BATU_BL_256M |
BATU_VS | BATU_VP)
Scott/Kim,
We still have the same issue on MPC8349EMDS, MPC8349ITX and sbc8349, But the 8360EMDS, 8315ERDB, 837xEMDS(RDB) have not this issue. It is strange we leave the Flash as non-guarded attribute at 8349 board and 8313ERDB, When did we leave them to non-guarded?
Thanks, Dave
On Tue, 17 Mar 2009 12:09:31 -0500 Scott Wood scottwood@freescale.com wrote:
This board currently sets DBAT6 to cover all of the final 256MiB of address space; however, not all of this space is covered by a device. In particular, flash sits at 0xfe000000-0xfe7fffff, and nothing is mapped at the far end of the address space.
In zlib, there is a loop that references p[-1] if p is non-NULL. Under some circumstances, this leads to the CPU speculatively loading from 0xfffffff8 if p is NULL. This leads to a machine check.
Signed-off-by: Scott Wood scottwood@freescale.com
applied, thanks.
Note that there are likely other board with the same issue.
patch forthcoming.
Kim
participants (8)
-
Anton Vorontsov -
Jerry Van Baren -
Kim Phillips -
Kumar Gala -
Liu Dave-R63238 -
Mike Frysinger -
Norbert van Bolhuis -
Scott Wood