coreboot-gerrit December 2021

coreboot-gerrit@coreboot.org

1 participants
5525 discussions

Change in coreboot[master]: libpayload: Implement new CBFS access API
by Julius Werner (Code Review) 14 Dec '21

14 Dec '21

Attention is currently required from: Jakub Czapiga, Paul Menzel. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59497 ) Change subject: libpayload: Implement new CBFS access API ...................................................................... Patch Set 7: (15 comments) File payloads/libpayload/include/cbfs.h: https://review.coreboot.org/c/coreboot/+/59497/comment/935b7e17_2771a67a PS7, Line 24: /* Removes a previously allocated CBFS mapping. */ (It's kinda weird that this has a documentation comment when everything else doesn't, though... I'd say this also counts under "look in coreboot's <cbfs.h>".) https://review.coreboot.org/c/coreboot/+/59497/comment/4f2a98a4_56e4ffbd PS7, Line 40: #include <cbfs_boot_device.h> Please move this to the includes at the top of the file. https://review.coreboot.org/c/coreboot/+/59497/comment/4c129a81_6995f891 PS7, Line 120: > Even though file sizes shouldn't be zero, I would write >= here to have the proper symmetry with the "failed" tests in the functions above. File payloads/libpayload/include/cbfs.h: https://review.coreboot.org/c/coreboot/+/59497/comment/f8e9d3bc_49e98e04 PS5, Line 35: /* Defined in include/cbfs_glue.h */ > I didn't want to include cbfs_glue. […] Oh, okay, that's a good point. File payloads/libpayload/libcbfs/Kconfig: https://review.coreboot.org/c/coreboot/+/59497/comment/0ef12ac4_ab7d9c53 PS7, Line 19: bool "Enable CBFS files verification" nit: I'd say "Enable CBFS verification" to match coreboot. File payloads/libpayload/libcbfs/cbfs.c: https://review.coreboot.org/c/coreboot/+/59497/comment/47c8190c_bba6a77f PS5, Line 10: #define CB_CBFS_CORE_WITH_LZ4 > You can just always include <lzma.h> and <lz4.h>, you don't need to guard that. https://review.coreboot.org/c/coreboot/+/59497/comment/dc2d27d3_75c3110f PS5, Line 34: if (!rw.mcache_size) { > I'd leave it here. […] Well, mcache_size is always used to determine whether the mcache is valid or not, and that doesn't relocate. File payloads/libpayload/libcbfs/cbfs.c: https://review.coreboot.org/c/coreboot/+/59497/comment/83e910e0_6454c586 PS7, Line 28: rw.dev.offset = (size_t)phys_to_virt(lib_sysinfo.cbfs_offset); Nonono, phys_to_virt() is for pointers, this is a flash offset. https://review.coreboot.org/c/coreboot/+/59497/comment/b889fd6e_d467405e PS7, Line 43: ro.dev.offset = (size_t)phys_to_virt(ro.dev.offset); same https://review.coreboot.org/c/coreboot/+/59497/comment/bb6807bd_78c86a4c PS7, Line 109: size nit: I'd maybe call it in_size for clarity https://review.coreboot.org/c/coreboot/+/59497/comment/544c2392_43966d6f PS7, Line 123: return size; Careful with the early return now... if you already introduce hashing, then you must have it here as well. https://review.coreboot.org/c/coreboot/+/59497/comment/bf3c6b8a_e8228e09 PS7, Line 131: if (cbfs_file_hash_mismatch(buffer, size, mdata)) This must happen after the boot_device_read(), obviously. Maybe this whole thing can be written nicely as: void *load = buffer; if (compression != CBFS_COMPRESS_NONE) { load = malloc(size); if (!load) { ERROR(...); return 0; } } if (boot_device_read(load, offset, size) != size) { ERROR(...); goto out; } if (cbfs_file_hash_mismatch(load, size, mdata)) goto out; switch (compression) { case CBFS_COMPRESS_NONE: out_size = size; break; case CBFS_COMPRESS_LZ4: if (!CONFIG(LP_LZ4)) goto out; out_size = ulz4fn(...); break; case ... } out: if (load != buffer) free(load); return out_size; (This is omitting the buffer_size < size check for COMPRESS_NONE which is already checked by the only caller in this case.) https://review.coreboot.org/c/coreboot/+/59497/comment/509c537f_a635dadd PS7, Line 139: ERROR("Failed to read contents of file\n"); Need to free() scratch here. https://review.coreboot.org/c/coreboot/+/59497/comment/b6fe7b11_163ca487 PS7, Line 185: { Forgot to delete the brace https://review.coreboot.org/c/coreboot/+/59497/comment/b0706503_dd32055e PS7, Line 197: NULL You gotta actually pass it if you want to use it, though. -- To view, visit https://review.coreboot.org/c/coreboot/+/59497 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I00da0658dbac0cddf92ad55611def947932d23c7 Gerrit-Change-Number: 59497 Gerrit-PatchSet: 7 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 13 Dec 2021 23:47:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Documentation/releases: Update 4.16 release notes
by Felix Held (Code Review) 14 Dec '21

14 Dec '21

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59564 ) Change subject: Documentation/releases: Update 4.16 release notes ...................................................................... Documentation/releases: Update 4.16 release notes * Add StarBook Mk V as new mainboard * Add option to disable Intel Management Engine via HECI Signed-off-by: Sean Rhodes <sean(a)starlabs.systems> Change-Id: I9675a6a8960d93ae6de285d8b25ffc48a763483e Reviewed-on: https://review.coreboot.org/c/coreboot/+/59564 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M Documentation/releases/coreboot-4.16-relnotes.md 1 file changed, 7 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/Documentation/releases/coreboot-4.16-relnotes.md b/Documentation/releases/coreboot-4.16-relnotes.md index a4c7af2..83b2760 100644 --- a/Documentation/releases/coreboot-4.16-relnotes.md +++ b/Documentation/releases/coreboot-4.16-relnotes.md @@ -17,3 +17,10 @@ ------------------- ### Add significant changes here + +### Option to disable Intel Management Engine +Disable the Intel (CS)Management Engine via HECI based on Intel Core processors +from Skylake to Alderlake. State is set baed on a cmos value of `me_state`. A +value of `0` will result in a (CS)ME state of `0` (working) and value of `1` +will result in a (CS)ME state of `3` (disabled). For an example cmos layout and +more info, see [cse.c](../../src/soc/intel/common/block/cse/cse.c). -- To view, visit https://review.coreboot.org/c/coreboot/+/59564 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9675a6a8960d93ae6de285d8b25ffc48a763483e Gerrit-Change-Number: 59564 Gerrit-PatchSet: 3 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: arch/x86/c_start.S: Remove duplicated "the" in comments
by Felix Held (Code Review) 14 Dec '21

14 Dec '21

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/60067 ) Change subject: arch/x86/c_start.S: Remove duplicated "the" in comments ...................................................................... arch/x86/c_start.S: Remove duplicated "the" in comments Change-Id: Ib1be1db6f475ad0e1f34703bfe1257d02b86742c Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> Reviewed-on: https://review.coreboot.org/c/coreboot/+/60067 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/arch/x86/c_start.S 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S index c3fdfd9..84fbed2 100644 --- a/src/arch/x86/c_start.S +++ b/src/arch/x86/c_start.S @@ -4,7 +4,7 @@ #include <cpu/x86/post_code.h> #include <arch/ram_segs.h> -/* Place the stack in the bss section. It's not necessary to define it in the +/* Place the stack in the bss section. It's not necessary to define it in * the linker script. */ .section .bss, "aw", @nobits .global _stack -- To view, visit https://review.coreboot.org/c/coreboot/+/60067 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib1be1db6f475ad0e1f34703bfe1257d02b86742c Gerrit-Change-Number: 60067 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/amd/cezanne: Move signed amdfw from FW_MAIN into SIGNED_AMDFW
by Kangheui Won (Code Review) 14 Dec '21

14 Dec '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Paul Menzel, Karthik Ramasubramanian, Felix Held. Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59867 ) Change subject: soc/amd/cezanne: Move signed amdfw from FW_MAIN into SIGNED_AMDFW ...................................................................... Patch Set 4: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59867/comment/96eb4c5d_ce64d559 PS3, Line 7: Separate signed amdfw > Maybe use: […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/59867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4fd3cff11a38d82afb8c5ce379f1d1b5b9adfbf Gerrit-Change-Number: 59867 Gerrit-PatchSet: 4 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 13 Dec 2021 23:01:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/cezanne: Move signed amdfw from FW_MAIN into SIGNED_AMDFW
by Kangheui Won (Code Review) 14 Dec '21

14 Dec '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Kangheui Won, Karthik Ramasubramanian, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Marshall Dawson, Karthik Ramasubramanian, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59867 to look at the new patch set (#4). Change subject: soc/amd/cezanne: Move signed amdfw from FW_MAIN into SIGNED_AMDFW ...................................................................... soc/amd/cezanne: Move signed amdfw from FW_MAIN into SIGNED_AMDFW Put signed amd firmwares into SIGNED_AMDFW_[AB] region which is outside FW_MAIN_[AB]. Vboot only verifies FW_MAIN_[AB] so these regions will not be verified by vboot; instead the PSP will verify them. As a result we have less to load and verify from SPI rom which means faster boot time. BUG=b:206909680 TEST=build guybrush with modified fmap and Kconfig Signed-off-by: Kangheui Won <khwon(a)chromium.org> Change-Id: If4fd3cff11a38d82afb8c5ce379f1d1b5b9adfbf --- M src/soc/amd/cezanne/Kconfig M src/soc/amd/cezanne/Makefile.inc 2 files changed, 28 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/59867/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/59867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4fd3cff11a38d82afb8c5ce379f1d1b5b9adfbf Gerrit-Change-Number: 59867 Gerrit-PatchSet: 4 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Kangheui Won <khwon(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: amdfwtool: Add options to separate signed firmwares
by Kangheui Won (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Raul Rangel, Karthik Ramasubramanian. Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59866 ) Change subject: amdfwtool: Add options to separate signed firmwares ...................................................................... Patch Set 2: (1 comment) File util/amdfwtool/amdfwtool.h: https://review.coreboot.org/c/coreboot/+/59866/comment/cd303bae_8bf11495 PS2, Line 225: amd_fw_header > Where did you get this from? Directly from the AMD. They confirmed that these information can be included in the upstream coreboot. -- To view, visit https://review.coreboot.org/c/coreboot/+/59866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f3610a7002b2a9c70946b083b0b3be6934200b0 Gerrit-Change-Number: 59866 Gerrit-PatchSet: 2 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 13 Dec 2021 22:59:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[4.11_branch]: mainboard/ocp/monolake: Expand BIOS region
by Marc Jones (Code Review) 13 Dec '21

13 Dec '21

Marc Jones has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/55366 ) Change subject: mainboard/ocp/monolake: Expand BIOS region ...................................................................... Abandoned Leave this the same as the BIOS. The vendor adds this patch during their build to change the size. -- To view, visit https://review.coreboot.org/c/coreboot/+/55366 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: 4.11_branch Gerrit-Change-Id: I4161c920de2f7cbafa103bb2068bda50ac02e992 Gerrit-Change-Number: 55366 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Javier Galindo <javiergalindo(a)sysproconsulting.com> Gerrit-Reviewer: Jay Talbott <JayTalbott(a)sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Rocky Phagura Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: insomniac <insomniac(a)slackware.it> Gerrit-MessageType: abandon

1 0

Change in coreboot[master]: libpayload: Enable vboot integration
by Julius Werner (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Jakub Czapiga, Jan Dabros, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60080 ) Change subject: libpayload: Enable vboot integration ...................................................................... Patch Set 2: (22 comments) File payloads/libpayload/Makefile: https://review.coreboot.org/c/coreboot/+/60080/comment/bec0bc52_1c733f19 PS2, Line 276: $(eval $(1)-libs:=) \ Since you need to filter for file extensions in the end anyway, I'm not so sure this really does much, and you might as well add them to objs directly, that's closer to how it works in coreboot. File payloads/libpayload/Makefile.inc: https://review.coreboot.org/c/coreboot/+/60080/comment/0d381648_1fa1a653 PS2, Line 82: $(addsuffix .a,$(addprefix $(obj)/,$(libraries))) Doesn't this also ask for an $(obj)/vboot.a (which it then tries to install with make install)? It looks like that would break to me. Maybe the simpler version is just to add vboot to classes-y after `libraries` was initially assigned, and then add the correct vboot and tlcl library names to it manually in vboot/Makefile? https://review.coreboot.org/c/coreboot/+/60080/comment/8147f7ba_42d9011d PS2, Line 98: cat <(printf "open %s\n" "$@") \ I'm not sure how portable this Makefile needs to be... I believe the <() pattern is bash-specific. Can you maybe achieve this just with make? Something like printf "open $@\n$(foreach lib,%(filter %.a,$^),addlib $(lib)\n)save\nend\n" | $(AR) -M File payloads/libpayload/vboot/Kconfig: https://review.coreboot.org/c/coreboot/+/60080/comment/46ff4372_20943c04 PS2, Line 6: VBoot It's `vboot`, all letters lower case. Alternatively you could say `Enable verified boot` or `Add support for verified boot` or something. https://review.coreboot.org/c/coreboot/+/60080/comment/2f7e5560_c8c39b98 PS2, Line 9: buildinf typo https://review.coreboot.org/c/coreboot/+/60080/comment/ccda784a_1144ee44 PS2, Line 14: string "vboot architecture" Are you intentionally making this menuconfig-configurable? I don't see why anyone would ever want to override it manually. I don't see why it needs to be a Kconfig at all, why not just do this translation in the Makefile? https://review.coreboot.org/c/coreboot/+/60080/comment/c97d2284_60fd2f59 PS2, Line 20: config VBOOT_DEBUG We should just always enable this for firmware builds, same as coreboot. https://review.coreboot.org/c/coreboot/+/60080/comment/92efe5f2_88f5756d PS2, Line 24: config VBOOT_DEBUG_PRINT This is equivalent to DEBUG for firmware builds. https://review.coreboot.org/c/coreboot/+/60080/comment/37bbfdc8_6bc7f561 PS2, Line 28: config VBOOT_FORCE_LOGGING_ON Irrelevant for firmware https://review.coreboot.org/c/coreboot/+/60080/comment/a0f57b34_2bad8acf PS2, Line 36: config VBOOT_GPT_SPI_NOR Irrelevant for firmware https://review.coreboot.org/c/coreboot/+/60080/comment/759bc25e_2007e017 PS2, Line 40: config VBOOT_TPM2_SIMULATOR same https://review.coreboot.org/c/coreboot/+/60080/comment/3dd0566e_2bdc4521 PS2, Line 44: config VBOOT_VTPM_PROXY same https://review.coreboot.org/c/coreboot/+/60080/comment/21e5d1c7_06e4566b PS2, Line 48: config VBOOT_STATIC same https://review.coreboot.org/c/coreboot/+/60080/comment/2ca13367_71a5f874 PS2, Line 52: config VBOOT_EXTRA_CFLAGS What did you add this for? FUZZ_FLAGS is irrelevant for firmware builds anyway. https://review.coreboot.org/c/coreboot/+/60080/comment/ec5a932f_4e544d65 PS2, Line 59: default y if ARCH_X86 This should probably just default to off, because both older Intel chips and AMD don't support it. Also, it should have a help text to explain what it does (in fact, all menuconfig-visible options here should have that). https://review.coreboot.org/c/coreboot/+/60080/comment/707b081e_f84f6427 PS2, Line 63: bool Well, if it's not menuconfig-visible you don't need to have it at all. FWIW, this is only really a code size issue which we don't tend to have in the payload, so I think we don't need the option (just always unroll loops). File payloads/libpayload/vboot/Makefile.inc: https://review.coreboot.org/c/coreboot/+/60080/comment/688eefb6_5ab6031d PS2, Line 7: TLCL_LIB = $(VBOOT_BUILD_DIR)/tlcl.a Since make doesn't track dependencies across sub-invocations, you should mark both of these targets as .PHONY, like depthcharge does it. https://review.coreboot.org/c/coreboot/+/60080/comment/a816923e_57009645 PS2, Line 9: ifeq ($(CONFIG_LP_VBOOT),y) Doesn't the subdirs-y already guard this whole file, do we need to check this here again? https://review.coreboot.org/c/coreboot/+/60080/comment/a780cd6a_502132fa PS2, Line 12: vboot-libs += $(TLCL_LIB) No, TLCL needs to always be added, for both TPM versions (that's what the TPM2_MODE flag mainly decides, whether it builds tlcl.a for TPM 1.2 or 2.0). https://review.coreboot.org/c/coreboot/+/60080/comment/32f04cde_d75f0499 PS2, Line 16: kconfig-to-binary=$(if $(strip $(1)),$(strip $(subst n,0,$(subst y,1,$(1)))),0) Can `n` even occur here? I thought this is always either defined to `y` or not defined at all. Then this expression could be much simpler (just `$(if $(1),1,0)` -- I don't think you need to strip either, `if` should already strip automatically). https://review.coreboot.org/c/coreboot/+/60080/comment/f3055bd3_f0c640c4 PS2, Line 23: +$(Q) unset CFLAGS CXXFLAGS LDFLAGS && \ For firmware targets, vboot should always be built with the same CFLAGS as the calling binary (at least that's how we have done it for years, and I don't want to have to go figure out what the gnarly old cruft in the vboot Makefile would try to do if you don't). See how coreboot and depthcharge are doing this in their Makefiles. So it shouldn't matter whether you unset them here, but you should definitely override them with libpayload's current CFLAGS. (You may also need to rewrite relative include paths to absolute ones like the coreboot Makefile is doing it, unless all of libpayload's paths are already absolute which I guess(?) is how it works for depthcharge.) https://review.coreboot.org/c/coreboot/+/60080/comment/9b3cbabb_b0857f96 PS2, Line 27: DEBUG=$(call kconfig-to-binary, $(CONFIG_LP_VBOOT_DEBUG)) \ This *should* be the same as just adding -DVBOOT_DEBUG to CFLAGS for firmware targets, but since both coreboot and depthcharge are doing it that other way I would do the same just to be safe. -- To view, visit https://review.coreboot.org/c/coreboot/+/60080 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d9d766a461edaa0081041c020ecf580fd2ca64e Gerrit-Change-Number: 60080 Gerrit-PatchSet: 2 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Jan Dabros <jsd(a)semihalf.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Jan Dabros <jsd(a)semihalf.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Mon, 13 Dec 2021 22:48:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cbfstool: Use converted buffer size for do_cbfs_locate()
by Raul Rangel (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Patrick Georgi, Julius Werner. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60084 ) Change subject: cbfstool: Use converted buffer size for do_cbfs_locate() ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/60084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1af35e8e388f78aae3593c029afcfb4e510d2b8f Gerrit-Change-Number: 60084 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Mon, 13 Dec 2021 22:19:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cbfstool: Clean up remnants of locate action
by Raul Rangel (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Patrick Georgi, Julius Werner. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60085 ) Change subject: cbfstool: Clean up remnants of locate action ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/60085 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib098278d68df65d348528fbfd2496b5737ca6246 Gerrit-Change-Number: 60085 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Mon, 13 Dec 2021 22:17:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2021