Bug #41
closed[Geode][LX] Zero-address accesses results in UD2
0%
Description
The following source files deliberately access addresses near zero.
In src/northbridge/amd/lx/northbridgeinit.c:
/* Now that the descriptor to memory is set up. */
/* The memory controller needs one read to synch its lines before it can be used. */
i = *(volatile int *)0;
In src/northbridge/amd/lx/raminit.c (sdram_enable):
/* The RAM dll needs a write to lock on so generate a few dummy writes */
/* Note: The descriptor needs to be enabled to point at memory */
volatile unsigned long *ptr;
for (i = 0; i < 5; i++) {
ptr = (void *)i;
*ptr = (unsigned long)i;
}
Recent versions of gcc, including the coreboot-provided crossgcc, do not generate the intended code. Look at the disassembly of sdram_enable, for instance:
00000000 <sdram_enable>:
0: 55 push %ebp
1: 57 push %edi
(...snip...)
14b: 6a 7e push $0x7e
14d: e8 fc ff ff ff call 14e <sdram_enable+0x14e>
14e: R_386_PC32 post_code
152: c7 05 00 00 00 00 00 movl $0x0,0x0
159: 00 00 00
15c: 0f 0b ud2
All the code after the first zero-address access is replaced by ud2. Needless to say, the machine crashes on boot. With the patch below coreboot boots properly:
diff --git a/src/northbridge/amd/lx/northbridgeinit.c b/src/northbridge/amd/lx/northbridgeinit.c
index a07a1ea..37bfde6 100644
--- a/src/northbridge/amd/lx/northbridgeinit.c
+++ b/src/northbridge/amd/lx/northbridgeinit.c
@@ -738,7 +738,7 @@ void northbridge_init_early(void)
/* Now that the descriptor to memory is set up. */
/* The memory controller needs one read to synch its lines before it can be used. */
- i = *(volatile int *)0;
+ __asm__ __volatile__ ("cmp (%0), %0" : : "r" (0) : "memory", "cc");
GeodeLinkPriority();
diff --git a/src/northbridge/amd/lx/raminit.c b/src/northbridge/amd/lx/raminit.c
index f20aed2..363b0ff 100644
--- a/src/northbridge/amd/lx/raminit.c
+++ b/src/northbridge/amd/lx/raminit.c
@@ -747,11 +747,17 @@ void sdram_enable(int controllers, const struct mem_controller *ctrl)
/* The RAM dll needs a write to lock on so generate a few dummy writes */
/* Note: The descriptor needs to be enabled to point at memory */
+#if 0
volatile unsigned long *ptr;
for (i = 0; i < 5; i++) {
ptr = (void *)i;
*ptr = (unsigned long)i;
}
+#else
+ for (i = 0; i < 5; i++) {
+ __asm__ __volatile__ ("mov %0, (%0)" : : "r" (4 * i) : "memory");
+ }
+#endif
/* SWAPSiF for PBZ 4112 (Errata 34) */
/* check for failed DLL settings now that we have done a memory write. */
msrnum = GLCP_DELAY_CONTROLS;
Updated by Martin Roth over 8 years ago
It looks like we missed these when we introduced zeroptr. Look at commits 23cc9b09 and ff8076d7.
It doesn't really look like we need to read from address 0 for either of these though. I'd try just changing them to read starting at address 1 instead of using asm to force the read at 0.
Updated by Patrick Georgi over 8 years ago
https://review.coreboot.org/14345 plus ancestors should fix this.
Updated by Martin Roth over 8 years ago
Achille, We believe this should be fixed at this point. Could you verify that?
Thanks.
Updated by Achille Fouilleul over 8 years ago
I checked out commit 3383a25f91fab26ce20f40f9981eeee5b2ba9eb0 a few days ago.
I had a chance to test it today and it worked fine.
Btw I think I have spotted another place where a zero address is accessed:
src/device/oprom/realmode/x86.c: setup_realmode_idt
For some reason gcc 5.3 does not generate an ud2 but some future version might.