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.
-
Julius Werner (Code Review)