Change in ...coreboot[master]: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86...
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33112
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64
This patch ensures required ramstage configs like COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are not enable if CONFIG_ARCH_RAMSTAGE_X86_32/64 is not selected.
Change-Id: I5d0c31152ffa8faf62b10a6bda12c9ad2e786d94 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33112/1
diff --git a/src/Kconfig b/src/Kconfig index d30aa99..ac7aad2 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -129,6 +129,7 @@
config COMPRESS_RAMSTAGE bool "Compress ramstage with LZMA" + depends on ARCH_RAMSTAGE_X86_32 || ARCH_RAMSTAGE_X86_64 # Default value set at the end of the file help Compress ramstage to save memory in the flash image. Note @@ -234,6 +235,7 @@
config RELOCATABLE_RAMSTAGE bool + depends on ARCH_RAMSTAGE_X86_32 || ARCH_RAMSTAGE_X86_64 default !NO_RELOCATABLE_RAMSTAGE select RELOCATABLE_MODULES help
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG@11 PS1, Line 11: is not selected. Why? What is the reasoning?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG@11 PS1, Line 11: is not selected.
Why? What is the reasoning?
In case we would like to deselect CONFIG_ARCH_RAMSTAGE_X86_32 config then ramstage won't be there but still COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE configs will be default enable.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG@11 PS1, Line 11: is not selected.
In case we would like to deselect CONFIG_ARCH_RAMSTAGE_X86_32 config then ramstage won't be there bu […]
reasoning is not in the commit message. I don't see such a binding current with RELOCATABLE_RAMSTAGE and x86_32. I'm very confused why there is an association being made as one doesn't exist now. If you want to hide such options based on the presence of ramstage there should be a proper kconfig for that. It should not be targeting a specific arch's Kconfig value.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG@11 PS1, Line 11: is not selected.
reasoning is not in the commit message. […]
i will update commit msg.
I think right now the assumption is that CONFIG_ARCH_RAMSTAGE_X86_32 or CONFIG_ARCH_RAMSTAGE_X86_64 kconfig meant for ramstage exist or not ?
Right now we don't have any other Kconfig to tell what all stages exist today. Are you suggesting to create new kconfig?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG@11 PS1, Line 11: is not selected.
i will update commit msg. […]
ramstage exists on more than just x86 arch. If you want first class principles then they should be added. Making implicit assumptions about the meaning of things only makes for brittle code that is hard to maintain.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Make ramstage kconfigs dependent on CONFIG_ARCH_RAMSTAGE_X86_32/64 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/1//COMMIT_MSG@11 PS1, Line 11: is not selected.
ramstage exists on more than just x86 arch.
Aaron, i'm so sorry, i just missed the point that i'm touching generic code and this has to be compliant to add arch. I will create separate Konfig for stage and select those based on supported arch. In that way, i can add right dependency.
If you want first class principles then they should be added. Making implicit assumptions about the meaning of things only makes for brittle code that is hard to maintain.
Hello ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33112
to look at the new patch set (#2).
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Kconfig: Align kconfigs with correct stage configs
This patch ensures below kconfigs are aligned with correct stage configs
1. COMPRESS_RAMSTAGE and RELOCATABLE_RAMSTAGE are now enable if CONFIG_STAGE_RAMSTAGE is selected. 2. COMPRESS_BOOTBLOCK will enable if CONFIG_STAGE_BOOTBLOCK if set 3. COMPRESS_PRERAM_STAGES will enable if CONFIG_STAGE_VERSTAGE and CONFIG_STAGE_ROMSTAGE is selected.
Change-Id: I5d0c31152ffa8faf62b10a6bda12c9ad2e786d94 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33112/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33112/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33112/2//COMMIT_MSG@13 PS2, Line 13: if set ?
https://review.coreboot.org/#/c/33112/2/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33112/2/src/Kconfig@173 PS2, Line 173: && I don't think this should be && since we can have combinations like this:
COMPRESS romstage but no verstage
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Patch Set 2:
One thing to note is that COMPRESS_RAMSTAGE doesn't actually always *just* compress the ramstage. It sets CBFS_COMPRESS_FLAG in the Makefile which is then used to compress a variety of interesting ramstage-related things (e.g. some reference code blob in the main Makefile.inc, and DRAM configuration files loaded by a couple of Arm boards).
I think our compression configs are already in a somewhat bad state and would just be piling on to that. The problem is that most of the time you don't really care about the thing that is compressed, you care about the thing that loads it (because that's what needs to have the respective decompression library linked in). That why we generally bundle up platform-specific stuff that's loaded from the romstage together with ramstage compression and not with romstage compression. So maybe we should reorganize these options to just be called "pre-RAM compression" (or loaded-from-verstage-and-earlier, except for bootblock compression which is completely its own thing) and "post-RAM compression" (or loaded-from-romstage-and-later), because that's what it's really mostly about, and then post-RAM compression may still be interesting for a system that doesn't have a ramstage.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Patch Set 2:
Patch Set 2:
One thing to note is that COMPRESS_RAMSTAGE doesn't actually always *just* compress the ramstage. It sets CBFS_COMPRESS_FLAG in the Makefile which is then used to compress a variety of interesting ramstage-related things (e.g. some reference code blob in the main Makefile.inc, and DRAM configuration files loaded by a couple of Arm boards).
I think our compression configs are already in a somewhat bad state and would just be piling on to that. The problem is that most of the time you don't really care about the thing that is compressed, you care about the thing that loads it (because that's what needs to have the respective decompression library linked in). That why we generally bundle up platform-specific stuff that's loaded from the romstage together with ramstage compression and not with romstage compression. So maybe we should reorganize these options to just be called "pre-RAM compression" (or loaded-from-verstage-and-earlier, except for bootblock compression which is completely its own thing) and "post-RAM compression" (or loaded-from-romstage-and-later), because that's what it's really mostly about, and then post-RAM compression may still be interesting for a system that doesn't have a ramstage.
my only point is to make COMPRESS_RAMSTAGE depends on RAMSTAGE Kconfig and similarly other stage config also should depend on required stage. Its very weird that i'm trying to build platform without ramstage but still i have COMPRESS_RAMSTAGE config set to enable. I don't think it does any work in that case ?
I understood the argument Aaron has given for pre_ram compress where VERSTAGE can't be mandatory enable, i will fix that but ROMSTAGE should be mandatory right in case of pre_ram compress ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Patch Set 2:
Patch Set 2:
One thing to note is that COMPRESS_RAMSTAGE doesn't actually always *just* compress the ramstage. It sets CBFS_COMPRESS_FLAG in the Makefile which is then used to compress a variety of interesting ramstage-related things (e.g. some reference code blob in the main Makefile.inc, and DRAM configuration files loaded by a couple of Arm boards).
[Subrata] Sorry, i missed this in my earlier reply. looks like you also said, it does "It sets CBFS_COMPRESS_FLAG in the Makefile which is then used to compress a variety of interesting ramstage-related things". when i don't have ramstage as part of cbfs then why do i care about this config ? please let me know if i'm still missing something ?
I think our compression configs are already in a somewhat bad state and would just be piling on to that. The problem is that most of the time you don't really care about the thing that is compressed, you care about the thing that loads it (because that's what needs to have the respective decompression library linked in). That why we generally bundle up platform-specific stuff that's loaded from the romstage together with ramstage compression and not with romstage compression. So maybe we should reorganize these options to just be called "pre-RAM compression" (or loaded-from-verstage-and-earlier, except for bootblock compression which is completely its own thing) and "post-RAM compression" (or loaded-from-romstage-and-later), because that's what it's really mostly about, and then post-RAM compression may still be interesting for a system that doesn't have a ramstage.
Hello ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33112
to look at the new patch set (#3).
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Kconfig: Align kconfigs with correct stage configs
This patch 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_STAGE_ROMSTAGE is selected.
Change-Id: I5d0c31152ffa8faf62b10a6bda12c9ad2e786d94 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33112/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Patch Set 3: Code-Review+1
I'll leave the PRERAM compression question to Julius. The current dependency doesn't necessarily seem correct to me. It likely might need another qualifier like VBOOT_STARTS_* along w/ SEPARATE_VERSTAGE.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33112 )
Change subject: Kconfig: Align kconfigs with correct stage configs ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Subrata Banik (Code Review)