Rev 5132 works for me on SimNOW with 1 fam10 processor.
Current head gets stuck in an infinite loop setting fixed MTRRs.
Any clues while I'm bisecting?
Thanks, Myles
On Wed, Feb 24, 2010 at 2:27 PM, Myles Watson mylesgw@gmail.com wrote:
Rev 5132 works for me on SimNOW with 1 fam10 processor.
Current head gets stuck in an infinite loop setting fixed MTRRs.
Any clues while I'm bisecting?
So far the only difference before the failure is the location of the CBFS header: Check CBFS header at fffeffe0 (working) Check CBFS header at ffffff68a (broken)
Thanks, Myles
On Wed, Feb 24, 2010 at 2:34 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Feb 24, 2010 at 2:27 PM, Myles Watson mylesgw@gmail.com wrote:
Rev 5132 works for me on SimNOW with 1 fam10 processor.
Current head gets stuck in an infinite loop setting fixed MTRRs.
Any clues while I'm bisecting?
So far the only difference before the failure is the location of the CBFS header: Check CBFS header at fffeffe0 (working) Check CBFS header at ffffff68a (broken)
That's not it, because that changes later.
Rev 5135 works Rev 5139 is broken.
Rev 5136-5138 don't build. Rev 5136 is pretty big, and I don't see anything that's obviously related to MTRRs.
Thanks, Myles
On Wed, Feb 24, 2010 at 3:02 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Feb 24, 2010 at 2:34 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Feb 24, 2010 at 2:27 PM, Myles Watson mylesgw@gmail.com wrote:
Rev 5132 works for me on SimNOW with 1 fam10 processor.
Current head gets stuck in an infinite loop setting fixed MTRRs.
Any clues while I'm bisecting?
So far the only difference before the failure is the location of the CBFS header: Check CBFS header at fffeffe0 (working) Check CBFS header at ffffff68a (broken)
That's not it, because that changes later.
Rev 5135 works Rev 5139 is broken.
Rev 5136-5138 don't build. Rev 5136 is pretty big, and I don't see anything that's obviously related to MTRRs.
Interestingly enough, 5150 fixes it. Then 5152 breaks it a different way.
Since the commit message from 5152 is:
Remove nonsensical wrapper for function in PS/2 keyboard API.
I guess it's probably some kind of stack corruption. Something makes it very fragile.
Suggestions welcome.
Thanks, Myles
I am pretty sure that you meet the same the same problem.
It seems to be ridiculous. Please check the maillist about this issue.
http://www.coreboot.org/pipermail/coreboot/2010-February/055730.html
If everything goes as I expected, the following patch can fix your problem
at any revision.
Zheng
Index: src/arch/i386/coreboot_ram.ld
===================================================================
--- src/arch/i386/coreboot_ram.ld (revision 5153)
+++ src/arch/i386/coreboot_ram.ld (working copy)
@@ -1,7 +1,7 @@
/*
* Memory map:
*
- * CONFIG_RAMBASE
+ * CONFIG_RAMBASE
* : data segment
* : bss segment
* : heap
@@ -19,7 +19,7 @@
*/
/*
* We use ELF as output format. So that we can
- * debug the code in some form.
+ * debug the code in some form.
*/
INCLUDE ldoptions
@@ -62,7 +62,7 @@
. = ALIGN(4);
_erodata = .;
- }
+ }
/*
* After the code we place initialized data (typically initialized
* global variables). This gets copied into ram by startup code.
@@ -101,11 +101,10 @@
_end = .;
. = ALIGN(CONFIG_STACK_SIZE);
_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);
- }
+ /* 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*/
+ /* TODO: workaround for the bug of GNU linker. */
+ . += ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
_estack = .;
_heap = .;
.heap . : {
@@ -117,7 +116,7 @@
/* The ram segment
* This is all address of the memory resident copy of coreboot.
*/
- _ram_seg = _text;
+ _ram_seg = _text;
_eram_seg = _eheap;
_bogus = ASSERT( ( _eram_seg < (CONFIG_RAMTOP)) , "please increase CONFIG_RAMTOP");
________________________________
From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Myles Watson Sent: Thursday, February 25, 2010 6:48 AM To: coreboot Subject: Re: [coreboot] Fam10 breakage
On Wed, Feb 24, 2010 at 3:02 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Feb 24, 2010 at 2:34 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Feb 24, 2010 at 2:27 PM, Myles Watson mylesgw@gmail.com wrote:
Rev 5132 works for me on SimNOW with 1 fam10 processor.
Current head gets stuck in an infinite loop setting fixed MTRRs.
Any clues while I'm bisecting?
So far the only difference before the failure is the location of the CBFS header: Check CBFS header at fffeffe0 (working) Check CBFS header at ffffff68a (broken)
That's not it, because that changes later.
Rev 5135 works Rev 5139 is broken.
Rev 5136-5138 don't build. Rev 5136 is pretty big, and I don't see anything that's obviously related to MTRRs.
Interestingly enough, 5150 fixes it. Then 5152 breaks it a different way.
Since the commit message from 5152 is:
Remove nonsensical wrapper for function in PS/2 keyboard API.
I guess it's probably some kind of stack corruption. Something makes it very fragile.
Suggestions welcome.
Thanks, Myles
Am 25.02.2010 02:46, schrieb Bao, Zheng:
I am pretty sure that you meet the same the same problem.
It seems to be ridiculous. Please check the maillist about this issue.
http://www.coreboot.org/pipermail/coreboot/2010-February/055730.html
If everything goes as I expected, the following patch can fix your problem
at any revision.
Just a data point: amd/serengeti_cheetah_fam10 works for me in SimNow without any changes. (using crossgcc as toolchain)
Patrick
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Patrick Georgi Sent: Thursday, February 25, 2010 12:58 AM To: coreboot@coreboot.org Subject: Re: [coreboot] Fam10 breakage
Am 25.02.2010 02:46, schrieb Bao, Zheng:
I am pretty sure that you meet the same the same problem.
It seems to be ridiculous. Please check the maillist about this issue.
http://www.coreboot.org/pipermail/coreboot/2010-February/055730.html
If everything goes as I expected, the following patch can fix your
problem
at any revision.
Just a data point: amd/serengeti_cheetah_fam10 works for me in SimNow without any changes. (using crossgcc as toolchain)
I'm using crossgcc. The fact that it breaks and fixes itself depending on seemingly unrelated changes means that it's broken and we're lucky when it works.
Thanks, Myles
Am 25.02.2010 15:38, schrieb Myles Watson:
Just a data point: amd/serengeti_cheetah_fam10 works for me in SimNow without any changes. (using crossgcc as toolchain)
I'm using crossgcc. The fact that it breaks and fixes itself depending on seemingly unrelated changes means that it's broken and we're lucky when it works.
Hmm.. thinking about it: My SimNow config was with one core. Mr. Bao's fix seems to increase the stack size, so other cores was space to work in. So it would even make sense... somewhat...
Patrick
On Wed, Feb 24, 2010 at 6:46 PM, Bao, Zheng Zheng.Bao@amd.com wrote:
I am pretty sure that you meet the same the same problem.
It seems to be ridiculous. Please check the maillist about this issue.
http://www.coreboot.org/pipermail/coreboot/2010-February/055730.html
If everything goes as I expected, the following patch can fix your problem
It fixes it for me, and fixes the APIC enumeration problem that Timothy Pearson was seeing. I hadn't seen it before, but now that it's fixed I see a line like this:
Unknown device path type: 0
So the device tree was getting corrupted.
I'd like to understand better why it fixes it.
Zheng: Could you commit the whitespace changes and then submit the patch to make it clearer?
Thanks, Myles
I'd like to understand better why it fixes it.
diff -urN 5152/coreboot_ram.map build/coreboot_ram.map --- 5152/coreboot_ram.map 2010-02-24 16:05:19.000000000 -0700 +++ build/coreboot_ram.map 2010-02-25 08:11:33.000000000 -0700 @@ -924,10 +924,10 @@ 002284a0 A _ebss 002284a0 A _end 00230000 A _stack -00240000 A _estack -00240000 A _heap -00300000 A _eheap -00300000 A _eram_seg +00530000 A _estack +00530000 A _heap +005f0000 A _eheap +005f0000 A _eram_seg 01000000 A CONFIG_RAMTOP 04000000 A CONFIG_AGP_APERTURE_SIZE fff80000 A CONFIG_XIP_ROM_BASE
The stack size did increase quite a bit.
Thanks, Myles
It is about the bug in the gnu linker. It seems to be ridiculous.
It can be tested by a small program.
In the ld script, if the expression is located in a section, like
.stack . : {
EXPRESSION;
}
the symbol and constant can not be in the expression at the same time,
like:
. = CONFIG_STACK_SIZE * 2;
or:
CONFIG_RAMBASE<0x100000 ? xxxxx : yyyyy;
This bug leads to the fact that we can not allocate enough space of
stack for each core. Then the data in the memory will be covered by AP
stack which doesnt know anything.
That is the fact I can find. It is a bug in the linker. But we can not
fix each linker on coreboot developer, not to mention the fact I have
no idea how to submit the bug. The bug will be fixed in the
future. But when the fixed linker will come into our machine? So I
believe we need the workaround patch to avoid this problem.
Signed-off-by: Zheng Bao zheng.bao@amd.com
Index: src/arch/i386/coreboot_ram.ld
===================================================================
--- src/arch/i386/coreboot_ram.ld (revision 5165)
+++ src/arch/i386/coreboot_ram.ld (working copy)
@@ -101,11 +101,11 @@
_end = .;
. = ALIGN(CONFIG_STACK_SIZE);
_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);
- }
+ /* 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*/
+ /* TODO: workaround for the bug of GNU linker. When the bug is fixed
+ * on almost every machine, Please change it back */
+ . += ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
_estack = .;
_heap = .;
.heap . : {
________________________________
From: Myles Watson [mailto:mylesgw@gmail.com] Sent: Thursday, February 25, 2010 11:11 PM To: Bao, Zheng; Timothy Pearson Cc: coreboot Subject: Re: [coreboot] Fam10 breakage
On Wed, Feb 24, 2010 at 6:46 PM, Bao, Zheng Zheng.Bao@amd.com wrote:
I am pretty sure that you meet the same the same problem.
It seems to be ridiculous. Please check the maillist about this issue.
http://www.coreboot.org/pipermail/coreboot/2010-February/055730.html
If everything goes as I expected, the following patch can fix your problem
It fixes it for me, and fixes the APIC enumeration problem that Timothy Pearson was seeing. I hadn't seen it before, but now that it's fixed I see a line like this:
Unknown device path type: 0
So the device tree was getting corrupted.
I'd like to understand better why it fixes it.
Zheng: Could you commit the whitespace changes and then submit the patch to make it clearer?
Thanks, Myles
Am 26.02.2010 03:48, schrieb Bao, Zheng:
This bug leads to the fact that we can not allocate enough space of stack for each core. Then the data in the memory will be covered by AP stack which doesnt know anything.
That is the fact I can find. It is a bug in the linker. But we can not fix each linker on coreboot developer, not to mention the fact I have no idea how to submit the bug. The bug will be fixed in the future. But when the fixed linker will come into our machine? So I believe we need the workaround patch to avoid this problem.
Just to make sure what the patch does: It simply removes the separate section and reserves an area at the same place, of the same size, just in a way that works with the linker?
If so,
Signed-off-by: Zheng Bao zheng.bao@amd.com
Acked-by: Patrick Georgi patrick.georgi@coresystems.de
We can still sort out if we want to keep that (nonsensical, in my opinion) behaviour of deciding the stack size based on the use of PCI option roms and various address layout issues.
Regards, Patrick Georgi
On Fri, Feb 26, 2010 at 5:56 AM, Patrick Georgi patrick@georgi-clan.dewrote:
Am 26.02.2010 03:48, schrieb Bao, Zheng:
This bug leads to the fact that we can not allocate enough space of stack for each core. Then the data in the memory will be covered by AP stack which doesnt know anything.
That is the fact I can find. It is a bug in the linker. But we can not fix each linker on coreboot developer, not to mention the fact I have no idea how to submit the bug. The bug will be fixed in the future. But when the fixed linker will come into our machine? So I believe we need the workaround patch to avoid this problem.
Just to make sure what the patch does: It simply removes the separate section and reserves an area at the same place, of the same size, just in a way that works with the linker?
I would like to double check this before it gets committed. I only tried it once, and the difference was very large. 3M of stack doesn't seem right.
diff -urN 5152/coreboot_ram.map build/coreboot_ram.map --- 5152/coreboot_ram.map 2010-02-24 16:05:19.000000000 -0700 +++ build/coreboot_ram.map 2010-02-25 08:11:33.000000000 -0700 @@ -924,10 +924,10 @@ 002284a0 A _ebss 002284a0 A _end 00230000 A _stack -00240000 A _estack -00240000 A _heap -00300000 A _eheap -00300000 A _eram_seg +00530000 A _estack +00530000 A _heap +005f0000 A _eheap +005f0000 A _eram_seg 01000000 A CONFIG_RAMTOP 04000000 A CONFIG_AGP_APERTURE_SIZE fff80000 A CONFIG_XIP_ROM_BASE
Thanks, Myles
Am 26.02.2010 15:14, schrieb Myles Watson:
I would like to double check this before it gets committed. I only tried it once, and the difference was very large. 3M of stack doesn't seem right.
Those 3M are CONFIG_MAX_CPUS*CONFIG_STACK_SIZE, right?
I think there is some code that assigns a local stack area for each CPU, the 3M aren't meant for a single instance of code running (which would indeed be huge).
Patrick
On Fri, Feb 26, 2010 at 7:37 AM, Patrick Georgi patrick@georgi-clan.dewrote:
Am 26.02.2010 15:14, schrieb Myles Watson:
I would like to double check this before it gets committed. I only tried it once, and the difference was very large. 3M of stack doesn't seem right.
Those 3M are CONFIG_MAX_CPUS*CONFIG_STACK_SIZE, right?
Yep. I didn't realize that CONFIG_MAX_CPUS was 48.
I think there is some code that assigns a local stack area for each CPU, the 3M aren't meant for a single instance of code running (which would indeed be huge).
For me, the only change that needs to be made is:
- . = ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
+ . += ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
Removing the .stack construct makes no difference.
I like the idea of minimizing the change.
Thanks,
Myles
Am 26.02.2010 16:35, schrieb Myles Watson:
For me, the only change that needs to be made is:
. = ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
. += ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
Removing the .stack construct makes no difference.
I like the idea of minimizing the change.
Sounds good, and should be stable (unless that's part of the bug Zheng Bao is experiencing).
I'd say, commit this (as it fixes things for you). If it's not enough, we can do the full change.
Acked-by: Patrick Georgi patrick.georgi@coresystems.de
On Fri, Feb 26, 2010 at 1:24 PM, Patrick Georgi patrick@georgi-clan.dewrote:
Am 26.02.2010 16:35, schrieb Myles Watson:
For me, the only change that needs to be made is:
. = ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
. += ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
Removing the .stack construct makes no difference.
I like the idea of minimizing the change.
Sounds good, and should be stable (unless that's part of the bug Zheng Bao is experiencing).
I'd say, commit this (as it fixes things for you). If it's not enough, we can do the full change.
Great.
Acked-by: Patrick Georgi patrick.georgi@coresystems.de
Rev 5166.
Thanks, Myles
Unfortunately, it doesn't fix my problem here. It is the coreboot_ram.map
of serengeti_cheetah_fam10 built on my machine. The _estack is not
what we expect. Myles, what is the result at you machine?
00000030 A CONFIG_MAX_CPUS ............... 00002000 A CONFIG_STACK_SIZE ............... 00228550 A _ebss 00228550 A _end 0022a000 A _stack 0022c000 A _estack 0022c000 A _heap 002ec000 A _eheap 002ec000 A _eram_seg 01000000 A CONFIG_RAMTOP 04000000 A CONFIG_AGP_APERTURE_SIZE fff80000 A CONFIG_XIP_ROM_BASE ffff0000 A CONFIG_ROMBASE
Zheng
Date: Fri, 26 Feb 2010 13:32:49 -0700 From: mylesgw@gmail.com To: patrick@georgi-clan.de CC: coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
On Fri, Feb 26, 2010 at 1:24 PM, Patrick Georgi patrick@georgi-clan.de wrote:
Am 26.02.2010 16:35, schrieb Myles Watson:
For me, the only change that needs to be made is:
. = ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
. += ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
Removing the .stack construct makes no difference.
I like the idea of minimizing the change.
Sounds good, and should be stable (unless that's part of the bug Zheng Bao is experiencing).
I'd say, commit this (as it fixes things for you). If it's not enough, we can do the full change.
Great.
Acked-by: Patrick Georgi patrick.georgi@coresystems.de Rev 5166.
Thanks, Myles _________________________________________________________________ Hotmail: Trusted email with powerful SPAM protection. https://signup.live.com/signup.aspx?id=60969
2010/2/27 Zheng Bao fishbaoz@hotmail.com
Unfortunately, it doesn't fix my problem here. It is the coreboot_ram.map of serengeti_cheetah_fam10 built on my machine. The _estack is not what we expect. Myles, what is the result at you machine?
002284a0 A _ebss 002284a0 A _end 00230000 A _stack 00530000 A _estack 00530000 A _heap 005f0000 A _eheap 005f0000 A _eram_seg
So it worked for me.
I think we should just get rid of the complexity. I don't think there are any boards with lots of APs and huge stack needs that start below 1M, so the check that is breaking for you doesn't help anyone.
Thanks, Myles
It doesn't matter whether it is a huge stack. The _estack - _stack should be MAX_CPUS * STACK_SIZE. The MAX_CPU could be set as 4, and STACK_SIZE could be 0x2000. So the stack is only 0x8000. Is it huge? But checking my coreboot_ram.map, _estack - _stack is only ONE STACK_SIZE. But the loader has no idea about that. The stack will overlap other section of data. I believe many other build machine will produce the same result with me.
I have tried compiling with the reference cross compiler from util/crossgcc, and it make no difference.
Zheng
________________________________________ From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Myles Watson Sent: Sunday, February 28, 2010 12:41 PM To: Zheng Bao Cc: coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
2010/2/27 Zheng Bao fishbaoz@hotmail.com Unfortunately, it doesn't fix my problem here. It is the coreboot_ram.map of serengeti_cheetah_fam10 built on my machine. The _estack is not what we expect. Myles, what is the result at you machine? 002284a0 A _ebss 002284a0 A _end 00230000 A _stack 00530000 A _estack 00530000 A _heap 005f0000 A _eheap 005f0000 A _eram_seg
So it worked for me.
I think we should just get rid of the complexity. I don't think there are any boards with lots of APs and huge stack needs that start below 1M, so the check that is breaking for you doesn't help anyone.
Thanks, Myles
Am 01.03.2010 06:42, schrieb Bao, Zheng:
It doesn't matter whether it is a huge stack. The _estack - _stack should be MAX_CPUS * STACK_SIZE. The MAX_CPU could be set as 4, and STACK_SIZE could be 0x2000. So the stack is only 0x8000. Is it huge? But checking my coreboot_ram.map, _estack - _stack is only ONE STACK_SIZE. But the loader has no idea about that. The stack will overlap other section of data. I believe many other build machine will produce the same result with me.
The main issue I see is that weird rule to decide whether to use one stack or many - it doesn't really make sense.
The only place where multiple stacks seem to be setup is src/cpu/x86/lapic/lapic_cpu_init.c (look for estack in that file). That place really indicates that the ldscript rule does the wrong thing: No matter if that weird conditional holds true, multiple stacks are set up, just in different ways, with special behaviour if the stacks are split between <1MB and >1MB memory.
So I can only support Myles' proposal to make the linker script line . += CONFIG_MAX_CPUS * CONFIG_STACK_SIZE without any cleverness. I believe that the line as it is in the linker script is wrong. From looking at the behaviour in the mentioned C file, it should decide between MAX_CPUS * STACK_SIZE and some more complex rule that creates a stack at >1MB. Something like:
(I'm not proposing to add this to the linker script!) . += ((CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE < 0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1)))?(0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS - (CONFIG_STACK_SIZE*index) - .):(CONFIG_STACK_SIZE*CONFIG_MAX_CPUS);
There are a couple of boards with RAMBASE<1MB and RAMTOP >1MB. (amd_db800, amd_norwich, artecgroup_dbe61, bcom_winnetp680, digitallogic_msm800sev, iei_pcisa-lx-800-r10, jetway_j7f24, lippert_roadrunner-lx, lippert_spacerunner-lx, pcengines_alix1c, technexion_tim5690, via_epia-cn, via_epia, via_epia-m700, via_epia-m, via_epia-n, via_pc2500e, via_vt8454c, winent_pl6064)
Only one of them actually has MAX_CPUS > 1, technexion/tim5690, so once that one is changed to have RAMBASE >= 1MB, we could drop this special rule without side effect (except for avoiding a linker bug and being able to simplify the code)
Patrick
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.
Zheng
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Patrick Georgi Sent: Monday, March 01, 2010 3:39 PM To: coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
Am 01.03.2010 06:42, schrieb Bao, Zheng:
It doesn't matter whether it is a huge stack. The _estack - _stack should be MAX_CPUS * STACK_SIZE. The MAX_CPU could be set as 4, and STACK_SIZE could be 0x2000. So the stack is only 0x8000. Is it huge? But checking my coreboot_ram.map, _estack - _stack is only ONE STACK_SIZE. But the loader has no idea about that. The stack will overlap other section of data. I believe many other build machine will produce the same result with me.
The main issue I see is that weird rule to decide whether to use one stack or many - it doesn't really make sense.
The only place where multiple stacks seem to be setup is src/cpu/x86/lapic/lapic_cpu_init.c (look for estack in that file). That place really indicates that the ldscript rule does the wrong thing: No matter if that weird conditional holds true, multiple stacks are set up, just in different ways, with special behaviour if the stacks are split between <1MB and >1MB memory.
So I can only support Myles' proposal to make the linker script line . += CONFIG_MAX_CPUS * CONFIG_STACK_SIZE without any cleverness. I believe that the line as it is in the linker script is wrong. From looking at the behaviour in the mentioned C file, it should decide between MAX_CPUS * STACK_SIZE and some more complex rule that creates a stack at >1MB. Something like:
(I'm not proposing to add this to the linker script!) . += ((CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE < 0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1)))?(0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS - (CONFIG_STACK_SIZE*index) - .):(CONFIG_STACK_SIZE*CONFIG_MAX_CPUS);
There are a couple of boards with RAMBASE<1MB and RAMTOP >1MB. (amd_db800, amd_norwich, artecgroup_dbe61, bcom_winnetp680, digitallogic_msm800sev, iei_pcisa-lx-800-r10, jetway_j7f24, lippert_roadrunner-lx, lippert_spacerunner-lx, pcengines_alix1c, technexion_tim5690, via_epia-cn, via_epia, via_epia-m700, via_epia-m, via_epia-n, via_pc2500e, via_vt8454c, winent_pl6064)
Only one of them actually has MAX_CPUS > 1, technexion/tim5690, so once that one is changed to have RAMBASE >= 1MB, we could drop this special rule without side effect (except for avoiding a linker bug and being able to simplify the code)
Patrick
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.
By now, I'm quite certain that the rule is wrong. Binutils bug or not.
What I worry is the bug in the crosstool will make the rule do the wrong thing, even if the rule is perfect.
Is that only in crossgcc or also in other binutils (eg. by distributions)?
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.
If it's a bug (and yes, it definitely looks like that), we can report it to upstream. That does not (in itself) fix broken boards.
Patrick
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
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
Am 01.03.2010 17:23, schrieb Myles Watson:
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.
True. Attached patch might do this (only moderately tested)
I think the only reason why we can't get rid of RAMBASE <1M completely is a couple of boards (Via based iirc) that have their own vgabios.c that breaks with RAMBASE >1M
The other RAMBASE we sometimes use (mostly on AMD boards) is RAMBASE==2M - what was the rationale for that again?
With those two gone, we could hide RAMBASE somewhere in Kconfig or eliminate it completely.
Patrick
Am 01.03.2010 17:23, schrieb Myles Watson:
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.
True. Attached patch might do this (only moderately tested)
Acked-by: Myles Watson mylesgw@gmail.com
I think the only reason why we can't get rid of RAMBASE <1M completely is a couple of boards (Via based iirc) that have their own vgabios.c that breaks with RAMBASE >1M
The other RAMBASE we sometimes use (mostly on AMD boards) is RAMBASE==2M
- what was the rationale for that again?
I don't know. I can't see a reason for it.
With those two gone, we could hide RAMBASE somewhere in Kconfig or eliminate it completely.
Wasn't there a board with RAMBASE at 32 M too?
Thanks, Myles
Am 01.03.2010 17:54, schrieb Myles Watson:
that checks for RAMBASE < 0xa0000 and eheap > 0xa0000? One large stack could just as easily break this.
True. Attached patch might do this (only moderately tested)
Acked-by: Myles Watson mylesgw@gmail.com
Thanks, r5181
With those two gone, we could hide RAMBASE somewhere in Kconfig or eliminate it completely.
Wasn't there a board with RAMBASE at 32 M too?
Might be Rudolf's, to help with suspend/resume.
Patrick
On Mon, Mar 1, 2010 at 9:48 AM, Patrick Georgi patrick@georgi-clan.de wrote:
Am 01.03.2010 17:23, schrieb Myles Watson:
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.
True. Attached patch might do this (only moderately tested)
I think the only reason why we can't get rid of RAMBASE <1M completely is a couple of boards (Via based iirc) that have their own vgabios.c that breaks with RAMBASE >1M
The other RAMBASE we sometimes use (mostly on AMD boards) is RAMBASE==2M
- what was the rationale for that again?
With those two gone, we could hide RAMBASE somewhere in Kconfig or eliminate it completely.
I'm coming to this discussion a bit late, but here is what I recall. Maybe someone else can confirm this?
Each core needs a stack large enough for the sysinfo structure and its own call stack. Stacks space was assigned starting at 0xC8000 and size of 0x2000 was enough pre-cbfs. When we switched to cbfs and lzma, the stack requirement went to 0x8000. I'm not positive since things have moved around, but I think that RAMBASE set to 2M is to leave room for the nodes CAR stacks. With the smaller 0x2000 stack 28 cores could be supported. Although I don't know any machines with that many cores, that isn't the max possible 32 ( 8cpus x 4cores )( I'm not sure where the 48 came from unless someone is already trying to support 6 core cpus?). So, RAMBASE was moved to 2M. This is more important with stacks of 0x8000 for each core as only 7 cores could be supported below 1M.
Now, does RAMBASE really need to be affected by the CAR stacks? I don't think so since the BSP does the decompress and the move after memory init and all the APs are halted. Also, how many stacks do we really need? I think that core0 for each node is the only one that must run to do HT and memory init on the node before coreboot_ram is run. The other cores can be setup later.
I think Patrick and Myles mentioned that we need to understand the bootblock while each cpu core initializing and walking the cbfs while not stepping on each others stacks. Has that been resolved?
I hope this made some sense and helps understand the problem.
Marc
-----Original Message----- From: Marc Jones [mailto:marcj303@gmail.com] Sent: Monday, March 01, 2010 12:00 PM To: Patrick Georgi Cc: Myles Watson; coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
On Mon, Mar 1, 2010 at 9:48 AM, Patrick Georgi patrick@georgi-clan.de wrote:
Am 01.03.2010 17:23, schrieb Myles Watson:
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.
True. Attached patch might do this (only moderately tested)
I think the only reason why we can't get rid of RAMBASE <1M completely is a couple of boards (Via based iirc) that have their own vgabios.c that breaks with RAMBASE >1M
The other RAMBASE we sometimes use (mostly on AMD boards) is RAMBASE==2M
- what was the rationale for that again?
With those two gone, we could hide RAMBASE somewhere in Kconfig or eliminate it completely.
I'm coming to this discussion a bit late, but here is what I recall. Maybe someone else can confirm this?
Each core needs a stack large enough for the sysinfo structure and its own call stack. Stacks space was assigned starting at 0xC8000
This looks like a CAR address. Most of the boards have RAMBASE 1M now, right?
size of 0x2000 was enough pre-cbfs. When we switched to cbfs and lzma, the stack requirement went to 0x8000. I'm not positive since things have moved around, but I think that RAMBASE set to 2M is to leave room for the nodes CAR stacks.
Shouldn't CAR stacks be below 1M?
With the smaller 0x2000 stack 28 cores could be supported. Although I don't know any machines with that many cores, that isn't the max possible 32 ( 8cpus x 4cores )( I'm not sure where the 48 came from unless someone is already trying to support 6 core cpus?).
Yep.
So, RAMBASE was moved to 2M. This is more important with stacks of 0x8000 for each core as only 7 cores could be supported below 1M.
Now, does RAMBASE really need to be affected by the CAR stacks? I don't think so since the BSP does the decompress and the move after memory init and all the APs are halted. Also, how many stacks do we really need? I think that core0 for each node is the only one that must run to do HT and memory init on the node before coreboot_ram is run.
Do all the core0 processors have to do init? I thought HT and memory init was all done by BSP core0.
Maybe we should add specific memory areas for lzma and page tables so we can go back to having smaller stacks. Otherwise, maybe we should have two sizes of stacks.
Thanks, Myles
On Mon, Mar 1, 2010 at 12:32 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: Marc Jones [mailto:marcj303@gmail.com] Sent: Monday, March 01, 2010 12:00 PM To: Patrick Georgi Cc: Myles Watson; coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
On Mon, Mar 1, 2010 at 9:48 AM, Patrick Georgi patrick@georgi-clan.de wrote:
Am 01.03.2010 17:23, schrieb Myles Watson:
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.
True. Attached patch might do this (only moderately tested)
I think the only reason why we can't get rid of RAMBASE <1M completely is a couple of boards (Via based iirc) that have their own vgabios.c that breaks with RAMBASE >1M
The other RAMBASE we sometimes use (mostly on AMD boards) is RAMBASE==2M
- what was the rationale for that again?
With those two gone, we could hide RAMBASE somewhere in Kconfig or eliminate it completely.
I'm coming to this discussion a bit late, but here is what I recall. Maybe someone else can confirm this?
Each core needs a stack large enough for the sysinfo structure and its own call stack. Stacks space was assigned starting at 0xC8000
This looks like a CAR address. Most of the boards have RAMBASE 1M now, right?
size of 0x2000 was enough pre-cbfs. When we switched to cbfs and lzma, the stack requirement went to 0x8000. I'm not positive since things have moved around, but I think that RAMBASE set to 2M is to leave room for the nodes CAR stacks.
Shouldn't CAR stacks be below 1M?
With the smaller 0x2000 stack 28 cores could be supported. Although I don't know any machines with that many cores, that isn't the max possible 32 ( 8cpus x 4cores )( I'm not sure where the 48 came from unless someone is already trying to support 6 core cpus?).
Yep.
So, RAMBASE was moved to 2M. This is more important with stacks of 0x8000 for each core as only 7 cores could be supported below 1M.
Now, does RAMBASE really need to be affected by the CAR stacks? I don't think so since the BSP does the decompress and the move after memory init and all the APs are halted. Also, how many stacks do we really need? I think that core0 for each node is the only one that must run to do HT and memory init on the node before coreboot_ram is run.
Do all the core0 processors have to do init? I thought HT and memory init was all done by BSP core0.
Maybe we should add specific memory areas for lzma and page tables so we can go back to having smaller stacks. Otherwise, maybe we should have two sizes of stacks.
I had to go look at the code to remeber the details. Each core does fid/vid setup before HT reset. You are correct, the BSP handles the memory init. I was thinking on msrs setup but that happens later in coreboot_ram (each core has to write some of its own msrs,TOM, TOM2 at least).
You can see when core0 and coreN are started in romstage.c. Every core runs init_cpus() which does different things depending on BSP, core0, and coreN. It looks like each core sets lapic and fid/vid. All cores on a cpu will have the same fid/vid capability, so only core0 should have to be setup, but currently every core is getting fid/vid setup (#define FAM10_SET_FIDVID_CORE0_ONLY 0). I think the complexity comes from what could be done and what is working today.
Marc
Have you tried compiling with the reference cross compiler from util/ crossgcc?
Stefan
On 28.02.2010, at 04:52, Zheng Bao fishbaoz@hotmail.com wrote:
Unfortunately, it doesn't fix my problem here. It is the coreboot_ram.map of serengeti_cheetah_fam10 built on my machine. The _estack is not what we expect. Myles, what is the result at you machine?
00000030 A CONFIG_MAX_CPUS ............... 00002000 A CONFIG_STACK_SIZE ............... 00228550 A _ebss 00228550 A _end 0022a000 A _stack 0022c000 A _estack 0022c000 A _heap 002ec000 A _eheap 002ec000 A _eram_seg 01000000 A CONFIG_RAMTOP 04000000 A CONFIG_AGP_APERTURE_SIZE fff80000 A CONFIG_XIP_ROM_BASE ffff0000 A CONFIG_ROMBASE
Zheng
Date: Fri, 26 Feb 2010 13:32:49 -0700 From: mylesgw@gmail.com To: patrick@georgi-clan.de CC: coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
On Fri, Feb 26, 2010 at 1:24 PM, Patrick Georgi <patrick@georgi-clan.de
wrote:
Am 26.02.2010 16:35, schrieb Myles Watson:
For me, the only change that needs to be made is:
. = ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&
(CONFIG_RAMTOP>0x100000)
) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
. += ((CONFIG_CONSOLE_VGA ||CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&
(CONFIG_RAMTOP>0x100000)
) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
Removing the .stack construct makes no difference.
I like the idea of minimizing the change.
Sounds good, and should be stable (unless that's part of the bug Zheng Bao is experiencing).
I'd say, commit this (as it fixes things for you). If it's not enough, we can do the full change. Great.
Acked-by: Patrick Georgi patrick.georgi@coresystems.de Rev 5166.
Thanks, Myles
Hotmail: Trusted email with powerful SPAM protection. Sign up now.
coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
I have tried. But it makes no difference. 00000030 A CONFIG_MAX_CPUS ............... 00008000 A CONFIG_STACK_SIZE ............... 00228500 A _ebss 00228500 A _end 00230000 A _stack 00238000 A _estack 00238000 A _heap 002f8000 A _eheap 002f8000 A _eram_seg
Zheng
________________________________________ From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Stefan Reinauer Sent: Sunday, February 28, 2010 5:27 PM To: Zheng Bao Cc: coreboot@coreboot.org; mylesgw@gmail.com Subject: Re: [coreboot] [patch] RE: Fam10 breakage
Have you tried compiling with the reference cross compiler from util/crossgcc?
Stefan
On 28.02.2010, at 04:52, Zheng Bao fishbaoz@hotmail.com wrote: Unfortunately, it doesn't fix my problem here. It is the coreboot_ram.map of serengeti_cheetah_fam10 built on my machine. The _estack is not what we expect. Myles, what is the result at you machine? 00000030 A CONFIG_MAX_CPUS ............... 00002000 A CONFIG_STACK_SIZE ............... 00228550 A _ebss 00228550 A _end 0022a000 A _stack 0022c000 A _estack 0022c000 A _heap 002ec000 A _eheap 002ec000 A _eram_seg 01000000 A CONFIG_RAMTOP 04000000 A CONFIG_AGP_APERTURE_SIZE fff80000 A CONFIG_XIP_ROM_BASE ffff0000 A CONFIG_ROMBASE
Zheng ________________________________________ Date: Fri, 26 Feb 2010 13:32:49 -0700 From: mylesgw@gmail.com To: patrick@georgi-clan.de CC: coreboot@coreboot.org Subject: Re: [coreboot] [patch] RE: Fam10 breakage
On Fri, Feb 26, 2010 at 1:24 PM, Patrick Georgi patrick@georgi-clan.de wrote: Am 26.02.2010 16:35, schrieb Myles Watson:
For me, the only change that needs to be made is:
- . = ((CONFIG_CONSOLE_VGA ||
CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
- . += ((CONFIG_CONSOLE_VGA ||
CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
Removing the .stack construct makes no difference.
I like the idea of minimizing the change.
Sounds good, and should be stable (unless that's part of the bug Zheng Bao is experiencing).
I'd say, commit this (as it fixes things for you). If it's not enough, we can do the full change. Great. Acked-by: Patrick Georgi patrick.georgi@coresystems.de Rev 5166.
Thanks, Myles ________________________________________ Hotmail: Trusted email with powerful SPAM protection. Sign up now.
-
Bao, Zheng -
Marc Jones -
Myles Watson -
Patrick Georgi -
Stefan Reinauer -
Zheng Bao