Re: [coreboot] [patch 3/3] Implement post_main/post_cache_as_ram resume hooks for romstage on Intel and use them for Slot-1 cpus if CONFIG_GENERATE_ACPI_TABLES=y. (rediffed against r6176)

[Stefan Reinauer]

Hi Tobias,

thanks a lot for your work. It's good to see people bringing new
features into more coreboot boards.

However, unfortunately, I have to put a big NACK on this one...

The below is very ugly, sorry to say. Please rework that code. I know
it's taken from the AMD cache as ram code, and I've been spending quite
some time of getting rid of post_cache_as_ram.c on all platforms. I
skipped AMD because I had no test platform and the code was harder to
transform than on the other platforms, but I'm very unhappy about seeing
this might sneak back in... There has to be a better way.

* Tobias Diedrich  [101213 23:46]:
> Index: src/cpu/intel/car/post_cache_as_ram.c
>
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ src/cpu/intel/car/post_cache_as_ram.c	2010-12-13 23:35:22.327299572 +0100
> @@ -0,0 +1,240 @@
> +/*
> This file is part of the coreboot project.
> original idea yhlu 6.2005 (assembler code)
> Copyright (C) 2010 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; version 2 of the License.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> be warned, this file will be used other cores and core 0 / node 0
> */
>
> +#include <string.h>
> +#include <arch/stages.h>
> +#include <console/console.h>
> +#include <cpu/x86/mtrr.h>
> +#include <arch/acpi.h>
> +#include <cbmem.h>
> +#include "cpu/x86/mtrr/earlymtrr.c"
>
> +#if CONFIG_RAMTOP <= 0x100000
> #error "You need to set CONFIG_RAMTOP greater than 1M"
>
> +#endif

RAMTOP should actually be set to exactly 1M. But this is not a good
place for this check.
> +#define DCACHE_RAM_BASE (CONFIG_DCACHE_RAM_TOP - CONFIG_DCACHE_RAM_SIZE)

Isn't the BASE defined somewhere already?
> +static inline void print_debug_pcar(const char *strval, uint32_t val)
> +{
> printk(BIOS_DEBUG, "%s%08x\n", strval, val);
>
> +}

Why this wrapper?
> > +/* 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)
> +{
> 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");
>
> +}

This is unneeded and was introduced to work around some shortcomings in
the AMD code that have been fixed since. If anything, it should be
dropped from the AMD code, too.
> +static u8 acpi_checksum(u8 *table, u32 length)
> +{
> u8 ret = 0;
> while (length--) {
> ret += *table;
> table++;
> }
> return -ret;
>
> +}
>
> +static int valid_rsdp(acpi_rsdp_t *rsdp)
> +{
> unsigned *sig;
> sig = (void*)rsdp;
> if (*sig != 0x20445352)
> return 0;
> sig++;
> if (*sig != 0x20525450)
> return 0;
> print_debug("Looking on ");
> print_debug_hex32((u32)rsdp);
> print_debug(" for valid checksum\n");
> if (acpi_checksum((void *)rsdp, 20) != 0)
> return 0;
> print_debug("Checksum 1 passed\n");
> if ((rsdp->revision > 1) &&
>    (acpi_checksum((void *)rsdp, rsdp->length) != 0))
> return 0;
> print_debug("Checksum 2 passed all OK\n");
> return 1;
>
> +}
>
> +struct cbmem_entry *get_cbmem_toc(void)
> +{
> char *p;
> acpi_rsdp_t *rsdp;
> cbmem_toc_ptr_t *cbmem_tocp;
> print_debug("Trying to find the backup area pointer...\n");
> /* Find RSDP. */
> rsdp = NULL;
> for (p = (char *)0xe0000; p < (char *)0xfffff; p += 16) {
> rsdp = (acpi_rsdp_t *)p;
> if (valid_rsdp(rsdp))
> 	break;
> rsdp = NULL;
> }
> if (rsdp == NULL) {
> print_debug("RSDP not found\n");
> return NULL;
> }
> cbmem_tocp = (cbmem_toc_ptr_t *)(rsdp->rsdt_address - sizeof(cbmem_toc_ptr_t));
> if (cbmem_tocp->sig != CBMEM_TOC_PTR_SIG) {
> printk(BIOS_DEBUG, "cbmem toc pointer not found at %p (sig %08x sz %d)\n", cbmem_tocp, cbmem_tocp->sig, sizeof(cbmem_toc_ptr_t));
> return NULL;
> }
> return cbmem_tocp->ptr;
>
> +}
> +static inline void *backup_resume(void) {

function curly brackets go on the next line
> unsigned long high_ram_base;
> void *resume_backup_memory;
> /* Start address of high memory tables */
> high_ram_base = (u32) get_cbmem_toc();
> print_debug_pcar("CBMEM TOC is at: ", high_ram_base);
> print_debug_pcar("CBMEM TOC 0-size: ", (high_ram_base + HIGH_MEMORY_SIZE + 4096));
> cbmem_reinit((u64)high_ram_base);
> resume_backup_memory = cbmem_find(CBMEM_ID_RESUME);
> /* copy 1MB - 64K to high tables ram_base to prevent memory corruption
> * through stage 2. We could keep stuff like stack and heap in high tables
> * memory completely, but that's a wonderful clean up task for another
> * day.
> */
> if (resume_backup_memory) {
> print_debug_pcar("Will copy coreboot region to: ", (uint32_t) resume_backup_memory);
> /* copy only backup only memory used for CAR */
> memcopy(resume_backup_memory+HIGH_MEMORY_SAVE-CONFIG_DCACHE_RAM_SIZE,
> 	(void *)((CONFIG_RAMTOP)-CONFIG_DCACHE_RAM_SIZE),
> 	 CONFIG_DCACHE_RAM_SIZE); //inline
> }
> return resume_backup_memory;
>
> +}

Can't this code be called from romstage.c?
> +void enable_pm(void);
> +int acpi_get_sleep_type(void);
>
> +void* acpi_resume_post_main(void);
> +void acpi_resume_post_cache_as_ram(void *resume_backup_memory);
>
> +void* acpi_resume_post_main(void)
> +{
> int sleep_type;
> void *resume_backup_memory = NULL;
> enable_pm();
> sleep_type = acpi_get_sleep_type();
>
> +#if 1
> {
> /* Check value of esp to verify if we have enough rom for stack in Cache as RAM */
> unsigned v_esp;
> __asm__ volatile (
> "movl   %%esp, %0\n\t"
> : "=a" (v_esp)
> );
> print_debug_pcar("v_esp=", v_esp);
> }
>
> +#endif
> unsigned testx = 0x5a5a5a5a;
> print_debug_pcar("testx = ", testx);
> /* copy data from cache as ram to
> ram need to set CONFIG_RAMTOP to 2M and use var mtrr instead.
> */
> if (sleep_type == 2 || sleep_type == 3)
> 	resume_backup_memory = backup_resume();
> printk(BIOS_DEBUG, "resume_backup_memory=%p\n", resume_backup_memory);
> print_debug("Copying data from cache to RAM -- switching to use RAM as stack... ");
> memcopy((void *)((CONFIG_RAMTOP)-CONFIG_DCACHE_RAM_SIZE), (void *)DCACHE_RAM_BASE, CONFIG_DCACHE_RAM_SIZE); //inline
> __asm__ volatile (
> /* set new esp */ /* before CONFIG_RAMBASE */
> "subl   %0, %%esp\n\t"
> ::"a"( (DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE)- (CONFIG_RAMTOP) )
> /* 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"
> );

This is very ugly and should be done completely in assembler, if it's
needed at all.  It's awfully fragile and ugly
> /* We can put data to stack again */
> /* only global variable sysinfo in cache need to be offset */
> print_debug("Done\n");
> print_debug_pcar("testx = ", testx);
> return resume_backup_memory;
>
> +}
>
> +void acpi_resume_post_cache_as_ram(void *resume_backup_memory)
> +{
> print_debug("After cache as ram disabled \n");
> printk(BIOS_DEBUG, "resume_backup_memory=%p\n", resume_backup_memory);
> /* now copy the rest of the area, using the WB method because we already
>   run normal RAM */
> if (resume_backup_memory) {
> memcopy(resume_backup_memory,
> 		(void *)(CONFIG_RAMBASE),
> 		(CONFIG_RAMTOP) - CONFIG_RAMBASE - CONFIG_DCACHE_RAM_SIZE);
> }
> print_debug("Clearing initial memory region: ");
> /* clear only coreboot used region of memory. Note: this may break ECC enabled boards */
> memset((void*) CONFIG_RAMBASE, 0, (CONFIG_RAMTOP) - CONFIG_RAMBASE - CONFIG_DCACHE_RAM_SIZE);
> print_debug("Done\n");
> /*copy and execute coreboot_ram */
> copy_and_run(0);
> /* We will not return */
> print_debug("should not be here -\n");
> for (;;);
>
> +}

Please have a look at how getac/p470 does resume.
> Index: src/cpu/intel/Makefile.inc
>
> --- src/cpu/intel/Makefile.inc.orig	2010-12-13 23:34:06.298921457 +0100
> +++ src/cpu/intel/Makefile.inc	2010-12-13 23:34:14.042912591 +0100
> @@ -16,6 +16,7 @@
>  subdirs-$(CONFIG_CPU_INTEL_SOCKET_PGA370) += socket_PGA370
>  subdirs-$(CONFIG_CPU_INTEL_SLOT_2) += slot_2
>  subdirs-$(CONFIG_CPU_INTEL_SLOT_1) += slot_1
> +subdirs-$(CONFIG_CPU_INTEL) += car
>
> #socket_mPGA604_533Mhz
>  #socket_mPGA604_800Mhz

This should only be added from the socket directory of a given CPU
> Index: src/cpu/intel/Kconfig
>
> --- src/cpu/intel/Kconfig.orig	2010-12-13 23:34:06.290921469 +0100
> +++ src/cpu/intel/Kconfig	2010-12-13 23:34:14.042912591 +0100
> @@ -28,3 +28,17 @@
>  source src/cpu/intel/socket_mPGA604/Kconfig
>  source src/cpu/intel/socket_PGA370/Kconfig
>  source src/cpu/intel/socket_441/Kconfig
>
> +config CPU_INTEL
> bool
> default n

This sounds too generic. Not all Intel CPUs use this.
> +config DCACHE_RAM_TOP
> hex
> default 0xd0000
> depends on CACHE_AS_RAM
> depends on CPU_INTEL

It looks like this value could be calculated from the cache as ram base
+ size. Should not be part of Kconfig, as it's another variable that
will get copied blindly over time.

Stefan