[coreboot] [PATCH] add proper "memory management" for high table area (aka CBMEM)

[Peter Stuge]

Stefan Reinauer wrote:
> CBMEM high table memory manager.

Nice.


> This code adds a very simple toc based memory manager for the high tables area.
> The purpose of this code is to make it simpler and more reliable to find
> certain data structures in memory. This will also make it possible to have ACPI
> S3 Resume working without an ugly hole at 31MB.
> 
> Signed-off-by: Stefan Reinauer <stepan at coresystems.de>

Acked-by: Peter Stuge <peter at stuge.se>


Maybe name it CBMM instead?



> +++ src/include/cbmem.h	(revision 0)
..
> +#if CONFIG_HAVE_ACPI_RESUME
> +#define HIGH_MEMORY_SIZE	( 1024 * 1024 )
> +#define HIGH_MEMORY_SAVE	( HIGH_MEMORY_SIZE - HIGH_MEMORY_TABLES )
> +#else
> +#define HIGH_MEMORY_SIZE	HIGH_MEMORY_TABLES
> +#endif

Note that _HAVE_ in general is meant for when the port has a
capability. It does not mean that the user enabled the capability
during build. Now, ACPI resume is not currently configurable, and I
don't know if it should be, but it's good to be consistent with
naming.


> +++ src/lib/cbmem.c	(revision 0)
..
> +// The CBMEM TOC reserves 512 bytes to keep
> +// the other entries somewhat aligned.
> +// Increase if MAX_CBMEM_ENTRIES exceeds 21
> +#define CBMEM_TOC_RESERVED	512
> +#define MAX_CBMEM_ENTRIES	16

It would be nice if only one of these was explicitly set, and the
other determined by the first.

#define MAX_CBMEM_ENTRIES 16
#define CBMEM_TOC_RESERVED ALIGN_UP(512, (MAX_CBMEM_ENTRIES * sizeof(struct cbmem_entry)))

Or something similar.


> +#define CBMEM_MAGIC		0x434f5245

Maybe CBMM rather than CORE? (with or without rename)


> +++ src/cpu/intel/model_6ex/cache_as_ram_disable.c	(working copy)
..
> -        {
> -        	/* 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)
> -	        );
> -	        printk_spew("v_esp=%08x\r\n", v_esp);
> -        }
> +#if CAR_DEBUG
> +        /* 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"
> +		: "=a" (v_esp)
> +	);
> +	printk_spew("v_esp=%08x\n", v_esp);
> +#endif

I guess this is unrelated, but it looks benign.


> -	if(cpu_reset==0) {
> -#define CLEAR_FIRST_1M_RAM 1
> -#include "cache_as_ram_post.c"
> -	} else {
>  #undef CLEAR_FIRST_1M_RAM 
>  #include "cache_as_ram_post.c"
> -	}

Um?



> +	/* For now: use rambase + 1MB - 64K (counting downwards) as stack. This
> +	 * makes sure that we stay completely within the 1M of memory we
> +	 * preserve with the memcpy above.
> +	 */
> +
> +#ifndef HIGH_MEMORY_SAVE
> +#define HIGH_MEMORY_SAVE ( (1024 - 64) * 1024 )
> +#endif
> +
>  	__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_RAMBASE )
> +		"movl %0, %%ebp\n"
> +		"movl %0, %%esp\n"
> +		:: "a" (CONFIG_RAMBASE + HIGH_MEMORY_SAVE)

Uhm again? :)


> -#ifdef DEACTIVATE_CAR
> -		print_debug("Deactivating CAR");
> -#include DEACTIVATE_CAR_FILE
> -		print_debug(" - Done.\r\n");
> -#endif

I guess this happens elsewhere then?


> -		/* We will not return */
>  	}
>  
> -	print_debug("sorry. parachute did not open.\r\n");
> +	/* We will not return */
> +	printk_debug("sorry. parachute did not open.\n");
>  }

Better message please? :) And maybe a die() call?


> +++ src/cpu/intel/model_6fx/cache_as_ram_disable.c	(working copy)

Same stuff as for model_6ex.


> +++ src/arch/i386/boot/tables.c	(working copy)
..
> +	/* Ok, this is a bit hacky still, because some day we want to have this
> +	 * completely dynamic. But right now we are setting fixed sizes. 
> +	 * It's probably still better than the old high_table_base code because
> +	 * now at least we know when we have an overflow in the area.

Maybe make the reserved size determined completely at run time then?


> +#if CONFIG_HAVE_ACPI_RESUME
> +	/* Let's prepare the ACPI S3 Resume area now already, so we can rely on
> +	 * it begin there during reboot time. We don't need the pointer, nor
> +	 * the result right now. If it fails, ACPI resume will be disabled.
> +	 */
> +	cbmem_add(CBMEM_ID_RESUME, 1024 * (1024-64));
> +#endif

So one chunk is only reserved with ACPI_RESUME, but the size from
which reservations are made is always the same? That makes for some
unfortunate asymmetry when that space is almost full.


> +	// Remove before sending upstream
> +	cbmem_list();

Are we upstream? :)


> +++ src/boot/hardwaremain.c	(working copy)
..
> +	/* console_init() MUST PRECEDE ALL printk()! */

What about printk_*() ?


//Peter