On 24.08.2008 21:55, svn@coreboot.org wrote:
Author: rminnich New Revision: 820
Grow rom space. This now gets a triple fault but I am hoping some smart person can fix it.
Could you post a boot log?
Depending on when the triple fault happens, I might know the solution. - Directly at AP startup: Easy. The global variables are overwritten by every AP and can lead to NULL pointer derefs. (Does not apply in single-core single-processor environments.) - Same for any point after AP startup. - Before AP startup or on uniprocessor: Either a stack overrun (bad) or CAR corruption (worse). - In theory, jumping to the wrong place could be as damaging.
Regards, Carl-Daniel
it's actually very simple. Stage0 needs to grow -- we might as well grow to 32768 and see how it goes.
It's also a bug. stage0 code is being truncated to 20480 bytes with on warning. The pci_find_device function could extend past ffffffff0, but it gets truncated in the middle -> triple fault.
For now, let's grow to 32768 for stage0. Stefan fixed this so only two things need to change. Stefan?
ron
On 24.08.2008 23:39, ron minnich wrote:
it's actually very simple. Stage0 needs to grow -- we might as well grow to 32768 and see how it goes.
It's also a bug. stage0 code is being truncated to 20480 bytes with on warning. The pci_find_device function could extend past ffffffff0, but it gets truncated in the middle -> triple fault.
Oh, I got an even worse stage0 layout during my quest to trigger linker bugs. Only code changes in stage1.c, no linker script was touched: [...] ffffec36 T set_bios_reset ffffec59 T distinguish_cpu_resets ffffec80 T _stage0 ffffecb0 t gdt16x ffffecc8 T gdtptr ffffecc8 t gdt ffffecc8 t gdt16xend ffffecf0 T protected_stage0 ffffecf0 t gdt_end ffffecff T __protected_stage0 ffffed17 t cache_as_ram_setup ffffed29 t enable_fixed_mtrr_dram_modify ffffed43 t clear_fixed_var_mtrr ffffed50 t clear_fixed_var_mtrr_out ffffede2 t fixed_mtrr_msr ffffee0e t var_mtrr_msr ffffee4e t var_iorr_msr ffffee5e t mem_top ffffee6a t cache_as_ram_setup_out fffff6f0 t algo_name fffff704 t console_test.1860 fffff758 t foo fffff900 T option_table fffffff0 A _ROMTOP fffffff0 T _resetjump fffffff0 t pow2_to_link_width.2169 fffffff8 t link_width_to_pow2.2168
Especially the last three lines are VERY disturbing. And no warning is triggered at all. It seems the linker doesn't even notice that it allocated the same space twice.
Having algo_name, the console test banner and the option table between stage0 asm and the reset vector also bothers me.
Regards, Carl-Daniel
the triple fault in pci_find_device is fixed.
Go ahead and svn up.
We *may* need a way to set the size of stage0 on a per-mainboard basis.
I will be out of contact most of next week. If you have changes run them by me, because I really do have a plan for initram :-) and it is based on talks with Marc :-) and I want to give it a chance.
I will continue to work as much as I can. I am really glad we have simnow.
ron
ron minnich wrote:
We *may* need a way to set the size of stage0 on a per-mainboard basis.
Why? That sounds suspicious.
On Mon, Aug 25, 2008 at 4:37 AM, Stefan Reinauer stepan@coresystems.de wrote:
ron minnich wrote:
We *may* need a way to set the size of stage0 on a per-mainboard basis.
Why? That sounds suspicious.
geode users may not want us setting 32k stage0, but k8 may require it.
But with luck we will not need to have a variable size.
ron
ron minnich wrote:
On Mon, Aug 25, 2008 at 4:37 AM, Stefan Reinauer stepan@coresystems.de wrote:
ron minnich wrote:
We *may* need a way to set the size of stage0 on a per-mainboard basis.
Why? That sounds suspicious.
geode users may not want us setting 32k stage0, but k8 may require it.
Again, why?
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something?
On Mon, Aug 25, 2008 at 8:00 AM, Stefan Reinauer stepan@coresystems.de wrote:
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something?
I don't think you missed anything. At the moment, I would bet that I have put un-needed stuff in stage 0. That is why I said "may" :-)
ron
On 25.08.2008 17:10, ron minnich wrote:
On Mon, Aug 25, 2008 at 8:00 AM, Stefan Reinauer stepan@coresystems.de wrote:
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something
I don't think you missed anything. At the moment, I would bet that I have put un-needed stuff in stage 0. That is why I said "may" :-)
There's also some stuff which can be either in initram or the bootblock. - Putting it in initram has the benefit of keeping bootblock size down, but if you have fallback and normal initram, you need the space twice. - Putting it in the bootblock saves space in case we have two initram (fallback/normal). We also may decide to switch to gcc -combine -fwhole-program for the bootblock. That would probably reduce bootblock size by 20%.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 25.08.2008 17:10, ron minnich wrote:
On Mon, Aug 25, 2008 at 8:00 AM, Stefan Reinauer stepan@coresystems.de wrote:
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something
I don't think you missed anything. At the moment, I would bet that I have put un-needed stuff in stage 0. That is why I said "may" :-)
There's also some stuff which can be either in initram or the bootblock.
If something can be put in initram (or in another stage like initram), it should not live in the boot block.
The bootblock is the bare minimum of functions needed to get things up (ie for loading the other stages/modules), plus very few functions like printk that are used in all module stages.
If anything suddenly starts bloating the bootblock, there is most likely a conceptional bug in that code.
We also may decide to switch to gcc -combine -fwhole-program for the bootblock. That would probably reduce bootblock size by 20%.
I heard you say that before. Is it as simple as enabling those switches in the makefiles? Or will it require further action?
I agree if there are 20% in for just enabling a compiler option, we should definitely use it. But using compiler revision dependent trickery and hacks to be able to go bloat elsewhere is not the right approach.
I will move things to initram as needed but ... stepan ... if it is code used in stage2 and initram, does it belong in bootblock or copied in those two stages?
ron
On 26.08.2008 00:50, ron minnich wrote:
I will move things to initram as needed but ... stepan ... if it is code used in stage2 and initram, does it belong in bootblock or copied in those two stages?
I believe Stefan answered this: "The bootblock is the bare minimum [...] plus very few functions like printk that are used in all module stages." AFAICS code used in stage2 and initram fits that description.
Regards, Carl-Daniel
On 25.08.2008 19:57, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 25.08.2008 17:10, ron minnich wrote:
On Mon, Aug 25, 2008 at 8:00 AM, Stefan Reinauer stepan@coresystems.de wrote:
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something
I don't think you missed anything. At the moment, I would bet that I have put un-needed stuff in stage 0. That is why I said "may" :-)
There's also some stuff which can be either in initram or the bootblock.
If something can be put in initram (or in another stage like initram), it should not live in the boot block.
The bootblock is the bare minimum of functions needed to get things up (ie for loading the other stages/modules), plus very few functions like printk that are used in all module stages.
If anything suddenly starts bloating the bootblock, there is most likely a conceptional bug in that code.
Absolutely agreed.
We also may decide to switch to gcc -combine -fwhole-program for the bootblock. That would probably reduce bootblock size by 20%.
I heard you say that before. Is it as simple as enabling those switches in the makefiles? Or will it require further action?
I just enabled it for initram in my testing because the initram makerules only needed to add -fwhole-program. My earlier patches which switched to source-based rules do make conversion to -fwhole-program rather easy as well.
Results with gcc 4.2.1, real size in the LAR: - is current state + is with -fwhole-program
amd_serengeti - normal/initram/segment0 (24060 bytes) + normal/initram/segment0 (640 bytes)
artecgroup_dbe62 - normal/initram/segment0 (6436 bytes) + normal/initram/segment0 (5364 bytes)
emulation_qemu-x86 - normal/initram/segment0 (420 bytes) + normal/initram/segment0 (376 bytes)
The savings vary between substantial and almost improbable. I do suspect something fishy in the serengeti target, but we'd need to test in Simnow.
I agree if there are 20% in for just enabling a compiler option, we should definitely use it.
Yes. I'll post patches in short order.
But using compiler revision dependent trickery and hacks to be able to go bloat elsewhere is not the right approach.
Fully agreed. We need to keep the bloat down. Once that is done, we can apply optimizations on top.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 25.08.2008 19:57, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 25.08.2008 17:10, ron minnich wrote:
On Mon, Aug 25, 2008 at 8:00 AM, Stefan Reinauer stepan@coresystems.de wrote:
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something
I don't think you missed anything. At the moment, I would bet that I have put un-needed stuff in stage 0. That is why I said "may" :-)
There's also some stuff which can be either in initram or the bootblock.
If something can be put in initram (or in another stage like initram), it should not live in the boot block.
The bootblock is the bare minimum of functions needed to get things up (ie for loading the other stages/modules), plus very few functions like printk that are used in all module stages.
If anything suddenly starts bloating the bootblock, there is most likely a conceptional bug in that code.
Absolutely agreed.
We also may decide to switch to gcc -combine -fwhole-program for the bootblock. That would probably reduce bootblock size by 20%.
I heard you say that before. Is it as simple as enabling those switches in the makefiles? Or will it require further action?
I just enabled it for initram in my testing because the initram makerules only needed to add -fwhole-program. My earlier patches which switched to source-based rules do make conversion to -fwhole-program rather easy as well.
Results with gcc 4.2.1, real size in the LAR:
- is current state
- is with -fwhole-program
amd_serengeti
- normal/initram/segment0 (24060 bytes)
- normal/initram/segment0 (640 bytes)
artecgroup_dbe62
- normal/initram/segment0 (6436 bytes)
- normal/initram/segment0 (5364 bytes)
emulation_qemu-x86
- normal/initram/segment0 (420 bytes)
- normal/initram/segment0 (376 bytes)
The savings vary between substantial and almost improbable. I do suspect something fishy in the serengeti target, but we'd need to test in Simnow.
The above looks like it seriously miscompiles the serengeti case. There's no way AMD ram init fits in 640 bytes, when qemu's nop is 376.
On 26.08.2008 02:18, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 25.08.2008 19:57, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 25.08.2008 17:10, ron minnich wrote:
On Mon, Aug 25, 2008 at 8:00 AM, Stefan Reinauer stepan@coresystems.de wrote:
Is the superio init code getting too complex? Or the "enable all flash" code? Or CAR init? Did I miss something
I don't think you missed anything. At the moment, I would bet that I have put un-needed stuff in stage 0. That is why I said "may" :-)
There's also some stuff which can be either in initram or the bootblock.
If something can be put in initram (or in another stage like initram), it should not live in the boot block.
The bootblock is the bare minimum of functions needed to get things up (ie for loading the other stages/modules), plus very few functions like printk that are used in all module stages.
If anything suddenly starts bloating the bootblock, there is most likely a conceptional bug in that code.
Absolutely agreed.
We also may decide to switch to gcc -combine -fwhole-program for the bootblock. That would probably reduce bootblock size by 20%.
I heard you say that before. Is it as simple as enabling those switches in the makefiles? Or will it require further action?
I just enabled it for initram in my testing because the initram makerules only needed to add -fwhole-program. My earlier patches which switched to source-based rules do make conversion to -fwhole-program rather easy as well.
Results with gcc 4.2.1, real size in the LAR:
- is current state
- is with -fwhole-program
amd_serengeti
- normal/initram/segment0 (24060 bytes)
- normal/initram/segment0 (640 bytes)
artecgroup_dbe62
- normal/initram/segment0 (6436 bytes)
- normal/initram/segment0 (5364 bytes)
emulation_qemu-x86
- normal/initram/segment0 (420 bytes)
- normal/initram/segment0 (376 bytes)
The savings vary between substantial and almost improbable. I do suspect something fishy in the serengeti target, but we'd need to test in Simnow.
The above looks like it seriously miscompiles the serengeti case. There's no way AMD ram init fits in 640 bytes, when qemu's nop is 376.
I looked at the code and there's no miscompilation. Current v3 serengeti initram looks like this: int main(void) { printk(BIOS_DEBUG, "Hi there from stage1\n"); post_code(POST_START_OF_MAIN);
printk(BIOS_DEBUG, "stage1 returns\n"); return 0; }
So the code is a NOP and gcc figured it out. Perfect.
Regards, Carl-Daniel
On Mon, Aug 25, 2008 at 5:32 PM, Carl-Daniel Hailfinger
I looked at the code and there's no miscompilation. Current v3 serengeti initram looks like this: int main(void) { printk(BIOS_DEBUG, "Hi there from stage1\n"); post_code(POST_START_OF_MAIN);
printk(BIOS_DEBUG, "stage1 returns\n"); return 0;}
So the code is a NOP and gcc figured it out. Perfect.
This is the kind of optimization I keep taking for granted, since this is how the Plan 9 linker works. Always interesting to see lots of unused code disappear :-)
ron
On 26.08.2008 05:11, ron minnich wrote:
On Mon, Aug 25, 2008 at 5:32 PM, Carl-Daniel Hailfinger
I looked at the code and there's no miscompilation. Current v3 serengeti initram looks like this: int main(void) { printk(BIOS_DEBUG, "Hi there from stage1\n"); post_code(POST_START_OF_MAIN);
printk(BIOS_DEBUG, "stage1 returns\n"); return 0;}
So the code is a NOP and gcc figured it out. Perfect.
This is the kind of optimization I keep taking for granted, since this is how the Plan 9 linker works. Always interesting to see lots of unused code disappear :-)
Thanks for that info. It seems we could enable --gc-sections in the ld flags and have ld perform garbage collection. However, according to the ld man page, this only applies to complete sections. Can you compile coreboot with the Plan 9 linker and share the results?
Regards, Carl-Daniel
On Tue, Aug 26, 2008 at 6:16 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Can you compile coreboot with the Plan 9 linker and share the results?
if only. No, there are too many gcc-isms in coreboot.
ron
There is also no AP startup yet. that comes later. This is all single socket single core for now.
ron
-
Carl-Daniel Hailfinger -
ron minnich -
Stefan Reinauer