coreboot-gerrit October 2022

coreboot-gerrit@coreboot.org

1 participants
5881 discussions

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by Maximilian Brune (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Angel Pons, Arthur Heymans. Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... Patch Set 1: (1 comment) File util/cbfstool/cbfstool.c: https://review.coreboot.org/c/coreboot/+/68160/comment/27a0bb90_ef092f81 PS1, Line 335: static int decode_mmap_arg(char *optarg) : { : if (optarg == NULL) : return 1; : : unsigned long flash_base; : unsigned long mmap_base; : unsigned long region_size; : char *suffix = NULL; : char *substring = strtok(optarg, ":"); : if (substring) : flash_base = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid flash base address '%s'.\n", : optarg); : return 1; : } : substring = strtok(NULL, ":"); : if (substring) : mmap_base = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid mmap base address '%s'.\n", : optarg); : return 1; : } : substring = strtok(NULL, ":"); : if (substring) : region_size = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid flash region size '%s'.\n", : optarg); : return 1; : } : : if (strtok(NULL, ":") != NULL) { : ERROR("Invalid argument, too many substrings '%s'.\n", : optarg); : : return 1; : } : : add_mmap_window(flash_base, mmap_base, region_size); : return 0; > It doesn't look *too* bad, but please try introducing a helper function that gets called three times […] In this case it doesn't seem like a helper function would actually reduce any code here. But I would recommend to remove the "if (substring)" and just place the "flash_base = ..." after the error checking -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:36:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by build bot (Jenkins) (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Arthur Heymans. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... Patch Set 3: (5 comments) File util/cbfstool/cbfstool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159802): https://review.coreboot.org/c/coreboot/+/68160/comment/65aa434c_0143b876 PS3, Line 393: * Default decode window lives just below 4G boundary in host space and maps up to a line length of 100 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159802): https://review.coreboot.org/c/coreboot/+/68160/comment/c402a065_6e8731ec PS3, Line 394: * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped line length of 104 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159802): https://review.coreboot.org/c/coreboot/+/68160/comment/fa58adcd_d585c7d2 PS3, Line 397: add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); line length of 119 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159802): https://review.coreboot.org/c/coreboot/+/68160/comment/b066d372_48ccf1c8 PS3, Line 406: ERROR("Flash space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", line length of 135 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159802): https://review.coreboot.org/c/coreboot/+/68160/comment/a02d025d_4d0ec22d PS3, Line 416: ERROR("Host space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", line length of 134 exceeds 96 columns -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:29:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by Arthur Heymans (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/68160 to look at the new patch set (#3). Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... util/cbfstool: Add a new mechanism to provide a memory mapped This replaces the mechanism with --ext-win-base --ext-win-size with a more generic mechanism where cbfstool can be provided with an arbitrary memory map. This will be useful for AMD platforms with flash sizes larger than 16M where only the lower 16M half gets memory mapped below 4G. Also on Intel system the IFD allows for a memory map where the "top of flash" != "below 4G". This is for instance the case by default on Intel APL. TEST: google/brya build for chromeos which used --ext-win-base remains the same after this change with BUILD_TIMELESS=1. Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/soc/intel/common/block/fast_spi/Makefile.inc M util/cbfstool/cbfstool.c 2 files changed, 139 insertions(+), 77 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/68160/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by build bot (Jenkins) (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Arthur Heymans. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... Patch Set 2: (5 comments) File util/cbfstool/cbfstool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159801): https://review.coreboot.org/c/coreboot/+/68160/comment/dd9e9723_604de611 PS2, Line 393: * Default decode window lives just below 4G boundary in host space and maps up to a line length of 100 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159801): https://review.coreboot.org/c/coreboot/+/68160/comment/7ad449cc_8c044ac2 PS2, Line 394: * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped line length of 104 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159801): https://review.coreboot.org/c/coreboot/+/68160/comment/c205d861_f09840b7 PS2, Line 397: add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); line length of 119 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159801): https://review.coreboot.org/c/coreboot/+/68160/comment/6957b103_7e7d5674 PS2, Line 406: ERROR("Flash space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", line length of 135 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159801): https://review.coreboot.org/c/coreboot/+/68160/comment/a7284715_9698f923 PS2, Line 416: ERROR("Host space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", line length of 134 exceeds 96 columns -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:28:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by Arthur Heymans (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/68160 to look at the new patch set (#2). Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... util/cbfstool: Add a new mechanism to provide a memory mapped This replaces the mechanism with --ext-win-base --ext-win-size with a more generic mechanism where cbfstool can be provided with an arbitrary memory map. This will be useful for AMD platforms with flash sizes larger than 16M where only the lower 16M half gets memory mapped below 4G. Also on Intel system the IFD allows for a memory map where the "top of flash" != "below 4G". This is for instance the case by default on Intel APL. TEST: google/brya build for chromeos which used --ext-win-base remains the same after this change with BUILD_TIMELESS=1. Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/soc/intel/common/block/fast_spi/Makefile.inc M util/cbfstool/cbfstool.c 2 files changed, 139 insertions(+), 77 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/68160/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newpatchset

1 0

[L] Change in libgfxinit[master]: common: Begin Tiger Lake integration
by Angel Pons (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Felix Singer, Sean Rhodes, Nico Huber, Tim Wawrzynczak, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/65087 ) Change subject: common: Begin Tiger Lake integration ...................................................................... Patch Set 9: Code-Review+1 (1 comment) Patchset: PS9: > Yeah nothing yet, that does look like a really cool device, I imagine they will be in demand and har […] Get a Clevo laptop and port it? 😄 -- To view, visit https://review.coreboot.org/c/libgfxinit/+/65087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: libgfxinit Gerrit-Branch: master Gerrit-Change-Id: Ic8d32598e5611453ce53f1deaeab981b9f2a952f Gerrit-Change-Number: 65087 Gerrit-PatchSet: 9 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:13:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by Angel Pons (Code Review) 06 Oct '22

06 Oct '22

Attention is currently required from: Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... Patch Set 1: (2 comments) File util/cbfstool/cbfstool.c: https://review.coreboot.org/c/coreboot/+/68160/comment/2ae7fab3_63c69a7b PS1, Line 313: // Should be enough for now Please be consistent with the existing C-style comments. https://review.coreboot.org/c/coreboot/+/68160/comment/7c0535d6_7ce0ec9d PS1, Line 335: static int decode_mmap_arg(char *optarg) : { : if (optarg == NULL) : return 1; : : unsigned long flash_base; : unsigned long mmap_base; : unsigned long region_size; : char *suffix = NULL; : char *substring = strtok(optarg, ":"); : if (substring) : flash_base = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid flash base address '%s'.\n", : optarg); : return 1; : } : substring = strtok(NULL, ":"); : if (substring) : mmap_base = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid mmap base address '%s'.\n", : optarg); : return 1; : } : substring = strtok(NULL, ":"); : if (substring) : region_size = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid flash region size '%s'.\n", : optarg); : return 1; : } : : if (strtok(NULL, ":") != NULL) { : ERROR("Invalid argument, too many substrings '%s'.\n", : optarg); : : return 1; : } : : add_mmap_window(flash_base, mmap_base, region_size); : return 0; > This looks bad. […] It doesn't look *too* bad, but please try introducing a helper function that gets called three times to read the three numbers. -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:10:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by Arthur Heymans (Code Review) 06 Oct '22

06 Oct '22

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... Patch Set 1: (1 comment) File util/cbfstool/cbfstool.c: https://review.coreboot.org/c/coreboot/+/68160/comment/67d2bcec_1aec5d1e PS1, Line 335: static int decode_mmap_arg(char *optarg) : { : if (optarg == NULL) : return 1; : : unsigned long flash_base; : unsigned long mmap_base; : unsigned long region_size; : char *suffix = NULL; : char *substring = strtok(optarg, ":"); : if (substring) : flash_base = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid flash base address '%s'.\n", : optarg); : return 1; : } : substring = strtok(NULL, ":"); : if (substring) : mmap_base = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid mmap base address '%s'.\n", : optarg); : return 1; : } : substring = strtok(NULL, ":"); : if (substring) : region_size = strtol(substring, &suffix, 0); : if (!substring || (suffix && *suffix)) { : ERROR("Invalid flash region size '%s'.\n", : optarg); : return 1; : } : : if (strtok(NULL, ":") != NULL) { : ERROR("Invalid argument, too many substrings '%s'.\n", : optarg); : : return 1; : } : : add_mmap_window(flash_base, mmap_base, region_size); : return 0; This looks bad. Any suggestions? I'm really not used to writing userspace programs ^^ -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:06:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by build bot (Jenkins) (Code Review) 06 Oct '22

06 Oct '22

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... Patch Set 1: (5 comments) File util/cbfstool/cbfstool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159800): https://review.coreboot.org/c/coreboot/+/68160/comment/db352115_b457b039 PS1, Line 398: * Default decode window lives just below 4G boundary in host space and maps up to a line length of 100 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159800): https://review.coreboot.org/c/coreboot/+/68160/comment/c512ff9c_d5a513c6 PS1, Line 399: * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped line length of 104 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159800): https://review.coreboot.org/c/coreboot/+/68160/comment/2637b603_07b21656 PS1, Line 402: add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); line length of 119 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159800): https://review.coreboot.org/c/coreboot/+/68160/comment/e68a291b_93e71643 PS1, Line 411: ERROR("Flash space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", line length of 135 exceeds 96 columns Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159800): https://review.coreboot.org/c/coreboot/+/68160/comment/498ea8f1_974ab9bc PS1, Line 421: ERROR("Host space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", line length of 134 exceeds 96 columns -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 06 Oct 2022 08:04:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/cbfstool: Add a new mechanism to provide a memory mapped
by Arthur Heymans (Code Review) 06 Oct '22

06 Oct '22

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/68160 ) Change subject: util/cbfstool: Add a new mechanism to provide a memory mapped ...................................................................... util/cbfstool: Add a new mechanism to provide a memory mapped This replaces the mechanism with --ext-win-base --ext-win-size with a more generic mechanism where cbfstool can be provided with an arbitrary memory map. This will be useful for AMD platforms with flash sizes larger than 16M where only the lower 16M half gets memory mapped below 4G. Also on Intel system the IFD allows for a memory map where the "top of flash" != "below 4G". This is for instance the case by default on Intel APL. TEST: google/brya build for chromeos which used --ext-win-base remains the same after this change with BUILD_TIMELESS=1. Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/soc/intel/common/block/fast_spi/Makefile.inc M util/cbfstool/cbfstool.c 2 files changed, 144 insertions(+), 77 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/68160/1 diff --git a/src/soc/intel/common/block/fast_spi/Makefile.inc b/src/soc/intel/common/block/fast_spi/Makefile.inc index 62c744f..40655fd 100644 --- a/src/soc/intel/common/block/fast_spi/Makefile.inc +++ b/src/soc/intel/common/block/fast_spi/Makefile.inc @@ -66,6 +66,32 @@ done; \ exit $$fail -CBFSTOOL_ADD_CMD_OPTIONS += --ext-win-base $(CONFIG_EXT_BIOS_WIN_BASE) --ext-win-size $(CONFIG_EXT_BIOS_WIN_SIZE) +# If the platform supports extended window and the SPI flash size is greater +# than 16MiB, then create a mapping for the extended window as well. +# The assumptions here are: +# 1. Top 16MiB is still decoded in the fixed decode window just below 4G +# boundary. +# 2. Rest of the SPI flash below the top 16MiB is mapped at the top of extended +# window. Even though the platform might support a larger extended window, the +# SPI flash part used by the mainboard might not be large enough to be mapped +# in the entire window. In such cases, the mapping is assumed to be in the top +# part of the extended window with the bottom part remaining unused. +# +# Example: +# ext_win_base = 0xF8000000 +# ext_win_size = 32 # MiB +# ext_win_limit = ext_win_base + ext_win_size - 1 = 0xF9FFFFFF +# +# If SPI flash is 32MiB, then top 16MiB is mapped from 0xFF000000 - 0xFFFFFFFF +# whereas the bottom 16MiB is mapped from 0xF9000000 - 0xF9FFFFFF. The extended +# window 0xF8000000 - 0xF8FFFFFF remains unused. +# + +ifeq ($(call int-gt, $(CONFIG_ROM_SIZE) 0x1000000), 1) +CBFSTOOL_ADD_CMD_OPTIONS += --mmap $(call int-subtract, $(CONFIG_ROM_SIZE) 0x1000000):0xff000000:0x1000000 +CBFSTOOL_ADD_CMD_OPTIONS += --mmap 0:$(call int-subtract, $(call int-add, $(CONFIG_EXT_BIOS_WIN_BASE) $(CONFIG_EXT_BIOS_WIN_SIZE)) \ + $(call int-subtract, $(CONFIG_ROM_SIZE) 0x1000000)):$(call int-subtract, $(CONFIG_ROM_SIZE) 0x1000000) +endif + endif # CONFIG_FAST_SPI_SUPPORTS_EXT_BIOS_WINDOW diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 5cb787d..65080d5 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -310,28 +310,71 @@ struct region host_space; }; -enum mmap_window_type { - X86_DEFAULT_DECODE_WINDOW, /* Decode window just below 4G boundary */ - X86_EXTENDED_DECODE_WINDOW, /* Extended decode window for mapping greater than 16MiB - flash */ - MMAP_MAX_WINDOWS, -}; +// Should be enough for now +#define MMAP_MAX_WINDOWS 3 /* Table of all the decode windows supported by the platform. */ +static int next_window; static struct mmap_window mmap_window_table[MMAP_MAX_WINDOWS]; -static void add_mmap_window(enum mmap_window_type idx, size_t flash_offset, size_t host_offset, +static void add_mmap_window(size_t flash_offset, size_t host_offset, size_t window_size) { - if (idx >= MMAP_MAX_WINDOWS) { - ERROR("Incorrect mmap window index(%d)\n", idx); + if (next_window >= MMAP_MAX_WINDOWS) { + ERROR("Too many memory map windows\n"); return; } - mmap_window_table[idx].flash_space.offset = flash_offset; - mmap_window_table[idx].host_space.offset = host_offset; - mmap_window_table[idx].flash_space.size = window_size; - mmap_window_table[idx].host_space.size = window_size; + mmap_window_table[next_window].flash_space.offset = flash_offset; + mmap_window_table[next_window].host_space.offset = host_offset; + mmap_window_table[next_window].flash_space.size = window_size; + mmap_window_table[next_window].host_space.size = window_size; + next_window++; +} + +static int decode_mmap_arg(char *optarg) +{ + if (optarg == NULL) + return 1; + + unsigned long flash_base; + unsigned long mmap_base; + unsigned long region_size; + char *suffix = NULL; + char *substring = strtok(optarg, ":"); + if (substring) + flash_base = strtol(substring, &suffix, 0); + if (!substring || (suffix && *suffix)) { + ERROR("Invalid flash base address '%s'.\n", + optarg); + return 1; + } + substring = strtok(NULL, ":"); + if (substring) + mmap_base = strtol(substring, &suffix, 0); + if (!substring || (suffix && *suffix)) { + ERROR("Invalid mmap base address '%s'.\n", + optarg); + return 1; + } + substring = strtok(NULL, ":"); + if (substring) + region_size = strtol(substring, &suffix, 0); + if (!substring || (suffix && *suffix)) { + ERROR("Invalid flash region size '%s'.\n", + optarg); + return 1; + } + + if (strtok(NULL, ":") != NULL) { + ERROR("Invalid argument, too many substrings '%s'.\n", + optarg); + + return 1; + } + + add_mmap_window(flash_base, mmap_base, region_size); + return 0; } #define DEFAULT_DECODE_WINDOW_TOP (4ULL * GiB) @@ -344,57 +387,45 @@ if (done) return done; - const size_t image_size = partitioned_file_total_size(param.image_file); - const size_t std_window_size = MIN(DEFAULT_DECODE_WINDOW_MAX_SIZE, image_size); - const size_t std_window_flash_offset = image_size - std_window_size; + // No memory map provided, use a default one + if (next_window == 0) { + const size_t image_size = partitioned_file_total_size(param.image_file); + printf("Image SIZE %lu\n", image_size); + const size_t std_window_size = MIN(DEFAULT_DECODE_WINDOW_MAX_SIZE, image_size); + const size_t std_window_flash_offset = image_size - std_window_size; - /* - * Default decode window lives just below 4G boundary in host space and maps up to a - * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped - * at the top of the host window just below 4G. - */ - add_mmap_window(X86_DEFAULT_DECODE_WINDOW, std_window_flash_offset, - DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); - - if (param.ext_win_size && (image_size > DEFAULT_DECODE_WINDOW_MAX_SIZE)) { /* - * If the platform supports extended window and the SPI flash size is greater - * than 16MiB, then create a mapping for the extended window as well. - * The assumptions here are: - * 1. Top 16MiB is still decoded in the fixed decode window just below 4G - * boundary. - * 2. Rest of the SPI flash below the top 16MiB is mapped at the top of extended - * window. Even though the platform might support a larger extended window, the - * SPI flash part used by the mainboard might not be large enough to be mapped - * in the entire window. In such cases, the mapping is assumed to be in the top - * part of the extended window with the bottom part remaining unused. - * - * Example: - * ext_win_base = 0xF8000000 - * ext_win_size = 32 * MiB - * ext_win_limit = ext_win_base + ext_win_size - 1 = 0xF9FFFFFF - * - * If SPI flash is 32MiB, then top 16MiB is mapped from 0xFF000000 - 0xFFFFFFFF - * whereas the bottom 16MiB is mapped from 0xF9000000 - 0xF9FFFFFF. The extended - * window 0xF8000000 - 0xF8FFFFFF remains unused. + * Default decode window lives just below 4G boundary in host space and maps up to a + * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped + * at the top of the host window just below 4G. */ - const size_t ext_window_mapped_size = MIN(param.ext_win_size, - image_size - std_window_size); - const size_t ext_window_top = param.ext_win_base + param.ext_win_size; - add_mmap_window(X86_EXTENDED_DECODE_WINDOW, - std_window_flash_offset - ext_window_mapped_size, - ext_window_top - ext_window_mapped_size, - ext_window_mapped_size); + add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); + } else { + /* + * Check provided memory map + */ + for (int i = 0; i < next_window; i++) { + for (int j = i + 1; j < next_window; j++) { + if (region_overlap(&mmap_window_table[i].flash_space, + &mmap_window_table[j].flash_space)) { + ERROR("Flash space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", + region_offset(&mmap_window_table[i].flash_space), + region_end(&mmap_window_table[i].flash_space), + region_offset(&mmap_window_table[j].flash_space), + region_end(&mmap_window_table[j].flash_space)); + return false; + } - if (region_overlap(&mmap_window_table[X86_EXTENDED_DECODE_WINDOW].host_space, - &mmap_window_table[X86_DEFAULT_DECODE_WINDOW].host_space)) { - const struct region *ext_region; - - ext_region = &mmap_window_table[X86_EXTENDED_DECODE_WINDOW].host_space; - ERROR("Extended window(base=0x%zx, limit=0x%zx) overlaps with default window!\n", - region_offset(ext_region), region_end(ext_region)); - - return false; + if (region_overlap(&mmap_window_table[i].host_space, + &mmap_window_table[j].host_space)) { + ERROR("Host space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", + region_offset(&mmap_window_table[i].flash_space), + region_end(&mmap_window_table[i].flash_space), + region_offset(&mmap_window_table[j].flash_space), + region_end(&mmap_window_table[j].flash_space)); + return false; + } + } } } @@ -1770,8 +1801,7 @@ /* begin after ASCII characters */ LONGOPT_START = 256, LONGOPT_IBB = LONGOPT_START, - LONGOPT_EXT_WIN_BASE, - LONGOPT_EXT_WIN_SIZE, + LONGOPT_MMAP, LONGOPT_END, }; @@ -1813,8 +1843,7 @@ {"mach-parseable",no_argument, 0, 'k' }, {"unprocessed", no_argument, 0, 'U' }, {"ibb", no_argument, 0, LONGOPT_IBB }, - {"ext-win-base", required_argument, 0, LONGOPT_EXT_WIN_BASE }, - {"ext-win-size", required_argument, 0, LONGOPT_EXT_WIN_SIZE }, + {"mmap", required_argument, 0, LONGOPT_MMAP }, {NULL, 0, 0, 0 } }; @@ -2255,19 +2284,9 @@ case LONGOPT_IBB: param.ibb = true; break; - case LONGOPT_EXT_WIN_BASE: - param.ext_win_base = strtoul(optarg, &suffix, 0); - if (!*optarg || (suffix && *suffix)) { - ERROR("Invalid ext window base '%s'.\n", optarg); + case LONGOPT_MMAP: + if (decode_mmap_arg(optarg)) return 1; - } - break; - case LONGOPT_EXT_WIN_SIZE: - param.ext_win_size = strtoul(optarg, &suffix, 0); - if (!*optarg || (suffix && *suffix)) { - ERROR("Invalid ext window size '%s'.\n", optarg); - return 1; - } break; case 'h': case '?': -- To view, visit https://review.coreboot.org/c/coreboot/+/68160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I38ab4c369704497f711e14ecda3ff3a8cdc0d089 Gerrit-Change-Number: 68160 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2022