From patchwork Mon Mar 1 11:47:57 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: RE: Fam10 breakage Date: Mon, 01 Mar 2010 11:47:57 -0000 From: Patrick Georgi X-Patchwork-Id: 997 Message-Id: <4B8BA96D.3060505@georgi-clan.de> To: "Bao, Zheng" Cc: coreboot@coreboot.org Am 01.03.2010 09:00, schrieb Bao, Zheng: > What I keep trying to make everyone understand is not what the rules we > should use to decide the stack size. What I worry is the bug in the > crosstool will make the rule do the wrong thing, even if the rule is > perfect. So far, no one seems to support me that there is a bug in the > toolchain. I admit it seems ridiculous But the it is quite clear. I looked a bit into it. According to the binutils documentation, expressions (which always lead to addresses) are either relative or absolute. They're relative when inside some section, absolute otherwise. But most importantly, ld does not know about boolean values! What I could think of (and validate with some tests) is that ld really messes up boolean operations within sections. I didn't find anything conclusive yet, but it could be that boolean operations, even intermediate values are "relative", and then evaluated to absolute addresses - in which case both "true" and "false" are (very likely) non-null values. I'm also not sure if that's the intended behaviour by the binutils developers, but it's probably fragile as the logical operations are also used for address manipulations. I guess for clarification this should be asked on the binutils mailing list. Solution to that: Move operations on non-addresses and non-sizes out of sections (which you did in your patch) However, this does not fix the bug in our stack size calculation. I'm not quite sure if the patch does the right thing, but it should be close. Signed-off-by: Patrick Georgi Index: src/cpu/x86/lapic/lapic_cpu_init.c =================================================================== --- src/cpu/x86/lapic/lapic_cpu_init.c (revision 5180) +++ src/cpu/x86/lapic/lapic_cpu_init.c (working copy) @@ -246,26 +246,8 @@ 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(index<1) { // only keep bsp on low - stack_end = ((unsigned long)_estack) - (CONFIG_STACK_SIZE*index) - sizeof(struct cpu_info); - } 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" -#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"); - } - stack_end -= sizeof(struct cpu_info); - } -#else stack_end = ((unsigned long)_estack) - (CONFIG_STACK_SIZE*index) - sizeof(struct cpu_info); -#endif - /* Record the index and which cpu structure we are using */ info = (struct cpu_info *)stack_end; info->index = index; Index: src/mainboard/technexion/tim5690/Kconfig =================================================================== --- src/mainboard/technexion/tim5690/Kconfig (revision 5180) +++ src/mainboard/technexion/tim5690/Kconfig (working copy) @@ -127,5 +127,5 @@ config RAMBASE hex - default 0x4000 + default 0x100000 depends on BOARD_TECHNEXION_TIM5690 Index: src/arch/i386/coreboot_ram.ld =================================================================== --- src/arch/i386/coreboot_ram.ld (revision 5180) +++ src/arch/i386/coreboot_ram.ld (working copy) @@ -100,11 +100,14 @@ _ebss = .; _end = .; . = ALIGN(CONFIG_STACK_SIZE); + + /* We require RAMBASE to be at least 1MB for SMP, to avoid writing into a-seg - f-seg */ + _bogus = ASSERT(CONFIG_MAX_CPUS == 1 || CONFIG_RAMBASE >= 0x100000, "Please move RAMBASE to 1MB"); + _stack = .; .stack . : { /* Reserve a stack for each possible cpu */ - /* the stack for ap will be put after pgtbl in 1M to CONFIG_RAMTOP range when VGA and ROM_RUN and CONFIG_RAMTOP>1M*/ - . += ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE); + . += CONFIG_MAX_CPUS*CONFIG_STACK_SIZE; } _estack = .; _heap = .;