Attention is currently required from: Jonathan Zhang, Johnny Lin, Paul Menzel, Martin Roth.
Ed Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68637 )
Change subject: commonlib/fsp_relocate: Fix Coverity Issues
......................................................................
Patch Set 4:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68637/comment/c35f1780_742785b7
PS3, Line 10: reported issues. This change attempts to fix the static
> s/attempts to fix/fixes/
Done
https://review.coreboot.org/c/coreboot/+/68637/comment/fe834bef_cc5736c9
PS3, Line 18: on my own to test my changes against coverity tests.
> The above paragraph is not needed as commit message. You could put them as gerrit comment. […]
It is not allowing me to add myself as reviewer to add Gerrit comment on the code line specifically for the line item where I am fixing the Coverity issue. It is also not allowing me to comment on code file in a non-reviewer mode.
--
To view, visit https://review.coreboot.org/c/coreboot/+/68637
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I635c62929e8be9a474a91a62c29c3b5ee9b0ee64
Gerrit-Change-Number: 68637
Gerrit-PatchSet: 4
Gerrit-Owner: Ed Sharma <aeddiesharma(a)fb.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 02:14:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69159 )
Change subject: security/tpm: make tis_probe() return tpm_family
......................................................................
Patch Set 1:
(1 comment)
File src/drivers/i2c/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/69159/comment/c3382c95_2ba23633
PS1, Line 461: *tpm_family = 1;
Are you sure this is guaranteed? I thought I recall that 9645 can be 2.0 as well but I might be mixing stuff up...
Is there no way to detect the version dynamically here as well?
--
To view, visit https://review.coreboot.org/c/coreboot/+/69159
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5006e0cdfef76ff79ce9e1cf280fcd5515ae01b0
Gerrit-Change-Number: 69159
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 02:13:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Johnny Lin, Ed Sharma, Paul Menzel, Martin Roth.
Hello build bot (Jenkins), Jonathan Zhang, Johnny Lin, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/68637
to look at the new patch set (#4).
Change subject: commonlib/fsp_relocate: Fix Coverity Issues
......................................................................
commonlib/fsp_relocate: Fix Coverity Issues
Recently committed change 1df1cf9 resulted in some Coverity
reported issues. This change fixes the static analysis issues
reported. The issues were reported as comments in [CB:66819].
TESTED=
This code is tested with Intel CooperLake-SP FSP version
2.2.0.33A for DeltaLake boot
Signed-off-by: Eddie Sharma <aeddiesharma(a)fb.com>
Change-Id: I635c62929e8be9a474a91a62c29c3b5ee9b0ee64
---
M src/commonlib/fsp_relocate.c
1 file changed, 29 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/68637/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/68637
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I635c62929e8be9a474a91a62c29c3b5ee9b0ee64
Gerrit-Change-Number: 68637
Gerrit-PatchSet: 4
Gerrit-Owner: Ed Sharma <aeddiesharma(a)fb.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Ed Sharma <aeddiesharma(a)fb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69023 )
Change subject: drivers/pc80/tpm: probe for TPM family of a device
......................................................................
Patch Set 2:
(1 comment)
File src/drivers/pc80/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/69023/comment/177203b9_00bb2938
PS2, Line 97: int tpm_family;
I think it might be nice to make an enum for this (especially since you start using it more widely in the next patch).
--
To view, visit https://review.coreboot.org/c/coreboot/+/69023
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5464771836c66bcc441efb7189ded416b8f53827
Gerrit-Change-Number: 69023
Gerrit-PatchSet: 2
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 02:04:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69023 )
Change subject: drivers/pc80/tpm: probe for TPM family of a device
......................................................................
Patch Set 2:
(2 comments)
File src/drivers/pc80/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/69023/comment/5ef1e90a_66d99018
PS2, Line 392: const char *vendor_name = device_name;
Why not just initialize these to NULL now so you don't have to strcmp?
https://review.coreboot.org/c/coreboot/+/69023/comment/ac9936f5_98314d3f
PS2, Line 435: return TPM_DRIVER_ERR;
This is a pretty big change in behavior (refusing to support anything not in the list). I don't really care since our boards don't use this driver, but you might want to check on the mailing list if the rest of the community is okay with this.
--
To view, visit https://review.coreboot.org/c/coreboot/+/69023
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5464771836c66bcc441efb7189ded416b8f53827
Gerrit-Change-Number: 69023
Gerrit-PatchSet: 2
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 02:02:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69022 )
Change subject: drivers/spi/tpm: verify device supports TPM2
......................................................................
Patch Set 1:
(1 comment)
File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/69022/comment/86b3077d_57f37ceb
PS1, Line 458: if (tpm2_read_reg(TPM_INTF_ID_REG, &intf_id, sizeof(intf_id)) != CB_SUCCESS) {
Can we wrap this in an `if (!CONFIG(TPM_GOOGLE))`? Just because our TPMs haven't always been 100% accurate in reflecting the spec (particularly on older versions) and I want to ensure this won't cause any issues. (Google TPMs are always TPM 2.0 anyway.)
--
To view, visit https://review.coreboot.org/c/coreboot/+/69022
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib2840a21b3be8928d39570281f86a0e26b38b5f9
Gerrit-Change-Number: 69022
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 01:57:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68991 )
Change subject: security/tpm/: turn tis_{init,open} into tis_probe
......................................................................
Patch Set 1:
(2 comments)
File src/drivers/i2c/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/68991/comment/716e5f33_2514d535
PS1, Line 127: if (chip.is_open) {
Does is_open really fulfill a purpose? The CRB implementation doesn't have that either.
I think we should just make tlcl_lib_init() the only entry point for all this, have the called-twice-check in there, say tis_probe() shouldn't get called from anywhere else, and then we can get rid of all the called-twice-checks in these individual implementations.
File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/68991/comment/c38a7b38_b3c44ee2
PS1, Line 188: uint32_t tlcl_lib_init(void)
Since these functions are the same between 1.2 and 2.0 it would also be good to merge them into some file that's common between the two.
--
To view, visit https://review.coreboot.org/c/coreboot/+/68991
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib4ce35ada24e3959ea1a518c29d431b4ae123809
Gerrit-Change-Number: 68991
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 01:53:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68989 )
Change subject: security/tpm: remove public tis_close()
......................................................................
Patch Set 1:
(1 comment)
File src/drivers/pc80/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/68989/comment/9c9445e0_09db623d
PS1, Line 742: if (!has_access && tis_close() < 0)
Oh, I completely missed that it's used down here too.
I find this isolated use of tis_open() and tis_close() down here pretty weird. How does it guarantee that tis_probe() has been called before getting here? Is the probe not needed before open in this case, or is it just relying on random boot order coincidence that something else would have taken care of that beforehand?
This seems to be pretty old code that was probably written before we had some of the other things in place... I think it would probably be cleaner nowadays if this just called tlcl_lib_init() so it can be sure to go through the same TPM init path in the same order as everything else (and then not do the close again afterwards).
--
To view, visit https://review.coreboot.org/c/coreboot/+/68989
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9df76adfc21fca9fa1d1af7c40635ec0684ceb0f
Gerrit-Change-Number: 68989
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 01:50:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68990 )
Change subject: drivers/i2c/tpm: splice tpm_vendor_specific struct
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/68990
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic3644290963aca9f8dc7cd8ef754352865ef8d2c
Gerrit-Change-Number: 68990
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 01:33:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment