Submitter | Myles Watson |
---|---|
Date | 2010-02-26 14:14:18 |
Message ID | <2831fecf1002260614g38565a2amcfde229241798755@mail.gmail.com> |
Download | mbox | patch |
Permalink | /patch/982/ |
State | Not Applicable |
Headers | show |
Comments
On Fri, Feb 26, 2010 at 7:37 AM, Patrick Georgi <patrick@georgi-clan.de>wrote: > 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.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
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
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
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
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
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
Patch
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