Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32414
Change subject: arch/x86: Add option for running Romstage out of RAM ......................................................................
arch/x86: Add option for running Romstage out of RAM
AMD's Picasso SOC brings up memory before releasing the X86 processor, and jumps directly to Romstage. Add a global config option to handle that.
TEST=None BUG=b:130804851
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M src/Kconfig M src/arch/x86/assembly_entry.S M src/arch/x86/memlayout.ld 3 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/1
diff --git a/src/Kconfig b/src/Kconfig index 62b3818..bfa0b51 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -146,6 +146,16 @@ time spent decompressing. Doesn't work for XIP stages (assume all ARCH_X86 for now) for obvious reasons.
+config ROMSTAGE_IN_RAM + bool + depends on ARCH_X86 + help + Some newer x86 processors come alive with memory enabled, and the + reset vector's physical address falling within DRAM. Select this + item to build romstage to execute in DRAM instead of XIP. Because + this is not a standard, the soc/ or cpu/ code must handle the reset + natively before jumping into the standard romstage code. + config COMPRESS_BOOTBLOCK bool help diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 4ead9ea..01c0865 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -26,6 +26,16 @@ * variables that are stage specific. */ .section ".text._start", "ax", @progbits +#if ENV_ROMSTAGE && IS_ENABLED(CONFIG_ROMSTAGE_IN_RAM) +/* + * Systems that run romstage in DRAM, i.e. where romstage is the first + * executed code, are responsible for getting the processor into protected + * mode, setting the stack pointer, and jumping to this location. + */ +.global _romstage_in_ram_continue +_romstage_in_ram_continue: + +#else .global _start _start:
@@ -34,6 +44,7 @@
/* reset stack pointer to CAR stack */ mov $_car_stack_end, %esp +#endif
/* clear CAR_GLOBAL area as it is not shared */ cld diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index cc72552..da63b05 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -57,6 +57,10 @@ #endif }
+#if ENV_ROMSTAGE && CONFIG(ROMSTAGE_IN_RAM) +#include <soc/romstage.ld> +#endif + #if ENV_BOOTBLOCK /* Bootblock specific scripts which provide more SECTION directives. */ #include <cpu/x86/16bit/entry16.ld>
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running Romstage out of RAM ......................................................................
Patch Set 1:
Can you describe at a high level how the boot flow for this platform will look like? What are your reasons for running the romstage from DRAM rather than skipping it entirely?
Also, in the other patch you mention that bootblock and verstage may run on the PSP... how is it going to transition from there? Would it maybe make sense to run the romstage on the PSP instead?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running Romstage out of RAM ......................................................................
Patch Set 1:
(5 comments)
As this is quite a big design change for coreboot, maybe it should be brought up on the mailing list.
https://review.coreboot.org/#/c/32414/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32414/1//COMMIT_MSG@7 PS1, Line 7: Romstage romstage
https://review.coreboot.org/#/c/32414/1//COMMIT_MSG@10 PS1, Line 10: Romstage romstage
https://review.coreboot.org/#/c/32414/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/32414/1/src/Kconfig@153 PS1, Line 153: x86 You spell x86 differently X86 in the commit messages, I believe.
https://review.coreboot.org/#/c/32414/1/src/Kconfig@154 PS1, Line 154: falling is falling? falls?
https://review.coreboot.org/#/c/32414/1/src/arch/x86/assembly_entry.S File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/#/c/32414/1/src/arch/x86/assembly_entry.S@29 PS1, Line 29: IS_ENABLED CONFIG
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running Romstage out of RAM ......................................................................
Patch Set 1:
Patch Set 1:
Can you describe at a high level how the boot flow for this platform will look like? What are your reasons for running the romstage from DRAM rather than skipping it entirely?
Also, in the other patch you mention that bootblock and verstage may run on the PSP... how is it going to transition from there? Would it maybe make sense to run the romstage on the PSP instead?
It was going to take significant additional changes to coreboot to begin x86 execution with ramstage (e.g. cbmem is assumed already to be online). As a result, we chose to do a small romstage, at least for now.
Here's the current general flow - I'm working on a design doc and will publish something both internally and at coreboot.org about the boot flow.
- Power On - PSP starts from On-Chip firmware - PSP loads minimal off-chip firmware from the SPI ROM. - PSP tries to load coreboot verstage from SPI. - If verstage is available, it does its verification and passes the AMD firmware directory table for the chosen FMAP region back to the PSP to load everything else. - If verstage is not available, the PSP continues from the original AMD firmware directory table. - PSP loads the ABL (AGESA Boot Loader) code and runs that. - The ABL loads the APCB (AGESA PSP Customization Block), a structure containing the setup for AGESA running in the PSP. This contains a number of configuration options as well as the information how to get the SPDs. - ABL initializes memory. I discussed with AMD whether we could continue to let coreboot do the memory initialization instead of the PSP, but the ability to do Cache-as-RAM has been removed, so while it might be possible, it would have to be all written in ASM. - PSP looks up the code destinations in the AMD firmware directory table then decompresses and copies the rest of the FMAP region into memory. It fixes up the x86 CS descriptor to appear to BIOS as any x86 does coming out of reset, except CS's base points to DRAM and not flash. The base is romstage_base + romstage_size - 64KB and IP=0xfff0. - PSP jumps to the x86 reset vector, in this case coreboot's romstage.
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32414
to look at the new patch set (#2).
Change subject: arch/x86: Add option for running romstage out of RAM ......................................................................
arch/x86: Add option for running romstage out of RAM
AMD's Picasso SOC brings up memory before releasing the x86 processor, and jumps directly torRomstage. Add a global config option to handle that.
TEST=None BUG=b:130804851
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M src/Kconfig M src/arch/x86/assembly_entry.S M src/arch/x86/memlayout.ld 3 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage out of RAM ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/#/c/32414/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32414/1//COMMIT_MSG@7 PS1, Line 7: Romstage
romstage
Done
https://review.coreboot.org/#/c/32414/1//COMMIT_MSG@10 PS1, Line 10: Romstage
romstage
Done
https://review.coreboot.org/#/c/32414/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/32414/1/src/Kconfig@153 PS1, Line 153: x86
You spell x86 differently X86 in the commit messages, I believe.
Fixed in the commit message.
https://review.coreboot.org/#/c/32414/1/src/Kconfig@154 PS1, Line 154: falling
is falling? falls?
Done
https://review.coreboot.org/#/c/32414/1/src/arch/x86/assembly_entry.S File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/#/c/32414/1/src/arch/x86/assembly_entry.S@29 PS1, Line 29: IS_ENABLED
CONFIG
Done. Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage out of RAM ......................................................................
Patch Set 2:
(2 comments)
Here's the current general flow - I'm working on a design doc and will publish something both internally and at coreboot.org about the boot flow.
Is this doc ready? That would be helpful to understand the changes that are being made here.
https://review.coreboot.org/#/c/32414/2/src/arch/x86/assembly_entry.S File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/#/c/32414/2/src/arch/x86/assembly_entry.S@19 PS2, Line 19: #if CONFIG(C_ENVIRONMENT_BOOTBLOCK) So,these boards are supposed to select C_ENVIRONMENT_BOOTBLOCK even if they won't have a true bootblock?
https://review.coreboot.org/#/c/32414/2/src/arch/x86/assembly_entry.S@49 PS2, Line 49: CAR_GLOBAL If the ROMSTAGE starts in RAM, why is CAR_GLOBAL area required?
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage out of RAM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32414/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32414/2//COMMIT_MSG@10 PS2, Line 10: r Remove this extra r here.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32414/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32414/2//COMMIT_MSG@10 PS2, Line 10: r
Remove this extra r here.
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 5:
General boot-flow document: https://doc.coreboot.org/soc/amd/family17h.html
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 5:
General boot-flow document: https://doc.coreboot.org/soc/amd/family17h.html
Thanks for providing that doc, it makes many things clearer. However, I still don't really follow why running a separate romstage (and apparently also postcar?) binary in DRAM is the best/easiest way to model this boot flow. The only real reason you provide is to run ROMSTAGE_CBMEM_INIT_HOOKs, but almost all those hooks exist to copy data from pre-RAM storage into CBMEM. You don't have that pre-RAM storage and you don't have any existing data you want to copy, so why do you want to run the hooks? Most code doing that (e.g. CBMEM console, timestamps) should be written robustly enough that it should "just work" if you only start running it in ramstage (because usually they just check if the CBMEM space exists and reinitialize it from scratch if it doesn't, no matter the stage), and if there's any code that doesn't it would probably be easier and cleaner to fix that than to build a fake romstage environment for it. The only hook I can find in common code that does anything you may actually need to run is some of the vboot stuff, and you have to do some work there anyway because you ran vboot on another processor with its own memory. (How are you passing vboot data from the verstage forward, anyway? Your doc doesn't address that, but it's definitely something you'll have to figure out somehow.)
If you're worried about initializing CBMEM itself, ramstage already does that (in lib/hardwaremain.c#main). It will reinitialize CBMEM as empty if there's no previous record in memory. (If you're worried about recovering stale CBMEM data, it's just one line that needs to be changed to cbmem_initialize_empty(), selected by a Kconfig.)
All the other stuff you're doing (e.g. switching to protected mode) is custom to your platform regardless (romstage normally starts in protected mode already). Inserting that at the start of ramstage shouldn't be any harder than inserting it at the start of romstage?
(I'm also curious about how verstage can interact with the rest of the PSP boot flow. Can an RW update be used to update the ABL code or the APCB data? That's not really clear from the doc.)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 5:
Patch Set 5:
General boot-flow document: https://doc.coreboot.org/soc/amd/family17h.html
Thanks for providing that doc, it makes many things clearer. However, I still don't really follow why running a separate romstage (and apparently also postcar?) binary in DRAM is the best/easiest way to model this boot flow.
It's the way we have working currently. Ultimately we may try to go back and move everything into ramstage, but that presents additional problems. We realize that there are issues with our current approach, but at least for now we still need romstage, as it's required to load AGESA.
Besides, if if we do everything in ramstage and Ron removes ramstage for linuxboot, what's left?
...
(I'm also curious about how verstage can interact with the rest of the PSP boot flow. Can an RW update be used to update the ABL code or the APCB data? That's not really clear from the doc.)
verstage will be called from the PSP, run, and pass back the pointer to the correct directory (RW-A, RW-B or RO). The PSP will load all the rest of the configuration, including the APCB from that directory.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 5:
Hey Julius, thanks for your thoughts. Besides what Martin said, I have a number of other items on my mind. Some were hinted at in the documentation and have not made their way into the patch stack yet. But we're trying to get this system up in a short amount of time, minimize the number of unforeseen pitfalls, and with minimal disruption to coreboot's x86 methodology. We've started capturing ideas for ways to streamline the bootflow once the system is fully functional, and ultimately I'd hope to pull this particular change back out once it's no longer useful.
All the other stuff you're doing (e.g. switching to protected mode) is custom to your platform regardless (romstage normally starts in protected mode already). Inserting that at the start of ramstage shouldn't be any harder than inserting it at the start of romstage?
Right, creating a hybrid ramstage would be no harder than a hybrid romstage, I think. Either way, it's a custom solution for Family 17h though.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
(3 comments)
I feel this messes up the 'big picture' of CAR and romstage. Can't you just use custom linker script in place of car.ld?
You already have several TODOs left behind like bootblock-y += cache_as_ram.S. Last time someone approved this from AMD contractors, it took 5 years or more to have them addressed. And it was not AMD contractor who sorted it out.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue: Comment says it's a jump. So you could just define the label without any preprocessor stuff here?
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */ None of this CAR is really correct when you already have DRAM...
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 61: #include <soc/romstage.ld> To include car.ld for this concept seems invalid to me. Also, we have ENV_CACHE_AS_RAM that should be adjusted accordingly and you should have CACHE_AS_RAM=n in your .config.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
(2 comments)
You already have several TODOs left behind like bootblock-y += cache_as_ram.S
Not in this patch.
MD: We've started capturing ideas for ways to streamline the bootflow once the system is fully functional, and ultimately I'd hope to pull this particular change back out once it's no longer useful.
The TODOs are indicators of what will require some pretty substantial work, and the intent to do so. I don't mind rewording it, however "todo" is a fairly common search term.
To include car.ld for this concept seems invalid to me. Also, we have ENV_CACHE_AS_RAM that should be adjusted accordingly and you should have CACHE_AS_RAM=n in your .config.
In my opinion, there are many things that are less than ideal with this initial approach. coreboot makes lots of assumptions for how an x86 behaves and they don't fit well with how Family 17h comes out of reset. When challenged with both porting new technology (which intends to rely on reference code that's not yet functional) and into a codebase that has no established paradigm for how it operates...
That's how we arrived at this approach, i.e. to make it functional first but with absolute minimal disruption to the non-amd directories. Otherwise, more stuff breaks than you can imagine by only looking at these patches. Once it's fully functional, then the changes required to make picasso a good port will be more understandable.
The alternative was to modify coreboot first before pushing picasso. However that seems like a much more difficult argument for a unique processor where no reviewers would be allowed to know how it works. I don't think people would be willing to consider those types of x86 patches without understanding the premise.
Last time someone approved this from AMD contractors, it took 5 years or more to have them addressed. And it was not AMD contractor who sorted it out.
I try not to get upset over things I can't change.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue:
Comment says it's a jump. […]
It's because the preprocessor removes _start from this file.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */
None of this CAR is really correct when you already have DRAM...
Completely agree. See non-inline comments.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig@161 PS6, Line 161: XIP XIP from boot media.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 43: call gdt_init Wouldn't we want gdt init as well? Or a way to supply a different set of descriptors? I'd expect them to be consistent.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */
Completely agree. See non-inline comments.
In this case it's effectively .bss which we can update the comments to reflect that. How is romstage being built and loaded? Is it only PROGBITS being loaded?
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 60: #if ENV_ROMSTAGE && CONFIG(ROMSTAGE_IN_RAM) Why isn't this line up near line 37?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 32: * executed code, are responsible for getting the processor into protected Who exactly is responsible? Systems? (reset_vector.S in followup, it seems)
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue:
It's because the preprocessor removes _start from this file.
Ah. I don't see why you need to define _start in CB:33759 at reset vector location. It used to be a symbol in protected mode, but is now located in realmode?
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 43: call gdt_init
Wouldn't we want gdt init as well? Or a way to supply a different set of descriptors? I'd expect the […]
I see some GDT init in followup reset_vector.S already.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 55: rep stosl Isn't this now redundant with wiping _car_region_start .. _end in reset_vector.S?
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 72: call car_stage_entry At the end of the day, does this entire file always reduce to the last call?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
Is it possible to smash some CLs together so it's easier to look at all the moving pieces? It's hard to flip through the CLs to see the whole picture. I think the requirements would be easier to assess by looking at it as a whole.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
Patch Set 6:
(2 comments)
You already have several TODOs left behind like bootblock-y += cache_as_ram.S
Not in this patch.
Right. I have little reason to be happy about CB:32413 being submitted with the amount of review it had. Sure, there were +2's given... Please file bugs on every TODO you leave behind so they eventually get assigned to someone and you don't forget about them.
MD: We've started capturing ideas for ways to streamline the bootflow once the system is fully functional, and ultimately I'd hope to pull this particular change back out once it's no longer useful.
Which is sort of admitting that this entire change is one big TODO.
The TODOs are indicators of what will require some pretty substantial work, and the intent to do so. I don't mind rewording it, however "todo" is a fairly common search term.
To include car.ld for this concept seems invalid to me. Also, we have ENV_CACHE_AS_RAM that should be adjusted accordingly and you should have CACHE_AS_RAM=n in your .config.
In my opinion, there are many things that are less than ideal with this initial approach. coreboot makes lots of assumptions for how an x86 behaves and they don't fit well with how Family 17h comes out of reset. When challenged with both porting new technology (which intends to rely on reference code that's not yet functional) and into a codebase that has no established paradigm for how it operates...
It has been known since like 2015 that these platforms will start romstage from DRAM. The on-going review will resolve how big a mess you will be allowed to (temporarily) leave behind in common directories until more optimal solution is available.
From what I know, amd/picasso is largely result of pair-programming and as such, I classify Martin acking Marshall's work as self-approval and vice-versa. I would appreciate if you did not submit code to arch/x86 without 'substantial' review votes and defer yourselves from self-approvals. Otherwise it becomes an insult towars the transparency of the review process itself.
That's how we arrived at this approach, i.e. to make it functional first but with absolute minimal disruption to the non-amd directories. Otherwise, more stuff breaks than you can imagine by only looking at these patches. Once it's fully functional, then the changes required to make picasso a good port will be more understandable.
I am fine with the approach. Just that I don't see the need to modify and include arch/x86/assembly_entry.S in your romstage at all.
The alternative was to modify coreboot first before pushing picasso. However that seems like a much more difficult argument for a unique processor where no reviewers would be allowed to know how it works. I don't think people would be willing to consider those types of x86 patches without understanding the premise.
Last time someone approved this from AMD contractors, it took 5 years or more to have them addressed. And it was not AMD contractor who sorted it out.
I try not to get upset over things I can't change.
Things you put in coreboot proper are things you can change. Trouble is when you try land TODOs in common code and they become PITA to everyone else.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 19: C_ENVIRONMENT_BOOTBLOCK I know this is for stages post bootblock where the platform uses C environment for bootblock. Just thinking out loud here. It looks like the platform would still have to select this config even though it does not have a real bootblock.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 23: The gdt is reloaded to accommodate : * platforms that are executing out of CAR. Comments in this file need to be updated in general to account for the changes being made.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 55: rep stosl
Isn't this now redundant with wiping _car_region_start .. _end in reset_vector. […]
Yes, looks like this is being done twice for this platform.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 60: #if ENV_ROMSTAGE && CONFIG(ROMSTAGE_IN_RAM)
Why isn't this line up near line 37?
Probably because romstage.ld in the follow-up patch provides .reset which was being done by reset16.ld here?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
Patch Set 6:
(2 comments)
You already have several TODOs left behind like bootblock-y += cache_as_ram.S
Not in this patch.
CB:34477 CB:34478
I think the documentation should have made it more clear that verstage will be executing on the Security Processor, not the x86. If this implies it will not be possible to boot with custom verstage builds (due to the signing procedure on PSP binaries) please add such information in the documentation.
People may be restricted to the more traditional verstage between bootblock and romstage -approach because of this. You would then want the first stage as read-only, small, and not call any blobs. In other words, reset vector would go back into bootblock.
Marshall Dawson has uploaded a new patch set (#7) to the change originally created by Martin Roth. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
arch/x86: Add option for using a custom assembly_entry
Create a new Kconfig symbol that allows an x86 cpu or soc use its own assembly entry code for romstage. Add an include into memlayout.ld to allow the soc to insert additional definitions.
TEST=None BUG=b:130804851
Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/memlayout.ld 3 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/7
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 6:
(11 comments)
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig@161 PS6, Line 161: XIP
XIP from boot media.
Changed my approach.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 19: C_ENVIRONMENT_BOOTBLOCK
I know this is for stages post bootblock where the platform uses C environment for bootblock. […]
Correct. BTW, even though the change to this file goes away in the next PS, I still need to select C_ENVIRONMENT_BOOTBLOCK for the system to run properly. This was one of the little gotchas that result from using romstage source.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 23: The gdt is reloaded to accommodate : * platforms that are executing out of CAR.
Comments in this file need to be updated in general to account for the changes being made.
Will be OBE in the next PS.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 32: * executed code, are responsible for getting the processor into protected
reset_vector.S in followup
Right.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue:
Ah. I don't see why you need to define _start in CB:33759 at reset vector location. […]
I could never link with two _start symbols, and I forget precisely the reason I needed it in the other file. It seems like when it was named differently, even if .globl, the linker wouldn't find it, or wouldn't position it properly -- it was something like that. So I went with _start = first instruction.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 43: call gdt_init
I see some GDT init in followup reset_vector.S already.
Right. Ideally I would've enjoyed starting inside this file at _start but (1) I already initialized the GDT, (2) I'm putting BIST and an initial timestamp on the stack. Line 46 would reset the pointer and wipe out the info.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */
How is romstage being built and loaded? Is it only PROGBITS being loaded?
Right, it's only PROGBITS: https://review.coreboot.org/c/coreboot/+/33401.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 55: rep stosl
Isn't this now redundant with wiping _car_region_start .. _end in reset_vector. […]
Agreed, it's executing the stosl 14 times. Wasn't ideal but I was trying to minimize changes to this file. Goes away in next PS.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 72: call car_stage_entry
At the end of the day, does this entire file always reduce to the last call?
Yes, well starting at line 50. My thinking was to preserve the ability to enable the options above, as well not make too many assumptions for how a competing similar product may behave.
BTW, I'd already tried adding the lines above to picasso/reset_vector.S before noticing your comments in the later patch. The next PS will use that instead of this file.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 60: #if ENV_ROMSTAGE && CONFIG(ROMSTAGE_IN_RAM)
Why isn't this line up near line 37?
Sounds good and I can remove "SECTIONS" from my soc/.ld file.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 61: #include <soc/romstage.ld>
To include car.ld for this concept seems invalid to me. […]
That's one of the examples of things you'd expect to work that don't. I want to get the platform up first, then make things outside of soc/amd/picasso more flexible where they make sense.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 7:
Marshall, If you've addressed all the comments, could you mark them as resolved?
Aaron, Kyösti, Furquan, Julius, are there any additional comments or questions? Is this patch ready at this point?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 7:
Aaron, Kyösti, Furquan, Julius, are there any additional comments or questions? Is this patch ready at this point?
Well, I'm still very unconvinced that this is the right direction to go about solving this in the first place, but that just seems to come down to opinion at this point. Since it's mostly x86 code I've decided to stay out of it and defer to the other reviewers now.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 7:
(15 comments)
https://review.coreboot.org/c/coreboot/+/32414/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/1/src/Kconfig@153 PS1, Line 153: x86
Fixed in the commit message.
Done
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig@161 PS6, Line 161: XIP
Changed my approach.
Ack
https://review.coreboot.org/c/coreboot/+/32414/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/1/src/arch/x86/assembly_entry... PS1, Line 29: IS_ENABLED
Done. Thanks.
Done
https://review.coreboot.org/c/coreboot/+/32414/2/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/2/src/arch/x86/assembly_entry... PS2, Line 19: #if CONFIG(C_ENVIRONMENT_BOOTBLOCK)
So,these boards are supposed to select C_ENVIRONMENT_BOOTBLOCK even if they won't have a true bootbl […]
I still can't boot w/o it. That's one of the things I want to fix later, as it'll take some careful consideration of how to make the generic x86 code more flexible.
https://review.coreboot.org/c/coreboot/+/32414/2/src/arch/x86/assembly_entry... PS2, Line 49: CAR_GLOBAL
If the ROMSTAGE starts in RAM, why is CAR_GLOBAL area required?
I'm using the FSP 2.0 driver later in the patch stack that won't currently function properly unless all the assumptions are in place.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 19: C_ENVIRONMENT_BOOTBLOCK
Correct. […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 23: The gdt is reloaded to accommodate : * platforms that are executing out of CAR.
Will be OBE in the next PS.
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 32: * executed code, are responsible for getting the processor into protected
reset_vector.S in followup […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue:
I could never link with two _start symbols, and I forget precisely the reason I needed it in the oth […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 43: call gdt_init
Right. […]
OBE in later PSs
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */
How is romstage being built and loaded? Is it only PROGBITS being loaded? […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 55: rep stosl
Agreed, it's executing the stosl 14 times. […]
OBE in later PSs
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 72: call car_stage_entry
Yes, well starting at line 50. […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 60: #if ENV_ROMSTAGE && CONFIG(ROMSTAGE_IN_RAM)
Sounds good and I can remove "SECTIONS" from my soc/.ld file.
Done
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 61: #include <soc/romstage.ld>
That's one of the examples of things you'd expect to work that don't. […]
Ack
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/7/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/7/src/arch/x86/memlayout.ld@3... PS7, Line 38: #include "car.ld" As noted earlier, I would rather see all references to CAR removed early on. And if your verstage/vboot shall not run on x86 many of the sections do not need to be defined. The file is also the reason for keeping ill-named DCACHE_RAM_xx around.
I am leaving it to someone else to evaluate if this is clean enough for initial support. I would rather arrange it with preprocessor to replace every occurence of car.ld in this file.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig@160 PS9, Line 160: for early stages. Things should be documented such that you need to have a _start symbol for linking.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig@160 PS9, Line 160: for early stages.
Things should be documented such that you need to have a _start symbol for linking.
Unsure precisely what you're requesting here. Should I add it to the help text that the implementer is responsible for providing the _start symbol? Or do you mean something in the Documentation directory? Maybe the discussion in https://review.coreboot.org/c/coreboot/+/33759 helps determine the right answer.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig@160 PS9, Line 160: for early stages.
Unsure precisely what you're requesting here. […]
It could just be here to help guide people who are thinking of utilizing the functionality. However, I do think you are right that we need to resolve the approach -- either in that CL or the bug.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 9:
(1 comment)
I quess you could have split this such that everything about postcar first got removed. But as long as this is going forwards..
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig@160 PS9, Line 160: for early stages.
It could just be here to help guide people who are thinking of utilizing the functionality. […]
Could this be named RESET_VECTOR_IN_DRAM? If so, you could use that to corrcetly leave ENV_CACHE_AS_RAM in <rules.h> unset.
Marshall Dawson has uploaded a new patch set (#10) to the change originally created by Martin Roth. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
arch/x86: Add option for using a custom assembly_entry
Create a new Kconfig symbol that allows an x86 cpu or soc use its own assembly entry code for romstage. Add an include into memlayout.ld to allow the soc to insert additional definitions.
TEST=None BUG=b:130804851
Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/memlayout.ld M src/cpu/x86/32bit/entry32.inc 4 files changed, 18 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/10
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/9/src/Kconfig@160 PS9, Line 160: for early stages.
Could this be named RESET_VECTOR_IN_DRAM?
I didn't like the corner that painted me into for this change. Added this instead: https://review.coreboot.org/c/coreboot/+/34913
Marshall Dawson has uploaded a new patch set (#12) to the change originally created by Martin Roth. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Introduce RESET_VECTOR_IN_DRAM option ......................................................................
x86: Introduce RESET_VECTOR_IN_DRAM option
Create a new Kconfig symbol that allows an x86 device to begin execution when its reset vector is in DRAM and not at the traditional 0xfffffff0.
This implementation assumes the soc or x86 code: * Builds on existing coreboot romstage infrastructure * Implements its own custom assembly entry * Provides all additional symbols and helpers required for linking
Because romstage features are used but the code is in DRAM, do not create a stage from romstage.elf, and do not include a stage into coreboot.rom. It is left to the soc or x86 implementation to best determine how to place the code in DRAM in time for the x86 to begin executing it at reset.
TEST=None BUG=b:130804851
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M Makefile.inc M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/memlayout.ld M src/cpu/x86/32bit/entry32.inc M src/include/rules.h 6 files changed, 21 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/12
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Introduce RESET_VECTOR_IN_DRAM option ......................................................................
Patch Set 12:
I have hijacked the src/Kconfig part on my branch setting up the ENV_ stuff. Perhaps rename this to "x86: Implement..."
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Introduce RESET_VECTOR_IN_DRAM option ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/12/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/12/src/arch/x86/memlayout.ld@... PS12, Line 38: #include "car.ld" Copied comment from abandoned CB:34913
KM: ... this RESET_VECTOR_IN_DRAM would also conditionalise uses of x86/car.ld in x86/memlayout.ld.
MD: I agree, however that causes a new problem -- not insurmountable but I'm still trying minimize changes outside the picasso directory. We're relying on the fsp2_0 driver that uses the symbols _car_relocatable_data_end and _car_region_end for what we need.
KM: Looks like preram_symbols_available() in symbols.h needs a fix too.
MD: Hmm, I overlooked that. However, since I need to keep them in my hybrid romstage for FSP anyway, I think that should stay as-is. It looks like vboot/common is the only place where that's used currently. The google implementation of vboot will run on the PSP and not the x86.
KM: new commentary With the lack of ENV_BOOTBLOCK, ENV_VERSTAGE and ENV_POSTCAR and with ENV_ROMSTAGE that you eventually might want to load as rmodule at dynamic address, I feel this entire memlayout.ld file might not be well suited for Picasso.
Marshall Dawson has uploaded a new patch set (#13) to the change originally created by Martin Roth. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Implement RESET_VECTOR_IN_DRAM option ......................................................................
x86: Implement RESET_VECTOR_IN_DRAM option
Create a new Kconfig symbol that allows an x86 device to begin execution when its reset vector is in DRAM and not at the traditional 0xfffffff0.
This implementation assumes the soc or x86 code: * Builds on existing coreboot romstage infrastructure * Implements its own custom assembly entry * Provides all additional symbols and helpers required for linking
Because romstage features are used but the code is in DRAM, do not create a stage from romstage.elf, and do not include a stage into coreboot.rom. It is left to the soc or x86 implementation to best determine how to place the code in DRAM in time for the x86 to begin executing it at reset.
TEST=None BUG=b:130804851
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M Makefile.inc M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/memlayout.ld M src/cpu/x86/32bit/entry32.inc M src/include/rules.h 6 files changed, 21 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/13
Marshall Dawson has uploaded a new patch set (#14) to the change originally created by Martin Roth. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Implement RESET_VECTOR_IN_DRAM option ......................................................................
x86: Implement RESET_VECTOR_IN_DRAM option
Add the ability for an x86 device to begin execution when its reset vector is in DRAM and not at the traditional 0xfffffff0.
This implementation assumes the soc or x86 code: * Builds on existing coreboot romstage infrastructure * Implements its own custom assembly entry * Provides all additional symbols and helpers required for linking
Because romstage features are used but the code is in DRAM, do not create a stage from romstage.elf, and do not include a stage into coreboot.rom. It is left to the soc or x86 implementation to best determine how to place the code in DRAM in time for the x86 to begin executing it at reset.
TEST=None BUG=b:130804851
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I678b1f74546ea30abcc655a0daed795d6cfa0034 --- M Makefile.inc M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/memlayout.ld M src/cpu/x86/32bit/entry32.inc M src/include/rules.h 6 files changed, 21 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32414/14
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Implement RESET_VECTOR_IN_DRAM option ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32414/14//COMMIT_MSG@21 PS14, Line 21: begin executing it at reset. Almost off-topic: If PSP shall run VBOOT, what is you plan about the toolchain used to build it? Proprietary or coreboot reference, or you throw the sources to AMD and get a signed blob back in return?
From GPL compliance guide:
Therefore, if you choose not to distribute the compiler, you should include a readme about where you got it, what version it was, and who to contact to acquire it, regardless of whether your compiler is FOSS, proprietary, or internally developed.
So, if you were to prepare coreboot toolchain anyways for verstage in PSP, I thought it would make sense to adapt rmodule loader such that PSP loads romstage as an rmodule.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Implement RESET_VECTOR_IN_DRAM option ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32414/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32414/14//COMMIT_MSG@21 PS14, Line 21: begin executing it at reset. I'm OOO so I can't dig into this for you. It seems the last I heard the intention was gcc but there's a high likelihood I'm wrong.
I thought it would make sense to adapt rmodule loader such that PSP loads romstage as an rmodule.
Although that's creative, I'm not aware there was any discussion of modifying the mechanism for the step of uncompressing into DRAM and starting execution. It just needs to run once it's there.
Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: x86: Implement RESET_VECTOR_IN_DRAM option ......................................................................
Abandoned
now covered by CB:35035 and CB:37491
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Sumeet R Pawnikar (Code Review)