coreboot-gerrit June 2023

coreboot-gerrit@coreboot.org

1 participants
5387 discussions

[S] Change in coreboot[master]: [WIP] acpi/acpigen: avoid truncating length in acpigen_resource_produ...
by Felix Held (Code Review) 03 Jun '23

03 Jun '23

Attention is currently required from: Lance Zhao, Nico Huber, Tim Wawrzynczak. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75613?usp=email ) Change subject: [WIP] acpi/acpigen: avoid truncating length in acpigen_resource_producer_io ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > I guess this could also be handled with DWordIO() or even QWordIO() (?!?) i've never seen this in any vendorcode i looked at. windows didn't dislike it, but linux disliked it; see the commit message of patchset 2. since this isn't needed for the rest of the patches, i'll mark this patch as WIP. i'm still undecided if the first or the second patchset should be the way to go; might also help if i ask one of our kernel engineers to look into this if he has some time for that -- To view, visit https://review.coreboot.org/c/coreboot/+/75613?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8a59cdfcfa30a8fdd13f8db3dc1447994c266c8b Gerrit-Change-Number: 75613 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Comment-Date: Sat, 03 Jun 2023 01:05:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: [WIP] acpi/acpigen: avoid truncating length in acpigen_resource_produ...
by Felix Held (Code Review) 03 Jun '23

03 Jun '23

Attention is currently required from: Felix Held, Lance Zhao, Nico Huber, Tim Wawrzynczak. Hello Lance Zhao, Nico Huber, Tim Wawrzynczak, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/75613?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review+1 by Nico Huber, Verified+1 by build bot (Jenkins) Change subject: [WIP] acpi/acpigen: avoid truncating length in acpigen_resource_producer_io ...................................................................... [WIP] acpi/acpigen: avoid truncating length in acpigen_resource_producer_io When an IO resource producer is generated that covers the whole IO space from 0 to 0xffff, the length field in the word resource ACPI type would overflow and be truncated which results in Linux not finding any usable IO space to use for the PCI IO BARs. Instead generate a double word IO resource producer in this case. TEST=Windows is ok with this while Linux complains a bit, but still ends up doing what it's expected to do in that case: acpi PNP0A08:00: host bridge window expanded to [io 0x0000-0xffff]; [io 0x0000-0xffff window] ignored Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I8a59cdfcfa30a8fdd13f8db3dc1447994c266c8b --- M src/acpi/acpigen.c 1 file changed, 25 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/75613/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/75613?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8a59cdfcfa30a8fdd13f8db3dc1447994c266c8b Gerrit-Change-Number: 75613 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in libgfxinit[master]: gma: Update DP training with new fields
by Tarun Tuli (Code Review) 03 Jun '23

03 Jun '23

Attention is currently required from: Angel Pons, Arthur Heymans, Jérémy Compostella, Nico Huber, Tim Wawrzynczak. Tarun Tuli has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/67490?usp=email ) Change subject: gma: Update DP training with new fields ...................................................................... Patch Set 18: (1 comment) File common/hw-gfx-dp_training.ads: https://review.coreboot.org/c/libgfxinit/+/67490/comment/552457cb_81a0c5b7 : PS8, Line 23: type T (<>) is limited private; > I was able to boil it down to the addition of `Port_Config.Is_eDP` and […] Hi Nico. I was working yesterday on a similar cleanup but like your version better. Lets get the patch train merged up to there and then apply yours to 67499. -- To view, visit https://review.coreboot.org/c/libgfxinit/+/67490?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: libgfxinit Gerrit-Branch: master Gerrit-Change-Id: I3f7f8a93408a74cfa29d1493a48c88ce73dc690c Gerrit-Change-Number: 67490 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-CC: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Sat, 03 Jun 2023 00:04:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: device/pci_rom: Add simple pci_rom_free()
by 9elements QA (Code Review) 03 Jun '23

03 Jun '23

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74778?usp=email ) Change subject: device/pci_rom: Add simple pci_rom_free() ...................................................................... Patch Set 15: Automatic boot test returned (PASS/FAIL/TOTAL): 15 / 1 / 16 PASS: x86_32 "Hermes CFL" , build config PRODRIVE_HERMES_ and payload TianoCore_UefiPayloadPkg : https://lava.9esec.io/r/166832 PASS: x86_32 "Hermes CFL" , build config PRODRIVE_HERMES and payload TianoCore_UefiPayloadPkg : https://lava.9esec.io/r/166830 PASS: x86_32 "ThinkPad T500" , build config LENOVO_T500 and payload SeaBIOS : https://lava.9esec.io/r/166829 PASS: x86_32 "HP Z220 SFF Workstation" , build config HP_Z220_SFF_WORKSTATION and payload LinuxBoot_BB_kexec : https://lava.9esec.io/r/166828 PASS: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload TianoCore : https://lava.9esec.io/r/166827 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/166826 PASS: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload TianoCore : https://lava.9esec.io/r/166825 PASS: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload SeaBIOS : https://lava.9esec.io/r/166824 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35_SMM_TSEG and payload TianoCore : https://lava.9esec.io/r/166823 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35_SMM_TSEG and payload SeaBIOS : https://lava.9esec.io/r/166822 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35 and payload TianoCore : https://lava.9esec.io/r/166821 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35 and payload SeaBIOS : https://lava.9esec.io/r/166820 PASS: x86_64 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_X86_64 and payload SeaBIOS : https://lava.9esec.io/r/166819 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ASAN and payload SeaBIOS : https://lava.9esec.io/r/166818 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ and payload SeaBIOS : https://lava.9esec.io/r/166817 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX and payload SeaBIOS : https://lava.9esec.io/r/166816 Please note: This test is under development and might not be accurate at all! -- To view, visit https://review.coreboot.org/c/coreboot/+/74778?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibc9aad34b6bf101a3a0c06b92ed2dc6f2d7b9b33 Gerrit-Change-Number: 74778 Gerrit-PatchSet: 15 Gerrit-Owner: Grzegorz Bernacki Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Konrad Adamczyk <konrada(a)google.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 23:07:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: Docs: Add help target and .phony identifiers
by Felix Singer (Code Review) 03 Jun '23

03 Jun '23

Felix Singer has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75496?usp=email ) Change subject: Docs: Add help target and .phony identifiers ...................................................................... Docs: Add help target and .phony identifiers This change adds a help target so we can see what the valid targets are and what they do, and also identifies the phony targets. Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: Ie61d44d87e8628e12d01f3355360e2a679b5ce85 Reviewed-on: https://review.coreboot.org/c/coreboot/+/75496 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> --- M Documentation/Makefile 1 file changed, 14 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved diff --git a/Documentation/Makefile b/Documentation/Makefile index 74e2bb8..12f0ec7 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -54,3 +54,17 @@ livesphinx: $(MAKE) -f Makefile.sphinx livehtml SPHINXOPTS="$(SPHINXOPTS)" + +help: + @echo "all - Builds coreboot porting guide PDF (outdated)" + @echo "sphinx - Builds html documentation in _build directory" + @echo "clean - Cleans intermediate files" + @echo "clean-sphinx - Removes sphinx output files" + @echo "distclean - Removes PDF files as well" + @echo + @echo " Makefile.sphinx builds - run with $(MAKE) -f Makefile-sphinx [target]" + @echo + @$(MAKE) -s -f Makefile.sphinx help 2>/dev/null + +.phony: help livesphinx sphinx test +.phony: distclean clean clean-sphinx -- To view, visit https://review.coreboot.org/c/coreboot/+/75496?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie61d44d87e8628e12d01f3355360e2a679b5ce85 Gerrit-Change-Number: 75496 Gerrit-PatchSet: 3 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in coreboot[master]: payloads/edk2: Set max variable size to 0x8000 for SMMSTORE
by Martin L Roth (Code Review) 03 Jun '23

03 Jun '23

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/74176?usp=email ) Change subject: payloads/edk2: Set max variable size to 0x8000 for SMMSTORE ...................................................................... payloads/edk2: Set max variable size to 0x8000 for SMMSTORE SMMSTORE requires 0x8000, compared to the default value of 0x10000. Signed-off-by: Sean Rhodes <sean(a)starlabs.systems> Change-Id: I10d2a5a0c1d729d83e0b62bb9430863317b883cc Reviewed-on: https://review.coreboot.org/c/coreboot/+/74176 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M payloads/external/edk2/Makefile 1 file changed, 4 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Matt DeVillier: Looks good to me, approved diff --git a/payloads/external/edk2/Makefile b/payloads/external/edk2/Makefile index e704093..c8d9c9f 100644 --- a/payloads/external/edk2/Makefile +++ b/payloads/external/edk2/Makefile @@ -71,6 +71,10 @@ ifeq ($(CONFIG_EDK2_FOLLOW_BGRT_SPEC),y) BUILD_STR += -D FOLLOW_BGRT_SPEC=TRUE endif +# MAX_VARIABLE_SIZE = 0x10000 +ifeq ($(CONFIG_SMMSTORE_V2),y) +BUILD_STR += --pcd gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize=0x8000 +endif # PCIE_BASE_ADDRESS = 0 ifneq ($(CONFIG_ECAM_MMCONF_LENGTH),) BUILD_STR += --pcd gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress=$(CONFIG_ECAM_MMCONF_BASE_ADDRESS) -- To view, visit https://review.coreboot.org/c/coreboot/+/74176?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I10d2a5a0c1d729d83e0b62bb9430863317b883cc Gerrit-Change-Number: 74176 Gerrit-PatchSet: 6 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: util/amdfwtool: Add ability to split hash table
by Karthik Ramasubramanian (Code Review) 03 Jun '23

03 Jun '23

Karthik Ramasubramanian has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75188?usp=email ) Change subject: util/amdfwtool: Add ability to split hash table ...................................................................... util/amdfwtool: Add ability to split hash table Hash table containing hashes of all signed PSP binaries is compiled at build time and installed into the concerned CBFS. During boot, PSP verstage reads the hash table binary and passes it to PSP bootloader. PSP bootloader in turn uses the hash table to verify the signed PSP binaries. Currently the hashes for all the signed PSP binaries are compiled into one hash table. On upcoming platforms with more number of signed PSP binaries, PSP bootloader does not have resources to handle one monolithic hash table. Instead PSP bootloader recommends splitting them into smaller hash tables (currently limited to 3 hash tables). Update amdfwtool tool to support splitting hash tables. This is done by adding an optional hash table id to the entries in the amdfw.cfg file. By default, one hash table binary is always compiled and it's name is of the format ${signed_rom}.hash. If an entry has a hash table id defined, then this utility will compile a separate hash table binary whose name is of the format ${signed_rom}.${N}.hash where N is the hash table id. BUG=b:277292697 TEST=Build Skyrim BIOS image and boot to OS. Ensure that the hash table is identical with and without this change. Perform suspend/resume cycles, warm/cold reset cycles for 50 iterations each. TEST=Artificially inject hash table id against some entries in amdfw.cfg and ensure that the concerned hash table binaries are getting compiled. Change-Id: I7ef338d67695a34c33b5c166924832939f381191 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/75188 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M util/amdfwtool/amdfwtool.h M util/amdfwtool/data_parse.c M util/amdfwtool/signed_psp.c 3 files changed, 128 insertions(+), 39 deletions(-) Approvals: Raul Rangel: Looks good to me, approved build bot (Jenkins): Verified diff --git a/util/amdfwtool/amdfwtool.h b/util/amdfwtool/amdfwtool.h index 7a2d24a..11b7996 100644 --- a/util/amdfwtool/amdfwtool.h +++ b/util/amdfwtool/amdfwtool.h @@ -356,6 +356,7 @@ /* Some files that don't have amd_fw_header have to be skipped from hashing. These files include but not limited to: *iKek*, *.tkn, *.stkn */ bool skip_hashing; + uint8_t hash_tbl_id; uint32_t num_hash_entries; amd_fw_entry_hash *hash_entries; bool generate_manifest; @@ -390,6 +391,9 @@ uint8_t reserved_80[128]; } __packed; +/* Based on the available PSP resources and increasing number of signed PSP binaries, + AMD recommends to split the hash table into 3 parts for now. */ +#define MAX_NUM_HASH_TABLES 3 struct psp_fw_hash_table { uint16_t version; uint16_t no_of_entries_256; diff --git a/util/amdfwtool/data_parse.c b/util/amdfwtool/data_parse.c index b94e46f..c2dc3e2 100644 --- a/util/amdfwtool/data_parse.c +++ b/util/amdfwtool/data_parse.c @@ -46,6 +46,19 @@ Lx1: Use default value for normal mode, level 1 for A/B mode */ "([[:space:]]+([Ll][12bxBX]{1,2}))?" + /* followed by an optional whitespace + chunk of nonwhitespace for hash table field + 1st char H: Indicator of field "Hash Table ID" + 2nd char: + Table ID to be dropped in. + 0: Table 0 / Default Unified Table + 1: Table 1 + ... + 9: Table 9 + + Examples: + H2: Put the hash for the concerned entry in Hash Table 2 + */ + "([[:space:]]+([Hh][0-9]+))?" /* followed by optional whitespace */ "[[:space:]]*$"; static regex_t entries_line_expr; @@ -55,6 +68,8 @@ FW_FILE, OPT_SPACE1, OPT_LEVEL, + OPT_SPACE2, + OPT_HASH_TABLE_ID, N_MATCHES, }; @@ -120,7 +135,7 @@ extern amd_bios_entry amd_bios_table[]; static uint8_t find_register_fw_filename_psp_dir(char *fw_name, char *filename, - char level_to_set, amd_cb_config *cb_config) + char level_to_set, uint8_t hash_tbl_id, amd_cb_config *cb_config) { amd_fw_type fw_type = AMD_FW_INVALID; amd_fw_entry *psp_tableptr; @@ -477,6 +492,7 @@ psp_tableptr->filename = filename; SET_LEVEL(psp_tableptr, level_to_set, PSP, cb_config->recovery_ab); + psp_tableptr->hash_tbl_id = hash_tbl_id; break; } psp_tableptr++; @@ -584,10 +600,12 @@ /* match[1]: FW type match[2]: FW filename match[4]: Optional directory level to be dropped + match[6]: Optional hash table ID to put the hash for the entry */ oneline[match[FW_TYPE].rm_eo] = '\0'; oneline[match[FW_FILE].rm_eo] = '\0'; oneline[match[OPT_LEVEL].rm_eo] = '\0'; + oneline[match[OPT_HASH_TABLE_ID].rm_eo] = '\0'; retval = 1; } else { retval = 0; @@ -611,11 +629,10 @@ return retval; } -char get_level_from_config(char *line, regoff_t level_index, amd_cb_config *cb_config) +static char get_level_from_config(char *line, regoff_t level_index, amd_cb_config *cb_config) { char lvl = 'x'; - /* If the optional level field is present, - extract the level char. */ + /* If the optional level field is present, extract the level char. */ if (level_index != -1) { if (cb_config->recovery_ab == 0) lvl = line[level_index + 1]; @@ -630,18 +647,29 @@ return lvl; } +static uint8_t get_hash_tbl_id(char *line, regoff_t hash_tbl_index) +{ + uint8_t tbl = 0; + /* If the optional hash table field is present, extract the table id char. */ + if (hash_tbl_index != -1) + tbl = (uint8_t)atoi(&line[hash_tbl_index + 1]); + + assert(tbl < MAX_NUM_HASH_TABLES); + return tbl; +} + static uint8_t process_one_line(char *oneline, regmatch_t *match, char *dir, amd_cb_config *cb_config) { char *path_filename, *fn = &(oneline[match[FW_FILE].rm_so]); char *fw_type_str = &(oneline[match[FW_TYPE].rm_so]); - char ch_lvl = 'x'; regoff_t ch_lvl_index = match[OPT_LEVEL].rm_so == match[OPT_LEVEL].rm_eo ? -1 : match[OPT_LEVEL].rm_so; - - /* If the optional level field is present, - extract the level char. */ - ch_lvl = get_level_from_config(oneline, ch_lvl_index, cb_config); + regoff_t ch_hash_tbl_index = + match[OPT_HASH_TABLE_ID].rm_so == match[OPT_HASH_TABLE_ID].rm_eo ? + -1 : match[OPT_HASH_TABLE_ID].rm_so; + char ch_lvl = get_level_from_config(oneline, ch_lvl_index, cb_config); + uint8_t ch_hash_tbl = get_hash_tbl_id(oneline, ch_hash_tbl_index); path_filename = malloc(MAX_LINE_SIZE * 2 + 2); if (strchr(fn, '/')) @@ -652,7 +680,7 @@ MAX_LINE_SIZE, dir, MAX_LINE_SIZE, fn); if (find_register_fw_filename_psp_dir( - fw_type_str, path_filename, ch_lvl, cb_config) == 0) { + fw_type_str, path_filename, ch_lvl, ch_hash_tbl, cb_config) == 0) { if (find_register_fw_filename_bios_dir( fw_type_str, path_filename, ch_lvl, cb_config) == 0) { fprintf(stderr, "Module's name \"%s\" is not valid\n", fw_type_str); diff --git a/util/amdfwtool/signed_psp.c b/util/amdfwtool/signed_psp.c index 644e904..2e19f72 100644 --- a/util/amdfwtool/signed_psp.c +++ b/util/amdfwtool/signed_psp.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <assert.h> #include <fcntl.h> #include <errno.h> #include <limits.h> @@ -21,7 +22,14 @@ SIG_ID_RSA2048, SIG_ID_RSA4096 = 2, }; + #define HASH_FILE_SUFFIX ".hash" +struct psp_fw_hash_file_info { + int fd; + bool present; + struct psp_fw_hash_table hash_header; +}; +static struct psp_fw_hash_file_info hash_files[MAX_NUM_HASH_TABLES]; static uint16_t get_psp_fw_type(enum platform soc_id, struct amd_fw_header *header) { @@ -126,26 +134,78 @@ write_or_fail(fd, entry->sha, entry->sha_len); } -static void write_psp_firmware_hash(const char *filename, - amd_fw_entry *fw_table) +static void open_psp_fw_hash_files(const char *file_prefix) { - struct psp_fw_hash_table hash_header = {0}; - int fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666); + size_t hash_file_strlen; + char *hash_file_name; - if (fd < 0) { - fprintf(stderr, "Error opening file: %s: %s\n", - filename, strerror(errno)); + /* Hash Table ID is part of the file name. For now only single digit ID is + supported and is sufficient. Hence assert MAX_NUM_HASH_TABLES < 10 before + constructing file name. Revisit later when > 10 hash tables are required. */ + assert(MAX_NUM_HASH_TABLES < 10); + /* file_prefix + ".[1-9]" + ".hash" + '\0' */ + hash_file_strlen = strlen(file_prefix) + 2 + strlen(HASH_FILE_SUFFIX) + 1; + hash_file_name = malloc(hash_file_strlen); + if (!hash_file_name) { + fprintf(stderr, "malloc(%lu) failed\n", hash_file_strlen); exit(-1); } - hash_header.version = HASH_HDR_V1; + for (unsigned int i = 0; i < MAX_NUM_HASH_TABLES; i++) { + /* Hash table IDs are expected to be contiguous and hence holes are not + expected. */ + if (!hash_files[i].present) + break; + + if (i) + snprintf(hash_file_name, hash_file_strlen, "%s.%d%s", + file_prefix, i, HASH_FILE_SUFFIX); + else + /* Default file name without number for backwards compatibility. */ + snprintf(hash_file_name, hash_file_strlen, "%s%s", + file_prefix, HASH_FILE_SUFFIX); + + hash_files[i].fd = open(hash_file_name, O_RDWR | O_CREAT | O_TRUNC, 0666); + if (hash_files[i].fd < 0) { + fprintf(stderr, "Error opening file: %s: %s\n", + hash_file_name, strerror(errno)); + free(hash_file_name); + exit(-1); + } + } + free(hash_file_name); +} + +static void close_psp_fw_hash_files(void) +{ + for (unsigned int i = 0; i < MAX_NUM_HASH_TABLES; i++) { + if (!hash_files[i].present) + break; + + close(hash_files[i].fd); + } +} + +static void write_psp_firmware_hash(amd_fw_entry *fw_table) +{ + uint8_t hash_tbl_id; + + for (unsigned int i = 0; i < MAX_NUM_HASH_TABLES; i++) { + if (!hash_files[i].present) + continue; + hash_files[i].hash_header.version = HASH_HDR_V1; + } + for (unsigned int i = 0; fw_table[i].type != AMD_FW_INVALID; i++) { + hash_tbl_id = fw_table[i].hash_tbl_id; + assert(hash_files[hash_tbl_id].present); + for (unsigned int j = 0; j < fw_table[i].num_hash_entries; j++) { if (fw_table[i].hash_entries[j].sha_len == SHA256_DIGEST_LENGTH) { - hash_header.no_of_entries_256++; + hash_files[hash_tbl_id].hash_header.no_of_entries_256++; } else if (fw_table[i].hash_entries[j].sha_len == SHA384_DIGEST_LENGTH) { - hash_header.no_of_entries_384++; + hash_files[hash_tbl_id].hash_header.no_of_entries_384++; } else if (fw_table[i].hash_entries[j].sha_len) { fprintf(stderr, "%s: Error invalid sha_len %d\n", __func__, fw_table[i].hash_entries[j].sha_len); @@ -154,28 +214,34 @@ } } - write_or_fail(fd, &hash_header, sizeof(hash_header)); + for (unsigned int i = 0; i < MAX_NUM_HASH_TABLES; i++) { + if (!hash_files[i].present) + continue; + write_or_fail(hash_files[i].fd, &hash_files[i].hash_header, + sizeof(hash_files[i].hash_header)); + } /* Add all the SHA256 hash entries first followed by SHA384 entries. PSP verstage processes the table in that order. Mixing and matching SHA256 and SHA384 entries will cause the hash verification failure at run-time. */ for (unsigned int i = 0; fw_table[i].type != AMD_FW_INVALID; i++) { + hash_tbl_id = fw_table[i].hash_tbl_id; for (unsigned int j = 0; j < fw_table[i].num_hash_entries; j++) { if (fw_table[i].hash_entries[j].sha_len == SHA256_DIGEST_LENGTH) - write_one_psp_firmware_hash_entry(fd, - &fw_table[i].hash_entries[j]); + write_one_psp_firmware_hash_entry(hash_files[hash_tbl_id].fd, + &fw_table[i].hash_entries[j]); } } for (unsigned int i = 0; fw_table[i].type != AMD_FW_INVALID; i++) { + hash_tbl_id = fw_table[i].hash_tbl_id; for (unsigned int j = 0; j < fw_table[i].num_hash_entries; j++) { if (fw_table[i].hash_entries[j].sha_len == SHA384_DIGEST_LENGTH) - write_one_psp_firmware_hash_entry(fd, - &fw_table[i].hash_entries[j]); + write_one_psp_firmware_hash_entry(hash_files[hash_tbl_id].fd, + &fw_table[i].hash_entries[j]); } } - close(fd); for (unsigned int i = 0; fw_table[i].type != AMD_FW_INVALID; i++) { if (!fw_table[i].num_hash_entries || !fw_table[i].hash_entries) continue; @@ -204,8 +270,6 @@ int signed_rom_fd; ssize_t bytes, align_bytes; uint8_t *buf; - char *signed_rom_hash; - size_t signed_rom_hash_strlen; struct amd_fw_header header; struct stat fd_stat; /* Every blob in amdfw*.rom has to start at address aligned to 0x100. Prepare an @@ -323,6 +387,7 @@ fw_table[i].fw_id = get_psp_fw_type(soc_id, &header); fw_table[i].addr_signed = signed_start_addr; fw_table[i].file_size = (uint32_t)fd_stat.st_size; + hash_files[fw_table[i].hash_tbl_id].present = true; signed_start_addr += fd_stat.st_size + align_bytes; @@ -332,15 +397,7 @@ close(signed_rom_fd); - /* signed_rom file name + ".hash" + '\0' */ - signed_rom_hash_strlen = strlen(signed_rom) + strlen(HASH_FILE_SUFFIX) + 1; - signed_rom_hash = malloc(signed_rom_hash_strlen); - if (!signed_rom_hash) { - fprintf(stderr, "malloc(%lu) failed\n", signed_rom_hash_strlen); - exit(-1); - } - strcpy(signed_rom_hash, signed_rom); - strcat(signed_rom_hash, HASH_FILE_SUFFIX); - write_psp_firmware_hash(signed_rom_hash, fw_table); - free(signed_rom_hash); + open_psp_fw_hash_files(signed_rom); + write_psp_firmware_hash(fw_table); + close_psp_fw_hash_files(); } -- To view, visit https://review.coreboot.org/c/coreboot/+/75188?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7ef338d67695a34c33b5c166924832939f381191 Gerrit-Change-Number: 75188 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: soc/amd/common/data_fabric/domain: provide amd_pci_domain_fill_ssdt
by Felix Held (Code Review) 03 Jun '23

03 Jun '23

Attention is currently required from: Arthur Heymans, Fred Reitberger, Jason Glenesk, Matt DeVillier, Nico Huber, Raul Rangel. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74843?usp=email ) Change subject: soc/amd/common/data_fabric/domain: provide amd_pci_domain_fill_ssdt ...................................................................... Patch Set 13: (1 comment) File src/soc/amd/common/block/data_fabric/domain.c: https://review.coreboot.org/c/coreboot/+/74843/comment/ec8bed40_3f8ceee9 : PS11, Line 169: if (base > PCI_IO_CONFIG_LAST_PORT || limit < PCI_IO_CONFIG_INDEX) { > It seems more like our code is too limited wrt. 16-bit length. So the […] read that part of the spec and it looks like a dword io producer resource should just work. i'll give this a try -- To view, visit https://review.coreboot.org/c/coreboot/+/74843?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaf6d38a8ef5bb0163c4d1c021bf892c323d9a448 Gerrit-Change-Number: 74843 Gerrit-PatchSet: 13 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 22:19:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: Docs: Add sphinx to the documentation's "all" target
by Martin L Roth (Code Review) 03 Jun '23

03 Jun '23

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75498?usp=email ) Change subject: Docs: Add sphinx to the documentation's "all" target ...................................................................... Docs: Add sphinx to the documentation's "all" target The all target really should build the expected output. At this point, that means sphinx as well. Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: Ibbceddd013605f5f7e11707480ae2a1e5d012a91 Reviewed-on: https://review.coreboot.org/c/coreboot/+/75498 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Elyes Haouas <ehaouas(a)noos.fr> Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> --- M Documentation/Makefile 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Elyes Haouas: Looks good to me, approved Felix Singer: Looks good to me, but someone else must approve diff --git a/Documentation/Makefile b/Documentation/Makefile index 8552c6d..74e2bb8 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -8,7 +8,7 @@ FIGS=codeflow.pdf hypertransport.pdf -all: corebootPortingGuide.pdf +all: sphinx corebootPortingGuide.pdf SVG2PDF=$(shell command -v svg2pdf) INKSCAPE=$(shell command -v inkscape) -- To view, visit https://review.coreboot.org/c/coreboot/+/75498?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibbceddd013605f5f7e11707480ae2a1e5d012a91 Gerrit-Change-Number: 75498 Gerrit-PatchSet: 2 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in coreboot[master]: Docs: Add sphinx to the documentation's "all" target
by Martin L Roth (Code Review) 03 Jun '23

03 Jun '23

Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75498?usp=email ) Change subject: Docs: Add sphinx to the documentation's "all" target ...................................................................... Patch Set 1: (1 comment) File Documentation/Makefile: https://review.coreboot.org/c/coreboot/+/75498/comment/309e4f34_5d41858b : PS1, Line 11: corebootPortingGuide > [out of scope] this document needs an update Agreed - it's way out of date. Should we remove it or turn it into markdown? -- To view, visit https://review.coreboot.org/c/coreboot/+/75498?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibbceddd013605f5f7e11707480ae2a1e5d012a91 Gerrit-Change-Number: 75498 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 02 Jun 2023 22:16:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2023