Patchwork RE: Fam10 breakage

login
register
about
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

Myles Watson - 2010-02-26 14:14:18
On Fri, Feb 26, 2010 at 5:56 AM, Patrick Georgi <patrick@georgi-clan.de>wrote:

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


Thanks,
Myles
Myles Watson - 2010-02-26 15:35:56
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
Patrick Georgi - 2010-02-26 20:24:42
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>
Myles Watson - 2010-02-26 20:32:49
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
Zheng Bao - 2010-02-28 03:52:03
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
Myles Watson - 2010-02-28 04:40:44
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
Stefan Reinauer - 2010-02-28 09:27:21
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
Bao, Zheng - 2010-03-01 05:35:26
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
Bao, Zheng - 2010-03-01 05:42:53
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
Patrick Georgi - 2010-03-01 07:39:17
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
Bao, Zheng - 2010-03-01 08:00:09
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
Patrick Georgi - 2010-03-01 08:11:23
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