coreboot-gerrit November 2020

coreboot-gerrit@coreboot.org

1 participants
3243 discussions

Change in coreboot[master]: mb/google/dedede/var/drawcia: Probe and enable DPTF configuration
by Karthik Ramasubramanian (Code Review) 05 Nov '20

05 Nov '20

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47010 ) Change subject: mb/google/dedede/var/drawcia: Probe and enable DPTF configuration ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… File src/mainboard/google/dedede/variants/drawcia/overridetree.cb: https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… PS5, Line 95: 65 > After comparison of existing and these new added entries, observed that only this temperature trip v […] Yes that is correct. I could have moved the difference into a .C file to override only that passive policy. But I am afraid, developers may not notice it and that will lead to more confusion. Hence added it in the device tree so that it will be more clear. https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… PS5, Line 139: # Default DPTF Policy for all drawcia boards if not overridden : register "options.tsr[0].desc" = ""Memory"" : register "options.tsr[1].desc" = ""Ambient"" : register "options.tsr[2].desc" = ""Charger"" : register "options.tsr[3].desc" = ""5V regulator"" > Is it possible to define these sensor description entries only once, instead of keeping for both the […] I may be wrong, but I am not aware of any such option. Do you see any issue moving the TSR1 trip value to 65 for the devices of all form factors. If so, then I am fine with moving to that value. That way we don't have to duplicate the common entries. -- To view, visit https://review.coreboot.org/c/coreboot/+/47010 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibf166a2e36fa5775e2dea7c1adcae843cc143d32 Gerrit-Change-Number: 47010 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 05 Nov 2020 05:45:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group
by Maulik V Vaghela (Code Review) 05 Nov '20

05 Nov '20

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46842 ) Change subject: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group ...................................................................... Patch Set 7: (2 comments) https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… File src/soc/intel/jasperlake/gpio.c: https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… PS6, Line 36: 0 > Ack Sure Furquan..I'll raise a cros bug. https://review.coreboot.org/c/coreboot/+/46842/7/src/soc/intel/jasperlake/i… File src/soc/intel/jasperlake/include/soc/gpio_soc_defs.h: https://review.coreboot.org/c/coreboot/+/46842/7/src/soc/intel/jasperlake/i… PS7, Line 306: #define GPIO_RSVD_24 240 : #define GPIO_RSVD_25 241 : #define GPIO_RSVD_26 242 : #define GPIO_RSVD_27 243 : #define GPIO_RSVD_28 244 : #define GPIO_RSVD_29 245 : #define GPIO_RSVD_30 246 : #define GPIO_RSVD_31 247 : #define GPIO_RSVD_32 248 : #define GPIO_RSVD_33 249 : #define GPIO_RSVD_34 250 : #define GPIO_RSVD_35 251 : #define GPIO_RSVD_36 252 > Why did these get dropped? Since these are the reserved at the end of the community, It won't impact the offset calculation within community. Also pin-ctl driver doesn't define this , so I though it's better to remove it since it won't be that useful. -- To view, visit https://review.coreboot.org/c/coreboot/+/46842 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id6013914c88c50f4b8c60ca9a9285a8e1b214d11 Gerrit-Change-Number: 46842 Gerrit-PatchSet: 7 Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 05 Nov 2020 04:52:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group
by Furquan Shaikh (Code Review) 05 Nov '20

05 Nov '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46842 ) Change subject: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group ...................................................................... Patch Set 7: (2 comments) https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… File src/soc/intel/jasperlake/gpio.c: https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… PS6, Line 37: INTEL_GPP(GPP_F0, GPIO_RSVD_0, GPIO_RSVD_8), > Ack […] Can you please raise a partner issue as well for this? https://review.coreboot.org/c/coreboot/+/46842/7/src/soc/intel/jasperlake/i… File src/soc/intel/jasperlake/include/soc/gpio_soc_defs.h: https://review.coreboot.org/c/coreboot/+/46842/7/src/soc/intel/jasperlake/i… PS7, Line 306: #define GPIO_RSVD_24 240 : #define GPIO_RSVD_25 241 : #define GPIO_RSVD_26 242 : #define GPIO_RSVD_27 243 : #define GPIO_RSVD_28 244 : #define GPIO_RSVD_29 245 : #define GPIO_RSVD_30 246 : #define GPIO_RSVD_31 247 : #define GPIO_RSVD_32 248 : #define GPIO_RSVD_33 249 : #define GPIO_RSVD_34 250 : #define GPIO_RSVD_35 251 : #define GPIO_RSVD_36 252 Why did these get dropped? -- To view, visit https://review.coreboot.org/c/coreboot/+/46842 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id6013914c88c50f4b8c60ca9a9285a8e1b214d11 Gerrit-Change-Number: 46842 Gerrit-PatchSet: 7 Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 05 Nov 2020 04:46:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group
by Maulik V Vaghela (Code Review) 05 Nov '20

05 Nov '20

1 0

Change in coreboot[master]: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group
by Maulik V Vaghela (Code Review) 05 Nov '20

05 Nov '20

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46842 ) Change subject: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group ...................................................................... Patch Set 7: (5 comments) https://review.coreboot.org/c/coreboot/+/46842/6//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/46842/6//COMMIT_MSG@27 PS6, Line 27: Correcting the sequence by moving some of the reserved gpio within : community (that's also same in pinctrl driver), helps with the issue. > What you are really doing is creating pad groups as per the EDS. […] Yes Furquan. I have updated comment and also provided ref to EDS. https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… File src/soc/intel/jasperlake/gpio.c: https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… PS6, Line 36: 0 > This is not correct. It should be 320: https://source.chromium. […] Ack https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… PS6, Line 37: INTEL_GPP(GPP_F0, GPIO_RSVD_0, GPIO_RSVD_8), > BTW, I think the kernel has a bug: https://source.chromium. […] Ack Sure Furquan. Let me check with this in kernel and also follow up internally. https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… PS6, Line 38: GPIO_RSVD_10 > Not for this change, but I think this GPIO should be named as per EDS. […] Ack https://review.coreboot.org/c/coreboot/+/46842/6/src/soc/intel/jasperlake/g… PS6, Line 39: GPIO_RSVD_11 > Same here and the other GPIO_RSVD* below. Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/46842 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id6013914c88c50f4b8c60ca9a9285a8e1b214d11 Gerrit-Change-Number: 46842 Gerrit-PatchSet: 7 Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 05 Nov 2020 04:39:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group
by Maulik V Vaghela (Code Review) 05 Nov '20

05 Nov '20

Hello build bot (Jenkins), Furquan Shaikh, Subrata Banik, Ronak Kanabar, Aamir Bohra, Patrick Rudolph, Karthik Ramasubramanian, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/46842 to look at the new patch set (#7). Change subject: soc/intel/jasperlake: Correct GPIO pad sequence for community pad group ...................................................................... soc/intel/jasperlake: Correct GPIO pad sequence for community pad group In gpio.c file, we have community group array for each comm, representing gpio groups within that community. Like there might be group H,D, VGPIO and C within community 1. Community also may have some reserved gpio and we also define those in an array which indicates OS can't use those GPIO (through PAD_BASE_NONE) Now when we define reserved pads in the middle of actual community pads, it creates an issue while calculating an offset for GPIO host own pad register. This is because function actually checks current gpio index (lets say vgpio_39 in our case) and tries to get group index from an array which we have defined. If we have defined reserved gpios in between 2 communities, index calculated will also account for reserved GPIO and register offset calculation will move to next set of register (offset 0xC instead of offset 0x8). Because of this coreboot won't configure HOST_OWN_PAD register correctly and driver will not be able to get non-SMI interrupts for related gpio. Align pad group as per EDS and pin-ctrl driver. Reference: DOC#618876 (EDS volume 2) BUG=None BRANCH=None TEST=VGPIO community index is correctly calculated. Drawlat board boots fine with this change and warm reset also works. Change-Id: Id6013914c88c50f4b8c60ca9a9285a8e1b214d11 Signed-off-by: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> --- M src/soc/intel/jasperlake/gpio.c M src/soc/intel/jasperlake/include/soc/gpio_soc_defs.h 2 files changed, 129 insertions(+), 146 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46842/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/46842 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id6013914c88c50f4b8c60ca9a9285a8e1b214d11 Gerrit-Change-Number: 46842 Gerrit-PatchSet: 7 Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/dedede/var/drawcia: Probe and enable DPTF configuration
by Sumeet R Pawnikar (Code Review) 05 Nov '20

05 Nov '20

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47010 ) Change subject: mb/google/dedede/var/drawcia: Probe and enable DPTF configuration ...................................................................... Patch Set 5: (4 comments) https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… File src/mainboard/google/dedede/variants/drawcia/overridetree.cb: https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… PS5, Line 95: 65 After comparison of existing and these new added entries, observed that only this temperature trip value changed from 51 to 65 deg C. It seems all other new entries added here are the same as previous existing ones. Please, correct me if I am missing any other thing here. Thanks. https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… PS5, Line 134: device generic 0 on : probe TABLETMODE TABLETMODE_DISABLED : end Just for better code readability, can we please add this after above "chip drivers/intel/dptf" line 84. https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… PS5, Line 139: # Default DPTF Policy for all drawcia boards if not overridden : register "options.tsr[0].desc" = ""Memory"" : register "options.tsr[1].desc" = ""Ambient"" : register "options.tsr[2].desc" = ""Charger"" : register "options.tsr[3].desc" = ""5V regulator"" Is it possible to define these sensor description entries only once, instead of keeping for both these modes (tablet and non-tablet) because these are common entries which won't change for the system ? https://review.coreboot.org/c/coreboot/+/47010/5/src/mainboard/google/deded… PS5, Line 188: device generic 1 on : probe TABLETMODE TABLETMODE_ENABLED : end Similar comment here as above, just for better code readability, can we please add this after above "chip drivers/intel/dptf" line 138. -- To view, visit https://review.coreboot.org/c/coreboot/+/47010 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibf166a2e36fa5775e2dea7c1adcae843cc143d32 Gerrit-Change-Number: 47010 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 05 Nov 2020 04:16:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: console: Allow VPD to disable an otherwise enabled coreboot console
by Julius Werner (Code Review) 05 Nov '20

05 Nov '20

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45208 ) Change subject: console: Allow VPD to disable an otherwise enabled coreboot console ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/45208/4//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/45208/4//COMMIT_MSG@18 PS4, Line 18: entry is added. > Yes, I meant an ELF section. And you are right, compression probably makes […] Yes, the VPD solution may be less bad on x86. Kinda depends on caching details I'm not super familiar with. The general problem with the VPD code (other than just being somewhat big in general) is that it always reloads the whole VPD in every stage... if x86 flash controllers will cache accesses to the same region across all stages, maybe that's less of a problem there. But I'm still not feeling great about standardizing that as an option available everywhere when it only works well on some platforms and the reasons are not very obvious. I don't know what Patrick intended here, I just saw it uploaded and felt like I should -1. ;) The earlier discussions on the same topic looked like the people who want it use x86 platforms to me. -- To view, visit https://review.coreboot.org/c/coreboot/+/45208 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4f1f5c45e5ea889176d04e0db438ca2aa7c536ee Gerrit-Change-Number: 45208 Gerrit-PatchSet: 4 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Dossym Nurmukhanov <dossym(a)google.com> Gerrit-Reviewer: Greg Edelston <gredelston(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Thu, 05 Nov 2020 02:29:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/acpi: move S0ix UUID to the condition
by Furquan Shaikh (Code Review) 05 Nov '20

05 Nov '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46468 ) Change subject: soc/intel/common/acpi: move S0ix UUID to the condition ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/46468 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic62c37090ad1b747f9d7d204363cc58f96ef67ef Gerrit-Change-Number: 46468 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 05 Nov 2020 00:33:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: haswell: Add Intel TXT support in romstage
by Angel Pons (Code Review) 04 Nov '20

04 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46608 ) Change subject: haswell: Add Intel TXT support in romstage ...................................................................... haswell: Add Intel TXT support in romstage Provide necessary romstage hooks to allow unblocking the memory with SCLEAN. Note that this is slow, and took four minutes with 4 GiB of RAM. Tested on Asrock B85M Pro4 with tboot. When Linux has tboot support compiled in, booting as well as S3 suspend and resume are functional. However, SINIT will TXT reset when the iGPU is enabled, and using a dGPU will result in DMAR-related problems as soon as the IOMMU is enabled. Change-Id: I60beb7d79a30f460bbd5d94e4cba0244318c124e Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/haswell/romstage.c M src/security/intel/txt/Makefile.inc M src/security/intel/txt/common.c A src/security/intel/txt/romstage.c M src/security/intel/txt/txt.h M src/security/intel/txt/txt_register.h 6 files changed, 200 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/46608/1 diff --git a/src/northbridge/intel/haswell/romstage.c b/src/northbridge/intel/haswell/romstage.c index 5b025eb..3227c02 100644 --- a/src/northbridge/intel/haswell/romstage.c +++ b/src/northbridge/intel/haswell/romstage.c @@ -4,11 +4,14 @@ #include <console/console.h> #include <cf9_reset.h> #include <device/device.h> +#include <device/mmio.h> #include <timestamp.h> #include <cpu/x86/lapic.h> #include <cbmem.h> #include <commonlib/helpers.h> #include <romstage_handoff.h> +#include <security/intel/txt/txt.h> +#include <security/intel/txt/txt_register.h> #include <cpu/intel/haswell/haswell.h> #include <northbridge/intel/haswell/chip.h> #include <northbridge/intel/haswell/haswell.h> @@ -108,12 +111,27 @@ report_platform_info(); + if (CONFIG(INTEL_TXT)) + intel_txt_romstage_init(); + copy_spd(&pei_data); sdram_initialize(&pei_data); timestamp_add_now(TS_AFTER_INITRAM); + if (CONFIG(INTEL_TXT)) { + printk(BIOS_DEBUG, "Check TXT_ERROR register after MRC\n"); + + intel_txt_log_acm_error(read32((void *)TXT_ERROR)); + + intel_txt_log_spad(); + + intel_txt_memory_has_secrets(); + + txt_dump_regions(); + } + post_code(0x3b); intel_early_me_status(); diff --git a/src/security/intel/txt/Makefile.inc b/src/security/intel/txt/Makefile.inc index 64726cd..f7d6f9c 100644 --- a/src/security/intel/txt/Makefile.inc +++ b/src/security/intel/txt/Makefile.inc @@ -1,6 +1,8 @@ ifeq ($(CONFIG_INTEL_TXT),y) +romstage-y += romstage.c romstage-y += getsec_sclean.S +romstage-y += getsec.c romstage-y += common.c romstage-$(CONFIG_INTEL_TXT_LOGGING) += logging.c diff --git a/src/security/intel/txt/common.c b/src/security/intel/txt/common.c index 88e2b5d..4dd4ad3 100644 --- a/src/security/intel/txt/common.c +++ b/src/security/intel/txt/common.c @@ -290,6 +290,48 @@ return acm_data; } +#define MCU_BASE_ADDR (TXT_BASE + 0x278) +#define BIOACM_ADDR (TXT_BASE + 0x27c) +#define APINIT_ADDR (TXT_BASE + 0x290) +#define SEMAPHORE (TXT_BASE + 0x294) + +/* Returns on failure, resets the computer on success */ +void intel_txt_run_sclean(void) +{ + struct region_device acm; + size_t acm_len; + + void *acm_data = intel_txt_prepare_bios_acm(&acm, &acm_len); + + if (!acm_data) + return; + + /* FIXME: Do we need to program these two? */ + //write32((void *)MCU_BASE_ADDR, 0xffe1a990); + //write32((void *)APINIT_ADDR, 0xfffffff0); + + write32((void *)BIOACM_ADDR, (uintptr_t)acm_data); + write32((void *)SEMAPHORE, 0); + + /* + * The time SCLEAN will take depends on the installed RAM size. + * On Haswell with 8 GiB of DDR3, it takes five or ten minutes. (rough estimate) + */ + printk(BIOS_ALERT, "TEE-TXT: Invoking SCLEAN. This can take several minutes.\n"); + + /* + * Invoke the BIOS ACM. If successful, the system will reset with memory unlocked. + */ + getsec_sclean((uintptr_t)acm_data, acm_len); + + /* + * However, if this function returns, the BIOS ACM could not be invoked. This is bad. + */ + printk(BIOS_CRIT, "TEE-TXT: getsec_sclean could not launch the BIOS ACM.\n"); + + rdev_munmap(&acm, acm_data); +} + /* * Test all bits for TXT execution. * diff --git a/src/security/intel/txt/romstage.c b/src/security/intel/txt/romstage.c new file mode 100644 index 0000000..7a12deb --- /dev/null +++ b/src/security/intel/txt/romstage.c @@ -0,0 +1,125 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/mmio.h> +#include <cf9_reset.h> +#include <console/console.h> +#include <cpu/intel/common/common.h> +#include <cpu/x86/msr.h> +#include <southbridge/intel/common/pmbase.h> +#include <timer.h> +#include <types.h> + +#include <security/tpm/tis.h> + +#include "txt.h" +#include "txt_register.h" +#include "txt_getsec.h" + +static bool is_establishment_bit_asserted(void) +{ + struct stopwatch timer; + uint8_t access; + + /* Spec says no less than 30 milliseconds */ + stopwatch_init_msecs_expire(&timer, 50); + + while (true) { + access = read8((void *)TPM_ACCESS_REG); + + /* Register returns all ones if TPM is missing */ + if (access == 0xff) + return false; + + if (access & TPM_ACCESS_VALID) + break; + + /* On timeout, assume that the TPM is not working */ + if (stopwatch_expired(&timer)) + return false; + } + + /* This bit uses inverted logic: if cleared, establishment is asserted */ + return !(access & TPM_ACCESS_ESTABLISHMENT); +} + +static bool is_txt_cpu(void) +{ + const uint32_t ecx = cpu_get_feature_flags_ecx(); + + return (ecx & (CPUID_SMX | CPUID_VMX)) == (CPUID_SMX | CPUID_VMX); +} + +static bool is_txt_chipset(void) +{ + uint32_t eax; + + const bool success = getsec_capabilities(&eax); + + return success && eax & 1; +} + +/* Print the bad news */ +static void print_memory_is_locked(void) +{ + if (!CONFIG(INTEL_TXT_LOGGING)) + return; + + printk(BIOS_EMERG, "FATAL: Cannot run SCLEAN. Memory will remain locked.\n"); + printk(BIOS_EMERG, "\n"); + printk(BIOS_EMERG, "If you still want to boot, your options are:\n"); + printk(BIOS_EMERG, "\n"); + printk(BIOS_EMERG, " 1. Flash a coreboot image with a valid BIOS ACM.\n"); + printk(BIOS_EMERG, " Then, try again and hope it works this time.\n"); + printk(BIOS_EMERG, "\n"); + printk(BIOS_EMERG, " 2. If possible, remove the TPM from the system.\n"); + printk(BIOS_EMERG, " Reinstalling the TPM might lock memory again.\n"); + printk(BIOS_EMERG, "\n"); + printk(BIOS_EMERG, " 3. Disconnect all power sources, and RTC battery.\n"); + printk(BIOS_EMERG, " This may not work on all TXT-enabled platforms.\n"); + printk(BIOS_EMERG, "\n"); +} + +void intel_txt_romstage_init(void) +{ + /* Bail early if the CPU doesn't support TXT */ + if (!is_txt_cpu()) + return; + + /* We need to use GETSEC here, so enable it */ + enable_getsec_or_reset(); + + if (!is_txt_chipset()) + return; + + const uint8_t txt_ests = read8((void *)TXT_ESTS); + + const bool establishment = is_establishment_bit_asserted(); + const bool is_wake_error = !!(txt_ests & TXT_ESTS_WAKE_ERROR_STS); + + if (CONFIG(INTEL_TXT_LOGGING)) { + + printk(BIOS_INFO, "TEE-TXT: TPM established: %s\n", + establishment ? "true" : "false"); + } + + if (establishment && is_wake_error) { + + printk(BIOS_ERR, "TEE-TXT: Secrets remain in memory. SCLEAN is required.\n"); + + if (txt_ests & TXT_ESTS_TXT_RESET_STS) { + printk(BIOS_ERR, "TEE-TXT: TXT_RESET bit set, doing full reset!\n"); + full_reset(); + } + + /* FIXME: Clear SLP_TYP# */ + write_pmbase32(4, read_pmbase32(4) & ~(0x7 << 10)); + + intel_txt_run_sclean(); + + /* If running the BIOS ACM is impossible, manual intervention is required */ + print_memory_is_locked(); + + /* FIXME: vboot A/B could be used to recover, but has not been tested */ + die("Could not execute BIOS ACM to unlock the memory.\n"); + } +} diff --git a/src/security/intel/txt/txt.h b/src/security/intel/txt/txt.h index fc5c49e..976cc74 100644 --- a/src/security/intel/txt/txt.h +++ b/src/security/intel/txt/txt.h @@ -17,10 +17,13 @@ #define ACM_E_UUID_NOT_MATCH 0x09 #define ACM_E_PLATFORM_IS_NOT_PROD 0x10 +void intel_txt_romstage_init(void); + void intel_txt_log_bios_acm_error(void); int intel_txt_log_acm_error(const uint32_t acm_error); void intel_txt_log_spad(void); bool intel_txt_memory_has_secrets(void); +void intel_txt_run_sclean(void); int intel_txt_run_bios_acm(const u8 input_params); bool intel_txt_prepare_txt_env(void); diff --git a/src/security/intel/txt/txt_register.h b/src/security/intel/txt/txt_register.h index c19ec13..bb735b6 100644 --- a/src/security/intel/txt/txt_register.h +++ b/src/security/intel/txt/txt_register.h @@ -99,6 +99,16 @@ #define TXT_E2STS_SECRET_STS (1ull << 1) /* + * TCG PC Client Platform TPM Profile (PTP) Specification + * + * Note: Only locality 0 registers are publicly accessible. + */ + +#define TPM_BASE 0xfed40000UL + +#define TPM_ACCESS_REG (TPM_BASE + 0x00) + +/* * TXT Memory regions * Chapter 5.3 * Intel Trusted Execution Technology Lab Handout -- To view, visit https://review.coreboot.org/c/coreboot/+/46608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60beb7d79a30f460bbd5d94e4cba0244318c124e Gerrit-Change-Number: 46608 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

3 8

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2020