Attention is currently required from: Tim Wawrzynczak, Kyösti Mälkki.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58896 )
Change subject: ChromeOS: Add legacy mainboard_ec_running_ro()
......................................................................
Patch Set 7:
(2 comments)
File src/include/bootmode.h:
https://review.coreboot.org/c/coreboot/+/58896/comment/d485a440_e56fc625
PS7, Line 17: bool mainboard_ec_running_ro(void);
> the EC is trusted when it is running RO, it is untrusted when running RW, so conceptually they are t […]
(see https://review.coreboot.org/c/coreboot/+/58896/7/src/vendorcode/google/chro…, they're not always equivalent)
File src/mainboard/google/butterfly/chromeos.c:
https://review.coreboot.org/c/coreboot/+/58896/comment/b1048ff5_6e521971
PS5, Line 14: void fill_lb_gpios(struct lb_gpios *gpios)
> I am having trouble finding the schematics for butterly, I will keep digging.
Butterfly did not use a Chrome EC, so I'm pretty sure it didn't have that line.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58896
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3cb95268424dc27f8c1e26b3d34eff1a7b8eab7f
Gerrit-Change-Number: 58896
Gerrit-PatchSet: 7
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Comment-Date: Thu, 18 Nov 2021 21:13:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Nick Vaccaro, Kyösti Mälkki, Karthik Ramasubramanian, Felix Held.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58897 )
Change subject: ChromeOS: Promote variant_cros_gpio()
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/58897
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5c2ac1dcea35f1f00dea401528404bc6ca0ab53c
Gerrit-Change-Number: 58897
Gerrit-PatchSet: 9
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:59:28 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Kyösti Mälkki.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58896 )
Change subject: ChromeOS: Add legacy mainboard_ec_running_ro()
......................................................................
Patch Set 7:
(2 comments)
File src/include/bootmode.h:
https://review.coreboot.org/c/coreboot/+/58896/comment/ed594f2d_1cd69f0e
PS7, Line 17: bool mainboard_ec_running_ro(void);
> I think the introduced get_ec_is_trusted() is the inverse of mainboard_ec_running_ro() now?
the EC is trusted when it is running RO, it is untrusted when running RW, so conceptually they are the same
File src/mainboard/google/butterfly/chromeos.c:
https://review.coreboot.org/c/coreboot/+/58896/comment/e177cb67_3e08ebed
PS5, Line 14: void fill_lb_gpios(struct lb_gpios *gpios)
> I do not see a "EC in RW" entry here.
I am having trouble finding the schematics for butterly, I will keep digging.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58896
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3cb95268424dc27f8c1e26b3d34eff1a7b8eab7f
Gerrit-Change-Number: 58896
Gerrit-PatchSet: 7
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:57:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Angel Pons, Patrick Rudolph.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58517 )
Change subject: soc/intel/cannonlake: Fix PEG1 _PRT generation
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
Patchset:
PS6:
very odd, I don't recall seeing anything fishy when I implemented this but google/hatch does not use PEG, so I guess I never could test that part.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58517
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5be6e4514f8c6a47bb887d9f9b95181c9f426a51
Gerrit-Change-Number: 58517
Gerrit-PatchSet: 6
Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:38:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Andrey Pronin, Yu-Ping Wu, Karthik Ramasubramanian.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59476 )
Change subject: src/security/vboot: Setup secure counter space in TPM NVRAM
......................................................................
Patch Set 1:
(7 comments)
File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/59476/comment/a53a2d50_78579711
PS1, Line 286: bool "Define Secure Counters in Verstage"
Not sure this needs to be menuconfig-visible? I'd suggest just `select`ing it from SoC Kconfig `if CHROMEOS`.
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59476/comment/f9a97568_7f906664
PS1, Line 151: static const TPMA_NV secure_counter_attr = {
In the interest of trying to keep these attributes function-describing and potentially reusable, I'd maybe call this rw_orderly_counter_attr.
https://review.coreboot.org/c/coreboot/+/59476/comment/d436a8db_576d70bb
PS1, Line 159: .TPMA_NV_PPWRITE = 1,
Similar to the discussion in https://review.coreboot.org/c/coreboot/+/59097/3..6/src/security/vboot/secd…, we have been setting NO_DA on all other counters for now, and the same arguments would probably apply to this one?
https://review.coreboot.org/c/coreboot/+/59476/comment/d5a8420e_c285d120
PS1, Line 355: return tlcl_nv_increment(index);
Does the counter need to be incremented in firmware? If the userspace code that needs it just increments it on first use, we can avoid having to pull support for the increment command into firmware (that's how we have been dealing with the existing counters for now).
https://review.coreboot.org/c/coreboot/+/59476/comment/30a275a9_41eb6044
PS1, Line 368: rv = tlcl_read(index, &value, SECURE_COUNTER_SIZE);
What's the point of this check? When we're going through factory init, this counter should never exist already.
https://review.coreboot.org/c/coreboot/+/59476/comment/2c57e186_81cd17a4
PS1, Line 416: RETURN_ON_FAILURE(setup_secure_counter_spaces());
The firmware space should always be the last one that is created, since this code uses that to confirm that factory initialization was fully completed. Need to move this call further up.
(Also, this is just aesthetics but I think it would be easier to follow and more consistent with other optional spaces to put the if (CONFIG(VBOOT_DEFINE_SECURE_COUNTERS)) right here, rather than hide it inside the function.)
File src/security/vboot/secure_counter.h:
https://review.coreboot.org/c/coreboot/+/59476/comment/a7e1a8de_3ba960c3
PS1, Line 14: SECURE_COUNTER4_NV_INDEX, /* 0x1012 */
These should be defined with the other TPM space indices in <antirollback.h>, so that we have them all in one place. I don't think you need this extra header.
Also, this name should be precise enough to make it's purpose clear and avoid potential future name clashes. "secure counter" could be anything. Maybe call it widevine_psp_counter or something?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59476
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I915fbdada60e242d911b748ad5dc28028de9b657
Gerrit-Change-Number: 59476
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Andrey Pronin <apronin(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Andrey Pronin <apronin(a)google.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:34:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Kane Chen, Tim Wawrzynczak, Patrick Rudolph.
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59355 )
Change subject: soc/intel/alderlake: Save/restore BAR registers when extract cpu crashlog
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
Patchset:
PS6:
Also please verify the end to end flow, including the decoding step, after this change; and indicate in comment whether the decoding was done successfully (without error).
--
To view, visit https://review.coreboot.org/c/coreboot/+/59355
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib7e6772be51ec4f26ef31fed6cb2bddef8ffc6be
Gerrit-Change-Number: 59355
Gerrit-PatchSet: 6
Gerrit-Owner: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Francois Toguo Fotso <francois.toguo.fotso(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:33:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Rob Barnes, Karthik Ramasubramanian.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59477 )
Change subject: mb/google/guybrush: Enable secure counters
......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/google/guybrush/Kconfig:
https://review.coreboot.org/c/coreboot/+/59477/comment/a4d19064_29e638c9
PS1, Line 57: VBOOT_STARTS_BEFORE_BOOTBLOCK
Why is it dependent on before bootblock? In theory we can do this if verstage runs in the x86.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59477
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6818c6f7905aa2eb815059e23c4f79437593f8ca
Gerrit-Change-Number: 59477
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Rob Barnes <robbarnes(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:29:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Andrey Pronin, Julius Werner, Yu-Ping Wu, Karthik Ramasubramanian.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59476 )
Change subject: src/security/vboot: Setup secure counter space in TPM NVRAM
......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59476/comment/8db1b7b6_4fdc8860
PS1, Line 35: 72057594037927936
Why is the value so high?
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59476/comment/305807c7_08d6869c
PS1, Line 349: SECURE_COUNTER_NAME
Is this any arbitrary name?
https://review.coreboot.org/c/coreboot/+/59476/comment/91dc8a2f_dd41dce1
PS1, Line 369: !=
Should we check for TPM_SUCCESS instead? I'm concerned this will let other errors through.
https://review.coreboot.org/c/coreboot/+/59476/comment/9c4eac4b_6267ce01
PS1, Line 386: * Of all NVRAM spaces defined by this function the firmware space
: * must be defined last, because its existence is considered an
: * indication that TPM factory initialization was successfully
: * completed.
Does this comment need updating, or do you need to move setup_secure_counter_spaces before setup_firmware_spaces?
File src/security/vboot/secure_counter.h:
https://review.coreboot.org/c/coreboot/+/59476/comment/5843ced6_60372bbd
PS1, Line 7: 8
Would doing a sizeof(uint64_t) make this more clear?
https://review.coreboot.org/c/coreboot/+/59476/comment/96a43a49_9f0ebeed
PS1, Line 11: 0x100f
Is this a google specific or cr50 specific number? Should we pass these in via a Kconfig instead?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59476
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I915fbdada60e242d911b748ad5dc28028de9b657
Gerrit-Change-Number: 59476
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Andrey Pronin <apronin(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Andrey Pronin <apronin(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:28:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Andrey Pronin, Christian Walter, Julius Werner, Karthik Ramasubramanian.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59475 )
Change subject: src/security/tpm: Add TPM2_NV_Increment command
......................................................................
Patch Set 1:
(3 comments)
File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/59475/comment/2e9abebd_ad8cc3de
PS1, Line 349: Will
nit: will
https://review.coreboot.org/c/coreboot/+/59475/comment/d390adee_edba8a59
PS1, Line 349:
nit: a
File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/59475/comment/f0fc602e_2349770c
PS1, Line 201:
nit: clang-format
--
To view, visit https://review.coreboot.org/c/coreboot/+/59475
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic86f8db5ad0926e9d1fd34a9ca5d55d884f76423
Gerrit-Change-Number: 59475
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Andrey Pronin <apronin(a)google.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Andrey Pronin <apronin(a)chromium.org>
Gerrit-Attention: Andrey Pronin <apronin(a)google.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Nov 2021 20:18:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment