coreboot-gerrit December 2021

coreboot-gerrit@coreboot.org

1 participants
5525 discussions

Change in coreboot[master]: mb/google/dedede/var/drawcia: Generate new SPD ID for new memory parts
by 9elements QA (Code Review) 04 Dec '21

04 Dec '21

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59642 ) Change subject: mb/google/dedede/var/drawcia: Generate new SPD ID for new memory parts ...................................................................... Patch Set 6: Automatic boot test returned (PASS/FAIL/TOTAL): 5 / 4 / 9 FAIL: x86_32 "ThinkPad T500" , build config LENOVO_T500 and payload SeaBIOS : https://lava.9esec.io/r/83345 FAIL: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload SeaBIOS : https://lava.9esec.io/r/83343 FAIL: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload SeaBIOS : https://lava.9esec.io/r/83342 FAIL: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35_SMM_TSEG and payload SeaBIOS : https://lava.9esec.io/r/83341 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35 and payload SeaBIOS : https://lava.9esec.io/r/83340 PASS: x86_64 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_X86_64 and payload SeaBIOS : https://lava.9esec.io/r/83339 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ASAN and payload SeaBIOS : https://lava.9esec.io/r/83338 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ and payload SeaBIOS : https://lava.9esec.io/r/83337 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX and payload SeaBIOS : https://lava.9esec.io/r/83336 Please note: This test is under development and might not be accurate at all! -- To view, visit https://review.coreboot.org/c/coreboot/+/59642 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb0f211508450b16b2e5d214ae6adc9852718a59 Gerrit-Change-Number: 59642 Gerrit-PatchSet: 6 Gerrit-Owner: Robert Chen <robert.chen(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Wisley Chen <wisley.chen(a)quanta.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Kevin Chiu <kevin.chiu.17802(a)gmail.com> Gerrit-Comment-Date: Sat, 04 Dec 2021 03:19:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/felwinter: Correct garage wake event
by EricR Lai (Code Review) 04 Dec '21

04 Dec '21

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59861 ) Change subject: mb/google/brya/var/felwinter: Correct garage wake event ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59861/comment/df1aee95_e7cfa015 PS1, Line 9: Eject event is high. Set wake event to active high. > suggestion, add something like: […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/59861 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I568e9175c7a66599f7a525c32e4def7a79b55a0a Gerrit-Change-Number: 59861 Gerrit-PatchSet: 2 Gerrit-Owner: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: John Su <john_su(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Sat, 04 Dec 2021 02:24:29 +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]: mb/google/brya/var/felwinter: Correct garage wake event
by EricR Lai (Code Review) 04 Dec '21

04 Dec '21

Attention is currently required from: EricR Lai. Hello build bot (Jenkins), Tim Wawrzynczak, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59861 to look at the new patch set (#2). Change subject: mb/google/brya/var/felwinter: Correct garage wake event ...................................................................... mb/google/brya/var/felwinter: Correct garage wake event Eject event is high. Set wake event to active high. The polarity of the SCI and the wakeup_event_action for the pen ejection feature were both backwards, and was causing the system to fail to enter sleep states because the event was always asserted. BUG=b:208937710 TEST=only release switch can wake system. Signed-off-by: Eric Lai <ericr_lai(a)compal.corp-partner.google.com> Change-Id: I568e9175c7a66599f7a525c32e4def7a79b55a0a --- M src/mainboard/google/brya/variants/felwinter/gpio.c M src/mainboard/google/brya/variants/felwinter/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/59861/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59861 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I568e9175c7a66599f7a525c32e4def7a79b55a0a Gerrit-Change-Number: 59861 Gerrit-PatchSet: 2 Gerrit-Owner: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: John Su <john_su(a)compal.corp-partner.google.com> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: libpayload: Add CBMEM_IMD_ENTRY support to coreboot tables parser
by 9elements QA (Code Review) 04 Dec '21

04 Dec '21

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59491 ) Change subject: libpayload: Add CBMEM_IMD_ENTRY support to coreboot tables parser ...................................................................... Patch Set 6: Automatic boot test returned (PASS/FAIL/TOTAL): 4 / 4 / 8 FAIL: x86_32 "ThinkPad T500" , build config LENOVO_T500 and payload SeaBIOS : https://lava.9esec.io/r/83334 FAIL: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload SeaBIOS : https://lava.9esec.io/r/83332 FAIL: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload SeaBIOS : https://lava.9esec.io/r/83331 FAIL: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35_SMM_TSEG and payload SeaBIOS : https://lava.9esec.io/r/83330 PASS: x86_64 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_X86_64 and payload SeaBIOS : https://lava.9esec.io/r/83328 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ASAN and payload SeaBIOS : https://lava.9esec.io/r/83327 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ and payload SeaBIOS : https://lava.9esec.io/r/83326 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX and payload SeaBIOS : https://lava.9esec.io/r/83325 Please note: This test is under development and might not be accurate at all! -- To view, visit https://review.coreboot.org/c/coreboot/+/59491 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5bd02a98ba2631f34014bc0f8e7ebd5a5ddd2321 Gerrit-Change-Number: 59491 Gerrit-PatchSet: 6 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-Comment-Date: Sat, 04 Dec 2021 01:42:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: region: Rename rdev_readat_full to rdev_read_full
by 9elements QA (Code Review) 03 Dec '21

03 Dec '21

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59820 ) Change subject: region: Rename rdev_readat_full to rdev_read_full ...................................................................... Patch Set 2: Automatic boot test returned (PASS/FAIL/TOTAL): 5 / 3 / 8 FAIL: x86_32 "ThinkPad T500" , build config LENOVO_T500 and payload SeaBIOS : https://lava.9esec.io/r/83323 FAIL: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload SeaBIOS : https://lava.9esec.io/r/83321 FAIL: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload SeaBIOS : https://lava.9esec.io/r/83320 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35 and payload SeaBIOS : https://lava.9esec.io/r/83318 PASS: x86_64 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_X86_64 and payload SeaBIOS : https://lava.9esec.io/r/83317 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ASAN and payload SeaBIOS : https://lava.9esec.io/r/83316 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ and payload SeaBIOS : https://lava.9esec.io/r/83315 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX and payload SeaBIOS : https://lava.9esec.io/r/83314 Please note: This test is under development and might not be accurate at all! -- To view, visit https://review.coreboot.org/c/coreboot/+/59820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60d595f0cbd161df171eaa4a76c7a00b6377e2b0 Gerrit-Change-Number: 59820 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 03 Dec 2021 23:58:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

03 Dec '21

Attention is currently required from: Raul Rangel, Patrick Georgi, Tim Wawrzynczak. Hello Raul Rangel, Patrick Georgi, Tim Wawrzynczak, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59877 to look at the new patch set (#2). 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 --- M util/cbfstool/cbfstool.c 1 file changed, 82 insertions(+), 68 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/59877/2 -- 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: 2 Gerrit-Owner: 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)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-MessageType: newpatchset

1 0

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

03 Dec '21

Attention is currently required from: Raul Rangel, Patrick Georgi, Tim Wawrzynczak. Hello Raul Rangel, Patrick Georgi, Tim Wawrzynczak, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/59877 to review the following change. 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 --- M util/cbfstool/cbfstool.c 1 file changed, 82 insertions(+), 67 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/59877/1 diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index c9db45a..27a6952 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,42 @@ { 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 +1315,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: 1 Gerrit-Owner: 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)google.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: [WIP] drivers/intel/fsp2_0: Add support for FSP_NON_VOLATILE_STORAGE_...
by build bot (Jenkins) (Code Review) 03 Dec '21

03 Dec '21

Attention is currently required from: Tim Wawrzynczak, Paul Menzel, Andrey Petrov, Patrick Rudolph. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59638 ) Change subject: [WIP] drivers/intel/fsp2_0: Add support for FSP_NON_VOLATILE_STORAGE_HOB2 ...................................................................... Patch Set 18: (2 comments) File src/drivers/intel/fsp2_0/hand_off_block.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-134932): https://review.coreboot.org/c/coreboot/+/59638/comment/7bdac82e_c297fbde PS18, Line 327: } trailing whitespace Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-134932): https://review.coreboot.org/c/coreboot/+/59638/comment/26d63952_0a771f68 PS18, Line 329: trailing whitespace -- To view, visit https://review.coreboot.org/c/coreboot/+/59638 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I27647e9ac1a4902256b3f1c34b60e1f0b787a06e Gerrit-Change-Number: 59638 Gerrit-PatchSet: 18 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 03 Dec 2021 23:12:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP] drivers/intel/fsp2_0: Add support for FSP_NON_VOLATILE_STORAGE_...
by Anil Kumar K (Code Review) 03 Dec '21

03 Dec '21

Attention is currently required from: Tim Wawrzynczak, Paul Menzel, Andrey Petrov, Patrick Rudolph. Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59638 ) Change subject: [WIP] drivers/intel/fsp2_0: Add support for FSP_NON_VOLATILE_STORAGE_HOB2 ...................................................................... Patch Set 17: (2 comments) File src/drivers/intel/fsp2_0/hand_off_block.c: https://review.coreboot.org/c/coreboot/+/59638/comment/02ad926e_4559fc30 PS17, Line 326: } > BTW, this won't compile right now if you enable the Kconfig, because in this path, if hob_walker. […] Ack https://review.coreboot.org/c/coreboot/+/59638/comment/17c9cdb2_8ee57f27 PS17, Line 316: if (CONFIG(PLATFORM_USES_FSP2_3)) { : union { : const struct fsp_nvs_hob2_data_region_header *hob; : const void *hob_descr; : } hob_walker; : hob_walker.hob = (const struct fsp_nvs_hob2_data_region_header *) : fsp_find_extension_hob_by_guid(fsp_nv_storage_guid_2, size); : if (hob_walker.hob != NULL) { : *size = hob_walker.hob->nvs_data_length; : return hob_walker.hob_descr; : } : } else { : return fsp_find_extension_hob_by_guid(fsp_nv_storage_guid, size); : } > I might return early just to avoid an extra indent, e.g.: […] Hi Tim. There could be potential cases of platforms adopting newer header revisions but still using the older HOBs.To take care of this the recomendation from FSP architecture doc is to search for HOB2 and if not found search for HOB1 . I updated my code by removing the else -- To view, visit https://review.coreboot.org/c/coreboot/+/59638 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I27647e9ac1a4902256b3f1c34b60e1f0b787a06e Gerrit-Change-Number: 59638 Gerrit-PatchSet: 17 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 03 Dec 2021 23:12:01 +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]: mb/acer/g43t-am3: Add documentation
by Michael Büchler (Code Review) 03 Dec '21

03 Dec '21

Attention is currently required from: Felix Singer, Paul Menzel, Angel Pons. Michael Büchler has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56838 ) Change subject: mb/acer/g43t-am3: Add documentation ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Can this be submitted? From my side yes, I checked this off as "ready" (in my head). -- To view, visit https://review.coreboot.org/c/coreboot/+/56838 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0e296b3efbff0260f32badc699f1062f9885fa53 Gerrit-Change-Number: 56838 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Büchler <michael.buechler(a)posteo.net> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 03 Dec 2021 22:48:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2021