| 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
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!
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
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
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
-----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-----
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
-----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-----
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)
-----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-----