Re: [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

Acked-by: Peter Stuge

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