Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Makefile.inc: Alloc .bss.* sections for "struct" file type
When the global variable of a "struct" CBFS file is zero (for example, CB:47696), the binary will appear in the .bss.* section in the ELF file (instead of .data). This results in an empty binary file added to CBFS, so that file size check will fail when reading it at runtime.
BUG=b:173751635 TEST=emerge-asurada coreboot TEST=Check sdram-lpddr4x-KMDP6001DA-B425-4GB is non-empty in CBFS BRANCH=none
Change-Id: Idfd17d10101a948de0eb0522a672afd5c2f83b04 Signed-off-by: Yu-Ping Wu yupingso@google.com --- M Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/47903/1
diff --git a/Makefile.inc b/Makefile.inc index 9273961..f3493d8 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -338,7 +338,7 @@ $(eval $(2): $(1) $(obj)/build.h $(KCONFIG_AUTOHEADER); \ printf " CC+STRIP $(@)\n"; \ $(CC_ramstage) -MMD $(CPPFLAGS_ramstage) $(CFLAGS_ramstage) $$(ramstage-c-ccopts) -include $(KCONFIG_AUTOHEADER) -MT $(2) -o $(2).tmp -c $(1) && \ - $(OBJCOPY_ramstage) -O binary $(2).tmp $(2); \ + $(OBJCOPY_ramstage) -O binary -set-section-flags .bss.*=alloc,contents,load $(2).tmp $(2); \ rm -f $(2).tmp) \ $(eval DEPENDENCIES += $(2).d)
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Patch Set 1:
TBH I don't know if this change will cause problems on other platforms.
Xi Chen has uploaded a new patch set (#3) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Makefile.inc: Alloc .bss.* sections for "struct" file type
When the global variable of a "struct" CBFS file is zero (for example, CB:47696), the binary will appear in the .bss.* section in the ELF file (instead of .data). This results in an empty binary file added to CBFS, so that file size check will fail when reading it at runtime.
BUG=b:173751635 TEST=emerge-asurada coreboot TEST=Check sdram-lpddr4x-KMDP6001DA-B425-4GB is non-empty in CBFS BRANCH=none
Change-Id: Idfd17d10101a948de0eb0522a672afd5c2f83b04 Signed-off-by: Yu-Ping Wu yupingso@google.com --- M Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/47903/3
Yu-Ping Wu has removed Julius Werner from this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Removed reviewer Julius Werner.
Yu-Ping Wu has removed Martin Roth from this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Removed reviewer Martin Roth.
Yu-Ping Wu has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Removed reviewer Patrick Georgi.
Xi Chen has uploaded a new patch set (#4) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Makefile.inc: Alloc .bss.* sections for "struct" file type
When the global variable of a "struct" CBFS file is zero (for example, CB:47696), the binary will appear in the .bss.* section in the ELF file (instead of .data). This results in an empty binary file added to CBFS, so that file size check will fail when reading it at runtime.
BUG=b:173751635 TEST=emerge-asurada coreboot TEST=Check sdram-lpddr4x-KMDP6001DA-B425-4GB is non-empty in CBFS BRANCH=none
Change-Id: Idfd17d10101a948de0eb0522a672afd5c2f83b04 Signed-off-by: Yu-Ping Wu yupingso@google.com --- M Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/47903/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss.* sections for "struct" file type ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Thanks, I think this is generally a good fix to have. However, I do somewhat question what you are doing there with SDRAM params on asurada in a more general sense. There's no big rule that says you *have* to store SDRAM params in a separate file no matter how trivial they are. The reason we do that is to avoid additional loading times for loading all the params when we only need one of them, but that only applies if they're sufficiently big to make this a time save. If your whole SDRAM params consist of a single enum, then please just hardcode that table rather than storing it in separate files (where storing the hardcoded filename of that file would take up more space than the enum value itself).
https://review.coreboot.org/c/coreboot/+/47903/4/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/47903/4/Makefile.inc@340 PS4, Line 340: .bss.* nit: does .bss* also work (so it would include a section just called ".bss", in case -fdata-sections wasn't on for some reason)?
Hello Xi Chen, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47903
to look at the new patch set (#5).
Change subject: Makefile.inc: Alloc .bss* sections for "struct" file type ......................................................................
Makefile.inc: Alloc .bss* sections for "struct" file type
When the global variable of a "struct" CBFS file is zero (for example, CB:47696), the binary will appear in the .bss* section in the ELF file (instead of .data). This results in an empty binary file added to CBFS, so that file size check will fail when reading it at runtime.
BUG=b:173751635 TEST=emerge-asurada coreboot TEST=Check sdram-lpddr4x-KMDP6001DA-B425-4GB is non-empty in CBFS BRANCH=none
Change-Id: Idfd17d10101a948de0eb0522a672afd5c2f83b04 Signed-off-by: Yu-Ping Wu yupingso@google.com --- M Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/47903/5
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss* sections for "struct" file type ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47903/4/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/47903/4/Makefile.inc@340 PS4, Line 340: .bss.*
nit: does .bss* also work (so it would include a section just called ". […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss* sections for "struct" file type ......................................................................
Patch Set 5: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47903 )
Change subject: Makefile.inc: Alloc .bss* sections for "struct" file type ......................................................................
Makefile.inc: Alloc .bss* sections for "struct" file type
When the global variable of a "struct" CBFS file is zero (for example, CB:47696), the binary will appear in the .bss* section in the ELF file (instead of .data). This results in an empty binary file added to CBFS, so that file size check will fail when reading it at runtime.
BUG=b:173751635 TEST=emerge-asurada coreboot TEST=Check sdram-lpddr4x-KMDP6001DA-B425-4GB is non-empty in CBFS BRANCH=none
Change-Id: Idfd17d10101a948de0eb0522a672afd5c2f83b04 Signed-off-by: Yu-Ping Wu yupingso@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47903 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index fafb9ec..95846a7 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -338,7 +338,7 @@ $(eval $(2): $(1) $(obj)/build.h $(KCONFIG_AUTOHEADER); \ printf " CC+STRIP $(@)\n"; \ $(CC_ramstage) -MMD $(CPPFLAGS_ramstage) $(CFLAGS_ramstage) $$(ramstage-c-ccopts) -include $(KCONFIG_AUTOHEADER) -MT $(2) -o $(2).tmp -c $(1) && \ - $(OBJCOPY_ramstage) -O binary $(2).tmp $(2); \ + $(OBJCOPY_ramstage) -O binary --set-section-flags .bss*=alloc,contents,load $(2).tmp $(2); \ rm -f $(2).tmp) \ $(eval DEPENDENCIES += $(2).d)
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Xi Chen (Code Review)
-
Yu-Ping Wu (Code Review)