Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33142
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Kconfig: Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. STAGE_BOOTBLOCK 2. STAGE_VERSTAGE 3. STAGE_ROMSTAGE 4. STAGE_RAMSTAGE
A stage can only be enabled if soc has selected its supported architecture(example: arm, ppc, riscv, x86)
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/1
diff --git a/src/Kconfig b/src/Kconfig index d30aa99..4c1fe32 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -18,6 +18,38 @@
menu "General setup"
+config STAGE_BOOTBLOCK + bool "Enable bootblock stage" + default y if ARCH_BOOTBLOCK_PPC64 || ARCH_BOOTBLOCK_ARMV8_64 || ARCH_BOOTBLOCK_ARM64 || ARCH_BOOTBLOCK_ARMV7 || ARCH_BOOTBLOCK_ARMV7_M || ARCH_BOOTBLOCK_ARMV7_R || ARCH_BOOTBLOCK_ARM || ARCH_BOOTBLOCK_ARMV4 || ARCH_BOOTBLOCK_RISCV || ARCH_BOOTBLOCK_X86_32 || ARCH_BOOTBLOCK_X86_64 || ARCH_BOOTBLOCK_MIPS + default n + help + This option is enabled if soc has selected any of supported + bootblock architecture (example: arm, ppc, riscv, x86). + +config STAGE_VERSTAGE + bool "Enable verstage stage" + default y if ARCH_VERSTAGE_PPC64 || ARCH_VERSTAGE_ARMV8_64 || ARCH_VERSTAGE_ARM64 || ARCH_VERSTAGE_ARMV7 || ARCH_VERSTAGE_ARMV7_M || ARCH_VERSTAGE_ARMV7_R || ARCH_VERSTAGE_ARM || ARCH_VERSTAGE_ARMV4 || ARCH_VERSTAGE_RISCV || ARCH_VERSTAGE_X86_32 || ARCH_VERSTAGE_X86_64 || ARCH_VERSTAGE_MIPS + default n + help + This option is enabled if soc has selected any of supported + verstage architecture (example: arm, ppc, riscv, x86). + +config STAGE_ROMSTAGE + bool "Enable romstage stage" + default y if ARCH_ROMSTAGE_PPC64 || ARCH_ROMSTAGE_ARMV8_64 || ARCH_ROMSTAGE_ARM64 || ARCH_ROMSTAGE_ARMV7 || ARCH_ROMSTAGE_ARMV7_R || ARCH_ROMSTAGE_ARM || ARCH_ROMSTAGE_ARMV4 || ARCH_ROMSTAGE_RISCV || ARCH_ROMSTAGE_X86_32 || ARCH_ROMSTAGE_X86_64 || ARCH_ROMSTAGE_MIPS + default n + help + This option is enabled if soc has selected any of supported + romstage architecture (example: arm, ppc, riscv, x86). + +config STAGE_RAMSTAGE + bool "Enable ramstage stage" + default y if ARCH_RAMSTAGE_PPC64 || ARCH_RAMSTAGE_ARMV8_64 || ARCH_RAMSTAGE_ARM64 || ARCH_RAMSTAGE_ARMV7 || ARCH_RAMSTAGE_ARMV7_R || ARCH_RAMSTAGE_ARM || ARCH_RAMSTAGE_ARMV4 || ARCH_RAMSTAGE_RISCV || ARCH_RAMSTAGE_X86_32 || ARCH_RAMSTAGE_X86_64 || ARCH_RAMSTAGE_MIPS + default n + help + This option is enabled if soc has selected any of supported + ramstage architecture (example: arm, ppc, riscv, x86). + config COREBOOT_BUILD bool default y
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33142/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/1/src/Kconfig@21 PS1, Line 21: config STAGE_BOOTBLOCK Can we add ENABLE or PRESENT to these names? I feel like they read better when used.
https://review.coreboot.org/#/c/33142/1/src/Kconfig@23 PS1, Line 23: default y if ARCH_BOOTBLOCK_PPC64 || ARCH_BOOTBLOCK_ARMV8_64 || ARCH_BOOTBLOCK_ARM64 || ARCH_BOOTBLOCK_ARMV7 || ARCH_BOOTBLOCK_ARMV7_M || ARCH_BOOTBLOCK_ARMV7_R || ARCH_BOOTBLOCK_ARM || ARCH_BOOTBLOCK_ARMV4 || ARCH_BOOTBLOCK_RISCV || ARCH_BOOTBLOCK_X86_32 || ARCH_BOOTBLOCK_X86_64 || ARCH_BOOTBLOCK_MIPS Would it be better to select this config from these Kconfig locations and have this one default to n? The reason I say that is someone will have to know to modify this option when adding a new arch where my suggestion people would see the 'select' line when using an existing arch as a guide. The options are tightly coupled and leaves less room for error.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 1:
(1 comment)
What's interesting to me is that I've recently had uses for coreboot images I could build that had not bootblock, verstage, romstage, ... so while this may seem crazy, it's part of our continuing coreboot story of amazing flexibility.
https://review.coreboot.org/#/c/33142/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/1/src/Kconfig@21 PS1, Line 21: config STAGE_BOOTBLOCK This, to me anyway, is confusing.
]Why can't we do a default y?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
What's interesting to me is that I've recently had uses for coreboot images I could build that had not bootblock, verstage, romstage, ... so while this may seem crazy, it's part of our continuing coreboot story of amazing flexibility.
There is definitely a real need for selecting which stages make it into the coreboot build.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 1:
What is this patch actually solving (at a high level)? I feel like it's just adding some more vines to the config jungle for no real benefit other than for people to trip over. Don't we already know which stages we have from other options? In general, coreboot always has a bootblock, a romstage and a ramstage. It has a verstage if CONFIG_SEPARATE_VERSTAGE is set. With your new feature, it does not have a ramstage if the main Kconfig for that feature is set (is that CONFIG_RAMPAYLOAD? I didn't keep track...).
So if you want something you can depend ramstage-specific stuff on, can't you just do 'depends on !RAMPAYLOAD'? Why does it need a separate option that's tied so weirdly into the architecture Kconfigs? Sure, if we eventually have 4 different cases where coreboot somehow doesn't have a ramstage it might make sense to factor those all out into a common option, but I don't really see that happening anytime soon?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 1:
(1 comment)
How do we envision these being used? Are we expecting platforms to default them to disabled?
https://review.coreboot.org/#/c/33142/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/1/src/Kconfig@23 PS1, Line 23: default y if Because of where these are at the top of the the Kconfig tree, individual platform or chips can't update these. Remember that the first valid default is taken and the rest are ignored.
I also don't think we need a prompt for these as they generally aren't something the user would change. They'll be set by the platform and left alone.
My thoughts: 1) Move these config options to the bottom of this file so the defaults can be set elsewhere 2) Remove the prompts 3) Set default to y for all in this file 4) Set default to n in the architecture Kconfig files where that makes sense. Or add the prompt there if appropriate.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 1:
(4 comments)
Patch Set 1:
What is this patch actually solving (at a high level)? I feel like it's just adding some more vines to the config jungle for no real benefit other than for people to trip over. Don't we already know which stages we have from other options? In general, coreboot always has a bootblock, a romstage and a ramstage. It has a verstage if CONFIG_SEPARATE_VERSTAGE is set. With your new feature, it does not have a ramstage if the main Kconfig for that feature is set (is that CONFIG_RAMPAYLOAD? I didn't keep track...).
So if you want something you can depend ramstage-specific stuff on, can't you just do 'depends on !RAMPAYLOAD'? Why does it need a separate option that's tied so weirdly into the architecture Kconfigs? Sure, if we eventually have 4 different cases where coreboot somehow doesn't have a ramstage it might make sense to factor those all out into a common option, but I don't really see that happening anytime soon?
[Subrata] coreboot somehow doesn't have a ramstage it might make sense to factor those all out into a common option, this is the whole purpose of this patch tree to build and boot platform without ramstage
https://review.coreboot.org/#/c/33142/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/1/src/Kconfig@21 PS1, Line 21: config STAGE_BOOTBLOCK
This, to me anyway, is confusing. […]
Ack
https://review.coreboot.org/#/c/33142/1/src/Kconfig@21 PS1, Line 21: config STAGE_BOOTBLOCK
Can we add ENABLE or PRESENT to these names? I feel like they read better when used.
Ack
https://review.coreboot.org/#/c/33142/1/src/Kconfig@23 PS1, Line 23: default y if ARCH_BOOTBLOCK_PPC64 || ARCH_BOOTBLOCK_ARMV8_64 || ARCH_BOOTBLOCK_ARM64 || ARCH_BOOTBLOCK_ARMV7 || ARCH_BOOTBLOCK_ARMV7_M || ARCH_BOOTBLOCK_ARMV7_R || ARCH_BOOTBLOCK_ARM || ARCH_BOOTBLOCK_ARMV4 || ARCH_BOOTBLOCK_RISCV || ARCH_BOOTBLOCK_X86_32 || ARCH_BOOTBLOCK_X86_64 || ARCH_BOOTBLOCK_MIPS
Would it be better to select this config from these Kconfig locations and have this one default to […]
Done
https://review.coreboot.org/#/c/33142/1/src/Kconfig@23 PS1, Line 23: default y if
Because of where these are at the top of the the Kconfig tree, individual platform or chips can't up […]
Ack
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#2).
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Kconfig: Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. ENABLE_STAGE_BOOTBLOCK 2. ENABLE_STAGE_VERSTAGE 3. ENABLE_STAGE_ROMSTAGE 4. ENABLE_STAGE_POSTCAR 5. ENABLE_STAGE_RAMSTAGE
A stage can only be enabled if soc has selected its supported architecture(example: arm, ppc, riscv, x86)
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/arm/Kconfig M src/arch/arm64/Kconfig M src/arch/arm64/armv8/Kconfig M src/arch/mips/Kconfig M src/arch/ppc64/Kconfig M src/arch/riscv/Kconfig M src/arch/x86/Kconfig 8 files changed, 69 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2: Code-Review+1
The other bit to get correct here is removing the root dependencies if these values are not selected. I think if you do that correctly then many of the follow up CLs aren't needed since Make won't try evaluating the rules.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+1
The other bit to get correct here is removing the root dependencies if these values are not selected. I think if you do that correctly then many of the follow up CLs aren't needed since Make won't try evaluating the rules.
Do you like to club all ENABLE_STAGE_<> related changes into this CL? In that way i believe we will loose traceability
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review+1
The other bit to get correct here is removing the root dependencies if these values are not selected. I think if you do that correctly then many of the follow up CLs aren't needed since Make won't try evaluating the rules.
Do you like to club all ENABLE_STAGE_<> related changes into this CL? In that way i believe we will loose traceability
Not necessarily, but as things currently stand this CL doesn't really do anything. We should be plumbing the removal of the root dependencies based on these Kconfigs in this CL, though.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review+1
The other bit to get correct here is removing the root dependencies if these values are not selected. I think if you do that correctly then many of the follow up CLs aren't needed since Make won't try evaluating the rules.
Do you like to club all ENABLE_STAGE_<> related changes into this CL? In that way i believe we will loose traceability
Not necessarily, but as things currently stand this CL doesn't really do anything. We should be plumbing the removal of the root dependencies based on these Kconfigs in this CL, though.
I can club https://review.coreboot.org/c/coreboot/+/33112/3 and https://review.coreboot.org/c/coreboot/+/33116/4 into this if you think that helps to justify things better for this CL.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Sorry, I still think this patch is a bad idea and just adding to confusion and redundancy in the configs. The architecture stage Kconfigs are just meant to select the right toolchain, they don't actually mean that that stage is included and I don't think they need to. ARCH_VERSTAGE_XXX is often set even when we don't actually have a verstage (and I also don't see how you're disabling ARCH_RAMSTAGE_XXX in your case?). Trying to solve this all at this level would pull a lot of stuff into the architecture configs where it needs to be unnecessarily duplicated six times.
If you want to know whether you have a verstage, check CONFIG_SEPARATE_VERSTAGE. If you want to know whether you have a ramstage, check CONFIG_RAMPAYLOAD. Those options already correspond 1-to-1 to what you want to know, I don't see the need for adding any further ones. If at some future point we have more than one case where there's no ramstage, we can add a helper option and select that based on RAMPAYLOAD. But for now I don't see why we need to add anything, and we definitely shouldn't need to pull all of this into the toolchain selectors.
https://review.coreboot.org/#/c/33142/2/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/#/c/33142/2/src/arch/x86/Kconfig@34 PS2, Line 34: select ENABLE_STAGE_VERSTAGE Is this actually doing what you want? This option is usually selected unconditionally by the SoC, regardless of whether SEPARATE_VERSTAGE is actually enabled or not. So this option will be on most of the time even if there is no verstage.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
(1 comment)
Sorry, I still think this patch is a bad idea and just adding to confusion and redundancy in the configs. The architecture stage Kconfigs are just meant to select the right toolchain, they don't actually mean that that stage is included and I don't think they need to. ARCH_VERSTAGE_XXX is often set even when we don't actually have a verstage (and I also don't see how you're disabling ARCH_RAMSTAGE_XXX in your case?). Trying to solve this all at this level would pull a lot of stuff into the architecture configs where it needs to be unnecessarily duplicated six times.
If you want to know whether you have a verstage, check CONFIG_SEPARATE_VERSTAGE. If you want to know whether you have a ramstage, check CONFIG_RAMPAYLOAD. Those options already correspond 1-to-1 to what you want to know, I don't see the need for adding any further ones. If at some future point we have more than one case where there's no ramstage, we can add a helper option and select that based on RAMPAYLOAD. But for now I don't see why we need to add anything, and we definitely shouldn't need to pull all of this into the toolchain selectors.
There's new requirements that are surfacing:
Being able to mix and match stages. e.g. remove bootblock, verstage, but leave romstage and ramstage.
The stage options that exist are for selecting the toolchain, but they can also indicate the presence and desire of a particular stage. There's 2 options on indicating positive selection of the present of the stages:
1. Annotate all the soc/higher level Kconfig selections (most likely higher change churn than just arch). 2. Update arch/ to select the presence of the stage. One-time change per arch. Adding new archs will be straight forward since they'll just copy what's there and not need to know about yet another Kconfig (previous versions of this patch did that).
They both need to deal with conditionally including and excluding things such as CONFIG_RAMPAYLOAD that may be selectable. So one might need to bite the bullet and update more, like in soc, along with some special helpers.
The soc/higher level Kconfigs are currently driving things as they are the root. I'm not sure why there would be unnecessary duplication.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
but they can also indicate the presence and desire of a particular stage.
Well, they don't at the moment (e.g. in the verstage case). I'm trying to argue that it's cleaner and easier to keep it that way and handle presence of a stage separately.
There's 2 options on indicating positive selection of the present of the stages:
- Annotate all the soc/higher level Kconfig selections (most likely higher change churn than just arch).
- Update arch/ to select the presence of the stage. One-time change per arch. Adding new archs will be straight forward since they'll just copy what's there and not need to know about yet another Kconfig (previous versions of this patch did that).
They both need to deal with conditionally including and excluding things such as CONFIG_RAMPAYLOAD that may be selectable. So one might need to bite the bullet and update more, like in soc, along with some special helpers.
The soc/higher level Kconfigs are currently driving things as they are the root. I'm not sure why there would be unnecessary duplication.
Sorry, I don't really understand what you're saying. Which options do you mean by "soc/higher level Kconfig"? Do you mean when an SoC's Kconfig file says 'select ARCH_RAMSTAGE_X86'? I don't think we should touch that at all. I think it's fine if ARCH_RAMSTAGE_X86 is enabled even if you're building with RAMPAYLOAD. All that option does is make sure CC_ramstage is initialized to something. That doesn't hurt anybody even if no ramstage actually gets built.
I think the positive selection of the presence of a stage should only be decided by the feature Kconfig that may add/exclude that stage (e.g. SEPARATE_VERSTAGE or RAMPAYLOAD), and nothing else. The Kconfigs that select the toolchain should just be orthogonal to that. If you absolutely want to have a CONFIG_ENABLE_RAMSTAGE then I guess we can add that, but it should just be initialized with 'default y if !RAMPAYLOAD', and it shouldn't be intertwined with ARCH_RAMSTAGE_X86.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
but they can also indicate the presence and desire of a particular stage.
Well, they don't at the moment (e.g. in the verstage case). I'm trying to argue that it's cleaner and easier to keep it that way and handle presence of a stage separately.
There's 2 options on indicating positive selection of the present of the stages:
- Annotate all the soc/higher level Kconfig selections (most likely higher change churn than just arch).
- Update arch/ to select the presence of the stage. One-time change per arch. Adding new archs will be straight forward since they'll just copy what's there and not need to know about yet another Kconfig (previous versions of this patch did that).
They both need to deal with conditionally including and excluding things such as CONFIG_RAMPAYLOAD that may be selectable. So one might need to bite the bullet and update more, like in soc, along with some special helpers.
The soc/higher level Kconfigs are currently driving things as they are the root. I'm not sure why there would be unnecessary duplication.
Sorry, I don't really understand what you're saying. Which options do you mean by "soc/higher level Kconfig"? Do you mean when an SoC's Kconfig file says 'select ARCH_RAMSTAGE_X86'? I don't think we should touch that at all. I think it's fine if ARCH_RAMSTAGE_X86 is enabled even if you're building with RAMPAYLOAD. All that option does is make sure CC_ramstage is initialized to something. That doesn't hurt anybody even if no ramstage actually gets built.
I think the positive selection of the presence of a stage should only be decided by the feature Kconfig that may add/exclude that stage (e.g. SEPARATE_VERSTAGE or RAMPAYLOAD), and nothing else. The Kconfigs that select the toolchain should just be orthogonal to that. If you absolutely want to have a CONFIG_ENABLE_RAMSTAGE then I guess we can add that, but it should just be initialized with 'default y if !RAMPAYLOAD', and it shouldn't be intertwined with ARCH_RAMSTAGE_X86.
Do you have a proposal for handling all the various stages and a way to select them in the positive as well as handling the user-configurable options? Just add kconfigs?
1. assert in positive *no* <STAGE> needed 2. assert in positive *yes* <STAGE> needed, but it depends on !*no* <STAGE>? And default of y?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Do you have a proposal for handling all the various stages and a way to select them in the positive as well as handling the user-configurable options? Just add kconfigs?
- assert in positive *no* <STAGE> needed
For the ramstage, use if (CONFIG(RAMPAYLOAD) and for the verstage if (!CONFIG(SEPARATE_VERSTAGE))? I'm not sure I understand the question.
- assert in positive *yes* <STAGE> needed, but it depends on !*no* <STAGE>? And default of y?
And this would assert the opposite (!RAMPAYLOAD or SEPARATE_VERSTAGE)? Not sure what you mean by "depends on !*no* <STAGE>".
If we want explicit options, we could add them like this:
config BOOTBLOCK_ENABLED default y #always 'y' in current coreboot
config VERSTAGE_ENABLED default y if SEPARATE_VERSTAGE default n
config ROMSTAGE_ENABLED default y #always 'y' in current coreboot
config POSTCAR_ENABLED default y if POSTCAR_STAGE default n
config RAMSTAGE_ENABLED default n if RAMPAYLOAD default y
But I don't think it's really worth adding those until we have a case where two orthogonal features would both disable the same default stage, since it does not come up very often. We can just use the respective options as a stand-in for now.)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
Do you have a proposal for handling all the various stages and a way to select them in the positive as well as handling the user-configurable options? Just add kconfigs?
- assert in positive *no* <STAGE> needed
For the ramstage, use if (CONFIG(RAMPAYLOAD) and for the verstage if (!CONFIG(SEPARATE_VERSTAGE))? I'm not sure I understand the question.
- assert in positive *yes* <STAGE> needed, but it depends on !*no* <STAGE>? And default of y?
And this would assert the opposite (!RAMPAYLOAD or SEPARATE_VERSTAGE)? Not sure what you mean by "depends on !*no* <STAGE>".
If we want explicit options, we could add them like this:
config BOOTBLOCK_ENABLED default y #always 'y' in current coreboot
config VERSTAGE_ENABLED default y if SEPARATE_VERSTAGE default n
config ROMSTAGE_ENABLED default y #always 'y' in current coreboot
config POSTCAR_ENABLED default y if POSTCAR_STAGE default n
config RAMSTAGE_ENABLED default n if RAMPAYLOAD default y
But I don't think it's really worth adding those until we have a case where two orthogonal features would both disable the same default stage, since it does not come up very often. We can just use the respective options as a stand-in for now.)
This is my point about the requirements. We need to be able to select which stages get put into the build itself. And we should be able to do that from platform or mainboard code. What you put up doesn't allow one to mix and match the stages selected. i.e. the 'current coreboot' doesn't meet the new requirements.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
but they can also indicate the presence and desire of a particular stage.
Well, they don't at the moment (e.g. in the verstage case). I'm trying to argue that it's cleaner and easier to keep it that way and handle presence of a stage separately.
There's 2 options on indicating positive selection of the present of the stages:
- Annotate all the soc/higher level Kconfig selections (most likely higher change churn than just arch).
- Update arch/ to select the presence of the stage. One-time change per arch. Adding new archs will be straight forward since they'll just copy what's there and not need to know about yet another Kconfig (previous versions of this patch did that).
They both need to deal with conditionally including and excluding things such as CONFIG_RAMPAYLOAD that may be selectable. So one might need to bite the bullet and update more, like in soc, along with some special helpers.
The soc/higher level Kconfigs are currently driving things as they are the root. I'm not sure why there would be unnecessary duplication.
Sorry, I don't really understand what you're saying. Which options do you mean by "soc/higher level Kconfig"? Do you mean when an SoC's Kconfig file says 'select ARCH_RAMSTAGE_X86'? I don't think we should touch that at all. I think it's fine if ARCH_RAMSTAGE_X86 is enabled even if you're building with RAMPAYLOAD. All that option does is make sure CC_ramstage is initialized to something. That doesn't hurt anybody even if no ramstage actually gets built.
[Subrata] Julius, I was kind of dealing with this same opinion for a while that its okay that I'm building coreboot with RAMPAYLOAD option enable so, i should be able to boot to payload from postcar (or what ever stage earlier) and let still coreboot build ramstage as it has ARCH_tool chain enable and ramstage is also default select, even ramstage also getting build and keeping as part of CBFS. But this has 2 problem
1. Ramstage is not a tiny stage, it still takes around 200KB of cbfs space (RO, RW-A/B, 200*3=600KB) 2. Adding to boot time, with RAMPAYLOAD is enable and having ramstage inside CBFS vs not inside CBFS is adding up to 100ms of boot time.
Thats the reason, i have started with experiment of coreboot should be able to build without ramstage. Later Ron and Aaron also share the thought process that coreboot should be that moduler so one can pick or drop certain stage if not required. But today i could see coreboot grown into some much of dependency with ramstage (good example of smm and ramstage)
I think the positive selection of the presence of a stage should only be decided by the feature Kconfig that may add/exclude that stage (e.g. SEPARATE_VERSTAGE or RAMPAYLOAD), and nothing else. The Kconfigs that select the toolchain should just be orthogonal to that. If you absolutely want to have a CONFIG_ENABLE_RAMSTAGE then I guess we can add that, but it should just be initialized with 'default y if !RAMPAYLOAD', and it shouldn't be intertwined with ARCH_RAMSTAGE_X86.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
This is my point about the requirements. We need to be able to select which stages get put into the build itself. And we should be able to do that from platform or mainboard code. What you put up doesn't allow one to mix and match the stages selected. i.e. the 'current coreboot' doesn't meet the new requirements.
Okay, then what requirements are those? I can't really imagine a use case where an SoC or mainboard would want to omit a whole stage without any involvement of a toplevel Kconfig that changes behavior in some generic code. I mean, run_romstage() or run_ramstage() are called from generic code, a mainboard can't just decide on its own to not have a romstage for some reason. It would have to select an option that changes all the generic behavior which normally expects the existence of that stage.
If you're thinking about the new AMD stuff I think that should certainly get a new toplevel option to control omission of the pre-RAM stages somewhere. Other than that I'm not aware of another upcoming need for excluding stages. I doubt we're suddenly going to see so many that handling them all as toplevel Kconfigs would become a maintenance problem?
- Ramstage is not a tiny stage, it still takes around 200KB of cbfs space (RO, RW-A/B, 200*3=600KB)
- Adding to boot time, with RAMPAYLOAD is enable and having ramstage inside CBFS vs not inside CBFS is adding up to 100ms of boot time.
Just to clarify, I'm not saying you should build a ramstage and include it in CBFS. You should have ifeqs in the Makefile to exclude the ramstage ELF from the dependencies and the files included in CBFS. I'm just saying you should use CONFIG_RAMPAYLOAD to control that, not the toolchain options. Having the toolchain defined doesn't mean you actually need to build anything (and thanks to Makefile dependencies you shouldn't need to touch all the details about how the ramstage is built, just make the final image not depend on it).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
This is my point about the requirements. We need to be able to select which stages get put into the build itself. And we should be able to do that from platform or mainboard code. What you put up doesn't allow one to mix and match the stages selected. i.e. the 'current coreboot' doesn't meet the new requirements.
Okay, then what requirements are those? I can't really imagine a use case where an SoC or mainboard would want to omit a whole stage without any involvement of a toplevel Kconfig that changes behavior in some generic code. I mean, run_romstage() or run_ramstage() are called from generic code, a mainboard can't just decide on its own to not have a romstage for some reason. It would have to select an option that changes all the generic behavior which normally expects the existence of that stage.
If you're thinking about the new AMD stuff I think that should certainly get a new toplevel option to control omission of the pre-RAM stages somewhere. Other than that I'm not aware of another upcoming need for excluding stages. I doubt we're suddenly going to see so many that handling them all as toplevel Kconfigs would become a maintenance problem?
- Ramstage is not a tiny stage, it still takes around 200KB of cbfs space (RO, RW-A/B, 200*3=600KB)
- Adding to boot time, with RAMPAYLOAD is enable and having ramstage inside CBFS vs not inside CBFS is adding up to 100ms of boot time.
Just to clarify, I'm not saying you should build a ramstage and include it in CBFS. You should have ifeqs in the Makefile to exclude the ramstage ELF from the dependencies and the files included in CBFS. I'm just saying you should use CONFIG_RAMPAYLOAD to control that, not the toolchain options. Having the toolchain defined doesn't mean you actually need to build anything (and thanks to Makefile dependencies you shouldn't need to touch all the details about how the ramstage is built, just make the final image not depend on it).
[Subrata] I understand your point but i think its looking things differently from one to another, when i have started implementing, i was actually guarding things against RAMPAYLOAD and it will work but then Aaron bring another point that ARCH are nothing but telling what all different soc/vendor toolchain is supporting a stage , rather keeping things dependent on architecture for an example: COMPRESS_RAMSTAGE (shouldn't enable if all atleast 1/10 ARCH_RAMSTAGE is not enable), we should have a definition of stage, which was missing in coreboot today. Only stage definition exist today is for verstage (seperate_verstage).
And if we have stage definitions then only we could pick stages from mainboard or soc.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Just to clarify, I'm not saying you should build a ramstage and include it in CBFS. You should have ifeqs in the Makefile to exclude the ramstage ELF from the dependencies and the files included in CBFS. I'm just saying you should use CONFIG_RAMPAYLOAD to control that, not the toolchain options. Having the toolchain defined doesn't mean you actually need to build anything (and thanks to Makefile dependencies you shouldn't need to touch all the details about how the ramstage is built, just make the final image not depend on it).
Hi Julius, building on your careful comments here, are you ok with the goal that we can, on a per stage basis, pick what to build and include in the ROM? I just had a requirement drop in my lap that's kind of interesting: the need to build a coreboot.rom with ONLY a ramstage and a payload, to be passed to kexec. This is the converse of RAMPAYLOAD.
I realize this "pick you stages" model is a very new request for coreboot, but it's not a new requirement, it's been on my plate since the first riscv port 5 years ago. I just can't figure out the right way to do it.
Can you advise on a good way to get where we're trying to go, if this CL is not the way? I can see the point of the reviewers, I'm just not sure what to do. It would be nice to get this right.
Thanks.
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#3).
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Kconfig: Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. ENABLE_STAGE_BOOTBLOCK 2. ENABLE_STAGE_VERSTAGE 3. ENABLE_STAGE_ROMSTAGE 4. ENABLE_STAGE_POSTCAR 5. ENABLE_STAGE_RAMSTAGE
A stage can only be enabled if soc has selected its supported architecture(example: arm, ppc, riscv, x86)
Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_ENABLE_STAGE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_STAGE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if (CONFIG_STAGE_VERSTAGE && CONFIG_VBOOT_STARTS_IN_ROMSTAGE) || CONFIG_STAGE_ROMSTAGE is selected.
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/arm/Kconfig M src/arch/arm64/Kconfig M src/arch/arm64/armv8/Kconfig M src/arch/mips/Kconfig M src/arch/ppc64/Kconfig M src/arch/riscv/Kconfig M src/arch/x86/Kconfig 8 files changed, 73 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/3
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#4).
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Kconfig: Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. ENABLE_STAGE_BOOTBLOCK 2. ENABLE_STAGE_VERSTAGE 3. ENABLE_STAGE_ROMSTAGE 4. ENABLE_STAGE_POSTCAR 5. ENABLE_STAGE_RAMSTAGE
A stage can only be enabled if soc has selected its supported architecture(example: arm, ppc, riscv, x86)
Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_ENABLE_STAGE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_STAGE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if (CONFIG_STAGE_VERSTAGE && CONFIG_VBOOT_STARTS_IN_ROMSTAGE) || CONFIG_STAGE_ROMSTAGE is selected.
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/arm/Kconfig M src/arch/arm64/Kconfig M src/arch/arm64/armv8/Kconfig M src/arch/mips/Kconfig M src/arch/ppc64/Kconfig M src/arch/riscv/Kconfig M src/arch/x86/Kconfig 8 files changed, 73 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 4:
And if we have stage definitions then only we could pick stages from mainboard or soc.
I'm still very skeptical about this mainboard and SoC argument... can you give a real example where you would want the mainboard to disable a stage without a new top-level Kconfig that changes code flow? All the stage transition code is common code -- a mainboard can't just decide to skip a stage without making sure the rest of coreboot can accommodate that.
Hi Julius, building on your careful comments here, are you ok with the goal that we can, on a per stage basis, pick what to build and include in the ROM?
I think we should cover all the explicit use cases that people have. But I don't think it makes sense that you can go into menuconfig and individually enable/disable every stage. Because having a bootblock and a ramstage without a romstage, for example, doesn't really make sense (maybe it will make sense one day in the context of a special feature, but then wouldn't that feature have an explicit Kconfig we'd use to decide that?).
I just had a requirement drop in my lap that's kind of interesting: the need to build a coreboot.rom with ONLY a ramstage and a payload, to be passed to kexec. This is the converse of RAMPAYLOAD.
This sounds to me like you'd want to add a new CONFIG_RAMSTAGE_ONLY or CONFIG_KEXECUTABLE or something like that, but it would be a new toplevel Kconfig that will control more things than just whether the other stage files are built, right? (For example, depending on the platform and exact use case it may need to do things like stack or page table setup that are normally expected to already be done by previous stages.)
(I'm also not sure if this is a good idea in general. I assume what you're trying to do is boot rampayload into LinuxBoot but then have the option to still run through coreboot's ramstage if the user selects to do it? So then your plan is to build coreboot twice -- once with RAMPAYLOAD and once with the option that makes it ramstage-only? Wouldn't it make more sense to just have another option -- dependent on RAMPAYLOAD -- that makes a single coreboot build output two CBFS files: the main coreboot.rom with all stages up to the rampayload and a second coreboot.kexec.rom that just has the ramstage and second payload?)
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#5).
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Kconfig: Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. ENABLE_STAGE_BOOTBLOCK 2. ENABLE_STAGE_VERSTAGE 3. ENABLE_STAGE_ROMSTAGE 4. ENABLE_STAGE_POSTCAR 5. ENABLE_STAGE_RAMSTAGE
Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_ENABLE_STAGE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_STAGE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if (CONFIG_STAGE_VERSTAGE && CONFIG_VBOOT_STARTS_IN_ROMSTAGE) || CONFIG_STAGE_ROMSTAGE is selected.
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 40 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33142/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1195 PS5, Line 1195: config ENABLE_STAGE_BOOTBLOCK I assume you're going to leave these and not try enable the ability to remove these stages?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/#/c/33142/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/5/src/Kconfig@141 PS5, Line 141: depends on !ARCH_X86 && (ENABLE_STAGE_ROMSTAGE || ENABLE_STAGE_VERSTAGE && VBOOT_STARTS_IN_ROMSTAGE) I assume this is supposed to mean !VBOOT_STARTS_IN_ROMSTAGE (or VBOOT_STARTS_IN_BOOTBLOCK, which is equivalent)? Not really necessary anyway, SEPARATE_VERSTAGE always implies STARTS_IN_BOOTBLOCK.
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1200 PS5, Line 1200: bootblock architecture (example: arm, ppc, riscv, x86). This help text seems odd
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1204 PS5, Line 1204: default y Shouldn't this depend on SEPARATE_VERSTAGE?
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1218 PS5, Line 1218: default y Shouldn't this depend on POSTCAR_STAGE?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33142/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1195 PS5, Line 1195: config ENABLE_STAGE_BOOTBLOCK
I assume you're going to leave these and not try enable the ability to remove these stages?
no Aaron, i will also try to enable each stage separately. but for now,i'm thinking to priorities rampayload work. Is that make sense?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/#/c/33142/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/5/src/Kconfig@141 PS5, Line 141: depends on !ARCH_X86 && (ENABLE_STAGE_ROMSTAGE || ENABLE_STAGE_VERSTAGE && VBOOT_STARTS_IN_ROMSTAGE)
I assume this is supposed to mean !VBOOT_STARTS_IN_ROMSTAGE (or VBOOT_STARTS_IN_BOOTBLOCK, which is […]
Ack
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1200 PS5, Line 1200: bootblock architecture (example: arm, ppc, riscv, x86).
This help text seems odd
Done
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1204 PS5, Line 1204: default y
Shouldn't this depend on SEPARATE_VERSTAGE?
Done
https://review.coreboot.org/#/c/33142/5/src/Kconfig@1218 PS5, Line 1218: default y
Shouldn't this depend on POSTCAR_STAGE?
Done
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#6).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependencies in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. ENABLE_STAGE_BOOTBLOCK 2. ENABLE_STAGE_VERSTAGE 3. ENABLE_STAGE_ROMSTAGE 4. ENABLE_STAGE_POSTCAR 5. ENABLE_STAGE_RAMSTAGE
B. Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_ENABLE_STAGE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_STAGE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if CONFIG_ENABLE_STAGE_VERSTAGE || CONFIG_ENABLE_STAGE_ROMSTAGE is selected.
C. Also fix compilation issue with !CONFIG_ENABLE_STAGE_RAMSTAGE
On x86 platform: Case 1: ramstage do exist: CONFIG_STAGE_RAMSTAGE=1
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_x86_32
Case 2: ramstage doesn't exist: CONFIG_STAGE_RAMSTAGE=0
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_
This patch fixes Case 2 usecase where platform doesn't select CONFIG_ENABLE_STAGE_RAMSTAGE.
Also add option to create sipi_vector.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_ENABLE_STAGE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M src/cpu/x86/Makefile.inc 3 files changed, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/6
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#7).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependencies in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. ENABLE_STAGE_BOOTBLOCK 2. ENABLE_STAGE_VERSTAGE 3. ENABLE_STAGE_ROMSTAGE 4. ENABLE_STAGE_POSTCAR 5. ENABLE_STAGE_RAMSTAGE
B. Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_ENABLE_STAGE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_STAGE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if CONFIG_ENABLE_STAGE_VERSTAGE || CONFIG_ENABLE_STAGE_ROMSTAGE is selected.
C. Also fix compilation issue with !CONFIG_ENABLE_STAGE_RAMSTAGE
On x86 platform: Case 1: ramstage do exist: CONFIG_STAGE_RAMSTAGE=1
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_x86_32
Case 2: ramstage doesn't exist: CONFIG_STAGE_RAMSTAGE=0
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_
This patch fixes Case 2 usecase where platform doesn't select CONFIG_ENABLE_STAGE_RAMSTAGE.
Also add option to create sipi_vector.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_ENABLE_STAGE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M src/cpu/x86/Makefile.inc 3 files changed, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/7
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/33142/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/7/src/Kconfig@1195 PS7, Line 1195: config ENABLE_STAGE_BOOTBLOCK It would be nice to align the naming with the already established HAVE_* pattern in coreboot. e.g. HAVE_BOOTBLOCK, HAVE_VERSTAGE, etc.
https://review.coreboot.org/#/c/33142/7/src/Kconfig@1199 PS7, Line 1199: This option is enabled to select bootblock architecture. I don't understand what `architecture` refers to here? Maybe this doesn't need a help text at all?
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#8).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependencies in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. HAVE_BOOTBLOCK 2. HAVE_VERSTAGE 3. HAVE_ROMSTAGE 4. HAVE_POSTCAR 5. HAVE_RAMSTAGE
B. Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_HAVE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_HAVE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if CONFIG_HAVE_VERSTAGE || CONFIG_HAVE_ROMSTAGE is selected.
C. Also fix compilation issue with !CONFIG_HAVE_RAMSTAGE
On x86 platform: Case 1: ramstage do exist: CONFIG_HAVE_RAMSTAGE=1
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_x86_32
Case 2: ramstage doesn't exist: CONFIG_HAVE_RAMSTAGE=0
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_
This patch fixes Case 2 usecase where platform doesn't select CONFIG_HAVE_RAMSTAGE.
Also add option to create sipi_vector.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M src/cpu/x86/Makefile.inc 3 files changed, 56 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/33142/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/7/src/Kconfig@1195 PS7, Line 1195: config ENABLE_STAGE_BOOTBLOCK
It would be nice to align the naming with the already established […]
Done
https://review.coreboot.org/#/c/33142/7/src/Kconfig@1199 PS7, Line 1199: This option is enabled to select bootblock architecture.
I don't understand what `architecture` refers to here? Maybe […]
Done
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33142
to look at the new patch set (#9).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependencies in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. HAVE_BOOTBLOCK 2. HAVE_VERSTAGE 3. HAVE_ROMSTAGE 4. HAVE_POSTCAR 5. HAVE_RAMSTAGE
B. Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_HAVE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_HAVE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if CONFIG_HAVE_VERSTAGE || CONFIG_HAVE_ROMSTAGE is selected.
C. Also fix compilation issue with !CONFIG_HAVE_RAMSTAGE
On x86 platform: Case 1: ramstage do exist: CONFIG_HAVE_RAMSTAGE=1
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_x86_32
Case 2: ramstage doesn't exist: CONFIG_HAVE_RAMSTAGE=0
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_
This patch fixes Case 2 usecase where platform doesn't select CONFIG_HAVE_RAMSTAGE.
Also add option to create sipi_vector.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M src/cpu/x86/Makefile.inc 3 files changed, 55 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33142/9
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33142/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/7/src/Kconfig@1195 PS7, Line 1195: config ENABLE_STAGE_BOOTBLOCK
Done
Thanks!
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 9: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependencies in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create coreboot separate stage kconfigs
This patch creates seperate stage configs as below 1. HAVE_BOOTBLOCK 2. HAVE_VERSTAGE 3. HAVE_ROMSTAGE 4. HAVE_POSTCAR 5. HAVE_RAMSTAGE
B. Also ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_HAVE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_HAVE_BOOTBLOCK is set 3. COMPRESS_PRERAM_STAGES will enable if CONFIG_HAVE_VERSTAGE || CONFIG_HAVE_ROMSTAGE is selected.
C. Also fix compilation issue with !CONFIG_HAVE_RAMSTAGE
On x86 platform: Case 1: ramstage do exist: CONFIG_HAVE_RAMSTAGE=1
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_x86_32
Case 2: ramstage doesn't exist: CONFIG_HAVE_RAMSTAGE=0
rmodules_$(ARCH-ramstage-y) will evaluate as rmodules_
This patch fixes Case 2 usecase where platform doesn't select CONFIG_HAVE_RAMSTAGE.
Also add option to create sipi_vector.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I0f7e4174619016c5a54c28bedd52699df417a5b7 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33142 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M Makefile.inc M src/Kconfig M src/cpu/x86/Makefile.inc 3 files changed, 55 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index d4f7597..14cd50c 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -1058,7 +1058,14 @@ FIT_OPTIONS += -q $(FIT_ENTRY) endif
-$(obj)/coreboot.rom: $(obj)/coreboot.pre $(objcbfs)/ramstage.elf $(CBFSTOOL) $$(INTERMEDIATE) +ifeq ($(CONFIG_HAVE_RAMSTAGE),y) +RAMSTAGE=$(objcbfs)/ramstage.elf +else +RAMSTAGE= +endif + +$(obj)/coreboot.rom: $(obj)/coreboot.pre $(RAMSTAGE) $(CBFSTOOL) $$(INTERMEDIATE) + @printf " CBFS $(subst $(obj)/,,$(@))\n" # The full ROM may be larger than the CBFS part, so create an empty # file (filled with \377 = 0xff) and copy the CBFS image over it. @@ -1128,8 +1135,8 @@ endif # CONFIG_NO_XIP_EARLY_STAGES endif # CONFIG_ARCH_ROMSTAGE_X86_32 / CONFIG_ARCH_ROMSTAGE_X86_64
-cbfs-files-y += $(CONFIG_CBFS_PREFIX)/ramstage -$(CONFIG_CBFS_PREFIX)/ramstage-file := $(objcbfs)/ramstage.elf +cbfs-files-$(CONFIG_HAVE_RAMSTAGE) += $(CONFIG_CBFS_PREFIX)/ramstage +$(CONFIG_CBFS_PREFIX)/ramstage-file := $(RAMSTAGE) $(CONFIG_CBFS_PREFIX)/ramstage-type := stage $(CONFIG_CBFS_PREFIX)/ramstage-compression := $(CBFS_COMPRESS_FLAG)
diff --git a/src/Kconfig b/src/Kconfig index 49f8e6e..5d74d67 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -129,6 +129,7 @@
config COMPRESS_RAMSTAGE bool "Compress ramstage with LZMA" + depends on HAVE_RAMSTAGE # Default value set at the end of the file help Compress ramstage to save memory in the flash image. Note @@ -137,7 +138,7 @@
config COMPRESS_PRERAM_STAGES bool "Compress romstage and verstage with LZ4" - depends on !ARCH_X86 + depends on !ARCH_X86 && (HAVE_ROMSTAGE || HAVE_VERSTAGE) # Default value set at the end of the file help Compress romstage and (if it exists) verstage with LZ4 to save flash @@ -148,6 +149,7 @@
config COMPRESS_BOOTBLOCK bool + depends on HAVE_BOOTBLOCK help This option can be used to compress the bootblock with LZ4 and attach a small self-decompression stub to its front. This can drastically @@ -234,6 +236,7 @@
config RELOCATABLE_RAMSTAGE bool + depends on HAVE_RAMSTAGE default !NO_RELOCATABLE_RAMSTAGE select RELOCATABLE_MODULES help @@ -1191,3 +1194,26 @@
config CBFS_SIZE default ROM_SIZE + +config HAVE_BOOTBLOCK + bool + default y + +config HAVE_VERSTAGE + bool + depends on VBOOT_SEPARATE_VERSTAGE + default y + +config HAVE_ROMSTAGE + bool + default y + +config HAVE_POSTCAR + bool + depends on POSTCAR_STAGE + default y + +config HAVE_RAMSTAGE + bool + default n if RAMPAYLOAD + default y diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index 3e8a664..65c0921 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -17,23 +17,27 @@ SIPI_BIN=$(SIPI_ELF:.elf=) SIPI_DOTO=$(SIPI_ELF:.elf=.o)
-ifeq ($(CONFIG_PARALLEL_MP),y) -ramstage-srcs += $(SIPI_BIN).manual -endif -rmodules_$(ARCH-ramstage-y)-$(CONFIG_PARALLEL_MP) += sipi_vector.S - -$(SIPI_DOTO): $(call src-to-obj,rmodules_$(ARCH-ramstage-y),src/cpu/x86/sipi_vector.S) - $(CC_rmodules_$(ARCH-ramstage-y)) $(CFLAGS_rmodules_$(ARCH-ramstage-y)) -nostdlib -r -o $@ $^ - -ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) -$(eval $(call rmodule_link,$(SIPI_ELF), $(SIPI_DOTO), 0,x86_32)) +ifeq ($(CONFIG_HAVE_RAMSTAGE),y) +TARGET_STAGE=ramstage +else ifeq ($(CONFIG_RAMPAYLOAD),y) +TARGET_STAGE=postcar else -$(eval $(call rmodule_link,$(SIPI_ELF), $(SIPI_DOTO), 0,x86_64)) +$(error Halting the build due to unknown TARGET_STAGE select) endif
+ifeq ($(CONFIG_PARALLEL_MP),y) +$(TARGET_STAGE)-srcs += $(SIPI_BIN).manual +endif +rmodules_$(ARCH-$(TARGET_STAGE)-y)-$(CONFIG_PARALLEL_MP) += sipi_vector.S + +$(SIPI_DOTO): $(call src-to-obj,rmodules_$(ARCH-$(TARGET_STAGE)-y),src/cpu/x86/sipi_vector.S) + $(CC_rmodules_$(ARCH-$(TARGET_STAGE)-y)) $(CFLAGS_rmodules_$(ARCH-$(TARGET_STAGE)-y)) -nostdlib -r -o $@ $^ + +$(eval $(call rmodule_link,$(SIPI_ELF), $(SIPI_DOTO), 0,$(ARCH-$(TARGET_STAGE)-y))) + $(SIPI_BIN): $(SIPI_RMOD) - $(OBJCOPY_ramstage) -O binary $< $@ + $(OBJCOPY_$(TARGET_STAGE)) -O binary $< $@
-$(call src-to-obj,ramstage,$(SIPI_BIN).manual): $(SIPI_BIN) +$(call src-to-obj,$(TARGET_STAGE),$(SIPI_BIN).manual): $(SIPI_BIN) @printf " OBJCOPY $(subst $(obj)/,,$(@))\n" - cd $(dir $<); $(OBJCOPY_rmodules_$(ARCH-ramstage-y)) -I binary $(notdir $<) $(target-objcopy) $(abspath $@) + cd $(dir $<); $(OBJCOPY_rmodules_$(ARCH-$(TARGET_STAGE)-y)) -I binary $(notdir $<) $(target-objcopy) $(abspath $@)
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
ron minnich (Code Review)
-
Subrata Banik (Code Review)