Bug #41

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

Added by Achille Fouilleul almost 3 years ago. Updated almost 3 years ago.

Status:ClosedStart date:04/11/2016
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:board support
Target version:-

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;

History

#1 Updated by Martin Roth almost 3 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.

#2 Updated by Patrick Georgi almost 3 years ago

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

#3 Updated by Martin Roth almost 3 years ago

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

Thanks.

#4 Updated by Achille Fouilleul almost 3 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.

#5 Updated by Martin Roth almost 3 years ago

  • Status changed from New to Closed

closing as fixed.

Also available in: Atom PDF