coreboot-gerrit October 2021

coreboot-gerrit@coreboot.org

1 participants
5514 discussions

Change in coreboot[master]: util/cse_fpt: Add a new tool for managing Intel CSE FPT binaries
by Furquan Shaikh (Code Review) 09 Oct '21

09 Oct '21

1 0

Change in coreboot[master]: util/cse_serger: Add a new tool for stitching CSE components
by Furquan Shaikh (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Cliff Huang, Martin Roth, Tim Wawrzynczak, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55503 ) Change subject: util/cse_serger: Add a new tool for stitching CSE components ...................................................................... Patch Set 23: (11 comments) File util/cbfstool/bpdt_formats/bpdt_1_7.c: https://review.coreboot.org/c/coreboot/+/55503/comment/6cf77a1d_48a8a7e7 PS22, Line 210: l->checksum = calculate_layout_checksum(l); > nit: both this line and `calculate_layout_checksum` set l->checksum `calculate_layout_checksum()` stashes what is in l->checksum to curr_checksum, sets it to 0 to perform the calculation, restores l->checksum and returns the calculated value. If it seems too confusing, I can see if there is a better way to write `calculate_layout_checksum`. https://review.coreboot.org/c/coreboot/+/55503/comment/d56259d2_04dbd3ca PS22, Line 235: printf("BP4 size: 0x%x\n", l->bp4_size); > remove this line (extra copy of line 237) Done https://review.coreboot.org/c/coreboot/+/55503/comment/37ed25c8_a7297dc8 PS22, Line 317: crc32(0xffff > so the layout checksum is inverted but the header checksum is not? It is on line 326 below once the checksum is calculated on header and all the entries. File util/cbfstool/bpdt_formats/subpart_entry_1.c: https://review.coreboot.org/c/coreboot/+/55503/comment/9e8bf6fe_a1cdcb80 PS22, Line 49: printf("%-25s%-25s%-25s%-25s%-25s%-25s\n", "Entry #", "Name", "Offset", "Huffman Compressed?", : "Length", "Rsvd"); > nit: the line is already over 96, either make it 1 line or break before 96 Done File util/cbfstool/cse_serger.h: https://review.coreboot.org/c/coreboot/+/55503/comment/0f86f7e6_3bd6cd3f PS21, Line 15: > nit: extra blank line Done https://review.coreboot.org/c/coreboot/+/55503/comment/56b830fa_7cbd556a PS21, Line 17: write_member(_buff, &(_x), sizeof(_x)) > nit: align with read_member above Done File util/cbfstool/cse_serger.c: https://review.coreboot.org/c/coreboot/+/55503/comment/da16cc83_a9e9dc29 PS22, Line 79: ifwi > nit: I think the struct name is superfluous Done https://review.coreboot.org/c/coreboot/+/55503/comment/48ec7306_2782e7fd PS22, Line 99: subpart_info > also superfluous? Done https://review.coreboot.org/c/coreboot/+/55503/comment/71a40b99_e819d661 PS22, Line 167: : void write_member(struct buffer *buff, void *src, size_t size) : { : void *dst = buffer_get(buff); : : switch (size) { : case 1: : write_le8(dst, *(uint8_t *)src); : break; : case 2: : write_le16(dst, *(uint16_t *)src); : break; : case 4: : write_le32(dst, *(uint32_t *)src); : break; : case 8: : write_le64(dst, *(uint64_t *)src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } : : void read_member(struct buffer *buff, void *dst, size_t size) : { : const void *src = buffer_get(buff); : : switch (size) { : case 1: : *(uint8_t *)dst = read_le8(src); : break; : case 2: : *(uint16_t *)dst = read_le16(src); : break; : case 4: : *(uint32_t *)dst = read_le32(src); : break; : case 8: : *(uint64_t *)dst = read_le64(src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } > is it worth it to break these out into a separate file for cse_fpt and cse_serger to share? it's not […] It makes sense. I will do that. Done: CB:58201 https://review.coreboot.org/c/coreboot/+/55503/comment/5092b8b3_c0d0ef71 PS22, Line 304: int > size_t Done https://review.coreboot.org/c/coreboot/+/55503/comment/7c512477_6b4cdbc7 PS22, Line 857: if (c >= LONGOPT_START && c < LONGOPT_END) : return true; : : return false; > nit: […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/55503 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I90dd809b47fd16afdc80e66431312721082496aa Gerrit-Change-Number: 55503 Gerrit-PatchSet: 23 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Andrew McRae <amcrae(a)google.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Attention: Andrew McRae <amcrae(a)google.com> Gerrit-Comment-Date: Fri, 08 Oct 2021 22:22:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/cse_serger: Add a new tool for stitching CSE components
by build bot (Jenkins) (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Cliff Huang, Martin Roth, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55503 ) Change subject: util/cse_serger: Add a new tool for stitching CSE components ...................................................................... Patch Set 23: (2 comments) File util/cbfstool/cse_serger.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130085): https://review.coreboot.org/c/coreboot/+/55503/comment/8b343999_f73b4804 PS23, Line 272: ERROR("Part(%d) exceeds file size. Part offset=0x%x, Part size = 0x%x, File size = 0x%zx\n", line over 96 characters Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130085): https://review.coreboot.org/c/coreboot/+/55503/comment/bfd82f89_73a4f3b2 PS23, Line 920: if (c < LONGOPT_START) { braces {} are not necessary for any arm of this statement -- To view, visit https://review.coreboot.org/c/coreboot/+/55503 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I90dd809b47fd16afdc80e66431312721082496aa Gerrit-Change-Number: 55503 Gerrit-PatchSet: 23 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Andrew McRae <amcrae(a)google.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Attention: Andrew McRae <amcrae(a)google.com> Gerrit-Comment-Date: Fri, 08 Oct 2021 22:22:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/cse*: Add cse_helpers.{c,h}
by Furquan Shaikh (Code Review) 09 Oct '21

09 Oct '21

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/58201 ) Change subject: util/cse*: Add cse_helpers.{c,h} ...................................................................... util/cse*: Add cse_helpers.{c,h} This change moves `read_member` and `write_member` helper functions out of cse_fpt.c and cse_serger.c into cse_helpers.c to avoid duplication. BUG=b:189177186,b:189167923 Change-Id: I7b646b29c9058d892bb0fc9824ef1b4340d2510c Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M util/cbfstool/Makefile.inc M util/cbfstool/cse_fpt.c M util/cbfstool/cse_fpt.h A util/cbfstool/cse_helpers.c A util/cbfstool/cse_helpers.h M util/cbfstool/cse_serger.c M util/cbfstool/cse_serger.h 7 files changed, 77 insertions(+), 84 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/58201/1 diff --git a/util/cbfstool/Makefile.inc b/util/cbfstool/Makefile.inc index 0a445b5..4651f51 100644 --- a/util/cbfstool/Makefile.inc +++ b/util/cbfstool/Makefile.inc @@ -100,12 +100,14 @@ cse_fpt_obj := cse_fpt_obj += cse_fpt.o cse_fpt_obj += common.o +cse_fpt_obj += cse_helpers.o cse_fpt_obj += $(foreach var, $(fpt_formats_obj), $(var)) include $(top)/util/cbfstool/bpdt_formats/Makefile.inc cse_serger_obj := cse_serger_obj += cse_serger.o cse_serger_obj += common.o +cse_serger_obj += cse_helpers.o cse_serger_obj += $(foreach var, $(bpdt_formats_obj), $(var)) TOOLCFLAGS ?= -Werror -Wall -Wextra -Wshadow diff --git a/util/cbfstool/cse_fpt.c b/util/cbfstool/cse_fpt.c index bde3a8c..51fa9d0 100644 --- a/util/cbfstool/cse_fpt.c +++ b/util/cbfstool/cse_fpt.c @@ -64,30 +64,6 @@ name, name); } -void read_member(struct buffer *buff, void *dst, size_t size) -{ - uint8_t *src = buffer_get(buff); - - switch (size) { - case 1: - *(uint8_t *)dst = read_le8(src); - break; - case 2: - *(uint16_t *)dst = read_le16(src); - break; - case 4: - *(uint32_t *)dst = read_le32(src); - break; - case 8: - *(uint64_t *)dst = read_le64(src); - break; - default: - memcpy(dst, src, size); - } - - buffer_seek(buff, size); -} - static int get_fpt_buff(struct buffer *input_buff, struct buffer *fpt_buff) { /* diff --git a/util/cbfstool/cse_fpt.h b/util/cbfstool/cse_fpt.h index eda8a07..3f9c185 100644 --- a/util/cbfstool/cse_fpt.h +++ b/util/cbfstool/cse_fpt.h @@ -7,12 +7,11 @@ #include <commonlib/region.h> #include "common.h" +#include "cse_helpers.h" #define FPT_MARKER "$FPT" #define FPT_ENTRY_VERSION 0x10 -#define READ_MEMBER(_buff, _x) read_member(_buff, &(_x), sizeof(_x)) - enum fpt_hdr_version { FPT_HDR_VERSION_20 = 0x20, FPT_HDR_VERSION_21 = 0x21, @@ -32,6 +31,4 @@ extern const struct fpt_hdr_ops fpt_hdr_20_ops; extern const struct fpt_hdr_ops fpt_hdr_21_ops; -void read_member(struct buffer *buff, void *dst, size_t size); - #endif /* __CBFSTOOL_CSE_FPT_H__ */ diff --git a/util/cbfstool/cse_helpers.c b/util/cbfstool/cse_helpers.c new file mode 100644 index 0000000..1fe0646 --- /dev/null +++ b/util/cbfstool/cse_helpers.c @@ -0,0 +1,57 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Helper functions for cse_serger and cse_fpt */ + +#include <commonlib/endian.h> + +#include "cse_helpers.h" + +void write_member(struct buffer *buff, void *src, size_t size) +{ + void *dst = buffer_get(buff); + + switch (size) { + case 1: + write_le8(dst, *(uint8_t *)src); + break; + case 2: + write_le16(dst, *(uint16_t *)src); + break; + case 4: + write_le32(dst, *(uint32_t *)src); + break; + case 8: + write_le64(dst, *(uint64_t *)src); + break; + default: + memcpy(dst, src, size); + break; + } + + buffer_seek(buff, size); +} + +void read_member(struct buffer *buff, void *dst, size_t size) +{ + const void *src = buffer_get(buff); + + switch (size) { + case 1: + *(uint8_t *)dst = read_le8(src); + break; + case 2: + *(uint16_t *)dst = read_le16(src); + break; + case 4: + *(uint32_t *)dst = read_le32(src); + break; + case 8: + *(uint64_t *)dst = read_le64(src); + break; + default: + memcpy(dst, src, size); + break; + } + + buffer_seek(buff, size); +} + diff --git a/util/cbfstool/cse_helpers.h b/util/cbfstool/cse_helpers.h new file mode 100644 index 0000000..58ba128 --- /dev/null +++ b/util/cbfstool/cse_helpers.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <stddef.h> + +#include "common.h" + +#define READ_MEMBER(_buff, _x) read_member(_buff, &(_x), sizeof(_x)) +#define WRITE_MEMBER(_buff, _x) write_member(_buff, &(_x), sizeof(_x)) + +/* Write data from `src` of length `size` to buffer in little-endian format and seek the buffer + forward by `size` bytes. */ +void write_member(struct buffer *buff, void *src, size_t size); + +/* Read data from buffer in little-endian format of length `size` to `dst` and seek the buffer + forward by `size` bytes. */ +void read_member(struct buffer *buff, void *dst, size_t size); diff --git a/util/cbfstool/cse_serger.c b/util/cbfstool/cse_serger.c index cc49671..c8bd104 100644 --- a/util/cbfstool/cse_serger.c +++ b/util/cbfstool/cse_serger.c @@ -165,56 +165,6 @@ return -1; } -void write_member(struct buffer *buff, void *src, size_t size) -{ - void *dst = buffer_get(buff); - - switch (size) { - case 1: - write_le8(dst, *(uint8_t *)src); - break; - case 2: - write_le16(dst, *(uint16_t *)src); - break; - case 4: - write_le32(dst, *(uint32_t *)src); - break; - case 8: - write_le64(dst, *(uint64_t *)src); - break; - default: - memcpy(dst, src, size); - break; - } - - buffer_seek(buff, size); -} - -void read_member(struct buffer *buff, void *dst, size_t size) -{ - const void *src = buffer_get(buff); - - switch (size) { - case 1: - *(uint8_t *)dst = read_le8(src); - break; - case 2: - *(uint16_t *)dst = read_le16(src); - break; - case 4: - *(uint32_t *)dst = read_le32(src); - break; - case 8: - *(uint64_t *)dst = read_le64(src); - break; - default: - memcpy(dst, src, size); - break; - } - - buffer_seek(buff, size); -} - static const struct bpdt_ops *get_bpdt_ops(const struct buffer *buff) { assert(buff || params.version_str); diff --git a/util/cbfstool/cse_serger.h b/util/cbfstool/cse_serger.h index b60a9c7..8727f59 100644 --- a/util/cbfstool/cse_serger.h +++ b/util/cbfstool/cse_serger.h @@ -7,14 +7,12 @@ #include <commonlib/region.h> #include "common.h" +#include "cse_helpers.h" #define BPDT_SIGNATURE (0x000055AA) #define BUFF_SIZE_ALIGN (4 * KiB) -#define READ_MEMBER(_buff, _x) read_member(_buff, &(_x), sizeof(_x)) -#define WRITE_MEMBER(_buff, _x) write_member(_buff, &(_x), sizeof(_x)) - enum bpdt_version { BPDT_VERSION_1_6 = 1, BPDT_VERSION_1_7 = 2, @@ -89,7 +87,4 @@ extern const struct subpart_entry_ops subpart_entry_1_ops; -void read_member(struct buffer *buff, void *dst, size_t size); -void write_member(struct buffer *buff, void *src, size_t size); - #endif /* __CBFSTOOL_CSE_SERGER_H__ */ -- To view, visit https://review.coreboot.org/c/coreboot/+/58201 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7b646b29c9058d892bb0fc9824ef1b4340d2510c Gerrit-Change-Number: 58201 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: util/cse_serger: Add a new tool for stitching CSE components
by Furquan Shaikh (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Cliff Huang, Furquan Shaikh, Martin Roth, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. Hello build bot (Jenkins), Cliff Huang, Martin Roth, Patrick Georgi, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Bernardo Perez Priego, Bernardo Perez Priego, Andrew McRae, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/55503 to look at the new patch set (#23). Change subject: util/cse_serger: Add a new tool for stitching CSE components ...................................................................... util/cse_serger: Add a new tool for stitching CSE components This change adds a new tool `cse_serger` which can be used to print, dump and stitch together different components for the CSE region. BUG=b:189177186 Change-Id: I90dd809b47fd16afdc80e66431312721082496aa Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M Makefile.inc M util/cbfstool/Makefile M util/cbfstool/Makefile.inc A util/cbfstool/bpdt_formats/Makefile.inc A util/cbfstool/bpdt_formats/bpdt_1_6.c A util/cbfstool/bpdt_formats/bpdt_1_7.c A util/cbfstool/bpdt_formats/subpart_entry_1.c A util/cbfstool/bpdt_formats/subpart_hdr_1.c A util/cbfstool/bpdt_formats/subpart_hdr_2.c A util/cbfstool/cse_serger.c A util/cbfstool/cse_serger.h 11 files changed, 1,957 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/55503/23 -- To view, visit https://review.coreboot.org/c/coreboot/+/55503 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I90dd809b47fd16afdc80e66431312721082496aa Gerrit-Change-Number: 55503 Gerrit-PatchSet: 23 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Andrew McRae <amcrae(a)google.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Attention: Andrew McRae <amcrae(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: arch/x86,cpu/x86,lib/thread: Remove usage of cpu_info from lib/thread
by Karthik Ramasubramanian (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Raul Rangel, Julius Werner. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58199 ) Change subject: arch/x86,cpu/x86,lib/thread: Remove usage of cpu_info from lib/thread ...................................................................... Patch Set 1: (1 comment) File src/lib/thread.c: https://review.coreboot.org/c/coreboot/+/58199/comment/78be20f2_cdb75a19 PS1, Line 249: !boot_cpu() Should we move this check inside set_current_thread. I am not sure if there is a possibility of this check getting removed and hence trashing the active_thread. -- To view, visit https://review.coreboot.org/c/coreboot/+/58199 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iea4622d52c36d529e100b7ea55f32c334acfdf3e Gerrit-Change-Number: 58199 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Fri, 08 Oct 2021 21:55:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/cse_serger: Add a new tool for stitching CSE components
by Tim Wawrzynczak (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Cliff Huang, Furquan Shaikh, Martin Roth, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55503 ) Change subject: util/cse_serger: Add a new tool for stitching CSE components ...................................................................... Patch Set 22: (11 comments) File util/cbfstool/bpdt_formats/bpdt_1_7.c: https://review.coreboot.org/c/coreboot/+/55503/comment/c5432680_5147c759 PS22, Line 210: l->checksum = calculate_layout_checksum(l); nit: both this line and `calculate_layout_checksum` set l->checksum https://review.coreboot.org/c/coreboot/+/55503/comment/c9680cf4_ec43d801 PS22, Line 235: printf("BP4 size: 0x%x\n", l->bp4_size); remove this line (extra copy of line 237) https://review.coreboot.org/c/coreboot/+/55503/comment/b346f316_a111f22b PS22, Line 317: crc32(0xffff so the layout checksum is inverted but the header checksum is not? File util/cbfstool/bpdt_formats/subpart_entry_1.c: https://review.coreboot.org/c/coreboot/+/55503/comment/5e50eb17_b23c127f PS22, Line 49: printf("%-25s%-25s%-25s%-25s%-25s%-25s\n", "Entry #", "Name", "Offset", "Huffman Compressed?", : "Length", "Rsvd"); nit: the line is already over 96, either make it 1 line or break before 96 File util/cbfstool/cse_serger.h: https://review.coreboot.org/c/coreboot/+/55503/comment/a109d88e_829b1975 PS21, Line 15: nit: extra blank line https://review.coreboot.org/c/coreboot/+/55503/comment/8bddb2b3_0b15bff9 PS21, Line 17: write_member(_buff, &(_x), sizeof(_x)) nit: align with read_member above File util/cbfstool/cse_serger.c: https://review.coreboot.org/c/coreboot/+/55503/comment/b6b1d570_45907a57 PS22, Line 79: ifwi nit: I think the struct name is superfluous https://review.coreboot.org/c/coreboot/+/55503/comment/cc7bce3f_e2241114 PS22, Line 99: subpart_info also superfluous? https://review.coreboot.org/c/coreboot/+/55503/comment/8f4dffce_4c889865 PS22, Line 167: : void write_member(struct buffer *buff, void *src, size_t size) : { : void *dst = buffer_get(buff); : : switch (size) { : case 1: : write_le8(dst, *(uint8_t *)src); : break; : case 2: : write_le16(dst, *(uint16_t *)src); : break; : case 4: : write_le32(dst, *(uint32_t *)src); : break; : case 8: : write_le64(dst, *(uint64_t *)src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } : : void read_member(struct buffer *buff, void *dst, size_t size) : { : const void *src = buffer_get(buff); : : switch (size) { : case 1: : *(uint8_t *)dst = read_le8(src); : break; : case 2: : *(uint16_t *)dst = read_le16(src); : break; : case 4: : *(uint32_t *)dst = read_le32(src); : break; : case 8: : *(uint64_t *)dst = read_le64(src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } is it worth it to break these out into a separate file for cse_fpt and cse_serger to share? it's not very much code, so maybe not. https://review.coreboot.org/c/coreboot/+/55503/comment/f254926a_18242439 PS22, Line 304: int size_t https://review.coreboot.org/c/coreboot/+/55503/comment/74531de7_510798fc PS22, Line 857: if (c >= LONGOPT_START && c < LONGOPT_END) : return true; : : return false; nit: `return c >= LONGOPT_START && c < LONGOPT_END;` -- To view, visit https://review.coreboot.org/c/coreboot/+/55503 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I90dd809b47fd16afdc80e66431312721082496aa Gerrit-Change-Number: 55503 Gerrit-PatchSet: 22 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Andrew McRae <amcrae(a)google.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Attention: Andrew McRae <amcrae(a)google.com> Gerrit-Comment-Date: Fri, 08 Oct 2021 21:39:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/cse_serger: Add a new tool for stitching CSE components
by build bot (Jenkins) (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Cliff Huang, Martin Roth, Tim Wawrzynczak, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55503 ) Change subject: util/cse_serger: Add a new tool for stitching CSE components ...................................................................... Patch Set 22: Verified+1 (3 comments) File util/cbfstool/bpdt_formats/subpart_entry_1.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130077): https://review.coreboot.org/c/coreboot/+/55503/comment/90e2cc0c_06896e03 PS22, Line 49: printf("%-25s%-25s%-25s%-25s%-25s%-25s\n", "Entry #", "Name", "Offset", "Huffman Compressed?", line over 96 characters File util/cbfstool/cse_serger.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130077): https://review.coreboot.org/c/coreboot/+/55503/comment/e502f87a_52159508 PS22, Line 272: ERROR("Part(%d) exceeds file size. Part offset=0x%x, Part size = 0x%x, File size = 0x%zx\n", line over 96 characters Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130077): https://review.coreboot.org/c/coreboot/+/55503/comment/41018b9c_f3fd128e PS22, Line 923: if (c < LONGOPT_START) { braces {} are not necessary for any arm of this statement -- To view, visit https://review.coreboot.org/c/coreboot/+/55503 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I90dd809b47fd16afdc80e66431312721082496aa Gerrit-Change-Number: 55503 Gerrit-PatchSet: 22 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Andrew McRae <amcrae(a)google.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.corp-partner.google.com> Gerrit-Attention: Andrew McRae <amcrae(a)google.com> Gerrit-Comment-Date: Fri, 08 Oct 2021 21:14:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/qualcomm/sc7280: Enable compression of SHRM
by Julius Werner (Code Review) 09 Oct '21

09 Oct '21

Attention is currently required from: Shelley Chen. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58191 ) Change subject: soc/qualcomm/sc7280: Enable compression of SHRM ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/58191 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaad8a8a02abe40bd01766d94ef0b61aac7671936 Gerrit-Change-Number: 58191 Gerrit-PatchSet: 3 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-CC: Sudheer Amrabadi <samrabad(a)codeaurora.org> Gerrit-CC: mturney mturney <mturney(a)codeaurora.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Comment-Date: Fri, 08 Oct 2021 21:10:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/cse_serger: Add a new tool for stitching CSE components
by Furquan Shaikh (Code Review) 09 Oct '21

09 Oct '21

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2021