Patchwork RE: Fam10 breakage

login
register
about
Submitter Patrick Georgi
Date 2010-03-01 11:47:57
Message ID <4B8BA96D.3060505@georgi-clan.de>
Download mbox | patch
Permalink /patch/997/
State Superseded
Headers show

Comments

Patrick Georgi - 2010-03-01 11:47:57
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 <patrick.georgi@coresystems.de>
Myles Watson - 2010-03-01 16:23:18
> 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 don't think you should feel like it's a lack of support.  We just want to
fix it to make it simpler at the same time.
 
> 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.
I don't think we need to make the SMP check.  Can't we just put in an assert
that checks for RAMBASE < 0xa0000 and eheap > 0xa0000?  One large stack
could just as easily break this.

Thanks,
Myles

Patch

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 = .;