coreboot-gerrit July 2021

coreboot-gerrit@coreboot.org

1 participants
5824 discussions

Change in coreboot[master]: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN
by Angel Pons (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Arthur Heymans, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56107 ) Change subject: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN ...................................................................... Patch Set 1: (1 comment) File src/southbridge/intel/common/firmware/Kconfig: https://review.coreboot.org/c/coreboot/+/56107/comment/c5011300_7f2e82e2 PS1, Line 27: config HAVE_ME_BIN : bool "Add Intel ME/TXE firmware" : depends on HAVE_IFD_BIN : help : The Intel processor in the selected system requires a special firmware : for an integrated controller. This might be called the Management : Engine (ME), the Trusted Execution Engine (TXE) or something else : depending on the chip. This firmware might or might not be available : in coreboot's 3rdparty/blobs repository. If it is not and if you don't : have access to the firmware from elsewhere, you can still build : coreboot without it. In this case however, you'll have to make sure : that you don't overwrite your ME/TXE firmware on your flash ROM. : : config ME_BIN_PATH : string "Path to management engine firmware" : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/me.bin" : depends on HAVE_ME_BIN : : config CHECK_ME : bool "Verify the integrity of the supplied ME/TXE firmware" : default n : depends on HAVE_ME_BIN && (NORTHBRIDGE_INTEL_IRONLAKE || \ : NORTHBRIDGE_INTEL_SANDYBRIDGE || \ : NORTHBRIDGE_INTEL_HASWELL || \ : SOC_INTEL_BROADWELL || SOC_INTEL_SKYLAKE || \ : SOC_INTEL_KABYLAKE || SOC_INTEL_BAYTRAIL || SOC_INTEL_BRASWELL) : help : Verify the integrity of the supplied Intel ME/TXE firmware before : proceeding with the build, in order to prevent an accidental loading : of a corrupted ME/TXE image. : : config ME_REGION_ALLOW_CPU_READ_ACCESS : bool "Allows HOST/CPU read access to ME region" : depends on HAVE_IFD_BIN : default y if SOC_INTEL_CSE_LITE_SKU : default n : help : The config ensures Host has read access to the ME region if it is locked : through LOCK_MANAGEMENT_ENGINE config. This config is enabled when the CSE : Lite SKU is integrated. : : config USE_ME_CLEANER : bool "Strip down the Intel ME/TXE firmware" : depends on HAVE_ME_BIN && (NORTHBRIDGE_INTEL_IRONLAKE || \ : NORTHBRIDGE_INTEL_SANDYBRIDGE || \ : NORTHBRIDGE_INTEL_HASWELL || \ : SOC_INTEL_BROADWELL || SOC_INTEL_SKYLAKE || \ : SOC_INTEL_KABYLAKE || SOC_INTEL_BAYTRAIL || SOC_INTEL_BRASWELL) : help : Use me_cleaner to remove all the non-fundamental code from the Intel : ME/TXE firmware. : The resulting Intel ME/TXE firmware will have only the code : responsible for the very basic hardware initialization, leaving the : ME/TXE subsystem essentially in a disabled state. : : Don't flash a modified ME/TXE firmware and a new coreboot image at the : same time, test them in two different steps. : : WARNING: this tool isn't based on any official Intel documentation but : only on reverse engineering and trial & error. : : See the project's page : https://github.com/corna/me_cleaner : or the wiki : https://github.com/corna/me_cleaner/wiki/How-to-apply-me_cleaner : https://github.com/corna/me_cleaner/wiki/How-does-it-work%3F : https://github.com/corna/me_cleaner/wiki/me_cleaner-status : for more info about this tool : : If unsure, say N. : : comment "Please test the modified ME/TXE firmware and coreboot in two steps" : depends on USE_ME_CLEANER : : config ME_CLEANER_ARGS : string : depends on USE_ME_CLEANER : default "-S" : : config MAINBOARD_USES_IFD_GBE_REGION : def_bool n : : config HAVE_GBE_BIN : bool "Add gigabit ethernet configuration" : depends on HAVE_IFD_BIN && MAINBOARD_USES_IFD_GBE_REGION : help : The integrated gigabit ethernet controller needs a configuration : file. Select this if you are going to use the PCH integrated : controller and want to add that file. : : config GBE_BIN_PATH : string "Path to gigabit ethernet configuration" : depends on HAVE_GBE_BIN : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/gbe.bin" : : config MAINBOARD_USES_IFD_EC_REGION : def_bool n : : config HAVE_EC_BIN : bool "Add EC firmware" : depends on HAVE_IFD_BIN && MAINBOARD_USES_IFD_EC_REGION : help : The embedded controller needs a firmware file. : : Select this if you are going to use the PCH integrated controller : and have the EC firmware. EC firmware will be added to final image : through ifdtool. : : config EC_BIN_PATH : string "Path to EC firmware" : depends on HAVE_EC_BIN : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/ec.bin" : : choice : prompt "Protect flash regions" if HAVE_IFD_BIN : default UNLOCK_FLASH_REGIONS if HAVE_IFD_BIN : help : This option allows you to protect flash regions. : : config DO_NOT_TOUCH_DESCRIPTOR_REGION : bool "Use the preset values to protect the regions" : help : Read and write access permissions to different regions in the flash : can be controlled via dedicated bitfields in the flash descriptor. : These permissions can be modified with the Intel Flash Descriptor : Tool (ifdtool). If you don't want to change these permissions and : keep the ones provided in the initial descriptor, use this option. : : config LOCK_MANAGEMENT_ENGINE : bool "Lock ME/TXE section" : help : The Intel Firmware Descriptor supports preventing write and read : accesses from the host to the ME or TXE section. If the section : is locked, it can only be overwritten with an external SPI flash : programmer or HECI HMRFPO_ENABLE command needs to be sent to CSE : before writing to the ME Section. If CSE Lite SKU is integrated, : the Kconfig prevents only writing to the ME section. : : If unsure, select "Unlock flash regions". : : config UNLOCK_FLASH_REGIONS : bool "Unlock flash regions" : help : All regions are completely unprotected and can be overwritten using : a flash programming tool. : : endchoice > This whole thing does not make sense without HAVE_IFD_BIN. […] I'm not sure if the if-clause would make some symbols disappear. Plus, most options already depend on others (e.g. HAVE_ME_BIN depends on HAVE_IFD_BIN), so I'm not sure what the benefit would be. -- To view, visit https://review.coreboot.org/c/coreboot/+/56107 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8736f32b4c695efbd68adf551e1376726c718b56 Gerrit-Change-Number: 56107 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 06 Jul 2021 12:11:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN
by Arthur Heymans (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Angel Pons, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56107 ) Change subject: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN ...................................................................... Patch Set 1: (1 comment) File src/southbridge/intel/common/firmware/Kconfig: https://review.coreboot.org/c/coreboot/+/56107/comment/cb7d4729_1e6a5814 PS1, Line 27: config HAVE_ME_BIN : bool "Add Intel ME/TXE firmware" : depends on HAVE_IFD_BIN : help : The Intel processor in the selected system requires a special firmware : for an integrated controller. This might be called the Management : Engine (ME), the Trusted Execution Engine (TXE) or something else : depending on the chip. This firmware might or might not be available : in coreboot's 3rdparty/blobs repository. If it is not and if you don't : have access to the firmware from elsewhere, you can still build : coreboot without it. In this case however, you'll have to make sure : that you don't overwrite your ME/TXE firmware on your flash ROM. : : config ME_BIN_PATH : string "Path to management engine firmware" : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/me.bin" : depends on HAVE_ME_BIN : : config CHECK_ME : bool "Verify the integrity of the supplied ME/TXE firmware" : default n : depends on HAVE_ME_BIN && (NORTHBRIDGE_INTEL_IRONLAKE || \ : NORTHBRIDGE_INTEL_SANDYBRIDGE || \ : NORTHBRIDGE_INTEL_HASWELL || \ : SOC_INTEL_BROADWELL || SOC_INTEL_SKYLAKE || \ : SOC_INTEL_KABYLAKE || SOC_INTEL_BAYTRAIL || SOC_INTEL_BRASWELL) : help : Verify the integrity of the supplied Intel ME/TXE firmware before : proceeding with the build, in order to prevent an accidental loading : of a corrupted ME/TXE image. : : config ME_REGION_ALLOW_CPU_READ_ACCESS : bool "Allows HOST/CPU read access to ME region" : depends on HAVE_IFD_BIN : default y if SOC_INTEL_CSE_LITE_SKU : default n : help : The config ensures Host has read access to the ME region if it is locked : through LOCK_MANAGEMENT_ENGINE config. This config is enabled when the CSE : Lite SKU is integrated. : : config USE_ME_CLEANER : bool "Strip down the Intel ME/TXE firmware" : depends on HAVE_ME_BIN && (NORTHBRIDGE_INTEL_IRONLAKE || \ : NORTHBRIDGE_INTEL_SANDYBRIDGE || \ : NORTHBRIDGE_INTEL_HASWELL || \ : SOC_INTEL_BROADWELL || SOC_INTEL_SKYLAKE || \ : SOC_INTEL_KABYLAKE || SOC_INTEL_BAYTRAIL || SOC_INTEL_BRASWELL) : help : Use me_cleaner to remove all the non-fundamental code from the Intel : ME/TXE firmware. : The resulting Intel ME/TXE firmware will have only the code : responsible for the very basic hardware initialization, leaving the : ME/TXE subsystem essentially in a disabled state. : : Don't flash a modified ME/TXE firmware and a new coreboot image at the : same time, test them in two different steps. : : WARNING: this tool isn't based on any official Intel documentation but : only on reverse engineering and trial & error. : : See the project's page : https://github.com/corna/me_cleaner : or the wiki : https://github.com/corna/me_cleaner/wiki/How-to-apply-me_cleaner : https://github.com/corna/me_cleaner/wiki/How-does-it-work%3F : https://github.com/corna/me_cleaner/wiki/me_cleaner-status : for more info about this tool : : If unsure, say N. : : comment "Please test the modified ME/TXE firmware and coreboot in two steps" : depends on USE_ME_CLEANER : : config ME_CLEANER_ARGS : string : depends on USE_ME_CLEANER : default "-S" : : config MAINBOARD_USES_IFD_GBE_REGION : def_bool n : : config HAVE_GBE_BIN : bool "Add gigabit ethernet configuration" : depends on HAVE_IFD_BIN && MAINBOARD_USES_IFD_GBE_REGION : help : The integrated gigabit ethernet controller needs a configuration : file. Select this if you are going to use the PCH integrated : controller and want to add that file. : : config GBE_BIN_PATH : string "Path to gigabit ethernet configuration" : depends on HAVE_GBE_BIN : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/gbe.bin" : : config MAINBOARD_USES_IFD_EC_REGION : def_bool n : : config HAVE_EC_BIN : bool "Add EC firmware" : depends on HAVE_IFD_BIN && MAINBOARD_USES_IFD_EC_REGION : help : The embedded controller needs a firmware file. : : Select this if you are going to use the PCH integrated controller : and have the EC firmware. EC firmware will be added to final image : through ifdtool. : : config EC_BIN_PATH : string "Path to EC firmware" : depends on HAVE_EC_BIN : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/ec.bin" : : choice : prompt "Protect flash regions" if HAVE_IFD_BIN : default UNLOCK_FLASH_REGIONS if HAVE_IFD_BIN : help : This option allows you to protect flash regions. : : config DO_NOT_TOUCH_DESCRIPTOR_REGION : bool "Use the preset values to protect the regions" : help : Read and write access permissions to different regions in the flash : can be controlled via dedicated bitfields in the flash descriptor. : These permissions can be modified with the Intel Flash Descriptor : Tool (ifdtool). If you don't want to change these permissions and : keep the ones provided in the initial descriptor, use this option. : : config LOCK_MANAGEMENT_ENGINE : bool "Lock ME/TXE section" : help : The Intel Firmware Descriptor supports preventing write and read : accesses from the host to the ME or TXE section. If the section : is locked, it can only be overwritten with an external SPI flash : programmer or HECI HMRFPO_ENABLE command needs to be sent to CSE : before writing to the ME Section. If CSE Lite SKU is integrated, : the Kconfig prevents only writing to the ME section. : : If unsure, select "Unlock flash regions". : : config UNLOCK_FLASH_REGIONS : bool "Unlock flash regions" : help : All regions are completely unprotected and can be overwritten using : a flash programming tool. : : endchoice This whole thing does not make sense without HAVE_IFD_BIN. Maybe add an if clause? -- To view, visit https://review.coreboot.org/c/coreboot/+/56107 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8736f32b4c695efbd68adf551e1376726c718b56 Gerrit-Change-Number: 56107 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 06 Jul 2021 12:09:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: include/types.h: #include <limits.h>
by Paul Menzel (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Sean Rhodes, Nico Huber, Angel Pons, Julius Werner, Kyösti Mälkki, Patrick Rudolph. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55320 ) Change subject: include/types.h: #include <limits.h> ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: Are the soc/intel changes related? -- To view, visit https://review.coreboot.org/c/coreboot/+/55320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3ba189b998644c68068b85f25b11cff4d70c037d Gerrit-Change-Number: 55320 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 06 Jul 2021 12:06:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN
by Paul Menzel (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Patrick Rudolph. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56107 ) Change subject: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/56107 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8736f32b4c695efbd68adf551e1376726c718b56 Gerrit-Change-Number: 56107 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 06 Jul 2021 12:05:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN
by Angel Pons (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Arthur Heymans, Patrick Rudolph. Hello Nico Huber, Tim Wawrzynczak, Arthur Heymans, Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/56107 to review the following change. Change subject: sb/intel/common: Hide IFD options if !HAVE_IFD_BIN ...................................................................... sb/intel/common: Hide IFD options if !HAVE_IFD_BIN When `HAVE_IFD_BIN` is not enabled, do not show IFD-related options. Change-Id: I8736f32b4c695efbd68adf551e1376726c718b56 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/southbridge/intel/common/firmware/Kconfig 1 file changed, 3 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/56107/1 diff --git a/src/southbridge/intel/common/firmware/Kconfig b/src/southbridge/intel/common/firmware/Kconfig index 36dde56..a1026e8 100644 --- a/src/southbridge/intel/common/firmware/Kconfig +++ b/src/southbridge/intel/common/firmware/Kconfig @@ -57,6 +57,7 @@ config ME_REGION_ALLOW_CPU_READ_ACCESS bool "Allows HOST/CPU read access to ME region" + depends on HAVE_IFD_BIN default y if SOC_INTEL_CSE_LITE_SKU default n help @@ -137,8 +138,8 @@ default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/ec.bin" choice - prompt "Protect flash regions" - default UNLOCK_FLASH_REGIONS + prompt "Protect flash regions" if HAVE_IFD_BIN + default UNLOCK_FLASH_REGIONS if HAVE_IFD_BIN help This option allows you to protect flash regions. -- To view, visit https://review.coreboot.org/c/coreboot/+/56107 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8736f32b4c695efbd68adf551e1376726c718b56 Gerrit-Change-Number: 56107 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: util/ifdtool: Add sklkbl to IFDv2 platforms
by Subrata Banik (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Jan Tatje, Paul Menzel, Stefan Reinauer. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56070 ) Change subject: util/ifdtool: Add sklkbl to IFDv2 platforms ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/56070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3f92b090e929336b5c18b442d1504ee1000f5594 Gerrit-Change-Number: 56070 Gerrit-PatchSet: 2 Gerrit-Owner: Jan Tatje <jan(a)jnt.io> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jan Tatje <jan(a)jnt.io> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Tue, 06 Jul 2021 11:57:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset
by Angel Pons (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Subrata Banik. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54305 ) Change subject: util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset ...................................................................... Patch Set 10: (1 comment) Patchset: PS10: > > > Earlier if your platform was entering into earlier if loop and touching bit 7:0 then it was doin […] I checked CB:56070 against document 551377 (KBL-H SPI Programming Guide) and all the implications of using IFD v2 are correct, i.e. the registers will be decoded properly. -- To view, visit https://review.coreboot.org/c/coreboot/+/54305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I25f69ce775454409974056d8326c02e29038ec8a Gerrit-Change-Number: 54305 Gerrit-PatchSet: 10 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Jan Tatje <jan(a)jnt.io> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Comment-Date: Tue, 06 Jul 2021 11:54:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jan Tatje <jan(a)jnt.io> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset
by Subrata Banik (Code Review) 06 Jul '21

06 Jul '21

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54305 ) Change subject: util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset ...................................................................... Patch Set 10: (1 comment) Patchset: PS10: > > Earlier if your platform was entering into earlier if loop and touching bit 7:0 then it was doing wrong programming. > > I either don't understand that sentence correctly or it is the opposite, the else clause is what touches bit 7:0. Its exactly opposite what i said, SKL SPI programming guide says, Bit 7:0 are reserved and other IFDv2 chipset has extended ranges hence it might need to protect. if (ifd_version >= IFD_VERSION_2) { /* Clear non-reserved bits */ fmba->flmstr1 &= 0xff; fmba->flmstr2 &= 0xff; fmba->flmstr3 &= 0xff; fmba->flmstr5 &= 0xff; } else { wr_shift = FLMSTR_WR_SHIFT_V1; rd_shift = FLMSTR_RD_SHIFT_V1; fmba->flmstr1 = 0; fmba->flmstr2 = 0; /* Requestor ID */ fmba->flmstr3 = 0x118; } > > If ifd_version is set to IFD_VERSION_2 like it was before this change, then it will not touch bits 7:0 and set read and write permissions to true. In the else-clause of the condition, which it uses now, both 7:0 reserved bits are changed and (I think this is what breaks boot) read permission bits 15:8 are set to 0. if you could download the SPI programming guide from this link https://www.intel.com/content/www/us/en/search.html?ws=text#q=550696&t=All and go to page 33, Flash Descriptor Master Section, you will see 7:0 are reserved and what I could sense is that, existing code itself had issue https://review.coreboot.org/c/coreboot/+/56070/1/util/ifdtool/ifdtool.c#1278 should be as below to protect bits 15:8 as well. fmba->flmstr1 = 0xffffff00; fmba->flmstr2 = 0xffffff00; > > > I would say, it was wrong that SKL was considered as IFDv2. > Even if IFDv2 is supposed to mean that 7:0 of the flash master are used for Extended Region Access Permissions somewhere else, in ifdtool it just decides whether FLMSTR uses 31:16 or 31:8 for permissions. > > IMHO adding SKLKBL to the ifd_2_platforms list is the easiest solution to fix this issue. This is the change I applied to make it work for me: https://review.coreboot.org/c/coreboot/+/56070 Adding SKL into IFDv2 would be an easy W/A as SKL flash descriptor master register offsets are very close to IFDv2 (except the last byte). Have you verified other fields as well with CB:56070 for skl platform? -- To view, visit https://review.coreboot.org/c/coreboot/+/54305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I25f69ce775454409974056d8326c02e29038ec8a Gerrit-Change-Number: 54305 Gerrit-PatchSet: 10 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Jan Tatje <jan(a)jnt.io> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Jul 2021 11:43:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jan Tatje <jan(a)jnt.io> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/ifdtool: Add sklkbl to IFDv2 platforms
by Jan Tatje (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Paul Menzel, Stefan Reinauer. Jan Tatje has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56070 ) Change subject: util/ifdtool: Add sklkbl to IFDv2 platforms ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/56070/comment/a19ce27c_7f1c58f2 PS1, Line 12: > Please add a Fixes: tag: […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/56070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3f92b090e929336b5c18b442d1504ee1000f5594 Gerrit-Change-Number: 56070 Gerrit-PatchSet: 2 Gerrit-Owner: Jan Tatje <jan(a)jnt.io> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Tue, 06 Jul 2021 11:30:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/ifdtool: Add sklkbl to IFDv2 platforms
by Jan Tatje (Code Review) 06 Jul '21

06 Jul '21

Attention is currently required from: Jan Tatje, Stefan Reinauer. Hello build bot (Jenkins), Stefan Reinauer, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56070 to look at the new patch set (#2). Change subject: util/ifdtool: Add sklkbl to IFDv2 platforms ...................................................................... util/ifdtool: Add sklkbl to IFDv2 platforms Currently ifdtool breaks the descriptor because it treats it as IFDv1. This change adds it to the list of IFDv2 platforms. Fixes boot for X11SSH-LN4F. Fixes: 8c082e5fef ("util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset") Change-Id: I3f92b090e929336b5c18b442d1504ee1000f5594 Signed-off-by: Jan Tatje <jan(a)jnt.io> --- M util/ifdtool/ifdtool.c 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/56070/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/56070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3f92b090e929336b5c18b442d1504ee1000f5594 Gerrit-Change-Number: 56070 Gerrit-PatchSet: 2 Gerrit-Owner: Jan Tatje <jan(a)jnt.io> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jan Tatje <jan(a)jnt.io> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2021