coreboot-gerrit March 2023

coreboot-gerrit@coreboot.org

1 participants
4880 discussions

[S] Change in coreboot[master]: soc/amd/*/acpi: drop unneeded mon_alrm FADT assignment
by Elyes Haouas (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Jason Glenesk, Raul Rangel, Matt DeVillier, Fred Reitberger, Felix Held. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73418 ) Change subject: soc/amd/*/acpi: drop unneeded mon_alrm FADT assignment ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/73418 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iabb5fc7367f1e4e7acea1a58abdb643fc46ca776 Gerrit-Change-Number: 73418 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> 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: 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: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sun, 05 Mar 2023 06:27:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by Maximilian Brune (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Stefan Reinauer. Hello Stefan Reinauer, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/73448 to look at the new patch set (#4). Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... util/ifdtool/ifdtool.c: Cleanup and Error printing - Remove region_name_fmap, since it's not needed. - Add warning if user doesn't supply a platform, since that can lead to dumps/layouts that do not include all IFD regions without the user even reliazing it. - Inform the User if IFD or Flashmap is not found. - Inform the User if there is not a single match between FMAP and IFD region Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 --- M src/southbridge/intel/common/firmware/Makefile.inc M util/ifdtool/ifdtool.c 2 files changed, 35 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/73448/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 4 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by build bot (Jenkins) (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Stefan Reinauer. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73448 ) Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... Patch Set 3: (1 comment) File util/ifdtool/ifdtool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-171442): https://review.coreboot.org/c/coreboot/+/73448/comment/311e1be4_0342f323 PS3, Line 2082: if (platform == -1) { braces {} are not necessary for single statement blocks -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 3 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Sun, 05 Mar 2023 04:28:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by Maximilian Brune (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Stefan Reinauer. Hello Stefan Reinauer, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/73448 to look at the new patch set (#3). Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... util/ifdtool/ifdtool.c: Cleanup and Error printing - Remove region_name_fmap, since it's not needed. - Add warning if user doesn't supply a platform, since that can lead to dumps/layouts that do not include all IFD regions without the user even reliazing it. - Inform the User if IFD or Flashmap is not found. - Inform the User if there is not a single match between FMAP and IFD region Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 --- M src/southbridge/intel/common/firmware/Makefile.inc M util/ifdtool/ifdtool.c 2 files changed, 36 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/73448/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 3 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: sb/intel/common: Validate IFD by default
by Maximilian Brune (Code Review) 04 Mar '23

04 Mar '23

Maximilian Brune has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73470 ) Change subject: sb/intel/common: Validate IFD by default ...................................................................... sb/intel/common: Validate IFD by default The results from overlapping IFD regions into FMAP regions can get hard to debug. To avoid that enable validation by default. Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: Ia11f56a5bbe3259c3ebae810036d1aac00a911f7 --- M src/southbridge/intel/common/Kconfig.common 1 file changed, 14 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/73470/1 diff --git a/src/southbridge/intel/common/Kconfig.common b/src/southbridge/intel/common/Kconfig.common index 0caf964..d6f6abd 100644 --- a/src/southbridge/intel/common/Kconfig.common +++ b/src/southbridge/intel/common/Kconfig.common @@ -86,7 +86,7 @@ config VALIDATE_INTEL_DESCRIPTOR depends on INTEL_DESCRIPTOR_MODE_CAPABLE bool "Validate Intel firmware descriptor" - default n + default y help This config enables validating the Intel firmware descriptor against the fmap layout. If the firmware descriptor layout does not match the fmap -- To view, visit https://review.coreboot.org/c/coreboot/+/73470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia11f56a5bbe3259c3ebae810036d1aac00a911f7 Gerrit-Change-Number: 73470 Gerrit-PatchSet: 1 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: cbfstool/default-x86.fmd: Rename BIOS -> SI_BIOS
by Maximilian Brune (Code Review) 04 Mar '23

04 Mar '23

Maximilian Brune has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73449 ) Change subject: cbfstool/default-x86.fmd: Rename BIOS -> SI_BIOS ...................................................................... cbfstool/default-x86.fmd: Rename BIOS -> SI_BIOS Currently ifdtool --validate will not correctly validate the FMAP against the IFD regions, since it will compare the IFD bios region with an FMAP region called SI_BIOS. It's probably a good idea to define default name for the BIOS FMAP region like we have for 'COREBOOT' or 'FMAP' FMAP region. Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I55eddfb5641b3011d4525893604ccf87fa05a1e2 --- M util/cbfstool/default-x86.fmd 1 file changed, 18 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/73449/1 diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd index 41be782..8991a81 100644 --- a/util/cbfstool/default-x86.fmd +++ b/util/cbfstool/default-x86.fmd @@ -8,7 +8,7 @@ # +-------------+ <-- 4GB / end of flash FLASH@##ROM_BASE## ##ROM_SIZE## { - BIOS@##BIOS_BASE## ##BIOS_SIZE## { + SI_BIOS@##BIOS_BASE## ##BIOS_SIZE## { ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## ##SMMSTORE_ENTRY## -- To view, visit https://review.coreboot.org/c/coreboot/+/73449 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I55eddfb5641b3011d4525893604ccf87fa05a1e2 Gerrit-Change-Number: 73449 Gerrit-PatchSet: 1 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by build bot (Jenkins) (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Stefan Reinauer. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73448 ) Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... Patch Set 2: (1 comment) File util/ifdtool/ifdtool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-171439): https://review.coreboot.org/c/coreboot/+/73448/comment/95f9dd26_e068dda2 PS2, Line 2082: if (platform == -1) { braces {} are not necessary for single statement blocks -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 2 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Sun, 05 Mar 2023 04:20:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by Maximilian Brune (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Stefan Reinauer. Hello Stefan Reinauer, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/73448 to look at the new patch set (#2). Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... util/ifdtool/ifdtool.c: Cleanup and Error printing - Remove region_name_fmap, since it's not needed. - Add warning if user doesn't supply a platform, since that can lead to dumps/layouts that do not include all IFD regions without the user even reliazing it. - Inform the User if IFD or Flashmap is not found. - Inform the User if there is not a single match between FMAP and IFD region Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 --- M util/ifdtool/ifdtool.c 1 file changed, 35 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/73448/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 2 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by build bot (Jenkins) (Code Review) 04 Mar '23

04 Mar '23

Attention is currently required from: Stefan Reinauer. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73448 ) Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... Patch Set 1: (1 comment) File util/ifdtool/ifdtool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-171438): https://review.coreboot.org/c/coreboot/+/73448/comment/9dcb0718_57dc2888 PS1, Line 2082: if (platform == -1) { braces {} are not necessary for single statement blocks -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 1 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Sun, 05 Mar 2023 04:18:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: util/ifdtool/ifdtool.c: Cleanup and Error printing
by Maximilian Brune (Code Review) 04 Mar '23

04 Mar '23

Maximilian Brune has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73448 ) Change subject: util/ifdtool/ifdtool.c: Cleanup and Error printing ...................................................................... util/ifdtool/ifdtool.c: Cleanup and Error printing - Remove region_name_fmap, since it's not needed. - Add warning if user doesn't supply a platform, since that can lead to dumps/layouts that do not include all IFD regions without the user even reliazing it. - Inform the User if IFD or Flashmap is not found. - Inform the User if there is not a single match between FMAP and IFD region Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 --- M src/southbridge/intel/common/Kconfig.common M util/ifdtool/ifdtool.c 2 files changed, 36 insertions(+), 20 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/73448/1 diff --git a/src/southbridge/intel/common/Kconfig.common b/src/southbridge/intel/common/Kconfig.common index 0caf964..d6f6abd 100644 --- a/src/southbridge/intel/common/Kconfig.common +++ b/src/southbridge/intel/common/Kconfig.common @@ -86,7 +86,7 @@ config VALIDATE_INTEL_DESCRIPTOR depends on INTEL_DESCRIPTOR_MODE_CAPABLE bool "Validate Intel firmware descriptor" - default n + default y help This config enables validating the Intel firmware descriptor against the fmap layout. If the firmware descriptor layout does not match the fmap diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 98afa4b..13e5b13c2 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -361,16 +361,6 @@ return region_names[region_type].pretty; } -static const char *region_name_fmap(unsigned int region_type) -{ - if (region_type >= max_regions) { - fprintf(stderr, "Invalid region type.\n"); - exit(EXIT_FAILURE); - } - - return region_names[region_type].fmapname; -} - static const char *region_name_short(unsigned int region_type) { if (region_type >= max_regions) { @@ -1077,26 +1067,26 @@ long int fmap_loc = fmap_find((uint8_t *)image, size); const frba_t *frba = find_frba(image, size); - if (fmap_loc < 0 || !frba) + if (fmap_loc < 0 || !frba) { + printf("Could not find FMAP (%p) or Intel Flash Descriptor (%p)\n", + (void *)fmap_loc, frba); exit(EXIT_FAILURE); + } fmap = (struct fmap *)(image + fmap_loc); + int matches = 0; for (i = 0; i < max_regions; i++) { - if (region_name_fmap(i) == NULL) - continue; - region_t region = get_region(frba, i); - if (region.size == 0) continue; - const struct fmap_area *area = - fmap_find_area(fmap, region_name_fmap(i)); - + const struct fmap_area *area = fmap_find_area(fmap, region_names[i].fmapname); if (!area) continue; + matches++; // found a match between FMAP and IFD region + if ((uint)region.base != area->offset || (uint)region.size != area->size) { printf("Region mismatch between %s and %s\n", @@ -1111,6 +1101,11 @@ } } + if (!matches) { + // At least a BIOS region should be present in both IFD and FMAP + fprintf(stderr, "Warning: Not a single IFD region found in FMAP\n"); + } + if (errors > 0) exit(EXIT_FAILURE); } @@ -2045,7 +2040,6 @@ fprintf(stderr, "Unknown platform: %s\n", optarg); exit(EXIT_FAILURE); } - fprintf(stderr, "Platform is: %s\n", optarg); break; case 't': mode_validate = 1; @@ -2085,6 +2079,10 @@ exit(EXIT_FAILURE); } + if (platform == -1) { + fprintf(stderr, "Warning: No platform specified. Output may be incomplete\n"); + } + char *filename = argv[optind]; int bios_fd = open(filename, O_RDONLY | O_BINARY); if (bios_fd == -1) { -- To view, visit https://review.coreboot.org/c/coreboot/+/73448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bbce63ecb2e920530394766f58b5ea6f72852e9 Gerrit-Change-Number: 73448 Gerrit-PatchSet: 1 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: newchange

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2023