Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37154 )
Change subject: Kconfig: Flip the meaning of C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
(11 comments)
Thanks. The comments marked ditto are borderline, I quess we can accept them but I feel they don't exactly fit the title 'flip the default'. Could be one preceding commit?
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/Makefile.inc@1... PS1, Line 131: else # !ROMCC_BOOTBLOCK without ! ?
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/assembly_entry... PS1, Line 22: * clear .bss/CAR_GLOBAL variables that are stage specific. I think you can already wipe CAR_GLOBAL here. It could have been done earlier but I overlooked it.
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/include/arch/c... File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/include/arch/c... PS1, Line 291: * When using not using a romcc bootblock the car_stage_entry() -using
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/37154/1/src/arch/x86/memlayout.ld@5... PS1, Line 52: /* arch/x86/bootblock.ld contains the logic for the romcc bootblock linking. */ Maybe have literal ROMCC_BOOTBLOCK here so we remember to revisit this line.
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/microcode/Kco... File src/cpu/intel/microcode/Kconfig:
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/microcode/Kco... PS1, Line 4: default n if ROMCC_BOOTBLOCK This got flipped?
default y if !ROMCC_BOOTBLOCK
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/model_1067x/K... File src/cpu/intel/model_1067x/Kconfig:
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/model_1067x/K... PS1, Line 15: select SETUP_XIP_CACHE Split, not strictly about flipping and should have been done before?
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/model_6fx/Kco... File src/cpu/intel/model_6fx/Kconfig:
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/model_6fx/Kco... PS1, Line 16: select SETUP_XIP_CACHE ditto
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/socket_LGA775... File src/cpu/intel/socket_LGA775/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37154/1/src/cpu/intel/socket_LGA775... PS1, Line 18: else ditto
https://review.coreboot.org/c/coreboot/+/37154/1/src/mainboard/facebook/fbg1... File src/mainboard/facebook/fbg1701/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37154/1/src/mainboard/facebook/fbg1... PS1, Line 25: bootblock-y += com_init.c ditto
https://review.coreboot.org/c/coreboot/+/37154/1/src/mainboard/portwell/m107... File src/mainboard/portwell/m107/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37154/1/src/mainboard/portwell/m107... PS1, Line 18: bootblock-y += com_init.c ditto
https://review.coreboot.org/c/coreboot/+/37154/1/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/verified_boot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37154/1/src/vendorcode/eltan/securi... PS1, Line 20: ifneq ($(CONFIG_ROMCC_BOOTBLOCK),y) Do you need the conditional here? While built, it will not get linked.