Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31639
Change subject: sb/intel/common/firmware: Add an option to use IFDTOOL ......................................................................
sb/intel/common/firmware: Add an option to use IFDTOOL
This patch makes the use of the IFDTOOL to modify the flash descriptor region selectable via a Kconfig option. This option is selected by default so that nothing changes for all mainboards that have activate 'Lock ME/TXE section'. If you don't want to use IFDTOOL for modification of flash descriptor, disable it. In this case, the preset values are retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc 2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/1
diff --git a/src/southbridge/intel/common/firmware/Kconfig b/src/southbridge/intel/common/firmware/Kconfig index 31a3df3..7a1bd82 100644 --- a/src/southbridge/intel/common/firmware/Kconfig +++ b/src/southbridge/intel/common/firmware/Kconfig @@ -141,6 +141,18 @@ depends on HAVE_EC_BIN default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/ec.bin"
+config MOD_INTEL_FLASH_DESCRIPTOR + bool "Use IFDTOOL to modifiy descriptor region" + default y + help + Read and write access permissions to different regions in the flash + can be controlled via dedicated bitfields in the flash descriptor. + These permissions can be modified with the Intel Flash Descriptor + Tool (IFDTOOL). If you don't want to change these permissions and + keep the ones provided in the initial descriptor, disable this switch. + +if MOD_INTEL_FLASH_DESCRIPTOR + config LOCK_MANAGEMENT_ENGINE bool "Lock ME/TXE section" default n @@ -154,6 +166,8 @@
If unsure, say N.
+endif #MOD_INTEL_FLASH_DESCRIPTOR + config CBFS_SIZE hex default 0x100000 diff --git a/src/southbridge/intel/common/firmware/Makefile.inc b/src/southbridge/intel/common/firmware/Makefile.inc index 774bb23..331382e 100644 --- a/src/southbridge/intel/common/firmware/Makefile.inc +++ b/src/southbridge/intel/common/firmware/Makefile.inc @@ -68,6 +68,8 @@ $(obj)/coreboot.pre mv $(obj)/coreboot.pre.new $(obj)/coreboot.pre endif + +ifeq ($(CONFIG_MOD_INTEL_FLASH_DESCRIPTOR),y) ifeq ($(CONFIG_LOCK_MANAGEMENT_ENGINE),y) printf " IFDTOOL Locking Management Engine\n" $(objutil)/ifdtool/ifdtool \ @@ -79,6 +81,11 @@ $(IFDTOOL_USE_CHIPSET) -u $(obj)/coreboot.pre mv $(obj)/coreboot.pre.new $(obj)/coreboot.pre endif +else + printf " Don't touch Intel Flash Descriptor\n" + cp $(obj)/coreboot.pre $(obj)/coreboot.pre.new + mv $(obj)/coreboot.pre.new $(obj)/coreboot.pre +endif
ifeq ($(CONFIG_EM100),y) printf " IFDTOOL Setting EM100 mode\n"
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use IFDTOOL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31639/1/src/southbridge/intel/common/firmwar... File src/southbridge/intel/common/firmware/Makefile.inc:
https://review.coreboot.org/#/c/31639/1/src/southbridge/intel/common/firmwar... PS1, Line 86: cp $(obj)/coreboot.pre $(obj)/coreboot.pre.new : mv $(obj)/coreboot.pre.new $(obj)/coreboot.pre Isn't this effectively copying $(obj)/coreboot.pre to $(obj)/coreboot.pre?
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use IFDTOOL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31639/1/src/southbridge/intel/common/firmwar... File src/southbridge/intel/common/firmware/Makefile.inc:
https://review.coreboot.org/#/c/31639/1/src/southbridge/intel/common/firmwar... PS1, Line 86: cp $(obj)/coreboot.pre $(obj)/coreboot.pre.new : mv $(obj)/coreboot.pre.new $(obj)/coreboot.pre
Isn't this effectively copying $(obj)/coreboot.pre to $(obj)/coreboot. […]
Yes, of course, you’re right. That stayed in from the first experiments with the IFDTOOL.
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31639
to look at the new patch set (#2).
Change subject: sb/intel/common/firmware: Add an option to use IFDTOOL ......................................................................
sb/intel/common/firmware: Add an option to use IFDTOOL
This patch makes the use of the IFDTOOL to modify the flash descriptor region selectable via a Kconfig option. This option is selected by default so that nothing changes for all mainboards that have activate 'Lock ME/TXE section'. If you don't want to use IFDTOOL for modification of flash descriptor, disable it. In this case, the preset values are retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use IFDTOOL ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31639/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31639/2//COMMIT_MSG@7 PS2, Line 7: IFDTOOL Please use "ifdtool" in lowercase, if this refers to util/ifdtool
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31639
to look at the new patch set (#3).
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
sb/intel/common/firmware: Add an option to use ifdtool
This patch makes the use of the ifdtool to modify the flash descriptor region selectable via a Kconfig option. This option is selected by default so that nothing changes for all mainboards that have activate 'Lock ME/TXE section'. If you don't want to use ifdtool for modification of flash descriptor, disable it. In this case, the preset values are retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/3
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31639/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31639/2//COMMIT_MSG@7 PS2, Line 7: IFDTOOL
Please use "ifdtool" in lowercase, if this refers to util/ifdtool
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 3:
I would pretty much prefer the opposite, i.e. only modify the descriptor if asked to.
In any case, as this implementation only affects the descriptor lock, a Kconfig choice would be better. Would make both Kconfig and Makefile simpler, I guess.
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 3:
Patch Set 3:
I would pretty much prefer the opposite, i.e. only modify the descriptor if asked to.
In any case, as this implementation only affects the descriptor lock, a Kconfig choice would be better. Would make both Kconfig and Makefile simpler, I guess.
Just so I get it right, you mean that else branch of CONFIG_LOCK_MANAGEMENT_ENGINE in the Makefile doesn’t use ifdtool, right?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 3:
I would pretty much prefer the opposite, i.e. only modify the descriptor if asked to.
In any case, as this implementation only affects the descriptor lock, a Kconfig choice would be better. Would make both Kconfig and Makefile simpler, I guess.
Just so I get it right, you mean that else branch of CONFIG_LOCK_MANAGEMENT_ENGINE in the Makefile doesn’t use ifdtool, right?
Um, no, there are three cases: 1. We want to leave the des- criptor untouched (I think this should be the default). 2. We want to make sure it's locked. 3. We want to make sure it's unlocked. Hence a Kconfig `choice` would reflect that much better.
In the Makefile this could be two separate `if` statements, one for 2. and one for 3. And if none are taken, 1. is implied.
Hello Patrick Rudolph, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31639
to look at the new patch set (#4).
Change subject: sb/intel/common/firmware: Add a choice to lock ME/TXE ......................................................................
sb/intel/common/firmware: Add a choice to lock ME/TXE
This patch makes the way to protect flash regions selectable. The new option is selected by default. In this case, no changes are made to the descriptor region with the ifdtool. For all mainboards which had selected 'Lock ME/TXE section' up to now, this setting will be retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc 2 files changed, 27 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/4
Hello Patrick Rudolph, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31639
to look at the new patch set (#5).
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
sb/intel/common/firmware: Add an option to use ifdtool
This patch makes the use of the ifdtool to modify the flash descriptor region selectable via a Kconfig option. This option is selected by default so that nothing changes for all mainboards that have activate 'Lock ME/TXE section'. If you don't want to use ifdtool for modification of flash descriptor, disable it. In this case, the preset values are retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/5
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 5:
Patch Set 3:
I would pretty much prefer the opposite, i.e. only modify the descriptor if asked to.
In any case, as this implementation only affects the descriptor lock, a Kconfig choice would be better. Would make both Kconfig and Makefile simpler, I guess.
Just so I get it right, you mean that else branch of CONFIG_LOCK_MANAGEMENT_ENGINE in the Makefile doesn’t use ifdtool, right?
Um, no, there are three cases: 1. We want to leave the des- criptor untouched (I think this should be the default). 2. We want to make sure it's locked. 3. We want to make sure it's unlocked. Hence a Kconfig `choice` would reflect that much better.
In the Makefile this could be two separate `if` statements, one for 2. and one for 3. And if none are taken, 1. is implied.
I think the change to ‘choice’ has two hocks. The first is that if someone did not activate the ME/TXE lock until now and assumes that the regions are all unlocked, they will now come to the default branch – ‘don’t touch’. If now an already been edited descriptor region is used, these protect settings are applied. And the second issue we are getting with switching to ‘choice’ is a conflict with the lynxpoint Kconfig file. This also contains an option with the name ‘LOCK_MANAGEMENT_ENGINE’.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 5:
(1 comment)
I think the change to ‘choice’ has two hocks. The first is that if someone did not activate the ME/TXE lock until now and assumes that the regions are all unlocked, they will now come to the default branch – ‘don’t touch’. If now an already been edited descriptor region is used, these protect settings are applied.
You are right, it's probably too late to change the default as people are used to the current behaviour. I don't care much about the default (IMHO, it's odd to do anything with the descriptor at all). Also see inline comment.
And the second issue we are getting with switching to ‘choice’ is a conflict with the lynxpoint Kconfig file. This also contains an option with the name ‘LOCK_MANAGEMENT_ENGINE’.
Yeah, that lynxpoint entry is a no-op and could be dropped.
https://review.coreboot.org/#/c/31639/4/src/southbridge/intel/common/firmwar... File src/southbridge/intel/common/firmware/Kconfig:
https://review.coreboot.org/#/c/31639/4/src/southbridge/intel/common/firmwar... PS4, Line 146: default USE_IFWI_REGION_ACCESS_CONTROL You could also put UNLOCK_FLASH_REGIONS here, which should resemble the old default.
Hello Patrick Rudolph, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31639
to look at the new patch set (#6).
Change subject: sb/intel/common/firmware: Don't touch descriptor region ......................................................................
sb/intel/common/firmware: Don't touch descriptor region
This patch makes the way to protect flash regions selectable. If you don't want to use ifdtool for modification of flash descriptor, enable the new option. Otherwise, the previous config settings for all mainboards will be retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc M src/southbridge/intel/lynxpoint/Kconfig 3 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/6
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Don't touch descriptor region ......................................................................
Patch Set 6:
Patch Set 5:
(1 comment)
I think the change to ‘choice’ has two hocks. The first is that if someone did not activate the ME/TXE lock until now and assumes that the regions are all unlocked, they will now come to the default branch – ‘don’t touch’. If now an already been edited descriptor region is used, these protect settings are applied.
You are right, it's probably too late to change the default as people are used to the current behaviour. I don't care much about the default (IMHO, it's odd to do anything with the descriptor at all). Also see inline comment.
And the second issue we are getting with switching to ‘choice’ is a conflict with the lynxpoint Kconfig file. This also contains an option with the name ‘LOCK_MANAGEMENT_ENGINE’.
Yeah, that lynxpoint entry is a no-op and could be dropped.
Well thanks, I corrected the adjustments once again.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Don't touch descriptor region ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
Well thanks, I corrected the adjustments once again.
Thank _you_ :)
https://review.coreboot.org/#/c/31639/6/src/southbridge/intel/common/firmwar... File src/southbridge/intel/common/firmware/Makefile.inc:
https://review.coreboot.org/#/c/31639/6/src/southbridge/intel/common/firmwar... PS6, Line 85: No double empty lines please.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Don't touch descriptor region ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31639/6/src/southbridge/intel/common/firmwar... File src/southbridge/intel/common/firmware/Kconfig:
https://review.coreboot.org/#/c/31639/6/src/southbridge/intel/common/firmwar... PS6, Line 169: If unsure, say N. Oh, just remembered, please drop this. Or change into sth. like
If unsure, select "Unlock flash regions".
Hello Patrick Rudolph, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31639
to look at the new patch set (#7).
Change subject: sb/intel/common/firmware: Don't touch descriptor region ......................................................................
sb/intel/common/firmware: Don't touch descriptor region
This patch makes the way to protect flash regions selectable. If you don't want to use ifdtool for modification of flash descriptor, enable the new option. Otherwise, the previous config settings for all mainboards will be retained.
Change-Id: I46ec6339008edcc78fe76682eed5714f85354937 Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/southbridge/intel/common/firmware/Kconfig M src/southbridge/intel/common/firmware/Makefile.inc M src/southbridge/intel/lynxpoint/Kconfig 3 files changed, 27 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/31639/7
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Don't touch descriptor region ......................................................................
Patch Set 7: Code-Review+2
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Mario Scheithauer (Code Review)
-
Nico Huber (Code Review)