Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown
This patch disables FSP-S chipset lockdown UPDs and let coreboot perform chipset lockdown in ramstage.
Change-Id: I7e53c4e4987a7b0e7f475c92b0f797d94fdd60f4 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34541/1
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 6a2c038..5e030b8 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -19,6 +19,7 @@ #include <fsp/api.h> #include <fsp/util.h> #include <intelblocks/xdci.h> +#include <intelpch/lockdown.h> #include <soc/intel/common/vbt.h> #include <soc/pci_devs.h> #include <soc/ramstage.h> @@ -405,6 +406,25 @@ configure_gspi_cs(i, config, ¶ms->SerialIoSpiCsPolarity[0], NULL, NULL); #endif + + /* Chipset Lockdown */ + if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) { + tconfig->PchLockDownGlobalSmi = 0; + tconfig->PchLockDownBiosInterface = 0; + params->PchLockDownBiosLock = 0; + params->PchLockDownRtcMemoryLock = 0; + /* + * TODO: Disable SpiFlashCfgLockDown config after FSP provides + * dedicated UPD + * + * Skip Spi Flash Lockdown from inside FSP. + * Making this config "0" means FSP won't set the FLOCKDN bit + * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL). + * So, it becomes coreboot's responsibility to set this bit + * before end of POST for security concerns. + */ + // params->SpiFlashCfgLockDown = 0; + } }
/* Mainboard GPIO Configuration */
Hello Patrick Rudolph, Aamir Bohra, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34541
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown
This patch disables FSP-S chipset lockdown UPDs and let coreboot perform chipset lockdown in ramstage.
BUG=b:138200201 TEST=FSP debug build suggested those UPDs are disable now.
Change-Id: I7e53c4e4987a7b0e7f475c92b0f797d94fdd60f4 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34541/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG@9 PS2, Line 9: let lets
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG@10 PS2, Line 10: chipset lockdown in ramstage. Why does coreboot need the chipset to be unlocked in ramstage?
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG@13 PS2, Line 13: suggested suggests
https://review.coreboot.org/c/coreboot/+/34541/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/34541/2/src/soc/intel/cannonlake/fs... PS2, Line 420: Spi SPI
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG@9 PS2, Line 9: let
lets
Done
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG@10 PS2, Line 10: chipset lockdown in ramstage.
Why does coreboot need the chipset to be unlocked in ramstage?
its like coreboot don't want FSP to lock down chipset during FSP-S and we (CB) might still using SPI registers to store "some" data hence we prefers FSP skip locking and cb locked those registers in finalize.c at end of POST
https://review.coreboot.org/c/coreboot/+/34541/2//COMMIT_MSG@13 PS2, Line 13: suggested
suggests
Done
https://review.coreboot.org/c/coreboot/+/34541/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/34541/2/src/soc/intel/cannonlake/fs... PS2, Line 420: Spi
SPI
Done
Hello Patrick Rudolph, Aamir Bohra, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34541
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown
This patch disables FSP-S chipset lockdown UPDs and lets coreboot perform chipset lockdown in ramstage.
BUG=b:138200201 TEST=FSP debug build suggests those UPDs are disable now.
Change-Id: I7e53c4e4987a7b0e7f475c92b0f797d94fdd60f4 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34541/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... PS3, Line 412: tconfig->PchLockDownGlobalSmi = 0; : tconfig->PchLockDownBiosInterface = 0; : params->PchLockDownBiosLock = 0; : params->PchLockDownRtcMemoryLock = 0; : /* Is the default for all these configs 1 in the UPD? I am asking because there is no else block for the check here.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... PS3, Line 412: tconfig->PchLockDownGlobalSmi = 0; : tconfig->PchLockDownBiosInterface = 0; : params->PchLockDownBiosLock = 0; : params->PchLockDownRtcMemoryLock = 0; : /*
Is the default for all these configs 1 in the UPD? I am asking because there is no else block for th […]
Last time i have checked this, those are default enable but i can crosscheck again.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 3: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... PS3, Line 412: tconfig->PchLockDownGlobalSmi = 0; : tconfig->PchLockDownBiosInterface = 0; : params->PchLockDownBiosLock = 0; : params->PchLockDownRtcMemoryLock = 0; : /*
Last time i have checked this, those are default enable but i can crosscheck again.
That doesn't even matter. If someone decides to change the default, it would break here. It's better to always set those to sane values.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 3: Code-Review-1
Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Hello Patrick Rudolph, Paul Fagerburg, Aamir Bohra, Patrick Rudolph, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34541
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown
This patch disables FSP-S chipset lockdown UPDs and lets coreboot perform chipset lockdown in ramstage.
BUG=b:138200201 TEST=FSP debug build suggests those UPDs are disable now.
Change-Id: I7e53c4e4987a7b0e7f475c92b0f797d94fdd60f4 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34541/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/34541/3/src/soc/intel/cannonlake/fs... PS3, Line 412: tconfig->PchLockDownGlobalSmi = 0; : tconfig->PchLockDownBiosInterface = 0; : params->PchLockDownBiosLock = 0; : params->PchLockDownRtcMemoryLock = 0; : /*
That doesn't even matter. […]
Ack
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 4: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 4: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
Patch Set 4: Code-Review+2
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34541 )
Change subject: soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown ......................................................................
soc/intel/cannonlake: Allow coreboot to handle required chipset lockdown
This patch disables FSP-S chipset lockdown UPDs and lets coreboot perform chipset lockdown in ramstage.
BUG=b:138200201 TEST=FSP debug build suggests those UPDs are disable now.
Change-Id: I7e53c4e4987a7b0e7f475c92b0f797d94fdd60f4 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34541 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 34 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 6fb3060..f696f79 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -19,6 +19,7 @@ #include <fsp/api.h> #include <fsp/util.h> #include <intelblocks/xdci.h> +#include <intelpch/lockdown.h> #include <soc/intel/common/vbt.h> #include <soc/pci_devs.h> #include <soc/ramstage.h> @@ -402,6 +403,39 @@ configure_gspi_cs(i, config, ¶ms->SerialIoSpiCsPolarity[0], NULL, NULL); #endif + + /* Chipset Lockdown */ + if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) { + tconfig->PchLockDownGlobalSmi = 0; + tconfig->PchLockDownBiosInterface = 0; + params->PchLockDownBiosLock = 0; + params->PchLockDownRtcMemoryLock = 0; + /* + * TODO: Disable SpiFlashCfgLockDown config after FSP provides + * dedicated UPD + * + * Skip SPI Flash Lockdown from inside FSP. + * Making this config "0" means FSP won't set the FLOCKDN bit + * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL). + * So, it becomes coreboot's responsibility to set this bit + * before end of POST for security concerns. + */ + // params->SpiFlashCfgLockDown = 0; + } else { + tconfig->PchLockDownGlobalSmi = 1; + tconfig->PchLockDownBiosInterface = 1; + params->PchLockDownBiosLock = 1; + params->PchLockDownRtcMemoryLock = 1; + /* + * TODO: Enable SpiFlashCfgLockDown config after FSP provides + * dedicated UPD + * + * Enable SPI Flash Lockdown from inside FSP. + * Making this config "1" means FSP will set the FLOCKDN bit + * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL). + */ + // params->SpiFlashCfgLockDown = 1; + } }
/* Mainboard GPIO Configuration */