coreboot-gerrit May 2021

coreboot-gerrit@coreboot.org

1 participants
6048 discussions

Change in coreboot[master]: mb/google/dedede/var/boten: Modify DPTF parameters
by Sumeet R Pawnikar (Code Review) 14 May '21

14 May '21

Attention is currently required from: Henry Sun, Stanley Wu, Paul Fagerburg, Karthik Ramasubramanian. Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52755 ) Change subject: mb/google/dedede/var/boten: Modify DPTF parameters ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/52755 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I43002db61de650d29cd85944a4eaea1b2f99aec4 Gerrit-Change-Number: 52755 Gerrit-PatchSet: 3 Gerrit-Owner: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com> Gerrit-Reviewer: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kevin Chang <kevin.chang(a)lcfc.corp-partner.google.com> Gerrit-CC: Melo Chuang <melo.chuang(a)lcfc.corp-partner.google.com> Gerrit-CC: Rasheed Hsueh <rasheed.hsueh(a)lcfc.corp-partner.google.com> Gerrit-CC: Sunshine Chao <sunshine.chao(a)lcfc.corp-partner.google.com> Gerrit-Attention: Henry Sun <henrysun(a)google.com> Gerrit-Attention: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com> Gerrit-Attention: Paul Fagerburg <pfagerburg(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 May 2021 08:28:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: security/intel/cbnt: Add logging
by Angel Pons (Code Review) 14 May '21

14 May '21

Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Arthur Heymans, Morgan Jang, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54093 ) Change subject: security/intel/cbnt: Add logging ...................................................................... Patch Set 7: Code-Review+1 (4 comments) File src/security/intel/cbnt/Kconfig: https://review.coreboot.org/c/coreboot/+/54093/comment/d0109226_33a6ca44 PS6, Line 20: config INTEL_CBNT_LOGGING > > Would it make sense to use the same TXT option? This way, we avoid having two separate options. > > > > I did not want to touch the TXT logging as it is mostly wrong for CBnT and maybe even for TXT, as I can't correlate the code to datasheets I have. Ack > > Also, this option is currently never build-tested. > > I'll just always compile the logging and make the calling dependent on the Kconfig option. I don't think unconditionally compiling in the code is a good idea. I'd rather add a config file to build-test CBnT with logging enabled (which is what I do with TXT) > I'll also add CBnT buildtesting. Some changes were needed in cbtn-prov as it did not work without IFD. Sounds good. File src/security/intel/cbnt/logging.c: https://review.coreboot.org/c/coreboot/+/54093/comment/b5989558_5d16cd21 PS7, Line 106: const char *bios_acm = "BIOS ACM Error", *sinit_acm = "SINIT ACM Error", : *btg = "Boot Guard Error", *reserved = "Reserved"; : : switch (type) { : case 0: : return bios_acm; : case 1: : return sinit_acm; : case 3: : return btg; : default: : return reserved; : } Er, what I meant in PS6 is: switch (type) { case 0: return "BIOS ACM Error"; case 1: return "SINIT ACM Error"; case 3: return "Boot Guard Error"; default: return "Reserved"; } https://review.coreboot.org/c/coreboot/+/54093/comment/6e1b42f1_14452521 PS7, Line 123: } Very minor nit: the opening brace has a space afterwards, but the closing brace doesn't have a space before https://review.coreboot.org/c/coreboot/+/54093/comment/e2e21f69_ff95b32d PS7, Line 159: ERRORCODE I'd also drop redundant `ERRORCODE` mentions from here. -- To view, visit https://review.coreboot.org/c/coreboot/+/54093 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8b57132bedbd944b9861ab0e2e0d14723cb61635 Gerrit-Change-Number: 54093 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 14 May 2021 08:26:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/dedede/var/boten: Modify DPTF parameters
by Stanley Wu (Code Review) 14 May '21

14 May '21

Attention is currently required from: Henry Sun, Sumeet R Pawnikar, Paul Fagerburg, Karthik Ramasubramanian. Stanley Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52755 ) Change subject: mb/google/dedede/var/boten: Modify DPTF parameters ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/52755/comment/823acb9b_888ec23e PS2, Line 9: paramerters > Nit: parameters Done -- To view, visit https://review.coreboot.org/c/coreboot/+/52755 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I43002db61de650d29cd85944a4eaea1b2f99aec4 Gerrit-Change-Number: 52755 Gerrit-PatchSet: 3 Gerrit-Owner: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com> Gerrit-Reviewer: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kevin Chang <kevin.chang(a)lcfc.corp-partner.google.com> Gerrit-CC: Melo Chuang <melo.chuang(a)lcfc.corp-partner.google.com> Gerrit-CC: Rasheed Hsueh <rasheed.hsueh(a)lcfc.corp-partner.google.com> Gerrit-CC: Sunshine Chao <sunshine.chao(a)lcfc.corp-partner.google.com> Gerrit-Attention: Henry Sun <henrysun(a)google.com> Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Attention: Paul Fagerburg <pfagerburg(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 May 2021 08:23:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/dedede/var/boten: Modify DPTF parameters
by Stanley Wu (Code Review) 14 May '21

14 May '21

Attention is currently required from: Henry Sun, Sumeet R Pawnikar, Stanley Wu, Paul Fagerburg. Hello build bot (Jenkins), Henry Sun, Sumeet R Pawnikar, Paul Fagerburg, Karthik Ramasubramanian, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/52755 to look at the new patch set (#3). Change subject: mb/google/dedede/var/boten: Modify DPTF parameters ...................................................................... mb/google/dedede/var/boten: Modify DPTF parameters DPTF parameters from thermal team. 1. Modify TSR1 sensor as charge sensor. 2. Modify P-state parameter BUG=b:180641150 BRANCH=dedede TEST=build and verified by thermal team. Change-Id: I43002db61de650d29cd85944a4eaea1b2f99aec4 Signed-off-by: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com> --- M src/mainboard/google/dedede/variants/boten/overridetree.cb 1 file changed, 36 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/52755/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/52755 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I43002db61de650d29cd85944a4eaea1b2f99aec4 Gerrit-Change-Number: 52755 Gerrit-PatchSet: 3 Gerrit-Owner: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com> Gerrit-Reviewer: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kevin Chang <kevin.chang(a)lcfc.corp-partner.google.com> Gerrit-CC: Melo Chuang <melo.chuang(a)lcfc.corp-partner.google.com> Gerrit-CC: Rasheed Hsueh <rasheed.hsueh(a)lcfc.corp-partner.google.com> Gerrit-CC: Sunshine Chao <sunshine.chao(a)lcfc.corp-partner.google.com> Gerrit-Attention: Henry Sun <henrysun(a)google.com> Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Attention: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com> Gerrit-Attention: Paul Fagerburg <pfagerburg(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/dedede/var/cret: Enable/disable LTE function based on FW_CO...
by Furquan Shaikh (Code Review) 14 May '21

14 May '21

Attention is currently required from: Dtrain Hsu, Henry Sun, Tim Wawrzynczak, Karthik Ramasubramanian. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54057 ) Change subject: mb/google/dedede/var/cret: Enable/disable LTE function based on FW_CONFIG ...................................................................... Patch Set 3: (2 comments) Patchset: PS3: > A question for fw_config. […] Tim - Yes, we do! I will raise a bug to see if I can get some help on it from kernel team. Dtrain - No. But, ACPI nodes will be generated based on device being on/off which can be controlled which fw_config. Thus, it provides a way to trigger certain ACPI _ON/_OFF routines without having to add special fw_config checks. File src/mainboard/google/dedede/variants/cret/variant.c: https://review.coreboot.org/c/coreboot/+/54057/comment/307bf076_1d160304 PS3, Line 18: Once the FW_CONFIG is provisioned, I know it is not the most ideal thing, but is there any harm in calling `power_off_lte_module()` unconditionally when going down into S5? For devices with LTE module, it will ensure proper power off sequence. For devices without LTE module, it will just toggle some pads and then go down into S5 after which we fall into G3 in 10 seconds. I don't think there should be any power leakage issues as well. -- To view, visit https://review.coreboot.org/c/coreboot/+/54057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib926e99aaf9df433a7cff71180ee55431d69f718 Gerrit-Change-Number: 54057 Gerrit-PatchSet: 3 Gerrit-Owner: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alan Lee <alan_lee(a)compal.corp-partner.google.com> Gerrit-CC: Ian Feng <ian_feng(a)compal.corp-partner.google.com> Gerrit-Attention: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com> Gerrit-Attention: Henry Sun <henrysun(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 May 2021 08:18:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: payload/tianocore: Drop TIANOCORE_TARGET_IA32
by Lance Zhao (Code Review) 14 May '21

14 May '21

Attention is currently required from: Bora Guvendik, Lijian Zhao, Martin Roth, Matt DeVillier, John Zhao, Stefan Reinauer, Angel Pons, Arthur Heymans. Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54189 ) Change subject: payload/tianocore: Drop TIANOCORE_TARGET_IA32 ...................................................................... Patch Set 8: (1 comment) File payloads/external/tianocore/Kconfig: https://review.coreboot.org/c/coreboot/+/54189/comment/d07d2c92_f6387951 PS7, Line 37: choice : prompt "Target architecture" : default TIANOCORE_TARGET_X64 : help : The Tianocore coreboot Payload Package binary can be : built for either only IA32 or both X64 and IA32 architectures. : Select which architecture(s) to build for; default is to build : for both X64 and IA32. : : config TIANOCORE_TARGET_X64 : bool "X64" : help : By selecting this option, the target architecture will be built : for X64 and IA32. : : endchoice > Does it make sense to keep this? Agree, totally remove it. -- To view, visit https://review.coreboot.org/c/coreboot/+/54189 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iadd9a3c455fad4eede8a0a017415acd2c57fba04 Gerrit-Change-Number: 54189 Gerrit-PatchSet: 8 Gerrit-Owner: Lance Zhao Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Lijian Zhao <lijian.zhao(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Lijian Zhao <lijian.zhao(a)intel.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: John Zhao <john.zhao(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Fri, 14 May 2021 08:15:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: security/intel/cbnt: Add logging
by build bot (Jenkins) (Code Review) 14 May '21

14 May '21

Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Angel Pons, Morgan Jang, Patrick Rudolph. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54093 ) Change subject: security/intel/cbnt: Add logging ...................................................................... Patch Set 7: (8 comments) File src/security/intel/cbnt/logging.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/b311a51c_cbba2a5a PS7, Line 21: uint64_t : 24; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/dc5a9033_e1e28397 PS7, Line 23: uint64_t : 1; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/8de76ad0_05ee2dfc PS7, Line 25: uint64_t : 29; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/747192e6_b06c48e0 PS7, Line 42: uint64_t : 59; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/eb488c08_bd2bbdc8 PS7, Line 57: uint32_t : 15; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/2ce3c5e9_902cd8b3 PS7, Line 67: uint32_t : 5; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/64ca225e_3c895739 PS7, Line 84: uint32_t : 2; space prohibited before that ':' (ctx:WxW) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119313): https://review.coreboot.org/c/coreboot/+/54093/comment/420e6ffd_74761fe8 PS7, Line 95: uint32_t : 6; space prohibited before that ':' (ctx:WxW) -- To view, visit https://review.coreboot.org/c/coreboot/+/54093 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8b57132bedbd944b9861ab0e2e0d14723cb61635 Gerrit-Change-Number: 54093 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 14 May 2021 08:13:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: security/intel/cbnt: Add logging
by Arthur Heymans (Code Review) 14 May '21

14 May '21

Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Angel Pons, Morgan Jang, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54093 ) Change subject: security/intel/cbnt: Add logging ...................................................................... Patch Set 7: (12 comments) File src/security/intel/cbnt/Kconfig: https://review.coreboot.org/c/coreboot/+/54093/comment/a094a74e_d4c9a93d PS6, Line 20: config INTEL_CBNT_LOGGING > Would it make sense to use the same TXT option? This way, we avoid having two separate options. > I did not want to touch the TXT logging as it is mostly wrong for CBnT and maybe even for TXT, as I can't correlate the code to datasheets I have. > Also, this option is currently never build-tested. I'll just always compile the logging and make the calling dependent on the Kconfig option. I'll also add CBnT buildtesting. Some changes were needed in cbtn-prov as it did not work without IFD. File src/security/intel/cbnt/cbnt.h: https://review.coreboot.org/c/coreboot/+/54093/comment/027a06dd_b89f391c PS6, Line 3: 0xfed30000 > Hmmm, but this matches TXT_PUBLIC_SPACE Done File src/security/intel/cbnt/logging.c: https://review.coreboot.org/c/coreboot/+/54093/comment/57f3ee1f_174ffbb3 PS6, Line 14: nem > nit: nem_enabled Done https://review.coreboot.org/c/coreboot/+/54093/comment/6245c6f8_7ce9f761 PS6, Line 16: tpm_succes > missing one s: tpm_succes*s* Done https://review.coreboot.org/c/coreboot/+/54093/comment/3d6f0b31_dd836212 PS6, Line 17: reserved1 > On CFL, this is `facb` Done https://review.coreboot.org/c/coreboot/+/54093/comment/42a24156_fd524b4c PS6, Line 32: > no space after `*`. Done https://review.coreboot.org/c/coreboot/+/54093/comment/bc3da29b_949393b6 PS6, Line 103: const > This `const` is meaningless. […] Done https://review.coreboot.org/c/coreboot/+/54093/comment/003dc538_1af5c191 PS6, Line 106: *btg = "Boot Guard Error", *reserved = "Reserved"; > You don't need these static variables here. String literals are already static and constant. Thanks. https://review.coreboot.org/c/coreboot/+/54093/comment/7e6e8cf6_adcac3ca PS6, Line 124: | ((uint64_t)sacm_info_msr.hi << 32)}; > Why not add `lo` and `hi` to `union sacm_info`? > > const union sacm_info acm_info = { > .lo = sacm_info_msr.lo, > .hi = sacm_info_msr.hi, > }; > > Oh wait, sacm_info is a MSR. Then, simply add a `msr_t` union field: > > union sacm_info_msr { > struct { > uint64_t nem : 1; > uint64_t tpm_type : 2; > uint64_t tpm_succes : 1; > uint64_t reserved1 : 1; > uint64_t measured_boot : 1; > uint64_t verified_boot : 1; > uint64_t revoked : 1; > uint64_t : 24; > uint64_t btg_cap : 1; > uint64_t : 1; > uint64_t txt_cap : 1; > uint64_t : 29; > }; > msr_t msr; > uint64_t raw; > }; > > Then, just initialise it directly: > > const union sacm_info acm_info = { > .msr = rdmsr(MSR_BOOT_GUARD_SACM_INFO), > }; Nice. https://review.coreboot.org/c/coreboot/+/54093/comment/600d860d_9c00ffc8 PS6, Line 126: CBNT:\tSACM INFO: > I wouldn't print this on each and every line. Just indent the lines after the first instance. […] Done https://review.coreboot.org/c/coreboot/+/54093/comment/d097c452_bafb184b PS6, Line 139: read64((void *)CBNT_BOOTSTATUS) > nit: read64p(CBNT_BOOTSTATUS) […] Done https://review.coreboot.org/c/coreboot/+/54093/comment/8ac31c2f_36fc40a4 PS6, Line 157: if (txt_err_valid) { : if (!btsts.txt_dis_pol) { > Join these two ifs to save one indentation level? […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/54093 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8b57132bedbd944b9861ab0e2e0d14723cb61635 Gerrit-Change-Number: 54093 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 14 May 2021 08:13:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: security/intel/cbnt: Add logging
by Arthur Heymans (Code Review) 14 May '21

14 May '21

Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Arthur Heymans, Morgan Jang, Patrick Rudolph. Hello build bot (Jenkins), Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Morgan Jang, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/54093 to look at the new patch set (#7). Change subject: security/intel/cbnt: Add logging ...................................................................... security/intel/cbnt: Add logging This decodes and logs the CBnT status and error registers. Change-Id: I8b57132bedbd944b9861ab0e2e0d14723cb61635 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/security/intel/cbnt/Kconfig M src/security/intel/cbnt/Makefile.inc A src/security/intel/cbnt/cbnt.h A src/security/intel/cbnt/logging.c M src/security/intel/txt/ramstage.c M src/soc/intel/xeon_sp/bootblock.c 6 files changed, 245 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/54093/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/54093 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8b57132bedbd944b9861ab0e2e0d14723cb61635 Gerrit-Change-Number: 54093 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/cherry: Add DRAM calibration support
by Yu-Ping Wu (Code Review) 14 May '21

14 May '21

Attention is currently required from: Rex-BC Chen. Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54269 ) Change subject: mb/google/cherry: Add DRAM calibration support ...................................................................... Patch Set 9: (1 comment) File src/mainboard/google/cherry/sdram_configs.c: https://review.coreboot.org/c/coreboot/+/54269/comment/c413a9bc_32c0dab1 PS9, Line 9: die Is this a typo? What is die size? -- To view, visit https://review.coreboot.org/c/coreboot/+/54269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib7677baef126ee60bf35da3a4eaf720eaa118a27 Gerrit-Change-Number: 54269 Gerrit-PatchSet: 9 Gerrit-Owner: Rex-BC Chen <rex-bc.chen(a)mediatek.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Rex-BC Chen <rex-bc.chen(a)mediatek.corp-partner.google.com> Gerrit-Comment-Date: Fri, 14 May 2021 08:07:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2021