Patchwork fix clobbers in the K8 CAR

login
register
about
Submitter Rudolf Marek
Date 2010-02-21 22:42:17
Message ID <4B81B6C9.3060201@assembler.cz>
Download mbox | patch
Permalink /patch/946/
State Accepted
Commit r5153
Headers show

Comments

Rudolf Marek - 2010-02-21 22:42:17
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

This patch adds clobber regs into the play as assembly trashes random registers.
I was bitten by this while doing MSR I/O because ECX got corrupted. I attempted
to create nice barrier for GCC while changing the stack location too.

The memcpy code was taken from Linux kernel, dont know to whom attribute the
copyright on it. If it is OK with the note, otherwise:

Signed-off-by: Rudolf Marek <r.marek@assembler.cz>

Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkuBtsgACgkQ3J9wPJqZRNXtXgCeJGvuA7dTdE5A+Tybw6zNIi2x
YbkAni1oBOZ9i6iqartAdKxaI8xJP76T
=ebvY
-----END PGP SIGNATURE-----
ron minnich - 2010-02-22 16:31:50
2010/2/21 Rudolf Marek <r.marek@assembler.cz>:

> This patch adds clobber regs into the play as assembly trashes random registers.
> I was bitten by this while doing MSR I/O because ECX got corrupted. I attempted
> to create nice barrier for GCC while changing the stack location too.

The complex ways in which we have to convince gcc to do __asm__
correctly make me wonder if we shouldn't start
putting assembly into assembly into .s files, not .c files. If we have
this much trouble with __asm__, how much are we really
gaining when we use it? Why not just set up a .s?

ron
Patrick Georgi - 2010-02-22 16:34:09
Am 22.02.2010 17:31, schrieb ron minnich:
> The complex ways in which we have to convince gcc to do __asm__
> correctly make me wonder if we shouldn't start
> putting assembly into assembly into .s files, not .c files. If we have
> this much trouble with __asm__, how much are we really
> gaining when we use it? Why not just set up a .s?
romcc in some cases.
The other issue is that in an .s file, we're bound to function calling
conventions. with __asm__, we can really include code.


Patrick
ron minnich - 2010-02-22 16:39:53
On Mon, Feb 22, 2010 at 8:34 AM, Patrick Georgi <patrick@georgi-clan.de> wrote:

> The other issue is that in an .s file, we're bound to function calling
> conventions. with __asm__, we can really include code.

I understand. But I work with a very nice operating system that has
never had a need for __asm__, so I question the need for it myself.

I'd still like to see the reason that memset is so slow.

Also, I realize we have dropped support for other architectures, but
coreboot is still in principle portable. This change will be the
beginning of the removal of the portability. Before we start to make
such a change we need to make a conscious decision that losing
portability is what we want.

Part of "portability", in this case, is eventually moving to native
64-bit mode for the x86_64. I think we'll have to do that when we have
machines with more memory than PAE can address.

thanks

ron
Patrick Georgi - 2010-02-23 21:43:46
Am 21.02.2010 23:42, schrieb Rudolf Marek:
> Hello,
> 
> This patch adds clobber regs into the play as assembly trashes random registers.
> I was bitten by this while doing MSR I/O because ECX got corrupted. I attempted
> to create nice barrier for GCC while changing the stack location too.
> 
> The memcpy code was taken from Linux kernel, dont know to whom attribute the
> copyright on it. If it is OK with the note, otherwise:
> 
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>

and committed as r5153

Patch

Index: src/cpu/amd/car/disable_cache_as_ram.c
===================================================================
--- src/cpu/amd/car/disable_cache_as_ram.c	(revision 5134)
+++ src/cpu/amd/car/disable_cache_as_ram.c	(working copy)
@@ -2,62 +2,49 @@ 
 /* be warned, this file will be used other cores and core 0 / node 0 */
 static inline __attribute__((always_inline)) void disable_cache_as_ram(void)
 {
-
-        __asm__ volatile (
-
+        __asm__ __volatile__ (
         /* We don't need cache as ram for now on */
         /* disable cache */
-        "movl    %cr0, %eax\n\t"
-        "orl    $(0x1<<30),%eax\n\t"
-        "movl    %eax, %cr0\n\t"
+        "movl    %%cr0, %%eax\n\t"
+        "orl    $(0x1<<30),%%eax\n\t"
+        "movl    %%eax, %%cr0\n\t"
 
         /* clear sth */
-        "movl    $0x269, %ecx\n\t"  /* fix4k_c8000*/
-        "xorl    %edx, %edx\n\t"
-        "xorl    %eax, %eax\n\t"
+        "movl    $0x269, %%ecx\n\t"  /* fix4k_c8000*/
+        "xorl    %%edx, %%edx\n\t"
+        "xorl    %%eax, %%eax\n\t"
 	"wrmsr\n\t"
 #if CONFIG_DCACHE_RAM_SIZE > 0x8000
-	"movl    $0x268, %ecx\n\t"  /* fix4k_c0000*/
+	"movl    $0x268, %%ecx\n\t"  /* fix4k_c0000*/
         "wrmsr\n\t"
 #endif
 
         /* disable fixed mtrr from now on, it will be enabled by coreboot_ram again*/
-        "movl    $0xC0010010, %ecx\n\t"
+        "movl    $0xC0010010, %%ecx\n\t"
 //        "movl    $SYSCFG_MSR, %ecx\n\t"
         "rdmsr\n\t"
-        "andl    $(~(3<<18)), %eax\n\t"
+        "andl    $(~(3<<18)), %%eax\n\t"
 //        "andl    $(~(SYSCFG_MSR_MtrrFixDramModEn | SYSCFG_MSR_MtrrFixDramEn)), %eax\n\t"
         "wrmsr\n\t"
 
         /* Set the default memory type and disable fixed and enable variable MTRRs */
-        "movl    $0x2ff, %ecx\n\t"
+        "movl    $0x2ff, %%ecx\n\t"
 //        "movl    $MTRRdefType_MSR, %ecx\n\t"
-        "xorl    %edx, %edx\n\t"
+        "xorl    %%edx, %%edx\n\t"
         /* Enable Variable and Disable Fixed MTRRs */
-        "movl    $0x00000800, %eax\n\t"
+        "movl    $0x00000800, %%eax\n\t"
         "wrmsr\n\t"
 
         /* enable cache */
-        "movl    %cr0, %eax\n\t"
-        "andl    $0x9fffffff,%eax\n\t"
-        "movl    %eax, %cr0\n\t"
-
+        "movl    %%cr0, %%eax\n\t"
+        "andl    $0x9fffffff,%%eax\n\t"
+        "movl    %%eax, %%cr0\n\t"
+        ::: "memory", "eax", "ecx", "edx"
         );
 }
 
 static void disable_cache_as_ram_bsp(void)
 {
-	__asm__ volatile (
-//		"pushl %eax\n\t"
- 		"pushl %edx\n\t"
- 		"pushl %ecx\n\t"
-	);
-
 	disable_cache_as_ram();
-        __asm__ volatile (
-                "popl %ecx\n\t"
-                "popl %edx\n\t"
-//                "popl %eax\n\t"
-        );
 }
 
Index: src/cpu/amd/car/post_cache_as_ram.c
===================================================================
--- src/cpu/amd/car/post_cache_as_ram.c	(revision 5134)
+++ src/cpu/amd/car/post_cache_as_ram.c	(working copy)
@@ -10,14 +10,20 @@ 
         printk_debug("%s%08x\r\n", strval, val);
 }
 
+/* from linux kernel 2.6.32 asm/string_32.h */
+
 static void inline __attribute__((always_inline))  memcopy(void *dest, const void *src, unsigned long bytes)
 {
-        __asm__ volatile(
-                "cld\n\t"
-                "rep; movsl\n\t"
-                : /* No outputs */
-                : "S" (src), "D" (dest), "c" ((bytes)>>2)
-                );
+	int d0, d1, d2;
+	asm volatile("cld ; rep ; movsl\n\t"
+			"movl %4,%%ecx\n\t"
+			"andl $3,%%ecx\n\t"
+			"jz 1f\n\t"
+			"rep ; movsb\n\t"
+			"1:"
+			: "=&c" (d0), "=&D" (d1), "=&S" (d2)
+			: "0" (bytes / 4), "g" (bytes), "1" ((long)dest), "2" ((long)src)
+			: "memory", "cc");
 }
 /* Disable Erratum 343 Workaround, see RevGuide for Fam10h, Pub#41322 Rev 3.33 */
 
@@ -66,28 +72,18 @@ 
 	/* from here don't store more data in CAR */
 	vErrata343();
 
-#if 0
-        __asm__ volatile (
-        	"pushl  %eax\n\t"
-        );
-#endif
-
         memcopy((void *)((CONFIG_RAMTOP)-CONFIG_DCACHE_RAM_SIZE), (void *)CONFIG_DCACHE_RAM_BASE, CONFIG_DCACHE_RAM_SIZE); //inline
 //        dump_mem((CONFIG_RAMTOP) - 0x8000, (CONFIG_RAMTOP) - 0x7c00);
 
         __asm__ volatile (
                 /* set new esp */ /* before CONFIG_RAMBASE */
-                "subl   %0, %%ebp\n\t"
                 "subl   %0, %%esp\n\t"
                 ::"a"( (CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE)- (CONFIG_RAMTOP) )
-        ); // We need to push %eax to the stack (CAR) before copy stack and pop it later after copy stack and change esp
-#if 0
-        __asm__ volatile (
-	        "popl   %eax\n\t"
+		/* discard all registers (eax is used for %0), so gcc redo everything
+		   after the stack is moved */
+		: "cc", "memory", "%ebx", "%ecx", "%edx", "%esi", "%edi", "%ebp"
         );
-#endif
 
-
 	/* We can put data to stack again */
 
         /* only global variable sysinfo in cache need to be offset */