Patchwork make AMD CAR disable a C function

login
register
about
Submitter Rudolf Marek
Date 2010-05-16 16:46:34
Message ID <4BF0216A.70706@assembler.cz>
Download mbox | patch
Permalink /patch/1345/
State Accepted
Headers show

Comments

Rudolf Marek - 2010-05-16 16:46:34
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

Following patch reworks car_disable into C. Tested, works here. I compared
also the GCC generated code and it looks all right. Please test on some
multicore CPU.

I added the "memory" clobber to read_cr0 / write_cr0 function as it is in Linux
Kernel. Seems that if this is missing, GCC is too smart and messes the order
of reads/writes to CR0 (not tested if really a problem here, but be safe for
future users of this function ;)

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

Maybe we can add nice copyright header too?

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

iEYEARECAAYFAkvwIWoACgkQ3J9wPJqZRNV7/QCcD+WycESr5QIRGThVOX0UJmjL
I0wAn39m4vC0SdvZdFLoI23cxPMyEQb1
=qavi
-----END PGP SIGNATURE-----
Stefan Reinauer - 2010-05-16 17:36:09
On 5/16/10 6:46 PM, Rudolf Marek wrote:
>
>
> OpenPGP: *Attachments to this message have not been signed or encrypted*
>
> ********* *BEGIN ENCRYPTED or SIGNED PART* *********
>
> Hello,
>
> Following patch reworks car_disable into C. Tested, works here. I compared
> also the GCC generated code and it looks all right. Please test on some
> multicore CPU.
>   

A potential Todo on my list was to straighten out cache_as_ram on K8/K10
in the same way as we did on all the other functions, and that would
mean calling post_cache_as_ram() from assembler directly, so we don't
have to add the call to each and every romstage.c:main() function. That
might
include a second jump to a C function, or significantly stripping down
post_cache_as_ram() for those CPUs so it can easily be added in
assembler like on the other CPUs. What do you think?

> I added the "memory" clobber to read_cr0 / write_cr0 function as it is in Linux
> Kernel. Seems that if this is missing, GCC is too smart and messes the order
> of reads/writes to CR0 (not tested if really a problem here, but be safe for
> future users of this function ;)
>
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
>   

Acked-by: Stefan Reinauer <stepan@coresystems.de>
> Maybe we can add nice copyright header too?
>   
Please do!
Kevin O'Connor - 2010-05-16 17:50:35
On Sun, May 16, 2010 at 06:46:34PM +0200, Rudolf Marek wrote:
> Following patch reworks car_disable into C. Tested, works here. I compared
> also the GCC generated code and it looks all right. Please test on some
> multicore CPU.

The AMD disable_cache_as_ram can be C code, but the other
architectures can not.  It would be nice if there was a single calling
convention for this capability across architectures.

Maybe something like:

void __regparm(3) __noreturn disable_car(void (*func)(u32), u32 param, u32 esp);

which would call func(param) on the stack specified by 'esp'.  The
caller would then do something like:

void romstage(u32 bist) {
    ...
    disable_car(load_ramstage, bist, CONFIG_RAMTOP)
}

void load_ramstage(u32 bist) {
    ...
}

> I added the "memory" clobber to read_cr0 / write_cr0 function as it is in Linux
> Kernel. Seems that if this is missing, GCC is too smart and messes the order
> of reads/writes to CR0 (not tested if really a problem here, but be safe for
> future users of this function ;)

Good catch!

-Kevin
Stefan Reinauer - 2010-05-16 18:35:05
On 5/16/10 7:50 PM, Kevin O'Connor wrote:
> The AMD disable_cache_as_ram can be C code, but the other
> architectures can not.  It would be nice if there was a single calling
> convention for this capability across architectures.
>
> Maybe something like:
>
> void __regparm(3) __noreturn disable_car(void (*func)(u32), u32 param, u32 esp);
>
> which would call func(param) on the stack specified by 'esp'.  The
> caller would then do something like:
>
> void romstage(u32 bist) {
>     ...
>     disable_car(load_ramstage, bist, CONFIG_RAMTOP)
> }
>
> void load_ramstage(u32 bist) {
>     ...
> }

I agree. Right now AMD (K8/K10) is the only exception to the coreboot
calling convention. All other CPU types return to cache_as_ram.inc from
romstage.c:main(). Then cache_as_ram.inc disables cache as ram, sets up
the stack and loads the ramstage with the C function copy_and_run().

Stefan
Patrick Georgi - 2010-05-16 19:55:58
Am 16.05.2010 19:50, schrieb Kevin O'Connor:
> The AMD disable_cache_as_ram can be C code, but the other
> architectures can not.  It would be nice if there was a single calling
> convention for this capability across architectures.
Good idea!


> void romstage(u32 bist) {
>     ...
>     disable_car(load_ramstage, bist, CONFIG_RAMTOP)
> }
Where would this code reside? Hopefully not (in this verbosity) in every
boards' romstage.c?

load_ramstage and the new stack location shouldn't be board specific
(and shouldn't need to be). With both pretty much set in stone, only
bist needs to be brought over.


Regards,
Patrick
Rudolf Marek - 2010-05-16 21:32:06
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

My plan is to add backup code for S3 into post_cache_as_ram, I have it working
already but needs cleanup. POC is in [PATCH] WIP - suspend/resume on AMD64 using
CBMEM.

Also I think we could enable whole ROM for caching in the enable_cache_as_ram.
It works fine here.

As the last point, just right after post_cache_as_ram I would like to setup
following MTRR instead of _RAMBASE - _RAMTOP

0MB - TOM WB
(or little over TOM if tom not a power of 2?)

0xA0000 - 0xC0000 UC

I think one could live with this setup until the MTRR are set again in ramstage.

More ideas?

Rudolf

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

iEYEARECAAYFAkvwZFYACgkQ3J9wPJqZRNUeeQCgvmc6+zv7kj11rdFiGAvZY40M
/2gAn3b/oMebk076ukXknWoiOJ8PcSmf
=asle
-----END PGP SIGNATURE-----
Kevin O'Connor - 2010-05-16 21:34:42
On Sun, May 16, 2010 at 09:55:58PM +0200, Patrick Georgi wrote:
> Am 16.05.2010 19:50, schrieb Kevin O'Connor:
> > void romstage(u32 bist) {
> >     ...
> >     disable_car(load_ramstage, bist, CONFIG_RAMTOP)
> > }
> Where would this code reside? Hopefully not (in this verbosity) in every
> boards' romstage.c?
> 
> load_ramstage and the new stack location shouldn't be board specific
> (and shouldn't need to be). With both pretty much set in stone, only
> bist needs to be brought over.

I agree with Stefan that the code that calls romstage() should also
call disable_car() - no need to repeat the code in every board.

What's not clear to me is how to handle S3 resume.  I would think we'd
want an ability to disable CAR and place the new stack somewhere other
than CONFIG_RAMTOP.

I'm still a bit confused on what the code flow is today during an S3
resume.  (Is it, tinybootblock->romstage->ramstage->restorememory->OS?)

-Kevin
Rudolf Marek - 2010-05-16 22:10:50
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> What's not clear to me is how to handle S3 resume.  I would think we'd
> want an ability to disable CAR and place the new stack somewhere other
> than CONFIG_RAMTOP.

Well check my patch
http://patchwork.coreboot.org/patch/1266/

I just backup the memory right before I place there the stack, because not
only stack is used, some sysinfo datastructures needs to be copied of
CAR too.

> I'm still a bit confused on what the code flow is today during an S3
> resume.  (Is it, tinybootblock->romstage->ramstage->restorememory->OS?)

Yes, but remember AMD is now broken for some months, the patch above tries to
fix it.

What is done in post_cache_as_ram just before the disable of car:

1) find if resume
2) if so find the backup memory
3) backup the memory bellow ramtop where CAR copy will be placed.
4) copy CAR to memory as usual
5) disable CAR
6) backup the rest of ram (RAMBASE-RAMTOP without the already copied area)
7) do normal boot
8) then right in the place where the ACPI is being written if resume
   find the OS waking vector, if ok, restore backup and jump to OS.

Note we still dont chain to Seabios which is  nice to have because we need to
run the post VGA too ;)

Rudolf

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

iEYEARECAAYFAkvwbWoACgkQ3J9wPJqZRNUbMgCdH9LJSP2MxzQkF/I3qR+q3Lim
/bYAnA4DqlSx+j55a56PFwoxxdrpQq/g
=w1bG
-----END PGP SIGNATURE-----
Kevin O'Connor - 2010-05-17 00:10:40
On Mon, May 17, 2010 at 12:10:50AM +0200, Rudolf Marek wrote:
> > What's not clear to me is how to handle S3 resume.  I would think we'd
> > want an ability to disable CAR and place the new stack somewhere other
> > than CONFIG_RAMTOP.
> 
> Well check my patch
> http://patchwork.coreboot.org/patch/1266/
> 
> I just backup the memory right before I place there the stack, because not
> only stack is used, some sysinfo datastructures needs to be copied of
> CAR too.

Okay.  So, resume is more like:

tinybootblock->romstage->backupmem->ramstage->restoremem->OS

It seems to me that the AMD car disable code is too complex.  I find
the VIA car code much easier to follow.

-Kevin

Patch

Index: src/include/cpu/x86/cache.h
===================================================================
--- src/include/cpu/x86/cache.h	(revision 5511)
+++ src/include/cpu/x86/cache.h	(working copy)
@@ -20,16 +20,19 @@ 
 #ifndef CPU_X86_CACHE
 #define CPU_X86_CACHE
 
+/* the memory clobber prevents the GCC from reordering the read/write order
+   of CR0 */
+
 static inline unsigned long read_cr0(void)
 {
 	unsigned long cr0;
-	asm volatile ("movl %%cr0, %0" : "=r" (cr0));
+	asm volatile ("movl %%cr0, %0" : "=r" (cr0) :: "memory");
 	return cr0;
 }
 
 static inline void write_cr0(unsigned long cr0)
 {
-	asm volatile ("movl %0, %%cr0" : : "r" (cr0));
+	asm volatile ("movl %0, %%cr0" : : "r" (cr0) : "memory");
 }
 
 static inline void invd(void)
@@ -39,7 +42,7 @@ 
 
 static inline void wbinvd(void)
 {
-	asm volatile ("wbinvd");
+	asm volatile ("wbinvd" ::: "memory");
 }
 
 static inline void enable_cache(void)
Index: src/cpu/amd/car/disable_cache_as_ram.c
===================================================================
--- src/cpu/amd/car/disable_cache_as_ram.c	(revision 5511)
+++ src/cpu/amd/car/disable_cache_as_ram.c	(working copy)
@@ -1,46 +1,37 @@ 
-/* by yhlu 6.2005 */
-/* be warned, this file will be used other cores and core 0 / node 0 */
+/* original idea yhlu 6.2005
+
+(C) Rudolf Marek <r.marek@assembler.cz>
+
+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__ (
-	/* We don't need cache as ram for now on */
+	msr_t msr;
+
 	/* disable cache */
-	"movl    %%cr0, %%eax\n\t"
-	"orl    $(0x1<<30),%%eax\n\t"
-	"movl    %%eax, %%cr0\n\t"
+	write_cr0(read_cr0() | (1 << 30));
 
-	/* clear sth */
-	"movl    $0x269, %%ecx\n\t"  /* fix4k_c8000*/
-	"xorl    %%edx, %%edx\n\t"
-	"xorl    %%eax, %%eax\n\t"
-	"wrmsr\n\t"
+	msr.lo = 0;
+	msr.hi = 0;
+	wrmsr(MTRRfix4K_C8000_MSR, msr);
 #if CONFIG_DCACHE_RAM_SIZE > 0x8000
-	"movl    $0x268, %%ecx\n\t"  /* fix4k_c0000*/
-	"wrmsr\n\t"
+	wrmsr(MTRRfix4K_C0000_MSR, msr);
 #endif
-
 	/* disable fixed mtrr from now on, it will be enabled by coreboot_ram again*/
-	"movl    $0xC0010010, %%ecx\n\t"
-//	"movl    $SYSCFG_MSR, %ecx\n\t"
-	"rdmsr\n\t"
-	"andl    $(~(3<<18)), %%eax\n\t"
-//	"andl    $(~(SYSCFG_MSR_MtrrFixDramModEn | SYSCFG_MSR_MtrrFixDramEn)), %eax\n\t"
-	"wrmsr\n\t"
 
+	msr = rdmsr(SYSCFG_MSR);
+	msr.lo &= ~(SYSCFG_MSR_MtrrFixDramEn | SYSCFG_MSR_MtrrFixDramModEn);
+	wrmsr(SYSCFG_MSR, msr);
+
 	/* Set the default memory type and disable fixed and enable variable MTRRs */
-	"movl    $0x2ff, %%ecx\n\t"
-//	"movl    $MTRRdefType_MSR, %ecx\n\t"
-	"xorl    %%edx, %%edx\n\t"
-	/* Enable Variable and Disable Fixed MTRRs */
-	"movl    $0x00000800, %%eax\n\t"
-	"wrmsr\n\t"
+	msr.hi = 0;
+	msr.lo = (1 << 11);
 
-	/* enable cache */
-	"movl    %%cr0, %%eax\n\t"
-	"andl    $0x9fffffff,%%eax\n\t"
-	"movl    %%eax, %%cr0\n\t"
-	::: "memory", "eax", "ecx", "edx"
-	);
+	wrmsr(MTRRdefType_MSR, msr);
+
+	enable_cache();
 }
 
 static void disable_cache_as_ram_bsp(void)