[coreboot-gerrit] Change in coreboot[master]: cr50: add marshaling/unmarshaling of vendor commands and pro...

[Vadim Bendebury (Code Review)]

Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/18945 )

Change subject: cr50: add marshaling/unmarshaling of vendor commands and process turn_on
......................................................................


Patch Set 2:

(8 comments)

https://review.coreboot.org/#/c/18945/2//COMMIT_MSG
Commit Message:

PS2, Line 9: CR50
> Ditto. Maybe elaborate here.
This is by far not the first patch dealing with Cr50 in the coreboot..


Line 15: is coming.
> I’d appreciate a pointer where the mechanism is described in more detail.
ok


https://review.coreboot.org/#/c/18945/2/src/include/tpm_lite/tlcl.h
File src/include/tpm_lite/tlcl.h:

Line 168:  * the timeout milliseconds.
> Discuss how 0 means don't actually reboot.
ok


Line 170:  * Returns nonzero value in reset_pending if there were restored header(s).
> Now that I'm seeing the other side of this I understand it better. We have 
I don't see why you call it "very fumbly": we send a command to the cr50: if needed, reboot in x ms, let us know if it was needed.

Then we have time to do whatever we want and wait for reset.

Sending one command instead of two achieving the same result is preferable IMO.


https://review.coreboot.org/#/c/18945/2/src/lib/tpm2_marshaling.c
File src/lib/tpm2_marshaling.c:

PS2, Line 618: if (*size != 1) {
             : 			*size = -1;
             : 			return;
             : 		}
> There is no data that we care about in the nvmem enable commits response.
yes, we still want to ensure that the response size is right.


https://review.coreboot.org/#/c/18945/2/src/lib/tpm2_tlcl.c
File src/lib/tpm2_tlcl.c:

PS2, Line 424: printk(BIOS_INFO, "%s: failed %x\n", __func__,
             : 				response->hdr.tpm_code);
             : 		else
             : 			printk(BIOS_INFO, "%s: failed\n", __func__);
> Should the debug level be decreased, as that’s an error condition?
will try to refactor this and the previous function's code.


Line 431: 	*reset_pending = response->vc.turn_on;
> This doesn't mean reset pending. This means there were headers that were up
will fix.


https://review.coreboot.org/#/c/18945/2/src/lib/tpm2_tlcl_structures.h
File src/lib/tpm2_tlcl_structures.h:

Line 283: struct vc_response {
> I take it this is supposed to mean vendor command response?
yes. 

Do you think this could be mistaken for 'venture capitalist' ?


-- 
To view, visit https://review.coreboot.org/18945
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic76d384d637c0eeaad206e0a8242cbb8e2b19b37
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes