| Submitter | Patrick Georgi |
|---|---|
| Date | 2010-03-01 16:48:04 |
| Message ID | <4B8BEFC4.1060802@georgi-clan.de> |
| Download | mbox | patch |
| Permalink | /patch/999/ |
| State | Accepted |
| Commit | r5181 |
| Headers | show |
Comments
> 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
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,11 @@ _ebss = .; _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); + . += CONFIG_MAX_CPUS*CONFIG_STACK_SIZE; } _estack = .; _heap = .; @@ -114,6 +114,10 @@ . = ALIGN(4); } _eheap = .; + + /* Avoid running into 0xa0000-0xfffff */ + _bogus = ASSERT(CONFIG_RAMBASE >= 0x100000 || _eheap < 0xa0000, "Please move RAMBASE to 1MB"); + /* The ram segment * This is all address of the memory resident copy of coreboot. */