[coreboot-gerrit] Change in coreboot[master]: cr50: add unmarshaling of vendor commands and process 'enabl...

[Vadim Bendebury (Code Review)]

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

Change subject: cr50: add unmarshaling of vendor commands and process 'enable_update'
......................................................................


Patch Set 3: Code-Review+1

(5 comments)

verified that this still works.

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

Line 170:  *
> I don't see why you call it "very fumbly": we send a command to the cr50: i
hopefully 1s will be more than enough. We do want the AP to be off (with plt_rst_l asserted) by the time of reset. If we are to do it in two commands we'd need to add another vendor command to reset after a timeout. 

I suggest we start using it as presented.


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

PS2, Line 618: tatic struct tpm2_response tpm2_static_resp CAR_GLOBAL;
             : 	struct tpm2_response *tpm2_resp = car_get_var_ptr(&tpm2_static_resp);
             : 	/*
             : 	 *
> yes, we still want to ensure that the response size is right.
having looked at the code around I think we can forgo all these checks here. The unmarshal functions will take care of indicating the error, and the caller will be alerted if the remaining size is not zero.


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

PS2, Line 424: 
             : 	*num_restored_headers = response->vcr.num_restored_headers;
             : 	return TPM_SUCCESS;
             : }
> will try to refactor this and the previous function's code.
resorted to using the more common processing case used in this file.


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

Line 283: struct vendor_command_response {
> It's just sort of floating here. virtual channel is another name. I'm sure 
ok, fixed.


Line 286: 		uint8_t num_restored_headers;
> turn_on_num_headers since this is really the count of headers flipped.
fixed


-- 
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: 3
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