Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
[WIP]soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR on its own so the only thing needed is to find a free MTRR for our own CAR region.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 66 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/1
diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index d5f5081..3ae4829 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -23,11 +23,20 @@ #include <rules.h> #include <intelblocks/msr.h>
+#define BOOTGUARD_MSR 0x13a + .global bootblock_pre_c_entry bootblock_pre_c_entry:
post_code(0x20)
+/* Bootguard setups up it's own CAR and needs separate handling */ +check_boot_guard: + movl $BOOTGUARD_MSR, %ecx + rdmsr + andl $1, %eax + jnz bootguard_car_setup + movl $no_reset, %esp /* return address */ jmp check_mtrr /* Check if CPU properly reset */
@@ -456,3 +465,60 @@
jmp car_init_done #endif + +bootguard_car_setup: + /* Configure MTRR_PHYS_MASK_HIGH for proper addressing above 4GB + * based on the physical address size supported for this processor + * This is based on read from CPUID EAX = 080000008h, EAX bits [7:0] + */ + movl $0x80000008, %eax /* Address sizes leaf */ + cpuid + sub $32, %al + movzx %al, %eax + xorl %esi, %esi + bts %eax, %esi + dec %esi /* esi <- MTRR_PHYS_MASK_HIGH */ + + /* Figure put how many MTRRs we have */ + mov $MTRR_CAP_MSR, %ecx + rdmsr + movzb %al, %ebx /* Number of variable MTRRs */ + + /* Find a free variable MTRR */ + movl $MTRR_PHYS_MASK(0), %ecx +test_is_free_mtrr: + rdmsr + test $MTRR_PHYS_MASK_VALID, %eax + jz found_free_mtrr + addl $2, %ecx + dec %ebx + jnz test_is_free_mtrr + + jmp .halt_forever + +found_free_mtrr: + /* Assume one MTRR is enough for now */ + decl %ecx /* MTRR_PHYS_BASE */ + movl $CONFIG_DCACHE_RAM_BASE, %eax + orl $MTRR_TYPE_WRBACK, %eax + xorl %edx, %edx + wrmsr + + /* Configure the MTRR mask for the size region */ + incl %ecx + mov $CONFIG_DCACHE_RAM_SIZE, %eax /* size mask */ + decl %eax + not %eax + or $MTRR_PHYS_MASK_VALID, %eax + movl %esi, %edx /* edx <- MTRR_PHYS_MASK_HIGH */ + wrmsr + +clear_cache_region: + /* Clear the cache memory region. This will also fill up the cache */ + movl $CONFIG_DCACHE_RAM_BASE, %edi + movl $CONFIG_DCACHE_RAM_SIZE, %ecx + shr $0x02, %ecx + xor %eax, %eax + cld + rep stosl + jmp car_init_done
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Awesome!
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36682/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/1/src/soc/intel/common/block/... PS1, Line 26: #define BOOTGUARD_MSR 0x13a move that to msr.h?
https://review.coreboot.org/c/coreboot/+/36682/1/src/soc/intel/common/block/... PS1, Line 33: setups up it's nit: sets up its
https://review.coreboot.org/c/coreboot/+/36682/1/src/soc/intel/common/block/... PS1, Line 482: put out
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is: - Build coreboot bios region image - Create a manifest and import it to the image using the Manifest Extension Utility - Sign the manifest with the MEU - Set the bootguard policy in FIT and add the pubkey hash - add the signed bios image - Build the final image with FIT
Can you confirm/correct this, please?
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
https://github.com/slimbootloader/slimbootloader/blob/896937483c79522f03bf5e...
https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Just look into slimmbootloader they have made all NDA structures open source
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Also interesting @Arthur https://github.com/slimbootloader/slimbootloader/blob/63c9353240c8093a339c93...
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Have fun guys :)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Added Amol and Sachin from intel IOT who has enabled bootguard with CB for sure to review your CL
Sachin Agrawal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is:
- Build coreboot bios region image
- Create a manifest and import it to the image using the Manifest Extension Utility
- Sign the manifest with the MEU
- Set the bootguard policy in FIT and add the pubkey hash
- add the signed bios image
- Build the final image with FIT
Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is:
- Build coreboot bios region image
- Create a manifest and import it to the image using the Manifest Extension Utility
- Sign the manifest with the MEU
- Set the bootguard policy in FIT and add the pubkey hash
- add the signed bios image
- Build the final image with FIT
Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Sachin Agrawal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is:
- Build coreboot bios region image
- Create a manifest and import it to the image using the Manifest Extension Utility
- Sign the manifest with the MEU
- Set the bootguard policy in FIT and add the pubkey hash
- add the signed bios image
- Build the final image with FIT
Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is:
- Build coreboot bios region image
- Create a manifest and import it to the image using the Manifest Extension Utility
- Sign the manifest with the MEU
- Set the bootguard policy in FIT and add the pubkey hash
- add the signed bios image
- Build the final image with FIT
Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is:
- Build coreboot bios region image
- Create a manifest and import it to the image using the Manifest Extension Utility
- Sign the manifest with the MEU
- Set the bootguard policy in FIT and add the pubkey hash
- add the signed bios image
- Build the final image with FIT
Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Well, for APL there is some document out there which describes how to enable BG with the MEU and FIT tool. For Skylake MEU does not provide signing, so there must be some other tool... but I couldn't find anything :/
Sachin Agrawal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
@Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest.
My understanding of the process is:
- Build coreboot bios region image
- Create a manifest and import it to the image using the Manifest Extension Utility
- Sign the manifest with the MEU
- Set the bootguard policy in FIT and add the pubkey hash
- add the signed bios image
- Build the final image with FIT
Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Well, for APL there is some document out there which describes how to enable BG with the MEU and FIT tool. For Skylake MEU does not provide signing, so there must be some other tool... but I couldn't find anything :/
@Michael : For SKL BtG enablement, you need access to BpmGen tool. Please request from your Intel rep.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
> Patch Set 1: > > @Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest. > > My understanding of the process is: > - Build coreboot bios region image > - Create a manifest and import it to the image using the Manifest Extension Utility > - Sign the manifest with the MEU > - Set the bootguard policy in FIT and add the pubkey hash > - add the signed bios image > - Build the final image with FIT > > Can you confirm/correct this, please?
In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point.
1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Well, for APL there is some document out there which describes how to enable BG with the MEU and FIT tool. For Skylake MEU does not provide signing, so there must be some other tool... but I couldn't find anything :/
@Michael : For SKL BtG enablement, you need access to BpmGen tool. Please request from your Intel rep.
So Intel wants us to a) have a proprietary blob in coreboot b) implement an opensource alternative but doesn't provide a way for testing? Well, then let's choose option c) drop FSP-T and do not implement BootGuard due to missing testing abilities.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
> > @Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest. > > > > My understanding of the process is: > > - Build coreboot bios region image > > - Create a manifest and import it to the image using the Manifest Extension Utility > > - Sign the manifest with the MEU > > - Set the bootguard policy in FIT and add the pubkey hash > > - add the signed bios image > > - Build the final image with FIT > > > > Can you confirm/correct this, please? > > In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point. > > 1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? > 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). > 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). > BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. > 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM.
Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Well, for APL there is some document out there which describes how to enable BG with the MEU and FIT tool. For Skylake MEU does not provide signing, so there must be some other tool... but I couldn't find anything :/
@Michael : For SKL BtG enablement, you need access to BpmGen tool. Please request from your Intel rep.
So Intel wants us to a) have a proprietary blob in coreboot b) implement an opensource alternative but doesn't provide a way for testing? Well, then let's choose option c) drop FSP-T and do not implement BootGuard due to missing testing abilities.
I guess what Michael wants to tell us is that he just wanted to help but can't, because he has no Intel representative. So some- body @Intel will have to test this.
Sachin Agrawal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
> > > @Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest. > > > > > > My understanding of the process is: > > > - Build coreboot bios region image > > > - Create a manifest and import it to the image using the Manifest Extension Utility > > > - Sign the manifest with the MEU > > > - Set the bootguard policy in FIT and add the pubkey hash > > > - add the signed bios image > > > - Build the final image with FIT > > > > > > Can you confirm/correct this, please? > > > > In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point. > > > > 1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? > > 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). > > 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). > > BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. > > 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM. > > Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. > 3) Should be easy to implement in coreboot if there'd be some public spec.
Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. Although, Slimboot code does contain the definitions of those data structures. https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Well, for APL there is some document out there which describes how to enable BG with the MEU and FIT tool. For Skylake MEU does not provide signing, so there must be some other tool... but I couldn't find anything :/
@Michael : For SKL BtG enablement, you need access to BpmGen tool. Please request from your Intel rep.
So Intel wants us to a) have a proprietary blob in coreboot b) implement an opensource alternative but doesn't provide a way for testing? Well, then let's choose option c) drop FSP-T and do not implement BootGuard due to missing testing abilities.
I guess what Michael wants to tell us is that he just wanted to help but can't, because he has no Intel representative. So some- body @Intel will have to test this.
Understood. Let me talk to folks inside Intel and see if we can allocate some resource/time to perform this validation of CAR changes for BtG enablement.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: [WIP]soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
> > > > @Subrata I tried to test this but miserably failed :D I have an unfused/uncommited SoC here. The main problem seems to be that I don't know how to create a valid Manifest. > > > > > > > > My understanding of the process is: > > > > - Build coreboot bios region image > > > > - Create a manifest and import it to the image using the Manifest Extension Utility > > > > - Sign the manifest with the MEU > > > > - Set the bootguard policy in FIT and add the pubkey hash > > > > - add the signed bios image > > > > - Build the final image with FIT > > > > > > > > Can you confirm/correct this, please? > > > > > > In summary, BtGuard role is to verify IBB (code which initializes memory), load it into NEM and pass control to Coreboot entry point. > > > > > > 1)Coreboot needs to define which CBFS's constitutes IBB. bootblock, fsp-m, verstage, romstage, etc ? > > > 2)We need to create FIT table with Type 7 entry and include those CBFS's. FIT spec is here (https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios...). > > > 3)We need to invoke Intel BpmGen tool to create BtGuard Policy Manifest(BPM) and BtGuard Key Manifests(KM). > > > BpmGen tool will use above FIT table to create BPM which will contain the cumulative hash of IBB. KM will contain the hash of key which is used for signing BPM. > > > 4)Build the final image using FIT and set appropriate BtGuard policy and the hash of the public key which was used for signing KM. > > > > Thanks to 9elements 1) and 2) are already done if you enable Intel TXT. > > 3) Should be easy to implement in coreboot if there'd be some public spec. > > Unfortunately, I am not aware of if there are any public specs which provides BPM/KM details. > Although, Slimboot code does contain the definitions of those data structures. > https://github.com/slimbootloader/slimbootloader/blob/5e10bd1e0761c7581d9233...
Got it but then why isn't the document disclosed publicly. I mean the most stuff is already disclosed by slimmbootloader right ?
Also, can Intel test our CAR changes and give us feedback if Boot Guard works with it?
Well, for APL there is some document out there which describes how to enable BG with the MEU and FIT tool. For Skylake MEU does not provide signing, so there must be some other tool... but I couldn't find anything :/
@Michael : For SKL BtG enablement, you need access to BpmGen tool. Please request from your Intel rep.
So Intel wants us to a) have a proprietary blob in coreboot b) implement an opensource alternative but doesn't provide a way for testing? Well, then let's choose option c) drop FSP-T and do not implement BootGuard due to missing testing abilities.
I guess what Michael wants to tell us is that he just wanted to help but can't, because he has no Intel representative. So some- body @Intel will have to test this.
Understood. Let me talk to folks inside Intel and see if we can allocate some resource/time to perform this validation of CAR changes for BtG enablement.
Any news?
Hello Amol N Sukerkar, Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Sachin Agrawal, David Hendricks, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36682
to look at the new patch set (#2).
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
UNTESTED.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/2
Hello Amol N Sukerkar, Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Sachin Agrawal, David Hendricks, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36682
to look at the new patch set (#3).
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
UNTESTED.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/3
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
looks reasonable. do you have done tests that the non-bootguard code path is still fine?
https://review.coreboot.org/c/coreboot/+/36682/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/4/src/soc/intel/common/block/... PS4, Line 75: setups up it's sets up its?
Hello Amol N Sukerkar, Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Sachin Agrawal, David Hendricks, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36682
to look at the new patch set (#6).
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
UNTESTED.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... PS6, Line 214: clear_car on Purley bit0 in MSR 0x139 is set when B_BOOT_GUARD_SACM_INFO_CAPABILITY is set in BOOTGUARD_MSR.
@Intel can you confirm that all FSP versions set this bit and it's not required to set it in CAR setup?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... PS6, Line 214: clear_car
on Purley bit0 in MSR 0x139 is set when B_BOOT_GUARD_SACM_INFO_CAPABILITY is set in BOOTGUARD_MSR. […]
Just remembered another thing that needs to be done: Stop the PBE timer before trying to do MP init. Otherwise, sending SIPIs to wake up the APs causes a system reset. To stop that timer, write one to MSR 0x139 bit 0.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6: Code-Review+2
It's been 4 weeks without any negative feedback, lets merge it!
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
kindly help to do some testing and its written untest. its common code
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
@Sachin/Amol, do you think this much is enough for bootguard ?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6: Code-Review+1
LGTM AFAICT. no testing for known reasons...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
(1 comment)
Patch Set 6:
@Sachin/Amol, do you think this much is enough for bootguard ?
Bit 0 on MSR 0x139 would need to be set, AFAIK.
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... PS6, Line 214: clear_car
Just remembered another thing that needs to be done: Stop the PBE timer before trying to do MP init. […]
Without setting bit 0 on MSR 0x139, this won't be enough.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
I tested this patch and it seems not working with BootGuard enabled.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
I tested this patch and it seems not working with BootGuard enabled.
Thanks.
Can you elaborate on the exact state of your coreboot tree? Did you mix this with your caching patch?
Also, did you see the comments about MSR 0x139, above? Maybe that's just what is missing.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
I tested this patch and it seems not working with BootGuard enabled.
Thanks.
Can you elaborate on the exact state of your coreboot tree? Did you mix this with your caching patch?
Also, did you see the comments about MSR 0x139, above? Maybe that's just what is missing.
I picked up other dependency patches with this patch and didn't include my patch.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/6/src/soc/intel/common/block/... PS6, Line 214: clear_car
Without setting bit 0 on MSR 0x139, this won't be enough.
looking at *some* code I can confirm this ;) @Arthur could you work on that so Gaggery can test with that change?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
I tested this patch and it seems not working with BootGuard enabled.
Thanks.
Can you elaborate on the exact state of your coreboot tree? Did you mix this with your caching patch?
Also, did you see the comments about MSR 0x139, above? Maybe that's just what is missing.
I picked up other dependency patches with this patch and didn't include my patch.
Hi, thanks for testing. What platform did you test this on? Also, when you mean "not working", does the board reset or hang, or is it something else? AFAIK, not setting MSR 0x139 would make the system reset after some time or when trying to wake up the APs.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
I tested this patch and it seems not working with BootGuard enabled.
Thanks.
Can you elaborate on the exact state of your coreboot tree? Did you mix this with your caching patch?
Also, did you see the comments about MSR 0x139, above? Maybe that's just what is missing.
I picked up other dependency patches with this patch and didn't include my patch.
Hi, thanks for testing. What platform did you test this on? Also, when you mean "not working", does the board reset or hang, or is it something else? AFAIK, not setting MSR 0x139 would make the system reset after some time or when trying to wake up the APs.
I saw it was stuck at post code 0x2A. I will look into it and provide more info later.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
I tested this patch and it seems not working with BootGuard enabled.
Thanks.
Can you elaborate on the exact state of your coreboot tree? Did you mix this with your caching patch?
Also, did you see the comments about MSR 0x139, above? Maybe that's just what is missing.
I picked up other dependency patches with this patch and didn't include my patch.
Hi, thanks for testing. What platform did you test this on? Also, when you mean "not working", does the board reset or hang, or is it something else? AFAIK, not setting MSR 0x139 would make the system reset after some time or when trying to wake up the APs.
I saw it was stuck at post code 0x2A. I will look into it and provide more info later.
0x2A means stuck in MTRR setup
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
I saw it was stuck at post code 0x2A. I will look into it and provide more info later.
0x2A means stuck in MTRR setup
Oh, hmm... not sure if that's right
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
@Gaggery any new results?
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
@Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
@Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init. In both cases that'd mean that CAR is not reached
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
@Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init.
I assume you are looking at skylake/ while Gaggery tested on a newer platform.
In both cases that'd mean that CAR is not reached
I don't follow, you are both referring to C functions. Those run after CAR setup.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
@Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init.
I assume you are looking at skylake/ while Gaggery tested on a newer platform.
Ouch. Indeed, sorry for the noise...
In both cases that'd mean that CAR is not reached
I don't follow, you are both referring to C functions. Those run after CAR setup.
I meant FSP-M not CAR :S
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
@Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init.
I assume you are looking at skylake/ while Gaggery tested on a newer platform.
Ouch. Indeed, sorry for the noise...
In both cases that'd mean that CAR is not reached
I don't follow, you are both referring to C functions. Those run after CAR setup.
I meant FSP-M not CAR :S
I'm not sure why it was stuck at https://github.com/coreboot/coreboot/blob/master/src/device/device_const.c#L...
Use DCI to with Intel System debugger to check, the address of path2 is 0xb which doesn't make sense. I'm not sure if something wrong in CAR which may lead to this result.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
@Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init.
I assume you are looking at skylake/ while Gaggery tested on a newer platform.
Ouch. Indeed, sorry for the noise...
In both cases that'd mean that CAR is not reached
I don't follow, you are both referring to C functions. Those run after CAR setup.
I meant FSP-M not CAR :S
I'm not sure why it was stuck at https://github.com/coreboot/coreboot/blob/master/src/device/device_const.c#L...
Use DCI to with Intel System debugger to check, the address of path2 is 0xb which doesn't make sense. I'm not sure if something wrong in CAR which may lead to this result.
I wonder if the machine hangs because of a timing issue. I'd try adding some delays to see if the hang happens on a different place.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
> @Gaggery any new results?
It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init.
I assume you are looking at skylake/ while Gaggery tested on a newer platform.
Ouch. Indeed, sorry for the noise...
In both cases that'd mean that CAR is not reached
I don't follow, you are both referring to C functions. Those run after CAR setup.
I meant FSP-M not CAR :S
I'm not sure why it was stuck at https://github.com/coreboot/coreboot/blob/master/src/device/device_const.c#L...
Use DCI to with Intel System debugger to check, the address of path2 is 0xb which doesn't make sense. I'm not sure if something wrong in CAR which may lead to this result.
I wonder if the machine hangs because of a timing issue. I'd try adding some delays to see if the hang happens on a different place.
Hmm.. I disabled PBE Timer by changing the setting in BPM. Enabled BOOTBLOCK_DEBUG_SPINLOOP to wait for DCI connection and ran single step. I think the hang should not be from timing issue since that SPINLOOP already introduces lots of delay but it still hung at the same location.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
Patch Set 8:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
> > @Gaggery any new results? > > It stuck at bootblock_pch_early_init -> gspi_early_bar_init -> gspi_get_cfg -> config_of_soc -> config_of -> pcidev_on_root -> pcidev_path_on_root -> pci_path_behind -> find_dev_path -> path_eq. It's an interesting location where it was stuck. Need some more time to debug. Welcome for suggestion. I tried to disable PBE timer by directly writing 0x1 to MSR 0x139 but it was hung at wrmsr.
gspi_early_bar_init is not called from bootblock_pch_early_init but from bootblock_soc_init, while bootblock_pch_early_init is called from bootblock_soc_early_init.
I assume you are looking at skylake/ while Gaggery tested on a newer platform.
Ouch. Indeed, sorry for the noise...
In both cases that'd mean that CAR is not reached
I don't follow, you are both referring to C functions. Those run after CAR setup.
I meant FSP-M not CAR :S
I'm not sure why it was stuck at https://github.com/coreboot/coreboot/blob/master/src/device/device_const.c#L...
Use DCI to with Intel System debugger to check, the address of path2 is 0xb which doesn't make sense. I'm not sure if something wrong in CAR which may lead to this result.
I wonder if the machine hangs because of a timing issue. I'd try adding some delays to see if the hang happens on a different place.
Hmm.. I disabled PBE Timer by changing the setting in BPM. Enabled BOOTBLOCK_DEBUG_SPINLOOP to wait for DCI connection and ran single step. I think the hang should not be from timing issue since that SPINLOOP already introduces lots of delay but it still hung at the same location.
Right, then it would seem that CAR isn't configured correctly. I will double check the code. Thanks for testing this, the results are very helpful.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36682/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/8/src/soc/intel/common/block/... PS8, Line 210: movl $BOOTGUARD_MSR, %ecx I would try skipping this code. I can't see why CAR shouldn't be cleared with BtG.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36682/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/8/src/soc/intel/common/block/... PS8, Line 210: movl $BOOTGUARD_MSR, %ecx
I would try skipping this code. I can't see why CAR shouldn't be cleared with BtG.
I tried to skip clear_car, the result is the same.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
(1 comment)
Right, so according to CB:38252 the BtG flows shouldn't try to disable CAR when bootguard is enabled. But then, when memory is working, how do we know we can disable CAR?
https://review.coreboot.org/c/coreboot/+/36682/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/8/src/soc/intel/common/block/... PS8, Line 210: movl $BOOTGUARD_MSR, %ecx
I tried to skip clear_car, the result is the same.
Ack
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8: Code-Review-1
I have some successes with CB:38252. When I achieve some reliable and reproducible testing images with that change, I will help with this one. Since nobody tested it with Boot Guard, I will give a -1 temporarily since Gaggery reported some issues and to indicate untested code. Let's merge it in a state "it works".
It should be noted that the MSRs will return all 0s if the ACM is not loaded and Boot Guard disabled.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
Any news here?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 8:
@Michal Zygowski Do I get that right, you got BootGuard working in coreboot? Any upstreaming plans? :-) https://blog.3mdeb.com/2020/2020-02-18-ew2020-btg-demo/
Attention is currently required from: Arthur Heymans. Arthur Heymans has uploaded a new patch set (#9) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR/NEM on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
TESTED on prodrive/hermes with bootguard enabled.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/9
Attention is currently required from: Arthur Heymans, Angel Pons, Michael Niewöhner, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 9:
(5 comments)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/comment/2e6d1b33_12fb3136 PS1, Line 26: #define BOOTGUARD_MSR 0x13a
move that to msr. […]
Done
https://review.coreboot.org/c/coreboot/+/36682/comment/d074aee9_1c3c82df PS1, Line 33: setups up it's
nit: sets up its
Done
https://review.coreboot.org/c/coreboot/+/36682/comment/498b7ab0_e7c39b0a PS1, Line 482: put
out
Done
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/comment/5d66a4f1_c20a9c18 PS4, Line 75: setups up it's
sets up its?
Done
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36682/comment/b2690910_32e22025 PS6, Line 214: clear_car
looking at *some* code I can confirm this ;) @Arthur could you work on that so Gaggery can test with […]
Done
Attention is currently required from: Arthur Heymans, Angel Pons, Michael Niewöhner, Patrick Rudolph. Hello build bot (Jenkins), Patrick Georgi, Vincent Zimmer, Subrata Banik, Julius Werner, Michael Niewöhner, Patrick Rudolph, Patrick Rudolph, Aaron Durbin, Gaggery Tsai, Nico Huber, Michał Żygowski, David Hendricks, Sachin Agrawal, Amol N Sukerkar,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36682
to look at the new patch set (#10).
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR/NEM on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
TESTED on prodrive/hermes with bootguard enabled.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cpu/intel/msr.h M src/soc/intel/common/block/cpu/car/cache_as_ram.S 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/10
Attention is currently required from: Arthur Heymans, Angel Pons, Michael Niewöhner, Patrick Rudolph. Hello build bot (Jenkins), Patrick Georgi, Vincent Zimmer, Subrata Banik, Julius Werner, Michael Niewöhner, Patrick Rudolph, Patrick Rudolph, Aaron Durbin, Gaggery Tsai, Nico Huber, Michał Żygowski, David Hendricks, Sachin Agrawal, Amol N Sukerkar,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36682
to look at the new patch set (#11).
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR/NEM on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
TESTED on prodrive/hermes with bootguard enabled.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cpu/intel/msr.h M src/soc/intel/common/block/cpu/car/cache_as_ram.S 2 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/36682/11
Attention is currently required from: Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 11: Code-Review+1
Attention is currently required from: Arthur Heymans, Patrick Rudolph. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 11: Code-Review+1
Attention is currently required from: Arthur Heymans, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 11: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
soc/intel/car: Add support for bootguard CAR
Bootguard sets up CAR/NEM on its own so the only thing needed is to find free MTRRs for our own CAR region and clear that area to fill in cache lines.
TESTED on prodrive/hermes with bootguard enabled.
Change-Id: Ifac5267f8f4b820a61519fb4a497e2ce7075cc40 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36682 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Michael Niewöhner foss@mniewoehner.de Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/include/cpu/intel/msr.h M src/soc/intel/common/block/cpu/car/cache_as_ram.S 2 files changed, 31 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Michael Niewöhner: Looks good to me, but someone else must approve
diff --git a/src/include/cpu/intel/msr.h b/src/include/cpu/intel/msr.h index 6b2db88..8efe4e2 100644 --- a/src/include/cpu/intel/msr.h +++ b/src/include/cpu/intel/msr.h @@ -12,6 +12,9 @@
#define MSR_PLATFORM_INFO 0xce
+#define MSR_BC_PBEC 0x139 +#define B_STOP_PBET (1 << 0) + #define MSR_BOOT_GUARD_SACM_INFO 0x13a #define V_TPM_PRESENT_MASK 0x06 #define B_BOOT_GUARD_SACM_INFO_NEM_ENABLED (1 << 0) diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index 5da453b..60ec6c5 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <commonlib/helpers.h> +#include <cpu/intel/msr.h> #include <cpu/x86/cache.h> #include <cpu/x86/cr.h> #include <cpu/x86/msr.h> @@ -63,6 +64,22 @@
post_code(0x20)
+/* Bootguard sets up its own CAR and needs separate handling */ +check_boot_guard: + movl $MSR_BOOT_GUARD_SACM_INFO, %ecx + rdmsr + andl $B_BOOT_GUARD_SACM_INFO_NEM_ENABLED, %eax + jz no_bootguard + + /* Disable PBE timer */ + movl $MSR_BC_PBEC, %ecx + movl $B_STOP_PBET, %eax + xorl %edx, %edx + wrmsr + + jmp setup_car_mtrr + +no_bootguard: movl $no_reset, %esp /* return address */ jmp check_mtrr /* Check if CPU properly reset */
@@ -108,6 +125,7 @@ MTRR_DEF_TYPE_FIX_EN), %eax wrmsr
+setup_car_mtrr: /* Configure MTRR_PHYS_MASK_HIGH for proper addressing above 4GB * based on the physical address size supported for this processor * This is based on read from CPUID EAX = 080000008h, EAX bits [7:0] @@ -186,6 +204,16 @@ #endif post_code(0x25)
+ movl $MSR_BOOT_GUARD_SACM_INFO, %ecx + rdmsr + andl $B_BOOT_GUARD_SACM_INFO_NEM_ENABLED, %eax + jz no_bootguard_car_continue + + clear_car + + jmp car_init_done + +no_bootguard_car_continue: /* Enable variable MTRRs */ mov $MTRR_DEF_TYPE_MSR, %ecx rdmsr
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS12: This breaks all Siemens APL-based boards. They do not boot any more, no coreboot log is visible on the serial console.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36682 )
Change subject: soc/intel/car: Add support for bootguard CAR ......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS12:
This breaks all Siemens APL-based boards. They do not boot any more, no coreboot log is visible on the serial console.
I'll have a look. I suspect the MSR does not exist on APL.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Gaggery Tsai (Code Review)
-
Michael Niewöhner (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Sachin Agrawal (Code Review)
-
Subrata Banik (Code Review)
-
Werner Zeh (Code Review)