[coreboot] [PATCH] RAMTOP fixes

Stefan Reinauer stepan at coresystems.de
Tue Apr 27 12:14:09 CEST 2010


On 10/16/09 7:33 PM, Myles Watson wrote:
> I think we should clean up memory allocation.  There are multiple
> places in the code where RAMTOP is used as an offset into ram and cast
> to a struct.
Maybe that's because the author of that code assumed that this area of
memory would be free to use. And I think it is, if  (RAMBASE + size of
stage2) < (RAMTOP - sizeof struct)

> I think if something is important enough that it needs to be allocated
> separately from the stack, it should show up in the ldscripts, not
> just in the code.
Or in cbmem? Or on the heap using malloc?
Definitely, it should not just be floating around somewhere

> What's important enough?
> K8 page tables for APs?
We need them for >4GB memory clearing?

> EHCI debug?
> Sysinfo structs?
How do they get there? are they magically copied over from romstage? I
would say this is a typical case for global variables or malloc'ed memory


> Signed-off-by: Myles Watson <mylesgw at gmail.com <mailto:mylesgw at gmail.com>>
>
> Thanks,
> Myles
> ramtop_fixes.diff
>
>
> Index: cbv2/src/cpu/x86/lapic/lapic_cpu_init.c
> ===================================================================
> --- cbv2.orig/src/cpu/x86/lapic/lapic_cpu_init.c
> +++ cbv2/src/cpu/x86/lapic/lapic_cpu_init.c
> @@ -246,19 +246,15 @@ int start_cpu(device_t cpu)
>  	index = ++last_cpu_index;
>  	
>  	/* Find end of the new processors stack */
> -#if (CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE < 0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1))
> +#if (CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE < 0x100000)
>  	if(index<1) { // only keep bsp on low 
> -		stack_end = ((unsigned long)_estack) - (CONFIG_STACK_SIZE*index) - sizeof(struct cpu_info);
> +		stack_end = ((unsigned long)_estack) - sizeof(struct cpu_info);
>   
Does this not break for multiple CPUs?

>  	} else {
>  		// for all APs, let use stack after pgtbl, 20480 is the pgtbl size for every cpu
>  		stack_end = 0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS - (CONFIG_STACK_SIZE*index);
>  #if (0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS) > (CONFIG_RAMTOP)
> -		#warning "We may need to increase CONFIG_RAMTOP, it need to be more than (0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS)\n"
> +		#error "Please increase CONFIG_RAMTOP, it need to be more than (0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS)\n"
>  #endif
> -		if(stack_end > (CONFIG_RAMTOP)) {
> -			printk_debug("start_cpu: Please increase the CONFIG_RAMTOP more than %luK\n", stack_end);
> -			die("Can not go on\n");
> -		}
>   
Maybe we can set RAMTOP automatically... it's not really "configuration"...?

> Index: cbv2/src/console/usbdebug_direct_console.c
> ===================================================================
> --- cbv2.orig/src/console/usbdebug_direct_console.c
> +++ cbv2/src/console/usbdebug_direct_console.c
> @@ -52,6 +52,7 @@ static void dbgp_init(void)
>  	/* At this point, all we have to do is copy the fixed address
>  	 * debug_info data structure to our version defined above. */
>  
> +	/* How do we deal with this? */
>  	dbg_infox = (struct ehci_debug_info *)
>  		((CONFIG_RAMTOP) - sizeof(struct ehci_debug_info));
>   

What do you suggest? Or, what's the issue?

>  
> Index: cbv2/src/cpu/amd/car/clear_init_ram.c
> ===================================================================
> --- cbv2.orig/src/cpu/amd/car/clear_init_ram.c
> +++ cbv2/src/cpu/amd/car/clear_init_ram.c
> @@ -9,9 +9,10 @@ static void __attribute__((noinline)) cl
>  
>  #if CONFIG_HAVE_ACPI_RESUME == 1
>  	/* clear only coreboot used region of memory. Note: this may break ECC enabled boards */
> -	clear_memory( CONFIG_RAMBASE, (CONFIG_RAMTOP) - CONFIG_RAMBASE - CONFIG_DCACHE_RAM_SIZE);
> +	clear_memory( CONFIG_RAMBASE, CONFIG_RAMTOP - CONFIG_RAMBASE - CONFIG_DCACHE_RAM_SIZE);
>  #else
> -        clear_memory(0, ((CONFIG_RAMTOP) - CONFIG_DCACHE_RAM_SIZE));
> +	/* Shouldn't we miss 0xa0000-0xc0000? */
> +        clear_memory(0, CONFIG_RAMTOP - CONFIG_DCACHE_RAM_SIZE);
>  #endif
>  }
>   
Do we need to clear that memory at all?

> -#if (CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE<0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1))
> -	/*
> -	 pgtbl is too big, so use last one 1M before CONFIG_LB_MEM_TOP, otherwise for 8 way dual core with vga support will push stack and heap cross 0xa0000, 
> -	 and that region need to be used as vga font buffer. Please make sure set CONFIG_RAMTOP=0x200000 in MB Config
> -	*/
> -	struct pg_table *pgtbl = (struct pg_table*)0x100000; //1M
> -
> -	unsigned x_end = 0x100000 + sizeof(struct pg_table) * CONFIG_MAX_CPUS;
> -#if (0x100000+20480*CONFIG_MAX_CPUS) > (CONFIG_RAMTOP)
> -                #warning "We may need to increase CONFIG_RAMTOP, it need to be more than (0x100000+20480*CONFIG_MAX_CPUS)\n"
> +/* Putting 20K of page tables on the stack, so check size */
> +#if CONFIG_STACK_SIZE < 32*1024
> +	#error "map_2M_page requires CONFIG_STACK_SIZE >= 32K"
>  #endif
>   
Do we need them on the stack? If we can put the lzma buffer and page
tables "somewhere else" we can considerably reduce our stack usage (I
guess 4k would be enough for all boards... always keep in mind that this
is per cpu. Or should we attempt to have one CPU with a big stack and
the others with smaller stacks?
Would this simplify things?

> Index: cbv2/src/mainboard/amd/serengeti_cheetah/apc_auto.c
> ===================================================================
> --- cbv2.orig/src/mainboard/amd/serengeti_cheetah/apc_auto.c
> +++ cbv2/src/mainboard/amd/serengeti_cheetah/apc_auto.c
> @@ -75,6 +75,7 @@ static inline unsigned get_nodes(void)
>  void hardwaremain(int ret_addr)
>  {
>  	struct sys_info *sysinfo = (CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE - CONFIG_DCACHE_RAM_GLOBAL_VAR_SIZE); // in CACHE
> +	/* How do we deal with this? */
>          struct sys_info *sysinfox = ((CONFIG_RAMTOP) - CONFIG_DCACHE_RAM_GLOBAL_VAR_SIZE); // in RAM
>  
>  	struct node_core_id id;
>   
How should we? :-)


> Index: cbv2/src/mainboard/asus/m2v-mx_se/mainboard.c
> ===================================================================
> --- cbv2.orig/src/mainboard/asus/m2v-mx_se/mainboard.c
> +++ cbv2/src/mainboard/asus/m2v-mx_se/mainboard.c
> @@ -41,6 +41,7 @@ int add_mainboard_resources(struct lb_me
>  #if CONFIG_HAVE_ACPI_RESUME == 1
>  	lb_add_memory_range(mem, LB_MEM_RESERVED,
>  		CONFIG_RAMBASE, ((CONFIG_RAMTOP) - CONFIG_RAMBASE));
> +	/* Is this really needed?  This region should never be written back. */
>  	lb_add_memory_range(mem, LB_MEM_RESERVED,
>  		CONFIG_DCACHE_RAM_BASE, CONFIG_DCACHE_RAM_SIZE);
>   
I don't think it is needed. If it is, something sounds wrong.



Stefan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20100427/4e42f748/attachment.html>


More information about the coreboot mailing list