Project

General

Profile

Actions

Bug #41

closed

[Geode][LX] Zero-address accesses results in UD2

Added by Achille Fouilleul almost 10 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
board support
Target version:
-
Start date:
04/11/2016
Due date:
% Done:

0%

Estimated time:
Affected versions:
Needs backport to:
Affected hardware:
Affected OS:

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;
Actions #1

Updated by Martin Roth almost 10 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.

Actions #2

Updated by Patrick Georgi almost 10 years ago

https://review.coreboot.org/14345 plus ancestors should fix this.

Actions #3

Updated by Martin Roth almost 10 years ago

Achille, We believe this should be fixed at this point. Could you verify that?

Thanks.

Actions #4

Updated by Achille Fouilleul over 9 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.

Actions #5

Updated by Martin Roth over 9 years ago

  • Status changed from New to Closed

closing as fixed.

Actions

Also available in: Atom PDF