coreboot-gerrit December 2021

coreboot-gerrit@coreboot.org

1 participants
5525 discussions

Change in coreboot[master]: mb/google/brya/var/primus: Fix PLD group order
by YH Lin (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Paul Menzel, Tim Wawrzynczak, Scott Chao. YH Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59953 ) Change subject: mb/google/brya/var/primus: Fix PLD group order ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/59953 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If5ce6ca061d9d56ba0bbb1f157b2ba278d3fa9c3 Gerrit-Change-Number: 59953 Gerrit-PatchSet: 3 Gerrit-Owner: Scott Chao <scott_chao(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Scott Chao <scott_chao(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Ariel Fang <ariel_fang(a)wistron.corp-partner.google.com> Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Scott Chao <scott_chao(a)wistron.corp-partner.google.com> Gerrit-Comment-Date: Tue, 07 Dec 2021 17:37:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/stoneyridge/southbridge: drop ENV_X86 check
by Felix Held (Code Review) 07 Dec '21

07 Dec '21

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59960 ) Change subject: soc/amd/stoneyridge/southbridge: drop ENV_X86 check ...................................................................... soc/amd/stoneyridge/southbridge: drop ENV_X86 check Stoneyridge selects ARCH_X86 unconditionally and all coreboot code will run on the x86 cores. On Picasso and later, the Chromebooks run verstage on the PSP which is an ARM V7 core which needs some special handling cases in the code, but this doesn't apply to Stoneyridge. TEST=Timeless build results in an identical image for Google/Careena. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I013efd13b56c0191af034a8c4b58e9b26a31c6e9 --- M src/soc/amd/stoneyridge/southbridge.c 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/59960/1 diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index de27ac5..1ed2b9a 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -230,8 +230,7 @@ static void sb_init_spi_base(void) { /* Make sure the base address is predictable */ - if (ENV_X86) - lpc_set_spibase(SPI_BASE_ADDRESS); + lpc_set_spibase(SPI_BASE_ADDRESS); lpc_enable_spi_rom(SPI_ROM_ENABLE); } -- To view, visit https://review.coreboot.org/c/coreboot/+/59960 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I013efd13b56c0191af034a8c4b58e9b26a31c6e9 Gerrit-Change-Number: 59960 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: brya4es: sync CB:58374 from brya0
by Tim Wawrzynczak (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: YH Lin. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59935 ) Change subject: brya4es: sync CB:58374 from brya0 ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59935 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I97489343b8f7a5b9457cd6f4a61cc37cd10ab450 Gerrit-Change-Number: 59935 Gerrit-PatchSet: 1 Gerrit-Owner: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: YH Lin <yueherngl(a)google.com> Gerrit-Comment-Date: Tue, 07 Dec 2021 17:25:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: libpayload: Provide includes for payloads
by Patrick Georgi (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Jakub Czapiga. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59916 ) Change subject: libpayload: Provide includes for payloads ...................................................................... Patch Set 2: (3 comments) File payloads/libpayload/Makefile.inc: https://review.coreboot.org/c/coreboot/+/59916/comment/df063b8a_aff30f02 PS1, Line 74: early-headers-install: > Is there any way to "cache" *.h files easily? I do not want to create dep-files by hand. I suppose this question will become moot when we don't have that early header install step anymore? File payloads/libpayload/Makefile.inc: https://review.coreboot.org/c/coreboot/+/59916/comment/102d7a76_c37557d6 PS2, Line 75: printf " INSTALL $(obj)/libpayload/include\n" : install -m 755 -d "$(obj)/libpayload/include" : for file in `find include -name *.h -type f`; do \ : install -m 755 -d "$(obj)/libpayload/`dirname $$file`"; \ : install -m 644 "$$file" "$(obj)/libpayload/$$file"; \ : done : for file in `find $(coreboottop)/src/commonlib/bsd/include -name *.h -type f`; do \ : dest_file=$$(realpath --relative-to=$(coreboottop)/src/commonlib/bsd/ $$file); \ : install -m 755 -d "$(obj)/libpayload/`dirname $$dest_file`"; \ : install -m 644 "$$file" "$(obj)/libpayload/$$dest_file"; \ : done > I am not sure, if we really need this. […] Seeing what this requires it might indeed be easier to just ensure that "make install" copies commonlib into place and ignore users that pick apart the build/ directory. https://review.coreboot.org/c/coreboot/+/59916/comment/5f933284_eb85862d PS2, Line 132: for file in `find $(coreboottop)/src/commonlib/bsd/include -name *.h -type f`; do \ > 1. Do we want to provide (PD)curses includes? […] The scope of this change is to copy commonlib in place, I'd ignore any issues about pdcurses here. Changing the build directory structure _might_ help, but it's also more involved than just copying the right files in the install step, so while I thought that might be a good idea, it apparently is more complicated than it's worth. -- To view, visit https://review.coreboot.org/c/coreboot/+/59916 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idc7175240f3077ec98280331f9a952310aae4341 Gerrit-Change-Number: 59916 Gerrit-PatchSet: 2 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Comment-Date: Tue, 07 Dec 2021 17:21:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/testing: combine code coverage data
by Paul Fagerburg (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Martin Roth. Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59959 ) Change subject: util/testing: combine code coverage data ...................................................................... Patch Set 3: (1 comment) Patchset: PS2: > It's missing the Signed-off-by Line, but otherwise LGTM Sorry 'bout that. Added the signed-off-by. -- To view, visit https://review.coreboot.org/c/coreboot/+/59959 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id99615ca8279f80a402d5371221b8fd36fb91d55 Gerrit-Change-Number: 59959 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Comment-Date: Tue, 07 Dec 2021 17:18:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Georgi <patrick(a)coreboot.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/testing: combine code coverage data
by Paul Fagerburg (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Martin Roth, Paul Fagerburg. Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59959 to look at the new patch set (#3). Change subject: util/testing: combine code coverage data ...................................................................... util/testing: combine code coverage data As part of the `what-jenkins-does` target, combine the code coverage data from all unit tests (currently just coreboot and libpayload). BUG=b:203800199 TEST=`make what-jenkins-does && ls -l coreboot-builds/coverage.info` Signed-off-by: Paul Fagerburg <pfagerburg(a)google.com> Change-Id: Id99615ca8279f80a402d5371221b8fd36fb91d55 --- M util/testing/Makefile.inc 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/59959/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/59959 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id99615ca8279f80a402d5371221b8fd36fb91d55 Gerrit-Change-Number: 59959 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: [TESTME] sb/amd/pi/hudson/early_init: fix setting SPI_USE_SPI100 in S...
by Michał Żygowski (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Michał Kopeć, Felix Held. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59934 ) Change subject: [TESTME] sb/amd/pi/hudson/early_init: fix setting SPI_USE_SPI100 in SPI100_ENABLE ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: Okay so at first glance there are no side effects neither before applying the patch nor after the patch. A little bit of performance stats (debug level 7, serial port enabled): 2,818,129 usecs - no spi100 2,798,503 usecs - spi100 before patch 2,804,461 usecs - spi100 after patch The discrepancy between SPI100 with and without patch may be a result of AGESA memory training (bad luck with seed or something). Anyway the gain from SPI100 in this case is about 15-20ms -- To view, visit https://review.coreboot.org/c/coreboot/+/59934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifbd960a9509542b28f03326a3066995540260bef Gerrit-Change-Number: 59934 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Michał Kopeć <michal.kopec(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ł Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 07 Dec 2021 17:17:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/testing: combine code coverage data
by Patrick Georgi (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Martin Roth, Paul Fagerburg. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59959 ) Change subject: util/testing: combine code coverage data ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Patchset: PS2: It's missing the Signed-off-by Line, but otherwise LGTM -- To view, visit https://review.coreboot.org/c/coreboot/+/59959 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id99615ca8279f80a402d5371221b8fd36fb91d55 Gerrit-Change-Number: 59959 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Comment-Date: Tue, 07 Dec 2021 17:04:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cbfstool: Fix offset calculation for aligned files
by Julius Werner (Code Review) 07 Dec '21

07 Dec '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59877 ) Change subject: cbfstool: Fix offset calculation for aligned files ...................................................................... cbfstool: Fix offset calculation for aligned files The placement calculation logic in cbfs_add_component() has become quite a mess, and this patch can only fix that to a limited degree. The interaction between all the different pathways of how the `offset` variable can be set and at what point exactly the final placement offset is decided can get quite convoluted. In particular, one existing problem is that the offset for a file added with the --align flag is decided before the convert() function is called, which may change the form (and thereby the size) of the file again after its location was found -- resulting in a location that ends up being too small, or being unable to find a location for a file that should fit. This used to be okay under the assumption that forced alignment should really only be necessary for use cases like XIP where the file is directly "used" straight from its location on flash in some way, and those cases can never be compressed -- however, recent AMD platforms have started using the --align flag to meet the requirements of their SPI DMA controller and broken this assumption. This patch fixes that particular problem and hopefully eliminates a bit of the convolution by moving the offset decision point in the --align case after the convert() step. This is safe when the steps in-between (add_topswap_bootblock() and convert() itself) do not rely on the location having already been decided by --align before that point. For the topswap case this is easy, because in practice we always call it with --base-address (and as far as I can tell that's the only way it was ever meant to work?) -- so codify that assumption in the function. For convert() this mostly means that the implementations that do touch the offset variable (mkstage and FSP) need to ensure they take care of the alignment themselves. The FSP case is particularly complex so I tried to rewrite the code in a slightly more straight-forward way and clearly document the supported cases, which should hopefully make it easier to see that the offset variable is handled correctly in all of them. For mkstage the best solution seems to be to only have it touch the offset variable in the XIP case (where we know compression must be disabled, so we can rely on it not changing the file size later), and have the extra space for the stage header directly taken care of by do_cbfs_locate() so that can happen after convert(). NOTE: This is changing the behavior of `cbfstool add -t fsp` when neither --base-address nor --xip are passed (e.g. FSP-S). Previously, cbfstool would implicitly force an alignment of 4K. As far as I can tell from the comments, this is unnecessary because this binary is loaded into RAM and CBFS placement does not matter, so I assume this is an oversight caused by accidentally reusing code that was only meant for the XIP case. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ia49a585988f7a74944a6630b77b3ebd79b3a9897 Reviewed-on: https://review.coreboot.org/c/coreboot/+/59877 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Patrick Georgi <patrick(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> --- M util/cbfstool/cbfstool.c 1 file changed, 82 insertions(+), 68 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, but someone else must approve Raul Rangel: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index c9db45a..87dcbb4 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -507,9 +507,10 @@ return 0; } -static int do_cbfs_locate(uint32_t *cbfs_addr, size_t metadata_size, - size_t data_size) +static int do_cbfs_locate(uint32_t *cbfs_addr, size_t data_size) { + uint32_t metadata_size = 0; + if (!param.filename) { ERROR("You need to specify -f/--filename.\n"); return 1; @@ -559,6 +560,8 @@ } if (param.precompression || param.compression != CBFS_COMPRESS_NONE) metadata_size += sizeof(struct cbfs_file_attr_compression); + if (param.type == CBFS_TYPE_STAGE) + metadata_size += sizeof(struct cbfs_file_attr_stageheader); /* Take care of the hash attribute if it is used */ if (param.hash != VB2_HASH_INVALID) @@ -776,6 +779,11 @@ { size_t bb_buf_size = buffer_size(buffer); + if (!param.baseaddress_assigned) { + ERROR("--topswap-size must also have --base-address\n"); + return 1; + } + if (bb_buf_size > param.topswap_size) { ERROR("Bootblock bigger than the topswap boundary\n"); ERROR("size = %zd, ts = %d\n", bb_buf_size, @@ -812,18 +820,37 @@ static int cbfs_add_component(const char *filename, const char *name, - uint32_t type, uint32_t headeroffset, convert_buffer_t convert) { - size_t len_align = 0; + /* + * The steps used to determine the final placement offset in CBFS, in order: + * + * 1. If --base-address was passed, that value is used. + * + * 2. The convert() function may write a location back to |offset|, usually by calling + * do_cbfs_locate(). In this case, it needs to ensure that the location found can fit + * the CBFS file in its final form (after any compression and conversion). + * + * 3. If --align was passed and the offset is still undecided at this point, + * do_cbfs_locate() is called to find an appropriately aligned location. + * + * 4. If |offset| is still 0 at the end, cbfs_add_entry() will find the first available + * location that fits. + */ uint32_t offset = param.baseaddress_assigned ? param.baseaddress : 0; + size_t len_align = 0; if (param.alignment && param.baseaddress_assigned) { ERROR("Cannot specify both alignment and base address\n"); return 1; } + if (param.stage_xip && param.compression != CBFS_COMPRESS_NONE) { + ERROR("Cannot specify compression for XIP.\n"); + return 1; + } + if (!filename) { ERROR("You need to specify -f/--filename.\n"); return 1; @@ -834,7 +861,7 @@ return 1; } - if (type == 0) { + if (param.type == 0) { ERROR("You need to specify a valid -t/--type.\n"); return 1; } @@ -855,12 +882,12 @@ } struct cbfs_file *header = - cbfs_create_file_header(type, buffer.size, name); + cbfs_create_file_header(param.type, buffer.size, name); /* Bootblock and CBFS header should never have file hashes. When adding the bootblock it is important that we *don't* look up the metadata hash yet (before it is added) or we'll cache an outdated result. */ - if (type != CBFS_TYPE_BOOTBLOCK && type != CBFS_TYPE_CBFSHEADER) { + if (param.type != CBFS_TYPE_BOOTBLOCK && param.type != CBFS_TYPE_CBFSHEADER) { enum vb2_hash_algorithm mh_algo = get_mh_cache()->cbfs_hash.algo; if (mh_algo != VB2_HASH_INVALID && param.hash != mh_algo) { if (param.hash == VB2_HASH_INVALID) { @@ -874,16 +901,11 @@ } } - /* This needs to run after potentially updating param.hash above. */ - if (param.alignment) - if (do_cbfs_locate(&offset, 0, 0)) - goto error; - /* * Check if Intel CPU topswap is specified this will require a * second bootblock to be added. */ - if (type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) + if (param.type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) if (add_topswap_bootblock(&buffer, &offset)) goto error; @@ -892,7 +914,12 @@ goto error; } - /* This needs to run after convert(). */ + /* This needs to run after convert() to take compression into account. */ + if (!offset && param.alignment) + if (do_cbfs_locate(&offset, 0)) + goto error; + + /* This needs to run after convert() to hash the actual final file data. */ if (param.hash != VB2_HASH_INVALID && cbfs_add_file_hash(header, &buffer, param.hash) == -1) { ERROR("couldn't add hash for '%s'\n", name); @@ -1028,45 +1055,44 @@ { uint32_t address; struct buffer fsp; - int do_relocation = 1; - - address = *offset; /* - * If the FSP component is xip, then ensure that the address is a memory - * mapped one. - * If the FSP component is not xip, then use param.baseaddress that is - * passed in by the caller. + * There are 4 different cases here: + * + * 1. --xip and --base-address: we need to place the binary at the given base address + * in the CBFS image and relocate it to that address. *offset was already filled in. + * + * 2. --xip but no --base-address: we implicitly force a 4K minimum alignment so that + * relocation can occur. Call do_cbfs_locate() here to find an appropriate *offset. + * + * 3. No --xip but a --base-address: special case where --base-address does not have its + * normal meaning, instead we use it as the relocation target address. We explicitly + * reset *offset to 0 so that the file will be placed wherever it fits in CBFS. + * + * 4. No --xip and no --base-address: this means that the FSP was pre-linked and should + * not be relocated. Just chain directly to convert_raw() for compression. */ + if (param.stage_xip) { - if (!IS_HOST_SPACE_ADDRESS(address)) - address = convert_addr_space(param.image_region, address); + if (!param.baseaddress_assigned) { + param.alignment = 4*1024; + if (do_cbfs_locate(offset, 0)) + return -1; + } + if (!IS_HOST_SPACE_ADDRESS(*offset)) + address = convert_addr_space(param.image_region, *offset); + else + address = *offset; } else { if (param.baseaddress_assigned == 0) { - INFO("Honoring pre-linked FSP module.\n"); - do_relocation = 0; + INFO("Honoring pre-linked FSP module, no relocation.\n"); + return cbfstool_convert_raw(buffer, offset, header); } else { address = param.baseaddress; - /* - * *offset should either be 0 or the value returned by - * do_cbfs_locate. do_cbfs_locate is called only when param.baseaddress - * is not provided by user. Thus, set *offset to 0 if user provides - * a baseaddress i.e. params.baseaddress_assigned is set. The only - * requirement in this case is that the binary should be relocated to - * the base address that is requested. There is no requirement on where - * the file ends up in the cbfs. - */ *offset = 0; } } - /* - * Nothing left to do if relocation is not being attempted. Just add - * the file. - */ - if (!do_relocation) - return cbfstool_convert_raw(buffer, offset, header); - /* Create a copy of the buffer to attempt relocation. */ if (buffer_create(&fsp, buffer_size(buffer), "fsp")) return -1; @@ -1100,15 +1126,12 @@ } /* - * If we already did a locate for alignment we need to locate again to - * take the stage header into account. XIP stage parsing also needs the - * location. But don't locate in other cases, because it will ignore - * compression (not applied yet) and thus may cause us to refuse adding - * stages that would actually fit once compressed. + * We need a final location for XIP parsing, so we need to call do_cbfs_locate() early + * here. That is okay because XIP stages may not be compressed, so their size cannot + * change anymore at a later point. */ - if ((param.alignment || param.stage_xip) && - do_cbfs_locate(offset, sizeof(struct cbfs_file_attr_stageheader), - data_size)) { + if (param.stage_xip && + do_cbfs_locate(offset, data_size)) { ERROR("Could not find location for stage.\n"); return 1; } @@ -1240,50 +1263,41 @@ { convert_buffer_t convert = cbfstool_convert_raw; - /* Set the alignment to 4KiB minimum for FSP blobs when no base address - * is provided so that relocation can occur. */ if (param.type == CBFS_TYPE_FSP) { - if (!param.baseaddress_assigned) - param.alignment = 4*1024; convert = cbfstool_convert_fsp; + } else if (param.type == CBFS_TYPE_STAGE) { + ERROR("stages can only be added with cbfstool add-stage\n"); + return 1; } else if (param.stage_xip) { - ERROR("cbfs add supports xip only for FSP component type\n"); + ERROR("cbfstool add supports xip only for FSP component type\n"); return 1; } return cbfs_add_component(param.filename, param.name, - param.type, param.headeroffset, convert); } static int cbfs_add_stage(void) { - if (param.stage_xip) { - if (param.baseaddress_assigned) { - ERROR("Cannot specify base address for XIP.\n"); - return 1; - } - - if (param.compression != CBFS_COMPRESS_NONE) { - ERROR("Cannot specify compression for XIP.\n"); - return 1; - } + if (param.stage_xip && param.baseaddress_assigned) { + ERROR("Cannot specify base address for XIP.\n"); + return 1; } + param.type = CBFS_TYPE_STAGE; return cbfs_add_component(param.filename, param.name, - CBFS_TYPE_STAGE, param.headeroffset, cbfstool_convert_mkstage); } static int cbfs_add_payload(void) { + param.type = CBFS_TYPE_SELF; return cbfs_add_component(param.filename, param.name, - CBFS_TYPE_SELF, param.headeroffset, cbfstool_convert_mkpayload); } @@ -1300,9 +1314,9 @@ "-e/--entry-point.\n"); return 1; } + param.type = CBFS_TYPE_SELF; return cbfs_add_component(param.filename, param.name, - CBFS_TYPE_SELF, param.headeroffset, cbfstool_convert_mkflatpayload); } -- To view, visit https://review.coreboot.org/c/coreboot/+/59877 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia49a585988f7a74944a6630b77b3ebd79b3a9897 Gerrit-Change-Number: 59877 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: util/testing: combine code coverage data
by Paul Fagerburg (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Patrick Georgi, Martin Roth, Paul Fagerburg. Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59959 ) Change subject: util/testing: combine code coverage data ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Turned out to be a lot simpler than I expected. I wanted to have one of the `coverage-report` targets gather all of the coverage for the sub-makes, but it turns out you can just `cat` all the LCOV files together. -- To view, visit https://review.coreboot.org/c/coreboot/+/59959 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id99615ca8279f80a402d5371221b8fd36fb91d55 Gerrit-Change-Number: 59959 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Comment-Date: Tue, 07 Dec 2021 16:57:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2021