Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: [WIP] arch/x86: Add linker script for early DRAM ......................................................................
[WIP] arch/x86: Add linker script for early DRAM
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- A src/arch/x86/early_dram.ld 1 file changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/1
diff --git a/src/arch/x86/early_dram.ld b/src/arch/x86/early_dram.ld new file mode 100644 index 0000000..24b9551 --- /dev/null +++ b/src/arch/x86/early_dram.ld @@ -0,0 +1,50 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2006 Advanced Micro Devices, Inc. + * Copyright (C) 2008-2010 coresystems GmbH + * Copyright 2015 Google Inc + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* This file is included inside a SECTIONS block */ +. = CONFIG_DCACHE_RAM_BASE; +.car.data . (NOLOAD) : { + _car_region_start = . ; + + /* Stack for CAR stages. Since it persists across all stages that + * use CAR it can be reused. The chipset/SoC is expected to provide + * the stack size. */ +#if CONFIG(C_ENVIRONMENT_BOOTBLOCK) + _car_stack_start = .; + . += CONFIG_DCACHE_BSP_STACK_SIZE; + _car_stack_end = .; +#endif + /* The pre-ram cbmem console as well as the timestamp region are fixed + * in size. Therefore place them above the car global section so that + * multiple stages (romstage and verstage) have a consistent + * link address of these shared objects. */ + PRERAM_CBMEM_CONSOLE(., CONFIG_PRERAM_CBMEM_CONSOLE_SIZE) + + TIMESTAMP(., 0x200) + + _car_ehci_dbg_info_start = .; + /* Reserve sizeof(struct ehci_dbg_info). */ + . += 80; + _car_ehci_dbg_info_end = .; + + _car_region_end = . + CONFIG_DCACHE_RAM_SIZE - (. - _car_region_start); +} + +_bogus = ASSERT((CONFIG_DCACHE_RAM_SIZE == 0) || (SIZEOF(.car.data) <= CONFIG_DCACHE_RAM_SIZE), "Cache as RAM area is too full"); +#if !CONFIG(CAR_GLOBAL_MIGRATION) +_bogus3 = ASSERT(CONFIG_DCACHE_BSP_STACK_SIZE > 0x0, "BSP stack size not configured"); +#endif
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: [WIP] arch/x86: Add linker script for early DRAM ......................................................................
Patch Set 1:
(4 comments)
For amd/picasso and RESET_VECTOR_IN_RAM=y implying ENV_CACHE_AS_RAM=n, I don't see anything absolutely requiring use of x86/car.ld. There's couple _car_xx symbols referenced under drivers/intel/fsp2_0, those seemed to be related to (older) FSP choosing pre-ram stack location dynamically and checking it is not trashing other CAR objects. Those references may hit garbage-collection with a strategically placed 'if (ENV_CACHE_AS_RAM)'.
For sure, I am lacking some details and most discussions you have had about this, but I hope this doesn't further stirr things or add confusion. I just feel the keyword CAR in context with amd/picasso is the biggest source of confusion so far.
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 27: _car_stack_start = .; There is probably some easy way to put this in between _program and _eprogram. Section .bss is already picked up from program.ld since amd/picasso has ENV_CACHE_AS_RAM=n.
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 35: PRERAM_CBMEM_CONSOLE(., CONFIG_PRERAM_CBMEM_CONSOLE_SIZE) If we have some CBMEM console before romstage, it would be from PSP execution. (Current information is x86 will skip bootblock and verstage). So any such DRAM region would be defined in a memlayout shared between x86 and PSP. So the entry here can be removed?
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 37: TIMESTAMP(., 0x200) Same thing with timestamps. Inherit/share memlayout with PSP, should we have any timestamps prior to x86 deassertion. Although I have no clue if there is any common timebase or monotonic timer in PSP such that we could scale and sync those with x86 TSC.
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 42: _car_ehci_dbg_info_end = .; Can be removed. This was to support usbdebug more smoothly thru bootblock and verstage. Maybe you don't even have EHCI controller anymore.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: [WIP] arch/x86: Add linker script for early DRAM ......................................................................
Patch Set 2:
Marshall, Martin: I guess you going through some discussions behind closed doors once again? I did not get any replies to coreboot mailing list either about PSP role...
Could you setup a mock emulation/qemu-picasso board such that your bootblock/romstage approaches see build testing?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: [WIP] arch/x86: Add linker script for early DRAM ......................................................................
Patch Set 2:
Marshall, Martin: I guess you going through some discussions behind closed doors once again?
No. I understand that no activity can give that impression, but Martin was out of the country and I told you I was out of the office last week. I'm still getting caught back up, not to mention getting picasso working again after what got merged while I was gone. I'll try to look at this patch stack today and understand where you're going.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: [WIP] arch/x86: Add linker script for early DRAM ......................................................................
Patch Set 2:
Patch Set 2:
Marshall, Martin: I guess you going through some discussions behind closed doors once again?
No. I understand that no activity can give that impression, but Martin was out of the country and I told you I was out of the office last week. I'm still getting caught back up, not to mention getting picasso working again after what got merged while I was gone. I'll try to look at this patch stack today and understand where you're going.
OK. I must have missed or misunderstood your message about out-of-office for a week.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: [WIP] arch/x86: Add linker script for early DRAM ......................................................................
Patch Set 3:
Thanks for your work on this stack of patches. I've redone the Picasso files to use what you've done. Rather that hijack this patch without warning, I made https://review.coreboot.org/c/coreboot/+/35264 to be squashable.
Hello Aaron Durbin, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35035
to look at the new patch set (#4).
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. Add a crt0 file that includes the traditional x86 reset code and a new linker script that describes the early region definitions. New Kconfig options allow the soc to determine the total early DRAM storage required, as well as the BSP and AP (if needed) stack sizes.
The current implementation piggy-backs off of the romstage build but does not require romstage to be in cbfs.
Similar to changes made when bootblock could begin executing C, the extra _start label and gdt_init.S must be excluded.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M Makefile.inc M src/arch/x86/Makefile.inc A src/arch/x86/early_dram.ld M src/arch/x86/include/arch/cpu.h M src/arch/x86/include/arch/symbols.h M src/arch/x86/memlayout.ld A src/arch/x86/reset_in_dram_crt0.S M src/cpu/Kconfig M src/cpu/x86/32bit/entry32.inc 9 files changed, 173 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/4
Marshall Dawson has uploaded a new patch set (#5) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. Add a crt0 file that includes the traditional x86 reset code and a new linker script that describes the early region definitions. New Kconfig options allow the soc to determine the total early DRAM storage required, as well as the BSP and AP (if needed) stack sizes.
The current implementation piggy-backs off of the romstage build but does not require romstage to be in cbfs.
Similar to changes made when bootblock could begin executing C, the extra _start label and gdt_init.S must be excluded.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc A src/arch/x86/early_dram.ld M src/arch/x86/include/arch/cpu.h M src/arch/x86/include/arch/symbols.h M src/arch/x86/memlayout.ld A src/arch/x86/reset_in_dram_crt0.S M src/cpu/Kconfig M src/cpu/x86/32bit/entry32.inc 10 files changed, 174 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig@87 PS6, Line 87: reset vector in RAM instead of the traditional 0xfffffff0 location. This should have been documented that the assumption is that romstage is the first stage when it is selected.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Makefile.inc@2... PS6, Line 223: ifneq ($(CONFIG_RESET_VECTOR_IN_RAM),y) Is this guard needed? Presumably C_ENVIRONMENT_BOOTBLOCK=n when RESET_VECTOR_IN_RAM=y? And vice versa?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld@... PS6, Line 25: _earlyram_region_start = . ; I don't understand why we're just shoving this region after the program. Any reason why we can't just assign a section for the stacks?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 37: jmp soc_reassign_stack /* expect returning jmp to zero_stacks */ Can we use a different ABI to indicate return symbol? like another register's value such as ebp, e.g.?
I know it's necessary, but I also feel like it's weird to zero out the stacks in this path as well. They don't technically require zero'ing out. But .bss certainly does, but I don't see that in this patch. And we have no provision for .bss with this feature? That seems really broken. I similarly also feel like we could modify assembly_entry.S to accommodate things. Admittedly that's just a gut feel.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 6:
Marshall; For the time being I won't be updating this commit so go ahead and modify as needed/requested.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig@87 PS6, Line 87: reset vector in RAM instead of the traditional 0xfffffff0 location.
This should have been documented that the assumption is that romstage is the first stage when it is […]
The name is just killing me now. Do you have an opinion on changing it so something more descriptive now that the entry and infrastructure is in arch? AFAIK this is very AMD/PSP-specific, and when I chose the name the implementation was way down in the soc directory.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Makefile.inc@2... PS6, Line 223: ifneq ($(CONFIG_RESET_VECTOR_IN_RAM),y)
Is this guard needed? Presumably C_ENVIRONMENT_BOOTBLOCK=n when RESET_VECTOR_IN_RAM=y? And vice vers […]
Ack. No longer needed.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld@... PS6, Line 25: _earlyram_region_start = . ;
I don't understand why we're just shoving this region after the program. […]
Do you mean within the program map instead of outside? At the moment, the elf file is only 64K and there's not adequate room for everything I'd like to have. I'll have to dig up some changes I once had that allowed the file to be any n*64K.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 37: jmp soc_reassign_stack /* expect returning jmp to zero_stacks */
Can we use a different ABI to indicate return symbol? like another register's value such as ebp, e.g.?
OK
I know it's necessary, but I also feel like it's weird to zero out the stacks in this path as well. They don't technically require zero'ing out.
I can skip zeroing. They come up w/garbage and I was allowing that to bother me.
And we have no provision for .bss with this feature? That seems really broken.
It took me a bit to recognize that .bss is mapped within the lowest and highest boundaries of the program. As a result, it's getting inherently zeroed due to the compression and build process. I agree it should be explicit instead.
I similarly also feel like we could modify assembly_entry.S to accommodate things. Admittedly that's just a gut feel.
I'll take another look. The first item I see that will need to be addressed is the _car_stack_end symbol which we won't have. The next is that assembly_entry.S clears _bss through _ebss, with the comment that it's not shared -- oh, maybe the comment meant not shared across stages.
...Because .bss _is_ "shared" among cores, and AMD APs begin at the reset vector just like the BSP. So we'd need to make sure APs don't re-zero .bss. Hmm, we would have a bug on stoney ridge due to that, if not for the fact we send APs directly out of bootblock into the middle of romstage. (Kyösti, I think that will be a problem as Family 14h/15h/16h and earlier work toward C_ENVIRONMENT_BOOTBLOCK too.) * So how about checking for BSP before clearing .bss in assembly_entry.S? Or did I miss something obvious?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 6:
(8 comments)
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig@87 PS6, Line 87: reset vector in RAM instead of the traditional 0xfffffff0 location.
The name is just killing me now. […]
Name is suitable for the purpose it is used in rules.h. I would like to keep the window open for separate bootblock/verstage/romstage approach, as nobody wants to answer questions on the mailing list about picasso VBOOT build process.
ARMs run romstage (and bootblock and verstage) from SRAM, maybe this can be generalised even more and drop 'depends on ARCH_X86'.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld@... PS6, Line 29: . += CONFIG_EARLYRAM_BSP_STACK_SIZE; Do you really need to execute APs in romstage?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld@... PS6, Line 58: ap_sipi_vector_in_rom = (_start16bit >> 12) & 0xff; Maybe just set ap_sipi_vector_in_rom to 0 and comment as unused. The related assert in failover.ld will disappear with next release.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/include/arch/c... File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/include/arch/c... PS6, Line 298: asmlinkage void soc_hybrid_romstage_entry(uint32_t bist, uint64_t early_tsc); I don't see why bist and early_tsc should be forwarded to a function in soc/?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 37: jmp soc_reassign_stack /* expect returning jmp to zero_stacks */
Can we use a different ABI to indicate return symbol? like another register's value such as ebp, e […]
AGESA/binaryPI C_ENVIRONMENT_BOOTBLOCK implementation would skip assembly_entry.S, like amd/stoneyridge does, for BSPs.
Maybe you should bring early_stack.S from CB:33759 into this commit.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 59: call soc_hybrid_romstage_entry Why soc_ prefix? Can you design this from day one such that code that is not specific to silicon is not under soc/.
TBH this looks very much like booblock_c_entry_bist from every aspect now, the point where you enter C after in protected mode? While the ChromeOS device may not need it, I would prefer if you kept most of the separate bootblock/romstage separation as an option.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 61: /* This is here for linking purposes. */ Is this really needed?
https://review.coreboot.org/c/coreboot/+/35035/6/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/6/src/cpu/Kconfig@32 PS6, Line 32: depends on RESET_VECTOR_IN_RAM Do you need a static value for this?
I would prefer these grouped under single 'if RESET_VECTOR_IN_RAM'.
Marshall Dawson has uploaded a new patch set (#7) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. Add a crt0 file that includes the traditional x86 reset code and a new linker script that describes the early region definitions. New Kconfig options allow the soc to determine the total early DRAM storage required, as well as the BSP stack size.
The current implementation piggy-backs off of the romstage build but does not require romstage to be in cbfs.
Similar to changes made when bootblock could begin executing C, the extra _start label and gdt_init.S must be excluded.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc A src/arch/x86/early_dram.ld M src/arch/x86/include/arch/cpu.h M src/arch/x86/memlayout.ld A src/arch/x86/reset_in_dram_crt0.S M src/cpu/Kconfig M src/cpu/x86/32bit/entry32.inc M src/include/memlayout.h 10 files changed, 142 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/7/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/7/src/include/memlayout.h@70 PS7, Line 70: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 8: Code-Review+1
Looks good to me
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 8:
My apologies for being extremely tardy on all of this. Would people appreciate/want me to take some time reviewing this patch more thoroughly?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 8:
Marshall, take commit ownership (reset author) and remove my signed-off-by.
AMD/Google/Silverback have failed to answer questions on x86/PSP interactions and build environments and have not provided NDA paperwork for me to participate in the discussion.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 8: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 27: /* : * .near_reset_vector is used to position code that must be reachable : * from the reset vector. For a program size <= 64KB, this happens by : * default, however with > 64KB it is artificially enforced. : */ : _NEAR_RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x100; : _RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x10; : : . = _NEAR_RESET_VECTOR; : .near_reset_vector . : { : *(.near_reset_vector); : } Nice. Can't this solution be generalised for x86 bootblocks too?
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/reset_in_dram_... PS8, Line 33: $(_ebss) drop the brackets around constants?
Marshall Dawson has uploaded a new patch set (#9) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. Add a crt0 file that includes the traditional x86 reset code and a new linker script that describes the early region definitions. New Kconfig options allow the soc to determine the total early DRAM storage required, as well as the BSP stack size.
The current implementation piggy-backs off of the romstage build but does not require romstage to be in cbfs.
Similar to changes made when bootblock could begin executing C, the extra _start label and gdt_init.S must be excluded.
The following is a snip of the elf header when compiled using later patches for a Mandolin board. [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 08000000 000060 007e80 00 AX 0 0 32 [ 2] .rel.text REL 00000000 052a2c 001b60 08 I 10 1 4 [ 3] .data PROGBITS 08007e80 007ee0 000008 00 WA 0 0 4 [ 4] .bss NOBITS 08007e88 007ee8 043ed0 00 WA 0 0 32 [ 5] .near_reset_vecto PROGBITS 0804ff00 04ff60 0000d5 00 AX 0 0 4 [ 6] .rel.near_reset_v REL 00000000 05458c 000058 08 I 10 5 4 [ 7] .reset PROGBITS 0804fff0 050050 000010 00 AX 0 0 1 [ 8] .rel.reset REL 00000000 0545e4 000008 08 I 10 7 4 [ 9] .gnu_debuglink PROGBITS 00000000 050060 000014 00 0 0 4 [10] .symtab SYMTAB 00000000 050074 001610 10 11 122 4 [11] .strtab STRTAB 00000000 051684 0013a5 00 0 0 1 [12] .shstrtab STRTAB 00000000 0545ec 000061 00 0 0 1
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc A src/arch/x86/early_dram.ld M src/arch/x86/include/arch/cpu.h M src/arch/x86/memlayout.ld A src/arch/x86/reset_in_dram_crt0.S M src/cpu/Kconfig M src/cpu/x86/32bit/entry32.inc M src/include/memlayout.h 10 files changed, 142 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/9/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/9/src/include/memlayout.h@70 PS9, Line 70: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9:
(2 comments)
Patch Set 8:
My apologies for being extremely tardy on all of this. Would people appreciate/want me to take some time reviewing this patch more thoroughly?
Yes, please. It's at a point that I finally feel pretty good about it. I looked at your suggestion of reusing assembly_entry.S but felt the necessary changes were detrimental to that file.
Patch Set 8:
Marshall, take commit ownership (reset author) and remove my signed-off-by.
AMD/Google/Silverback have failed to answer questions on x86/PSP interactions and build environments and have not provided NDA paperwork for me to participate in the discussion.
Done. AFAIK it's not possible to change the Owner field within gerrit.
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 27: /* : * .near_reset_vector is used to position code that must be reachable : * from the reset vector. For a program size <= 64KB, this happens by : * default, however with > 64KB it is artificially enforced. : */ : _NEAR_RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x100; : _RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x10; : : . = _NEAR_RESET_VECTOR; : .near_reset_vector . : { : *(.near_reset_vector); : }
Nice. […]
Hmm, sure. AFAIK there's never been a need to worry about it because bootblock is never more than 64K. Do you think that's worth considering (or in the future)? coreboot still essentially allows systems with two different types of bootblocks, and I'm not currently using reset16.ld at all. I'm just trying to picture how things would need to change to make it happen, and whether that ultimately makes things more confusing for the reader.
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/reset_in_dram_... PS8, Line 33: $(_ebss)
drop the brackets around constants?
Sure. We have differing styles in our .S files. I picked the one I thought would go through without objection, but I prefer without.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 37: *(.near_reset_vector); Does this work? I did some tests using the same code but with the regular below 4G bootblock and it needed KEEP(), but I'm not really sure why.
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 27: /* : * .near_reset_vector is used to position code that must be reachable : * from the reset vector. For a program size <= 64KB, this happens by : * default, however with > 64KB it is artificially enforced. : */ : _NEAR_RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x100; : _RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x10; : : . = _NEAR_RESET_VECTOR; : .near_reset_vector . : { : *(.near_reset_vector); : }
Hmm, sure. AFAIK there's never been a need to worry about it because bootblock is never more than 64K. Do you think that's worth considering (or in the future)? coreboot still essentially allows systems with two different types of bootblocks, and I'm not currently using reset16.ld at all. I'm just trying to picture how things would need to change to make it happen, and whether that ultimately makes things more confusing for the reader.
Currently you have either a romcc top aligned bootblock that uses a custom linker script or the C_ENVIRONMENT_BOOTBLOCK which uses arch/x86/memlayout.ld. The latter one has a predefined size that currently goes only go up to 64KiB, but with this code placing the 16 to 32 bit entry at a fixed point allows it to grow beyond 64KiB. This is currently not a problem as 64KiB is enough but it's nice to know the solution is there.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 37: *(.near_reset_vector);
Does this work? I did some tests using the same code but with the regular below 4G bootblock and it […]
Yes, I just repushed with an updated commit message ICYMI. I pasted the regions from readelf and you can see the >64K region with the .near_reset_vector tucked up near the very top of PROGBITS.
I'm not sure why KEEP() would've been required. FWIW I mocked it up on for C_ENVIRONMENT_BOOTBLOCK and built a google/grunt. I was able to build successfully once I provided a little more room between near_reset_vector and the reset vector. I can send that to you if you're interested. BTW, I didn't grow the bootblock; only inserted the new section name and verified it that reset/entry16/entry32 functionality moved.
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 27: /* : * .near_reset_vector is used to position code that must be reachable : * from the reset vector. For a program size <= 64KB, this happens by : * default, however with > 64KB it is artificially enforced. : */ : _NEAR_RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x100; : _RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x10; : : . = _NEAR_RESET_VECTOR; : .near_reset_vector . : { : *(.near_reset_vector); : }
Hmm, sure. […]
Right. It's been a while since I've dealt with the romcc one. Agree, there's a solution if we need to go >64K.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 37: *(.near_reset_vector);
Yes, I just repushed with an updated commit message ICYMI. I pasted the regions from readelf and you can see the >64K region with the .near_reset_vector tucked up near the very top of PROGBITS.
I'm not sure why KEEP() would've been required. FWIW I mocked it up on for C_ENVIRONMENT_BOOTBLOCK and built a google/grunt. I was able to build successfully once I provided a little more room between near_reset_vector and the reset vector. I can send that to you if you're interested. BTW, I didn't grow the bootblock; only inserted the new section name and verified it that reset/entry16/entry32 functionality moved.
Thanks for testing. Yes, it's be interested in seeing what I did wrong.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld@... PS8, Line 27: /* : * .near_reset_vector is used to position code that must be reachable : * from the reset vector. For a program size <= 64KB, this happens by : * default, however with > 64KB it is artificially enforced. : */ : _NEAR_RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x100; : _RESET_VECTOR = CONFIG_ROMSTAGE_ADDR + CONFIG_RAM_RESET_VECTOR_STAGE_SIZE - 0x10; : : . = _NEAR_RESET_VECTOR; : .near_reset_vector . : { : *(.near_reset_vector); : }
Right. It's been a while since I've dealt with the romcc one. […]
I think the restriction today (and in the future) is that reset16, entry16 and entry32 (.inc) all need to reside in the top 64 KiB (for !RESET_VECTOR_IN_RAM). Since we fall from entry32.inc to bootblock_crt0.S without a jump, this would effectively make bootblock_pre_c_entry the first symbol that could be outside top 64 KiB. My reasoning is that it is at the end of entry32.inc where linear mode addressing is completely set up.
Whether cache_as_ram.S is linked in the top 64 KiB should not matter. These assembly parts would probably all fit in the top 4 KiB (unless there are FIT or something else consuming same space), adding some section-statements should achieve this if ever necessary.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 9: Code-Review+1
This unblocks things, but I think we can do this better and not so much duplication. I have some notes and thinking about how to do this. However, I can't commit to providing such a solution in the immediate future.
Marshall Dawson has uploaded a new patch set (#10) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
Due the nature of a system where the DRAM is alive before the x86, a number of design options were considered, including the first stage as ramstage. Beginning with ramstage was infeasible because the processor must still go through the typical initial steps and be put into flat protected mode.
This patch is phase 1 of adding support and it builds a traditional bootblock image. Although running from DRAM, the unused bootblock is kept in cbfs. The details for placing the bootblock into memory are left to the soc implementation.
Sample build using amd/mandolin (currently WIP): Name Offset Type Size Comp cbfs master header 0x0 cbfs header 32 none fallback/romstage 0x80 stage 21300 none fallback/ramstage 0x5440 stage 76115 none config 0x17e00 raw 398 none revision 0x18000 raw 674 none payload_config 0x18300 raw 1593 none payload_revision 0x18980 raw 262 none (empty) 0x18b00 null 664 none fspm.bin 0x18dc0 fsp 786432 none (empty) 0xd8e00 null 3992 none fsps.bin 0xd9dc0 fsp 262144 none pci1002,15d8.rom 0x119e00 optionrom 54272 none fallback/dsdt.aml 0x127280 raw 8365 none fallback/payload 0x129380 simple elf 68099 none (empty) 0x139dc0 null 2383832 none bootblock 0x37fdc0 bootblock 524288 none
readelf of build/cbfs/fallback/bootblock.elf: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 02000000 000060 005318 00 AX 0 0 32 [ 2] .rel.text REL 00000000 006f34 0010d8 08 I 5 1 4 [ 3] .earlyram.data NOBITS 0802d000 000000 045ed8 00 WA 0 0 32 [ 4] .gnu_debuglink PROGBITS 00000000 005378 000014 00 0 0 4 [ 5] .symtab SYMTAB 00000000 00538c 000e50 10 6 61 4 [ 6] .strtab STRTAB 00000000 0061dc 000d57 00 0 0 1 [ 7] .shstrtab STRTAB 00000000 00800c 000043 00 0 0 1
91: 0802d000 0 NOTYPE GLOBAL DEFAULT 3 _earlyram_stack 68: 0802d800 0 NOTYPE GLOBAL DEFAULT 3 _eearlyram_stack 175: 0802d800 0 NOTYPE GLOBAL DEFAULT 3 _preram_cbmem_console 199: 0802ee00 0 NOTYPE GLOBAL DEFAULT 3 _epreram_cbmem_console 120: 0802ee00 0 NOTYPE GLOBAL DEFAULT 3 _timestamp 221: 0802f000 0 NOTYPE GLOBAL DEFAULT 3 _etimestamp 153: 0802f000 0 NOTYPE GLOBAL DEFAULT 3 _bss 130: 08072ed8 0 NOTYPE GLOBAL DEFAULT 3 _ebss
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 79 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/10/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/10/src/include/memlayout.h@73 PS10, Line 73: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Marshall Dawson has uploaded a new patch set (#11) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
Due the nature of a system where the DRAM is alive before the x86, a number of design options were considered, including the first stage as ramstage. Beginning with ramstage was infeasible because the processor must still go through the typical initial steps and be put into flat protected mode.
This patch is phase 1 of adding support and it builds a traditional bootblock image. Although running from DRAM, the unused bootblock is kept in cbfs. The details for placing the bootblock into memory are left to the soc implementation.
Sample build using amd/mandolin (currently WIP): Name Offset Type Size Comp cbfs master header 0x0 cbfs header 32 none fallback/romstage 0x80 stage 21300 none fallback/ramstage 0x5440 stage 76115 none config 0x17e00 raw 398 none revision 0x18000 raw 674 none payload_config 0x18300 raw 1593 none payload_revision 0x18980 raw 262 none (empty) 0x18b00 null 664 none fspm.bin 0x18dc0 fsp 786432 none (empty) 0xd8e00 null 3992 none fsps.bin 0xd9dc0 fsp 262144 none pci1002,15d8.rom 0x119e00 optionrom 54272 none fallback/dsdt.aml 0x127280 raw 8365 none fallback/payload 0x129380 simple elf 68099 none (empty) 0x139dc0 null 2383832 none bootblock 0x37fdc0 bootblock 524288 none
readelf of build/cbfs/fallback/bootblock.elf: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 02000000 000060 005318 00 AX 0 0 32 [ 2] .rel.text REL 00000000 006f34 0010d8 08 I 5 1 4 [ 3] .earlyram.data NOBITS 0802d000 000000 045ed8 00 WA 0 0 32 [ 4] .gnu_debuglink PROGBITS 00000000 005378 000014 00 0 0 4 [ 5] .symtab SYMTAB 00000000 00538c 000e50 10 6 61 4 [ 6] .strtab STRTAB 00000000 0061dc 000d57 00 0 0 1 [ 7] .shstrtab STRTAB 00000000 00800c 000043 00 0 0 1
91: 0802d000 0 NOTYPE GLOBAL DEFAULT 3 _earlyram_stack 68: 0802d800 0 NOTYPE GLOBAL DEFAULT 3 _eearlyram_stack 175: 0802d800 0 NOTYPE GLOBAL DEFAULT 3 _preram_cbmem_console 199: 0802ee00 0 NOTYPE GLOBAL DEFAULT 3 _epreram_cbmem_console 120: 0802ee00 0 NOTYPE GLOBAL DEFAULT 3 _timestamp 221: 0802f000 0 NOTYPE GLOBAL DEFAULT 3 _etimestamp 153: 0802f000 0 NOTYPE GLOBAL DEFAULT 3 _bss 130: 08072ed8 0 NOTYPE GLOBAL DEFAULT 3 _ebss
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 79 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/11/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/11/src/include/memlayout.h@73 PS11, Line 73: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Marshall Dawson has uploaded a new patch set (#12) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
Due the nature of a system where the DRAM is alive before the x86, a number of design options were considered, including the first stage as ramstage. Beginning with ramstage was infeasible because the processor must still go through the typical initial steps and be put into flat protected mode.
This patch is phase 1 of adding support and it builds a traditional bootblock image. Although running from DRAM, the unused bootblock is kept in cbfs. The details for placing the bootblock into memory are left to the soc implementation.
Sample build using amd/mandolin (currently WIP): Name Offset Type Size Comp cbfs master header 0x0 cbfs header 32 none fallback/romstage 0x80 stage 21188 none fallback/ramstage 0x53c0 stage 76271 none config 0x17e00 raw 334 none revision 0x17fc0 raw 674 none payload_config 0x182c0 raw 1621 none payload_revision 0x18980 raw 264 none (empty) 0x18b00 null 664 none fspm.bin 0x18dc0 fsp 720896 none (empty) 0xc8e00 null 3992 none fsps.bin 0xc9dc0 fsp 327680 none pci1002,15d8.rom 0x119e00 optionrom 54272 none fallback/dsdt.aml 0x127280 raw 8372 none fallback/payload 0x129380 simple elf 69496 none (empty) 0x13a340 null 2382424 none bootblock 0x37fdc0 bootblock 524288 none
readelf of build/cbfs/fallback/bootblock.elf: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 08000000 001000 0049c0 00 AX 0 0 32 [ 2] .rel.text REL 00000000 082f00 001240 08 I 9 1 4 [ 3] .earlyram.data NOBITS 0802cf4a 0059c0 002106 00 WA 0 0 4 [ 4] .near_reset_vecto PROGBITS 0807f000 080000 0000bf 00 AX 0 0 4096 [ 5] .rel.near_reset_v REL 00000000 084140 000040 08 I 9 4 4 [ 6] .reset PROGBITS 0807fff0 080ff0 000010 00 AX 0 0 1 [ 7] .rel.reset REL 00000000 084180 000008 08 I 9 6 4 [ 8] .gnu_debuglink PROGBITS 00000000 081000 000014 00 0 0 4 [ 9] .symtab SYMTAB 00000000 081014 0010a0 10 10 96 4 [10] .strtab STRTAB 00000000 0820b4 000e49 00 0 0 1 [11] .shstrtab STRTAB 00000000 084188 000065 00 0 0 1
137: 0802cf50 0 NOTYPE GLOBAL DEFAULT 3 _earlyram_stack 107: 0802d750 0 NOTYPE GLOBAL DEFAULT 3 _eearlyram_stack 216: 0802d750 0 NOTYPE GLOBAL DEFAULT 3 _preram_cbmem_console 240: 0802ed50 0 NOTYPE GLOBAL DEFAULT 3 _epreram_cbmem_console 162: 0802ed50 0 NOTYPE GLOBAL DEFAULT 3 _timestamp 258: 0802ef50 0 NOTYPE GLOBAL DEFAULT 3 _etimestamp 128: 0802ef50 0 NOTYPE GLOBAL DEFAULT 3 _fmap_cache 159: 0802f006 0 NOTYPE GLOBAL DEFAULT 3 _efmap_cache 190: 0802f008 0 NOTYPE GLOBAL DEFAULT 3 _bss 174: 0802f050 0 NOTYPE GLOBAL DEFAULT 3 _ebss
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 92 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/12/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/12/src/include/memlayout.h@73 PS12, Line 73: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
Patch Set 12:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 27: _car_stack_start = .;
There is probably some easy way to put this in between _program and _eprogram. Section . […]
Resolving - stale now.
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 35: PRERAM_CBMEM_CONSOLE(., CONFIG_PRERAM_CBMEM_CONSOLE_SIZE)
If we have some CBMEM console before romstage, it would be from PSP execution. […]
Resolving - stale now.
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 37: TIMESTAMP(., 0x200)
Same thing with timestamps. […]
Resolving - stale now.
https://review.coreboot.org/c/coreboot/+/35035/1/src/arch/x86/early_dram.ld@... PS1, Line 42: _car_ehci_dbg_info_end = .;
Can be removed. This was to support usbdebug more smoothly thru bootblock and verstage. […]
Done
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/include/arch/c... File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/include/arch/c... PS6, Line 298: asmlinkage void soc_hybrid_romstage_entry(uint32_t bist, uint64_t early_tsc);
In the case of early_tsc, it's not too different than with the bootblock arrangement, i.e. […]
Not passing tsc but keeping bist.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/13/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/13/src/include/memlayout.h@73 PS13, Line 73: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM for bootblock ......................................................................
Patch Set 13:
I have started CB:37895 as an approach to drop static C_ENV_BOOTBLOCK_SIZE and align the required x86 realmode/assembly parts towards the end of the bootblock.
Marshall Dawson has uploaded a new patch set (#14) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Remove the postcar stage.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new earlyram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the earlyram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 73 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/14/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/14/src/include/memlayout.h@73 PS14, Line 73: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 14:
I've spent a few minutes rebasing this on CB:37895 but am still missing something important. Am getting overlapping regions from early_ram.ld and need to figure out what to do with the BOOTBLOCK() section having gone away.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 14:
Patch Set 14:
I've spent a few minutes rebasing this on CB:37895 but am still missing something important. Am getting overlapping regions from early_ram.ld and need to figure out what to do with the BOOTBLOCK() section having gone away.
I have no motivation to complete CB:37895 anytime near future.
Marshall Dawson has uploaded a new patch set (#15) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Remove the postcar stage.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new earlyram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the earlyram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 71 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/15
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/15/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/15/src/include/memlayout.h@73 PS15, Line 73: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc@1162 PS15, Line 1162: # Ignore segments for .car.data or .earlyram.data while adding romstage. Isn't an empty .data already asserted by the linker scripts? Aaron has made some suggestions to add .data to remaining stages but I don't know if that is pushed for review yet.
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld@... PS15, Line 66: #if !CONFIG(RESET_VECTOR_IN_RAM) The linker script alone does not add anything in .id section so I don't immediately see the need for the quard.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 15: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/Kconfig@104 PS15, Line 104: bootblock, or a hybrid romstage We decided to drop hybrid romstage right?
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/assembly_entr... PS15, Line 36: CAR Update comment.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/16/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/16/src/include/memlayout.h@72 PS16, Line 72: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Felix Held has uploaded a new patch set (#17) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Remove the postcar stage.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new earlyram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the earlyram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 60 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/17/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/17/src/include/memlayout.h@61 PS17, Line 61: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 17:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/Kconfig@104 PS15, Line 104: bootblock, or a hybrid romstage
We decided to drop hybrid romstage right?
Done
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/assembly_entr... PS15, Line 36: CAR
Update comment.
Done
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld@... PS15, Line 66: #if !CONFIG(RESET_VECTOR_IN_RAM)
The linker script alone does not add anything in . […]
removing the guard results in the build process failing with a segfault. haven't investigated further
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 59: call soc_hybrid_romstage_entry
soc_ prefix... […]
file doesn't exist in the current patchset any more
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc@1162 PS15, Line 1162: # Ignore segments for .car.data or .earlyram.data while adding romstage.
Isn't an empty . […]
any news on this? can this be closed?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 17:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@20 PS17, Line 20: Remove the postcar stage. Disable? It's not really removed, just not built and added.
https://review.coreboot.org/c/coreboot/+/35035/17/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/17/Makefile.inc@1175 PS17, Line 1175: ifneq `ifneq (...,y)` is hard to read, the y sticks out. How about `ifeq` and switching the cases?
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 6: /* Encouraged comment style wants a line break after the /*
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20; Please add a comment what exactly this accounts for.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld@... PS17, Line 57: #include <arch/x86/id.ld> How does it get into the coreboot.rom then?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@27 PS17, Line 27: they can be linked with their .data and .bss as normal fwiw this is not specific to running a stage from dram, but can be done on all stages not running XIP. See (WIP) 36612.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 23: _PAD_FOR_ALIGNS This is a bit weird? Can't you use an ALIGN macro to align things?
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 25: EARLYRAM_STACK(., _STACK_SIZE) : PRERAM_CBMEM_CONSOLE(., _CONSOLE_SIZE) : TIMESTAMP(., _TIMESTAMPS_SIZE) : #if !CONFIG(NO_FMAP_CACHE) : FMAP_CACHE(., FMAP_SIZE) : #endif Don't you want VBOOT2_WORK somewhere in there?
Felix Held has uploaded a new patch set (#18) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new earlyram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the earlyram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 61 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/18
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/18/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/18/src/include/memlayout.h@61 PS18, Line 61: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 18:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@20 PS17, Line 20: Remove the postcar stage.
Disable? It's not really removed, just not built and added.
probably some remains from an older version of this patch. removed it
https://review.coreboot.org/c/coreboot/+/35035/17/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/17/Makefile.inc@1175 PS17, Line 1175: ifneq
`ifneq (...,y)` is hard to read, the y sticks out. How about `ifeq` and […]
good point; fixed
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 6: /*
Encouraged comment style wants a line break after the /*
i don't particularly like this, since it just adds another line for no gain, but changed for consistency
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld@... PS17, Line 57: #include <arch/x86/id.ld>
How does it get into the coreboot. […]
It doesn't. Traditionally this was added to bootblock, but bootblock on AMD fam17h+ works very differently compared to all other architectures; basically PSP puts it into DRAM after optional decompression and the x86 parts starts executing this from DRAM
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 18:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35035/14/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/14/src/arch/x86/early_ram.ld@... PS14, Line 32: /* Account for the size of .near_reset_vector regardless if it's used */ : _NEAR_RESET_VECTOR = CONFIG_X86_RESET_VECTOR - 0x1000 + 0x10; : With the bootblock approach, I'm nowhere close to needing >64K. I'll change the comment and naming here and let's do away with the .near_reset_vector change for now.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20;
Please add a comment what exactly this accounts for.
Nico, Arthur, here's what this is for and I would like a prettier solution, of course.
You probably guessed that the intent is to place the regions at l.25 - l.30 as close to the reset vector as possible to keep things dense. However the EARLYRAM_STACK(), PRERAM_CBMEM_CONSOLE(), TIMESTAMP(), etc. macros in memlayout.h perform their own alignments if necessary. This can result in the linker's location counter advancing more than, say CONFIG_PRERAM_CBMEM_CONSOLE_SIZE, if that region would otherwise begin on an unaligned address.
The _PAD_FOR_ALIGNS allows for a little free space to prevent the linker from erroring out as the result of alignment corrections in memlayout.h.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 23: _PAD_FOR_ALIGNS
This is a bit weird? Can't you use an ALIGN macro to align things?
See description above.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 25: EARLYRAM_STACK(., _STACK_SIZE) : PRERAM_CBMEM_CONSOLE(., _CONSOLE_SIZE) : TIMESTAMP(., _TIMESTAMPS_SIZE) : #if !CONFIG(NO_FMAP_CACHE) : FMAP_CACHE(., FMAP_SIZE) : #endif
Don't you want VBOOT2_WORK somewhere in there?
Can we add that in a follow-on change? You may know that the PSP will be able to run vboot before the x86 comes out of reset (certain SKUs only). So while the answer to your question is likely "yes", the mechanism for telling PSP where to put the workbuf is still TBD.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld@... PS17, Line 57: #include <arch/x86/id.ld>
How does it get into the coreboot. […]
At the moment, it's not there and we're not attempting to use it for anything. It seems like Kyosti had suggested the ID was used by flashrom, but I couldn't find that, even after asking around a bit. Can you state the practical reasons for keeping it there?
Our bootblock is compressed inside the flash image. (Inside the amdfw.rom artifact to be precise.) Since the reset vector isn't at 4GB-0x10, there's no need to leave an uncompressed copy of bootblock there. And once we accept that premise, it follows that an ID embedded in the bootblock won't be at the top of flash. Nor will it be immediately readable, as it's compressed.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 18:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@20 PS17, Line 20: Remove the postcar stage.
probably some remains from an older version of this patch. […]
Sorry, I wasn't clear. I just meant the wording. The postcar stage is indeed disable in case of RESET_VECTOR_IN_RAM (by a depends on !...) by this patch. So that should be mentioned here.
It's hard to overlook in the huge patch because it's implicit and just one line... ;)
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 6: /*
i don't particularly like this, since it just adds another line for no gain, but changed for consist […]
The other style option is /* and */ on the first and last line without preceding *s. i.e.
/* multi- line comment */
However, for top-level comments, I prefer the current style with wasted space.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20;
Nico, Arthur, here's what this is for and I would like a prettier solution, of course. […]
Thanks, that was clear to me already, though. What I'm asking for is the specific number `0x20` aka. 32. I looked into `memlayout.h` and the only region with an explicit ALIGN_COUNTER() is the new EARLYRAM_STACK(). It's not using 32, though, but 16.
And now that I looked further into it, do we need the explicit ALIGN_ COUNTER() at all? Do we really expect any size to not be a multiple of 16? Aren't the assertions enough?
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 30: #endif We can also assert here that `. <= CONFIG_X86_RESET_VECTOR`.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld@... PS17, Line 57: #include <arch/x86/id.ld>
At the moment, it's not there and we're not attempting to use it for anything. It seems like Kyosti had suggested the ID was used by flashrom, but I couldn't find that, even after asking around a bit. Can you state the practical reasons for keeping it there?
Where did you ask? did you try the coreboot or flashrom mailing list? Took me less than a minute to find it, cb_check_image() in `cbtables.c` (I'm not a fan of that code, but it's there).
Another example is the flashupdate feature in FILO.
Well, I guess we can expect that every company that uses coreboot has built some infrastructure around it. Having the `id` section at a specific location was always an invariant for coreboot on x86.
I once had to deal with it because on Apollo Lake there is no CBFS at the end of the `coreboot.rom`. It turned out that there were such assumptions all over the place. I can't say how it looks like in other companies, but I would be surprised if we are the only one.
I don't say it has to be in the bootblock. If, for instance, you have nothing else that has to be at the end of CBFS, you could just put it there as usual.
(marking as resolved because any solution wouldn't belong into this change)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/19/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/19/src/include/memlayout.h@61 PS19, Line 61: #define EARLYRAM_STACK(addr, size) \ Macros with complex values should be enclosed in parentheses
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20;
Thanks, that was clear to me already, though. What I'm asking for is the […]
Agree, none of the other macros use ALIGN_COUNTER. The REGION macro also verifies the alignment. I would opt for removing the padding and just make it a compile error.
Marshall Dawson has uploaded a new patch set (#20) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 6 files changed, 58 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/20
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@20 PS17, Line 20: Remove the postcar stage.
Sorry, I wasn't clear. I just meant the wording. The postcar stage is […]
Oh, I see what you mean. postcar was removed from the execution flow of any of these processors, but not removed out of coreboot.
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@27 PS17, Line 27: they can be linked with their .data and .bss as normal
fwiw this is not specific to running a stage from dram, but can be done on all stages not running XI […]
Agree. Better wording for my intention was preram stages, not all stages. I'll describe it better.
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc@1162 PS15, Line 1162: # Ignore segments for .car.data or .earlyram.data while adding romstage.
any news on this? can this be closed?
I don't see anything asserting empty .data. If I missed it, or if it's in review I believe it can stay separate from this change.
However, given the current nature of this patch, the change here is no longer a requirement. We're keeping the .data section in romstage and there seems to be no harm in telling cbfs to strip a nonexistent .car.data.
https://review.coreboot.org/c/coreboot/+/35035/14/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/14/src/arch/x86/early_ram.ld@... PS14, Line 32: /* Account for the size of .near_reset_vector regardless if it's used */ : _NEAR_RESET_VECTOR = CONFIG_X86_RESET_VECTOR - 0x1000 + 0x10; :
With the bootblock approach, I'm nowhere close to needing >64K. […]
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 6: /*
The other style option is /* and */ on the first and last line without preceding *s. i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20;
Agree, none of the other macros use ALIGN_COUNTER. The REGION macro also verifies the alignment. […]
Ah, you're mostly right 😊 Although the stack itself must be aligned, the region in memory may be unaligned. However, it looks like I can't remove all alignment here. Although the reset vector is on a 16-byte boundary, the subtraction below doesn't result in a nice address -- the FMAP_SIZE value below is 0xb6. So even if EARLYRAM_STACK is a don't-care, console and timestamps will subsequently fail their asserts of 4 and 8.
I'll use a more natural looking way to align it here and remove the alignment from memlayout.h.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 23: _PAD_FOR_ALIGNS
See description above.
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 30: #endif
We can also assert here that `. <= CONFIG_X86_RESET_VECTOR`.
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 25: EARLYRAM_STACK(., _STACK_SIZE) : PRERAM_CBMEM_CONSOLE(., _CONSOLE_SIZE) : TIMESTAMP(., _TIMESTAMPS_SIZE) : #if !CONFIG(NO_FMAP_CACHE) : FMAP_CACHE(., FMAP_SIZE) : #endif
Can we add that in a follow-on change? You may know that the PSP will be able to run vboot before t […]
Ack.
Thinking a little more about this, the first implementation will run vboot on the PSP itself and it'll ship the workbuf to DRAM for us. We need to provide a target address for the PSP to use and give that to amdfwtool, ensuring it matches a region we declare here.
A vboot implementation I don't want to rule out is one that runs more like the traditional model, i.e. bootblock->verstage->romstage. In that case, a VBOOT2_WORK region anywhere we declare it should be fine. No need to coordinate with the PSP's amdfw.rom build.
BTW option 1 above will only be available on certain SKUs. Option 2 does not afford the ability to verify the memory init functionality (ABLs).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc@1162 PS15, Line 1162: # Ignore segments for .car.data or .earlyram.data while adding romstage.
I don't see anything asserting empty .data. […]
In arch/x86/car.ld we had .illegal globals for all .data.* and additionally the definition of ENV_STAGE_HAS_DATA_SECTION asserted empty .data with ENV_CACHE_AS_RAM. But I was really commenting on (.car and .earlyram).data references we saw here.
The first, .car.data, has always(?) been declared with (NOLOAD) and even after reading CB:7305 commit message I do not see the need to exclude it on the commandline. But elf_to_stage in cbfs-mkstage.c also evolved a lot since that commit, with separation to XIP and non-XIP loading so I have not tried to find a failure case either.
This change seemed equally unnecessary for .earlyram.data so I thought you would have had the answer why it is needed, but I see it now got removed for patchset 20 at least.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld@... PS15, Line 66: #if !CONFIG(RESET_VECTOR_IN_RAM)
removing the guard results in the build process failing with a segfault. […]
nit: I would prefer guards, if necessary, inside id.ld with some commentary.
Looks like assumed location near 4G to me:
. = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; vs . = CONFIG_X86_RESET_VECTOR - CONFIG_ID_SECTION_OFFSET + 0x10 - (__id_end - __id_start);
While this would not make it appear at the end of coreboot.rom image it would still be in the intermediate bootblock.bin just below reset vector. Although that would not flashrom or FILO at all.
Related CB:37895 and CB:37955 discussion, id.ld and fit.ld considered somewhat broken and true fix is out of the scope.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld@... PS17, Line 57: #include <arch/x86/id.ld>
(marking as resolved because any solution wouldn't belong into this change)
Since it is far from obvious, I would like to see the lack of ID field and expected flashrom usage complications (-p internal will need some force?) mentioned in the commit message. Preferably this would also be filed as a bug, if you agree it breaks existing expectations on what coreboot.rom files look like and you have no absolute need to leave the ID out on these platforms.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld@... PS15, Line 66: #if !CONFIG(RESET_VECTOR_IN_RAM)
nit: I would prefer guards, if necessary, inside id.ld with some commentary. […]
I think I like the guards a little bit better here, but it's not a strong preference at all. I could go with either way.
On the value of including the ID into a picasso image, I struggled to find any. It won't be discoverable in the ROM image because it's inside of a compressed image. And if it's assumed to be in place runtime just below 4GB, a better solution might be a small non-bootblock piece of data we can place at the top of flash.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init Aren't we in 16-bit mode on these AMD parts when get launched? What's the stack pointer value? No calls should be valid and this code is 32-bit...
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
Aren't we in 16-bit mode on these AMD parts when get launched? What's the stack pointer value? No ca […]
By the time we get to assembly_entry.S, we're past bootblock when all that was taken care of. This code only executes in romstage (and any home-grown verstage on Picasso). That is, I believe the comment on line 7 is still accurate, even for Picasso. I don't recall now whether I was using the file for the obsolete hybrid romstage, and if that muddled things.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld@... PS15, Line 66: #if !CONFIG(RESET_VECTOR_IN_RAM)
I think I like the guards a little bit better here, but it's not a strong preference at all. I could go with either way.
On the value of including the ID into a picasso image, I struggled to find any.
Nico already answered about flashrom and FILO expectations, and that the fix is not in the scope of this patch.
I suggest fixing id.ld first. It's somewhat nasty that you unconditionally link in id.S, but intentionally let garbage-collection have it before it lands on a section (now) unknown to the linker.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
By the time we get to assembly_entry.S, we're past bootblock when all that was taken care of. […]
OK. That was my expectation, but since we're using RESET_VECTOR_IN_RAM it's actually confusing things. We're doing #if/#else just for different symbol names on a construct that should fundamentally not matter to romstage. I feel like we could just change the name of the symbols to a common name to reduce the noise.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/15/src/arch/x86/memlayout.ld@... PS15, Line 66: #if !CONFIG(RESET_VECTOR_IN_RAM)
I think I like the guards a little bit better here, but it's not a strong preference at all. […]
To be clear, I didn't mean any solution that would add the ID into the bootblock. Just in case that we can keep it at the top of CBFS (even though the bootblock is not there), that would be nice.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
We're doing #if/#else just for different symbol names on a construct that should fundamentally not matter to romstage. I feel like we could just change the name of the symbols to a common name to reduce the noise.
_car_stack/_ecar_stack seems to be used in several x86 files and 1 riskv one; maybe 17 total. I'm not opposed to a patch to commonize the name, but I'm not sure how others would feel about it. We'd started out attempting to piggyback on the _car names but IIRC Kyõsti didn't think that was appropriate.
Otherwise I'm not sure how else I'd reduce the noise. The #if/else/endif selection above could maybe be moved into rules.h and maybe call it something like PRERAM_STACK_TOP.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
We're doing #if/#else just for different symbol names on a construct that should fundamentally not […]
I can clean it up later.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
I can clean it up later.
I left some comments about the stack and .earlyram.data.
Pretty much everything has changed from early April wrt executed x86 stages. You can, of course, evaluate once again if car.ld would be more suitable for you instead of the separate earlyram.ld approach. The isolation was suggested partly due the (scheduled) CAR_MIGRATION removal work that would likely have hit merge conflicts both ways.
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/early_ram.ld@... PS21, Line 35: _bogus2 = ASSERT(. <= CONFIG_X86_RESET_VECTOR, "Earlyram data regions don't fit below the reset vector!"); This can probably wait for verstage development, but I'll comment early on.
a) You may need to lock the layout of .earlyram.data with first shipped bootblock. Depends of how your root-of-trust will look like. b) Stack requirement with Intel FSP2.1 has blown thru the roof to some 128KB. While that is mostly due to raminit, you may want to have some reserve there for the stack. c) Increasing stack size could make _start16bit too far away from reset vector. You may need to reconsider .near_reset_vector or complete CB:37895. d) The motivation to share stack for early stages was to spare cachelines. You don't have such restrictions. e) The section .earlyram.data appears inside bootblock stage. Traditionally next stage was allowed to discard previous stage program section. f) If verstage shall verify bootblock integrity (althought it was already executed), you only have the modified bootblock program region left in DRAM and the compressed one in SPI.
Just saying, I think .earlyram.data was better kept separate and outside bootblock program. I did not look for argumentation why and when this changed from some of the early patchsets.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
You can, of course, evaluate once again if car.ld would be more suitable for you instead of the separate earlyram.ld approach.
Sorry if I wasn't clear. I was referring to consistent symbol names. It's just noisy. I may still want to evaluate car.ld (or whatever we want to rename it to), but I think we still have some preliminary work to do in the semantics of the stages and linkers (bss, data section, etc) that is more first-class. We have the start, but I think there's some more polishing needed.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
You can, of course, evaluate once again if car. […]
Can this comment be resolved then? Sounds like Aaron is going to clean the names up later.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/35035/20/src/arch/x86/assembly_entr... PS20, Line 24: call gdt_init
Can this comment be resolved then? Sounds like Aaron is going to clean the names up later.
resolved
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld@... PS21, Line 56: #if !CONFIG(RESET_VECTOR_IN_RAM) Not really resolved.
I have to assume that whoever put the guard here, discovered the segfault like 12 months ago and we already spent 20 times more time on this particular block review, compared to what I suspected would have been a oneliner fix in id.S.
Please change your workflows such that you alwaya leave TODOs on workarounds you implement, specially in common code. Preferably do not squash them with other works. They are higly likely to be resolved during community review, at latest.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld@... PS21, Line 56: #if !CONFIG(RESET_VECTOR_IN_RAM)
Not really resolved. […]
I have started a patch series here to move id completely out of .ld: https://review.coreboot.org/c/coreboot/+/40377. There is some discussion ongoing to decide whether it lands in CBFS or a FMAP region. But, I can close in the upcoming weeks. Marking this as resolved based on the above effort.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
Can you please rebase this on ToT and remove its dependency on CB:38700?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE; This doesn't really take into consideration CONFIG_C_ENV_BOOTBLOCK_SIZE. Wouldn't you need that as well?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
This doesn't really take into consideration CONFIG_C_ENV_BOOTBLOCK_SIZE. […]
From https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/memlayout.ld#..., it looks like the way things are organized is code lies before the reset vector. And this file seems to be putting all the data components before the reset vector as well. So, considering that, you need to take CONFIG_C_ENV_BOOTBLOCK_SIZE into account here.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/22//COMMIT_MSG@30 PS22, Line 30: Felix - Can you please add the appropriate BUG# here? BUG=b:????
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld@... PS21, Line 56: #if !CONFIG(RESET_VECTOR_IN_RAM)
I have started a patch series here to move id completely out of .ld: https://review.coreboot. […]
A twoliner fix for this was suggested as solution in some previous patchset, together with some cases of external utilities and payloads that depend of current id pointer location near the reset vector. I pushed untested CB:40583 based on that.
The guard used here is also logically wrong, although in this patch series RESET_VECTOR_IN_RAM was the trigger.
Related to x86 bootblock, I am continuing CB:37895.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
From https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/memlayout. […]
C_ENV_BOOTBLOCK_SIZE is an arbitrary size that simply must be large enough to determine the total size of bootblock. It encompasses this area, and not the other way around. I think any problem w/C_ENV_BOOTBLOCK_SIZE here would be a similar problem in a traditional platform too.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
It encompasses this area, and not the other way around.
I don't think that is true. On previous platforms, C_ENV_BOOTBLOCK_SIZE basically captured the read-only data and code which was just XIP from memory-mapped SPI flash.
I think any problem w/C_ENV_BOOTBLOCK_SIZE here would be a similar problem in a traditional platform too.
I don't think so. These regions would be in the CAR which had a different base. Now, you are calculating the base of this data region using RESET_VECTOR in DRAM. I believe this would result in overlap of .earlyram.data region with the code region.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
It encompasses this area, and not the other way around. […]
Just highlighting one comment from earlier patchset here.
https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/early_ram.ld#...
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
Just highlighting one comment from earlier patchset here. […]
Furquan is correct, without C_ENV_BOOTBLOCK_SIZE we get the following error: /opt/coreboot-sdk/bin/i386-elf-ld.bfd: section .earlyram.data VMA [000000000807daee,000000000807ffe9] overlaps section .text VMA [0000000008078000,000000000807f4cf] /opt/coreboot-sdk/bin/i386-elf-ld.bfd: section .data VMA [000000000807f4d0,000000000807f507] overlaps section .earlyram.data VMA [000000000807daee,000000000807ffe9]
I mapped out the memory layout while debugging: https://docs.google.com/spreadsheets/d/1ucO-bU5IPHj7OGCYNWChhJYuFwUw7uJDLNOC...
$ objdump -h /tmp/coreboot/trembyle-upstream/cbfs/fallback/bootblock.debug
/tmp/coreboot/trembyle-upstream/cbfs/fallback/bootblock.debug: file format elf32-i386
Sections: Idx Name Size VMA LMA File off Algn 0 .text 000074d0 08078000 08078000 00001000 2**12 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 1 .data 00000038 0807f4d0 0807f4d0 000084d0 2**2 CONTENTS, ALLOC, LOAD, DATA 2 .bss 00000048 0807f508 0807f508 00008508 2**2 ALLOC 3 .earlyram.data 000024fc 08075aee 08075aee 00000000 2**0 ALLOC 4 .reset 00000010 0807fff0 0807fff0 00008ff0 2**0 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 5 .debug_info 000254a7 00000000 00000000 00009000 2**0 CONTENTS, RELOC, READONLY, DEBUGGING 6 .debug_abbrev 0000735a 00000000 00000000 0002e4a7 2**0 CONTENTS, READONLY, DEBUGGING 7 .debug_aranges 00000dd0 00000000 00000000 00035808 2**3 CONTENTS, RELOC, READONLY, DEBUGGING 8 .debug_line 0000acf3 00000000 00000000 000365d8 2**0 CONTENTS, RELOC, READONLY, DEBUGGING 9 .debug_str 00009655 00000000 00000000 000412cb 2**0 CONTENTS, READONLY, DEBUGGING 10 .debug_loc 0000b7ce 00000000 00000000 0004a920 2**0 CONTENTS, RELOC, READONLY, DEBUGGING 11 .debug_ranges 000029c0 00000000 00000000 000560f0 2**3 CONTENTS, RELOC, READONLY, DEBUGGING
I think this is correct. I'll update the patch.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
Furquan is correct, without C_ENV_BOOTBLOCK_SIZE we get the following error: […]
Increasing C_ENV_BOOTBLOCK_SIZE fixes the overlaps. I'll push a CL that clarifies some of this.
Raul Rangel has uploaded a new patch set (#23) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 6 files changed, 67 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/23
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 23:
(2 comments)
PTAL
https://review.coreboot.org/c/coreboot/+/35035/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/22//COMMIT_MSG@30 PS22, Line 30:
Felix - Can you please add the appropriate BUG# here? BUG=b:????
Done
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/22/src/arch/x86/early_ram.ld@... PS22, Line 23: . = CONFIG_X86_RESET_VECTOR - ARCH_POINTER_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE;
Increasing C_ENV_BOOTBLOCK_SIZE fixes the overlaps. I'll push a CL that clarifies some of this.
I refactored the patch a bit.
I moved the stack out of .earlyram.data and renamed .earlyram.data to .persistent since this data persists across stages.
I also added an assert that will tell if you if the earlyram data overlaps with the program data.
Building on a trembyle I get the following: Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
I did have to increase the C_ENV_BOOTBLOCK_SIZE on my platform so it would compile.
Raul Rangel has uploaded a new patch set (#24) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 6 files changed, 67 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/24
Raul Rangel has uploaded a new patch set (#25) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 6 files changed, 67 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/25
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 25: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/25//COMMIT_MSG@40 PS25, Line 40: reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 Any reason why the reset vector is not kept along with the .text?
https://review.coreboot.org/c/coreboot/+/35035/25/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/25/src/arch/x86/early_ram.ld@... PS25, Line 20: regions Also, the FMAP_CACHE (if available)?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/25//COMMIT_MSG@40 PS25, Line 40: reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0
Any reason why the reset vector is not kept along with the . […]
The reset vector is placed in its own region: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...
In theory .text could be moved to the top of bootblock and we could have the reset vector be the last thing in there. This might be what https://review.coreboot.org/c/coreboot/+/37895 tries to solve.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/25//COMMIT_MSG@40 PS25, Line 40: reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0
The reset vector is placed in its own region: https://source.chromium. […]
SGTM. We can start with this and follow-up with https://review.coreboot.org/c/coreboot/+/37895.
Raul Rangel has uploaded a new patch set (#26) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 6 files changed, 66 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/26
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/25/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/25/src/arch/x86/early_ram.ld@... PS25, Line 20: regions
Also, the FMAP_CACHE (if available)?
Done
Raul Rangel has uploaded a new patch set (#27) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 68 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/27
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 27:
Modified the code so bootblock.bin is not added to cbfs anymore:
$ cbfstool /tmp/coreboot/trembyle-upstream/coreboot.rom print FMAP REGION: COREBOOT Name Offset Type Size Comp cbfs master header 0x0 cbfs header 32 none fallback/romstage 0x80 stage 82148 none fallback/ramstage 0x141c0 stage 90638 none config 0x2a440 raw 1524 none revision 0x2aa80 raw 696 none spd.bin 0x2ad80 spd 1024 none (empty) 0x2b1c0 null 3544 none fspm.bin 0x2bfc0 fsp 720896 none (empty) 0xdc000 null 3992 none fsps.bin 0xdcfc0 fsp 327680 none pci1002,15d8.rom 0x12d000 optionrom 54272 none fallback/dsdt.aml 0x13a480 raw 10995 none (empty) 0x13cfc0 null 1499096 none apu/amdfw 0x2aafc0 raw 1219584 none (empty) 0x3d4c00 null 746456 none
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 27:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/27/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/27/Makefile.inc@1032 PS27, Line 1032: ifneq ($(CONFIG_RESET_VECTOR_IN_RAM),y) These are separate semantic concepts:
1. reset vector in ram 2. add bootblock to cbfs
This isn't the right check. It should be a separate Kconfig check.
I can fix it as a follow up.
https://review.coreboot.org/c/coreboot/+/35035/27/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/27/src/include/memlayout.h@62 PS27, Line 62: 4 This should probably be 16 bytes and assume the stack size is a multiple of 16 bytes.
Raul Rangel has uploaded a new patch set (#28) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/include/arch/memlayout.h M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 8 files changed, 71 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/28
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35035/27/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/27/Makefile.inc@1032 PS27, Line 1032: ifneq ($(CONFIG_RESET_VECTOR_IN_RAM),y)
These are separate semantic concepts: […]
Ack
https://review.coreboot.org/c/coreboot/+/35035/27/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/35035/27/src/include/memlayout.h@62 PS27, Line 62: 4
This should probably be 16 bytes and assume the stack size is a multiple of 16 bytes.
Done. I added a new #define for it.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc@1055 PS28, Line 1055: $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS) CB:34477 has a clue the very end of coreboot.rom will be modified by cbfstool. If you don't place actual bootblock.bin there you should still prevent it from being used for something else.
I was actually planning on hitting revert for CB:34477, it was submitted for the purpose of (now abandoned) hybrid-romstage approach.
Maybe this is something to discuss with keeping compatible .id section there at the end for flashrom and switching to top-aligned bootblock program.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc@1055 PS28, Line 1055: $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS)
CB:34477 has a clue the very end of coreboot.rom will be modified by cbfstool. […]
Kyösti, thanks for the reminder on that. I'm not a fan of the 'add-master-header' as it is because of the manipulation of content not really a part of the program that resides there. I do agree we need to sort out the .id section as well. I think this add-master-header should also be optional. I'm not sure you should revert CB:34477 just yet.
That said, picasso's boot architecture doesn't necessitate bootblock (or whatever the first thing x86 runs) being in cbfs proper. However, that's going to take some amdfwtool changes to make such a setup work because it's my understanding that utility just puts *everything* into one block.
I'm going to remove these guards about adding bootblock.bin and push a new patch.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc@1055 PS28, Line 1055: $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS)
Kyösti, thanks for the reminder on that. […]
It's just going to leave a duplicate copy of bootblock in cbfs for the time being, and I think that's ok.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc@1055 PS28, Line 1055: $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS)
It's just going to leave a duplicate copy of bootblock in cbfs for the time being, and I think that' […]
And I guess you can revert CB:34477 when no one is using HAVE_BOOTBLOCK any longer which should be when CB:37490 is submitted. I'll push a revert for that CL in the train.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/28/Makefile.inc@1055 PS28, Line 1055: $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS)
And I guess you can revert CB:34477 when no one is using HAVE_BOOTBLOCK any longer which should be w […]
On second thought, I'll leave it. We can convert it to a INCLUDE_BOOTBLOCK_IN_CBFS Kconfig later.
Aaron Durbin has uploaded a new patch set (#29) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/include/arch/memlayout.h M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 69 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35035/29
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 29: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 29: Code-Review+2
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
arch/x86: Implement RESET_VECTOR_IN_RAM
Add support for devices with the reset vector pointing into DRAM. This is a specific implementation that assumes a paradigm of AMD Family 17h (a.k.a. "Zen"). Until the first ljmpl for protected mode, the core's state appears to software like other designs, and then the actual physical addressing becomes recognizable.
These systems cannot implement cache-as-RAM as in more traditional x86 products. Therefore instead of reusing CAR names and variables, a substitute called "earlyram" is introduced. This change makes adjustments to CAR-aware files accordingly.
Enable NO_XIP_EARLY_STAGES. The first stage is already in DRAM, and running subsequent stages as XIP in the boot device would reduce performance.
Finally, add a new early_ram.ld linker file. Because all stages run in DRAM, they can be linked with their .data and .bss as normal, i.e. they don't need to rely on storage available only at a fixed location like CAR systems. The primary purpose of the early_ram.ld is to provide consistent locations for PRERAM_CBMEM_CONSOLE, TIMESTAMP regions, etc. across stages until cbmem is brought online.
BUG=b:147042464 TEST=Build for trembyle, and boot to ramstage. $ objdump -h cbfs/fallback/bootblock.debug Idx ,Name ,Size ,VMA ,LMA ,File off Algn 0 ,.text ,000074d0 ,08076000 ,08076000 ,00001000 2**12 1 ,.data ,00000038 ,0807d4d0 ,0807d4d0 ,000084d0 2**2 2 ,.bss ,00000048 ,0807d508 ,0807d508 ,00008508 2**2 3 ,.stack ,00000800 ,0807daf0 ,0807daf0 ,00000000 2**0 4 ,.persistent ,00001cfa ,0807e2f0 ,0807e2f0 ,00000000 2**0 5 ,.reset ,00000010 ,0807fff0 ,0807fff0 ,0000aff0 2**0 6 ,.debug_info ,0002659c ,00000000 ,00000000 ,0000b000 2**0 7 ,.debug_abbrev ,000074a2 ,00000000 ,00000000 ,0003159c 2**0 8 ,.debug_aranges,00000dd0 ,00000000 ,00000000 ,00038a40 2**3 9 ,.debug_line ,0000ad65 ,00000000 ,00000000 ,00039810 2**0 10 ,.debug_str ,00009655 ,00000000 ,00000000 ,00044575 2**0 11 ,.debug_loc ,0000b7ce ,00000000 ,00000000 ,0004dbca 2**0 12 ,.debug_ranges ,000029c0 ,00000000 ,00000000 ,00059398 2**3
Change-Id: I9c084ff6fdcf7e9154436f038705e8679daea780 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35035 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/Kconfig M src/arch/x86/assembly_entry.S A src/arch/x86/early_ram.ld M src/arch/x86/include/arch/memlayout.h M src/arch/x86/memlayout.ld M src/cpu/Kconfig M src/include/memlayout.h 7 files changed, 69 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 21107aa..11733bd 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -89,9 +89,10 @@ config RESET_VECTOR_IN_RAM bool depends on ARCH_X86 + select NO_XIP_EARLY_STAGES help - Select this option if the x86 soc implements custom code to handle the - reset vector in RAM instead of the traditional 0xfffffff0 location. + Select this option if the x86 processor's reset vector is in + preinitialized DRAM instead of the traditional 0xfffffff0 location.
# Aligns 16bit entry code in bootblock so that hyper-threading CPUs # can boot AP CPUs to enable their shared caches. @@ -206,6 +207,7 @@ config POSTCAR_STAGE def_bool y depends on ARCH_X86 + depends on !RESET_VECTOR_IN_RAM
config VERSTAGE_DEBUG_SPINLOOP bool diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index f36e7da..59b34c8 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -9,6 +9,13 @@ * continue with C code execution one needs to set stack pointer and * clear .bss variables that are stage specific. */ + +#if CONFIG(RESET_VECTOR_IN_RAM) + #define _STACK_TOP _eearlyram_stack +#else + #define _STACK_TOP _ecar_stack +#endif + .section ".text._start", "ax", @progbits .global _start _start: @@ -16,8 +23,8 @@ /* Migrate GDT to this text segment */ call gdt_init
- /* reset stack pointer to CAR stack */ - mov $_ecar_stack, %esp + /* reset stack pointer to CAR/EARLYRAM stack */ + mov $_STACK_TOP, %esp
/* clear .bss section as it is not shared */ cld diff --git a/src/arch/x86/early_ram.ld b/src/arch/x86/early_ram.ld new file mode 100644 index 0000000..941c385 --- /dev/null +++ b/src/arch/x86/early_ram.ld @@ -0,0 +1,43 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +/* This file is included inside a SECTIONS block */ + +_STACK_SIZE = CONFIG_EARLYRAM_BSP_STACK_SIZE; +_ = ASSERT(_STACK_SIZE > 0x0, "EARLYRAM_BSP_STACK_SIZE is not configured"); + +_CONSOLE_SIZE = CONFIG_PRERAM_CBMEM_CONSOLE_SIZE; +_ = ASSERT(_CONSOLE_SIZE > 0x0, "PRERAM_CBMEM_CONSOLE_SIZE is not configured"); + +_TIMESTAMPS_SIZE = 0x200; +#if !CONFIG(NO_FMAP_CACHE) +_FMAP_SIZE = FMAP_SIZE; +#else +_FMAP_SIZE = 0; +#endif + +/* + * The PRERAM_CBMEM_CONSOLE, TIMESTAMP, and FMAP_CACHE regions are shared + * between the pre-ram stages (bootblock, romstage, etc). We need to assign a + * fixed size and consistent link address so they can be shared between stages. + * + * The stack area is not shared between stages, but is defined here for + * convenience. + */ +. = CONFIG_X86_RESET_VECTOR - ARCH_STACK_ALIGN_SIZE - _STACK_SIZE - _CONSOLE_SIZE - _TIMESTAMPS_SIZE - _FMAP_SIZE; + +_ = ASSERT(. > _eprogram, "Not enough room for .earlyram.data. Try increasing C_ENV_BOOTBLOCK_SIZE, or decreasing either EARLYRAM_BSP_STACK_SIZE or PRERAM_CBMEM_CONSOLE_SIZE."); + +.stack ALIGN(ARCH_STACK_ALIGN_SIZE) (NOLOAD) : { + EARLYRAM_STACK(., _STACK_SIZE) +} + +.persistent ALIGN(ARCH_POINTER_ALIGN_SIZE) (NOLOAD) : { + PRERAM_CBMEM_CONSOLE(., _CONSOLE_SIZE) + TIMESTAMP(., _TIMESTAMPS_SIZE) + #if !CONFIG(NO_FMAP_CACHE) + FMAP_CACHE(., FMAP_SIZE) + #endif +} + +_ = ASSERT(. <= CONFIG_X86_RESET_VECTOR, "Earlyram data regions don't fit below the reset vector!"); diff --git a/src/arch/x86/include/arch/memlayout.h b/src/arch/x86/include/arch/memlayout.h index 2eea83f..34d1bd2 100644 --- a/src/arch/x86/include/arch/memlayout.h +++ b/src/arch/x86/include/arch/memlayout.h @@ -8,4 +8,7 @@ # error "CONFIG_RAMTOP not configured" #endif
+/* Intel386 psABI requires a 16 byte aligned stack. */ +#define ARCH_STACK_ALIGN_SIZE 16 + #endif /* __ARCH_MEMLAYOUT_H */ diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 5e1ef24..31767b3 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -9,7 +9,7 @@ #if ENV_CACHE_AS_RAM #define EARLY_MEMLAYOUT "car.ld" #else -#error "Early DRAM environment for x86 is work-in-progress. */ +#define EARLY_MEMLAYOUT "early_ram.ld" #endif #endif
@@ -53,7 +53,9 @@ /* Bootblock specific scripts which provide more SECTION directives. */ #include <cpu/x86/16bit/entry16.ld> #include <cpu/x86/16bit/reset16.ld> +#if !CONFIG(RESET_VECTOR_IN_RAM) #include <arch/x86/id.ld> +#endif #if CONFIG(CPU_INTEL_FIRMWARE_INTERFACE_TABLE) #include <cpu/intel/fit/fit.ld> #endif diff --git a/src/cpu/Kconfig b/src/cpu/Kconfig index c1b84f9..933e50f 100644 --- a/src/cpu/Kconfig +++ b/src/cpu/Kconfig @@ -15,6 +15,10 @@ config DCACHE_BSP_STACK_SIZE hex
+config EARLYRAM_BSP_STACK_SIZE + depends on RESET_VECTOR_IN_RAM + hex + config SMP bool default y if MAX_CPUS != 1 diff --git a/src/include/memlayout.h b/src/include/memlayout.h index bef3637..af277ea 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -58,6 +58,9 @@ #define PRERAM_CBMEM_CONSOLE(addr, size) \ REGION(preram_cbmem_console, addr, size, 4)
+#define EARLYRAM_STACK(addr, size) \ + REGION(earlyram_stack, addr, size, ARCH_STACK_ALIGN_SIZE) + /* Use either CBFS_CACHE (unified) or both (PRERAM|POSTRAM)_CBFS_CACHE */ #define CBFS_CACHE(addr, size) \ REGION(cbfs_cache, addr, size, 4) \
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Matt Papageorge (Code Review)
-
Nico Huber (Code Review)
-
Raul Rangel (Code Review)