Gaggery Tsai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
soc/intel/common/block/cpu/car: Enable caching before FSP-T
This patch is required for Boot Guard enabled platform. Enable caching before entering FSP-T.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and esnure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 --- M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/1
diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index 091fc4a..fd22903 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -35,6 +35,12 @@ cache_as_ram: post_code(0x21)
+ /* Enable caching */ + mov %cr0, %eax + and $~(CR0_CD | CR0_NW), %eax + invd + mov %eax, %cr0 + /* find fsp in cbfs */ lea fsp_name, %esi mov $1f, %esp
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38252
to look at the new patch set (#2).
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
soc/intel/common/block/cpu/car: Enable caching before FSP-T
This patch is required for Boot Guard enabled platform. Enable caching before entering FSP-T.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and esnure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com --- M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/2//COMMIT_MSG@11 PS2, Line 11: Please elaborate, why this is required.
Hello Pratikkumar V Prajapati, Patrick Rudolph, Subrata Banik, Balaji Manigandan, Rizwan Qureshi, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38252
to look at the new patch set (#3).
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
soc/intel/common/block/cpu/car: Enable caching before FSP-T
This patch is required for Boot Guard enabled platform. When system is powered on, cache is default enabled. BIOS is fobidden to disable cache while in NEM mode with BtG enabled.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and esnure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/Kconfig 2 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/3
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/2//COMMIT_MSG@11 PS2, Line 11:
Please elaborate, why this is required.
BIOS is forbidden to disable cache while in NEM mode with BtG enabled, that is skip all instructions that might invalidate cache.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 3:
So did BtG never work with upstream coreboot? We've been told for roughly four years that we can't remove FSP-T support because it would break BtG, now it looks like it never worked?
Please also do all further testing without FSP-T but with CB:36682.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 3:
Patch Set 3:
So did BtG never work with upstream coreboot? We've been told for roughly four years that we can't remove FSP-T support because it would break BtG, now it looks like it never worked?
Please also do all further testing without FSP-T but with CB:36682.
Yes. That patch CB:36682 didn't work for me.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 3:
Patch Set 3:
So did BtG never work with upstream coreboot? We've been told for roughly four years that we can't remove FSP-T support because it would break BtG, now it looks like it never worked?
\o/ that means all complaints in cb:36622 are lies?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/16bit/entry16.i... PS3, Line 120: orl $0x01, %eax /* PE = 1 */ that should read MSR 0x13a to see if bootguard is enabled
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/Kconfig@191 PS3, Line 191: cache sets up CAR NEM
Hello Patrick Rudolph, Pratikkumar V Prajapati, Subrata Banik, Balaji Manigandan, Rizwan Qureshi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38252
to look at the new patch set (#4).
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
soc/intel/common/block/cpu/car: Enable caching before FSP-T
This patch is required for Boot Guard enabled platform. When system is powered on, cache is default enabled. BIOS is fobidden to disable cache while in NEM mode with BtG enabled.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and esnure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/Kconfig 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/4
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/16bit/entry16.i... PS3, Line 120: orl $0x01, %eax /* PE = 1 */
that should read MSR 0x13a to see if bootguard is enabled
Done
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/38252/3/src/cpu/x86/Kconfig@191 PS3, Line 191: cache
sets up CAR NEM
Tweak the description a little bit.
Hello Patrick Rudolph, Pratikkumar V Prajapati, Subrata Banik, Balaji Manigandan, Rizwan Qureshi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38252
to look at the new patch set (#5).
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
soc/intel/common/block/cpu/car: Enable caching before FSP-T
This patch is required for Boot Guard enabled platform. When system is powered on, cache is default enabled. BIOS is fobidden to disable cache while in NEM mode with BtG enabled.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and esnure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/Kconfig 2 files changed, 34 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG@10 PS5, Line 10: fobidden missing `r`: foRbidden
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG@15 PS5, Line 15: esnure ensure
Hello Patrick Rudolph, Pratikkumar V Prajapati, Angel Pons, Subrata Banik, Balaji Manigandan, Rizwan Qureshi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38252
to look at the new patch set (#6).
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
soc/intel/common/block/cpu/car: Enable caching before FSP-T
This patch is required for Boot Guard enabled platform. When system is powered on, cache is default enabled. BIOS is forbidden to disable cache while in NEM mode with BtG enabled.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and ensure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/Kconfig 2 files changed, 34 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/6
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG@10 PS5, Line 10: fobidden
missing `r`: foRbidden
Done
https://review.coreboot.org/c/coreboot/+/38252/5//COMMIT_MSG@15 PS5, Line 15: esnure
ensure
Done
Pratikkumar V Prajapati has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 6: Code-Review+1
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 6: Code-Review+1
Tested and successfully booted Kaby Lake platform with this patch and Boot Guard VE profile. Verification passed. However, I noticed some hangs in FSP-T. Giving +1 for now, after some more thorough testing I can give +2.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 6: Code-Review+2
Patch Set 6: Code-Review+1
Tested and successfully booted Kaby Lake platform with this patch and Boot Guard VE profile. Verification passed. However, I noticed some hangs in FSP-T. Giving +1 for now, after some more thorough testing I can give +2.
Okay, it seems it was my fault that caused the hangs. Tested few more times with various BPM parameters and works.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38252/6/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/38252/6/src/cpu/x86/16bit/entry16.i... PS6, Line 134: #else given that you just want to avoid setting 2 bits depending on the bootguard bit, how about the following?
movl %cr0, %ebx andl $0x7FFAFFD1, %ebx /* PG,AM,WP,NE,TS,EM,MP = 0 */ orl $0x60000001, %ebx /* CD, NW, PE = 1 */
#if CONFIG(BOOTGUARD_CACHE_ENABLE) /* DO NOT disable cache if Intel BootGuard is supported */ movl $BOOTGUARD_MSR, %ecx rdmsr test $1, %eax jz 1f andl ~$(0x60000000), %ebx 1: #endif
movl %ebx, %cr0
https://review.coreboot.org/c/coreboot/+/38252/6/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/38252/6/src/cpu/x86/Kconfig@188 PS6, Line 188: BOOTGUARD_CACHE_ENABLE Do you expect to need other Kconfig symbols depending on MAINBOARD_HAS_BOOT_GUARD? Otherwise it seems a bit moot to have 2 symbols?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: soc/intel/common/block/cpu/car: Enable caching before FSP-T ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/6/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/38252/6/src/cpu/x86/16bit/entry16.i... PS6, Line 134: #else
given that you just want to avoid setting 2 bits depending on the bootguard bit, how about the follo […]
Small typo, should be: andl $(~0x60000000), %ebx
Michał Żygowski has uploaded a new patch set (#7) to the change originally created by Gaggery Tsai. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
cpu/x86/entry16: Do not disable cache when Boot Guard is supported
This patch is required for Boot Guard enabled platform. When system is powered on, cache is default enabled. BIOS is forbidden to disable cache while in NEM mode with BtG enabled.
TEST=Stitch boot guard ACM with signed KM and BPM && Enable FSP-T and boot all the way to the OS && Read MSR 0x13a and ensure boot guard verified boot and measured boot are enabled.
Change-Id: Ie1def754f7b0024725638fcea481fd3273ef3d24 Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/cpu/x86/16bit/entry16.inc 1 file changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38252/7
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/7/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/38252/7/src/cpu/x86/16bit/entry16.i... PS7, Line 125: test $1, %eax Maybe mention that bit 0 indicates whether the BootGuard ACM enabled NEM? Maybe name this bit:
https://github.com/slimbootloader/slimbootloader/blob/master/Silicon/Coffeel...
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/7/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/38252/7/src/cpu/x86/16bit/entry16.i... PS7, Line 120: #if CONFIG(INTEL_BOOTGUARD) Just my personal preference, there is a read-modify-write sequence on cr0 that I would rather not see split to so many lines. Specially with rdmsr there in the middle register usage is not so obvious.
#if CONFIG(INTEL_BOOTGUARD) /* DO NOT disable cache if Intel BootGuard is supported */ movl $BOOTGUARD_SACM_INFO, %ecx rdmsr test $1, %eax jz 1f movl %cr0, %eax andl $0x7FFAFFD1, %eax /* PG,AM,WP,NE,TS,EM,MP = 0 */ orl $0x01, %eax /* PE = 1 */ movl %eax, %cr0 jmp 2f #endif 1: movl %cr0, %eax andl $0x7FFAFFD1, %eax /* PG,AM,WP,NE,TS,EM,MP = 0 */ orl $0x60000001, %eax /* CD, NW, PE = 1 */ movl %eax, %cr0 2:
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/7//COMMIT_MSG@10 PS7, Line 10: BIOS is forbidden to disable : cache while in NEM mode with BtG enabled. Can you please add a reference to the specification?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38252/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38252/7//COMMIT_MSG@10 PS7, Line 10: BIOS is forbidden to disable : cache while in NEM mode with BtG enabled.
Can you please add a reference to the specification?
This kind of stuff is only documented in the `Boot Guard BIOS Writer's Guide`. As usual with BWGs, it's Intel Confidential.
In any case, feel free to add:
As per the Boot Guard BWG, BIOS is forbidden to disable cache while in NEM mode with BtG enabled.
Attention is currently required from: Gaggery Tsai, Michał Żygowski. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7: Done in CB:54010. Can probably be abandoned.
Attention is currently required from: Gaggery Tsai, Michał Żygowski, Arthur Heymans. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
Done in CB:54010. Can probably be abandoned.
Now that CB:54010 is merged should this commit be abandoned?
Attention is currently required from: Gaggery Tsai, Arthur Heymans, Werner Zeh. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38252 )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
Now that CB:54010 is merged should this commit be abandoned?
It probably should.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38252?usp=email )
Change subject: cpu/x86/entry16: Do not disable cache when Boot Guard is supported ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Gaggery Tsai (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin L Roth (Code Review)
-
Michael Niewöhner (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Pratikkumar V Prajapati (Code Review)
-
Werner Zeh (Code Review)