Change in coreboot[master]: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms
PBET timer has to be stopped before APs are launched and initialized. Otherwise the platform will reset. The PBET expiration time may be very low so stop timer as quickly as possible. The expiration time is defined in Boot Guard manifests.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I545a36d60597fe37a30b8207336ae7fa7831674d --- M src/cpu/x86/16bit/entry16.inc 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/43395/1
diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index babed02..c555c5c 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -29,6 +29,7 @@ #include <cpu/x86/post_code.h>
#define BOOTGUARD_SACM_INFO 0x13a +#define BOOTGUARD_PBEC 0x139
/* Symbol _start16bit must be aligned to 4kB to start AP CPUs with * Startup IPI message without RAM. @@ -118,6 +119,14 @@ andl $0x7FFAFFD1, %ebx /* PG,AM,WP,NE,TS,EM,MP = 0 */ orl $0x60000001, %ebx /* CD, NW, PE = 1 */ #if CONFIG(INTEL_BOOTGUARD) + /* + * Stop PBET timer. It is recommended to stop the PBET timer + * regardless of Boot Guard status. + */ + movl $BOOTGUARD_PBEC, %ecx + movl $0, %edx + movl $1, %eax + wrmsr
/* DO NOT disable cache if Intel BootGuard is supported */ movl $BOOTGUARD_SACM_INFO, %ecx
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 129: wrmsr I wonder if you really need to do this in assembly.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/43395/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43395/1//COMMIT_MSG@7 PS1, Line 7: PBET timer nit: this is saying "Protect BIOS Environment Timer timer"
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 32: #define BOOTGUARD_PBEC 0x139 I'd order the MSR definitions in numerically increasing order (0x139 < 0x13a)
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 129: wrmsr
I wonder if you really need to do this in assembly.
As coreboot may take an arbitrarily long time to execute (because debugging stuff and the like), it's better to do this really early.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 129: wrmsr
I wonder if you really need to do this in assembly.
FSP-M is supposed to have an option to stop PBET and the minimum expiration time is 60 seconds according the spec/ On the second hand whet PBET value is set to 0, the timer whouls not run, but still BIOS is responsible to write to this MSR. Due to this confusion I think it is better to stop it here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 129: wrmsr
FSP-M is supposed to have an option to stop PBET and the minimum expiration time is 60 seconds accor […]
I am aware of BOOTBLOCK_DEBUG_SPINLOOP. Other than that, I was just surprised this is done before FSP-T, when apparently Intel design was to do this inside FSP-M. Unless Michal meant to say "option in FSP-T" above.
My suggestion would have been bootblock_soc_early_init() but I don't mind if this is done here.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 129: wrmsr
I am aware of BOOTBLOCK_DEBUG_SPINLOOP. […]
Unfotunately no, it is *FSP-M* that has an option to stop PBET.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/43395/1/src/cpu/x86/16bit/entry16.i... PS1, Line 123: recommended Please add a reference to the documentation.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43395 )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Patch Set 1: Code-Review+1
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43395?usp=email )
Change subject: cpu/x86/16bit/entry16.inc: Stop PBET timer on Boot Guard platforms ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Michael Niewöhner (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)