May 2020 - coreboot-gerrit - mail.coreboot.org

Change in coreboot[master]: [TEMP]: NVME Free running Clock
by Maulik V Vaghela (Code Review) May 4, 2020

May 4, 2020

Maulik V Vaghela has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40288 ) Change subject: [TEMP]: NVME Free running Clock ...................................................................... Abandoned Not needed now -- To view, visit https://review.coreboot.org/c/coreboot/+/40288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib7ff83ad7d01ad9be9ade5d4f63d5ee882829221 Gerrit-Change-Number: 40288 Gerrit-PatchSet: 18 Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-MessageType: abandon

1 0

Change in coreboot[master]: lib/spd_bin: add get_spd_sn function
by Jamie Chen (Code Review) May 4, 2020

May 4, 2020

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Edward O'Callaghan, Angel Pons, Arthur Heymans, Kane Chen, Aaron Durbin, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/40414 to look at the new patch set (#4). Change subject: lib/spd_bin: add get_spd_sn function ...................................................................... lib/spd_bin: add get_spd_sn function This patch adds the get_spd_sn function. It's for reading SODIMM serial number. In spd_cache implementation it can use to get serial number before reading whole SPD by smbus. BUG=b:146457985 BRANCH=None TEST=Wrote sample code to get the serial number and ran on puff. It can get the serial number correctly. Change-Id: I406bba7cc56debbd9851d430f069e4fb96ec937c Signed-off-by: Jamie Chen <jamie.chen(a)intel.com> --- M src/include/spd_bin.h M src/soc/intel/common/block/smbus/smbuslib.c 2 files changed, 47 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/40414/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/40414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I406bba7cc56debbd9851d430f069e4fb96ec937c Gerrit-Change-Number: 40414 Gerrit-PatchSet: 4 Gerrit-Owner: Jamie Chen <jamie.chen(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com> 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: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/puff: add a region to cache SPD data
by Jamie Chen (Code Review) May 4, 2020

May 4, 2020

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Edward O'Callaghan, Kane Chen, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/40415 to look at the new patch set (#5). Change subject: mb/google/puff: add a region to cache SPD data ...................................................................... mb/google/puff: add a region to cache SPD data This patch adds a SPI rom region RW_SPD_CACHE on Puff and it can be used on spd_cache to reduce reading SPD data from SODIMM by smbus. It's for saving the boot time and it can be used to trigger MRC retraining when memory DIMM is changed. BUG=b:146457985 BRANCH=None TEST=Build puff successfully and verified below two items. 1. To change memory DIMM can trigger retraining. 2. one DIMM save the boot time : 158ms two DIMM save the boot time : 265ms Change-Id: I8d07fddf113a767d62394cb31e33b56f22f74351 Signed-off-by: Jamie Chen <jamie.chen(a)intel.com> --- M src/mainboard/google/hatch/Kconfig A src/mainboard/google/hatch/chromeos-16MiB-spd.fmd A src/mainboard/google/hatch/chromeos-spd.fmd M src/mainboard/google/hatch/romstage_spd_smbus.c 4 files changed, 133 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/40415/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/40415 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8d07fddf113a767d62394cb31e33b56f22f74351 Gerrit-Change-Number: 40415 Gerrit-PatchSet: 5 Gerrit-Owner: Jamie Chen <jamie.chen(a)intel.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: src/soc/intel/tigerlake: Fix wrong SPD information printed
by Derek Huang (Code Review) May 4, 2020

May 4, 2020

Derek Huang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40661 ) Change subject: src/soc/intel/tigerlake: Fix wrong SPD information printed ...................................................................... src/soc/intel/tigerlake: Fix wrong SPD information printed The SPD infomation printed by print_spd_info() is wrong due to wrong argument. Fix it by passing correct argument. Change-Id: I0479235849f761b946badb00d6f3e6adb3173b95 Signed-off-by: derek.huang <derek.huang(a)intel.corp-partner.google.com> --- M src/soc/intel/tigerlake/meminit.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40661/1 diff --git a/src/soc/intel/tigerlake/meminit.c b/src/soc/intel/tigerlake/meminit.c index 8664547..e94ccb2 100644 --- a/src/soc/intel/tigerlake/meminit.c +++ b/src/soc/intel/tigerlake/meminit.c @@ -221,8 +221,6 @@ } else { die("Not a valid location(%d) for Memory-down SPD!\n", info->md_spd_loc); } - - print_spd_info((unsigned char *)data); } void meminit_lpddr4x(FSP_M_CONFIG *mem_cfg, const struct lpddr4x_cfg *board_cfg, @@ -243,6 +241,8 @@ read_md_spd(info, &spd_data, &spd_len); mem_cfg->MemorySpdDataLen = spd_len; + print_spd_info((unsigned char *)spd_data); + for (i = 0; i < LPDDR4X_CHANNELS; i++) { if (LPDDR4X_CHANNEL_UNPOPULATED(i, half_populated)) init_spd_upds_empty(mem_cfg, i); -- To view, visit https://review.coreboot.org/c/coreboot/+/40661 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0479235849f761b946badb00d6f3e6adb3173b95 Gerrit-Change-Number: 40661 Gerrit-PatchSet: 1 Gerrit-Owner: Derek Huang <derek.huang(a)intel.corp-partner.google.com> Gerrit-MessageType: newchange

1 1

Change in coreboot[master]: nb/intel/i945/memmap: Convert to 96 characters line length
by HAOUAS Elyes (Code Review) May 4, 2020

May 4, 2020

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40802 ) Change subject: nb/intel/i945/memmap: Convert to 96 characters line length ...................................................................... nb/intel/i945/memmap: Convert to 96 characters line length Change-Id: I2ef938573e75022dcb31c935dde7d3055e7a53f0 Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> --- M src/northbridge/intel/i945/memmap.c 1 file changed, 3 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/40802/1 diff --git a/src/northbridge/intel/i945/memmap.c b/src/northbridge/intel/i945/memmap.c index ee9f100..0183ea2 100644 --- a/src/northbridge/intel/i945/memmap.c +++ b/src/northbridge/intel/i945/memmap.c @@ -65,11 +65,10 @@ return (void *) top_of_ram; } -/** Decodes used Graphics Mode Select (GMS) to kilobytes. */ +/* Decodes used Graphics Mode Select (GMS) to kilobytes. */ u32 decode_igd_memory_size(const u32 gms) { - static const u16 ggc2uma[] = { 0, 1, 4, 8, 16, 32, - 48, 64 }; + static const u16 ggc2uma[] = { 0, 1, 4, 8, 16, 32, 48, 64 }; if (gms >= ARRAY_SIZE(ggc2uma)) die("Bad Graphics Mode Select (GMS) setting.\n"); @@ -91,9 +90,7 @@ * RAM to cover both cbmem as the TSEG region. */ top_of_ram = (uintptr_t)cbmem_top(); - postcar_frame_add_mtrr(pcf, top_of_ram - 8*MiB, 8*MiB, - MTRR_TYPE_WRBACK); + postcar_frame_add_mtrr(pcf, top_of_ram - 8*MiB, 8*MiB, MTRR_TYPE_WRBACK); postcar_frame_add_mtrr(pcf, northbridge_get_tseg_base(), northbridge_get_tseg_size(), MTRR_TYPE_WRBACK); - } -- To view, visit https://review.coreboot.org/c/coreboot/+/40802 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2ef938573e75022dcb31c935dde7d3055e7a53f0 Gerrit-Change-Number: 40802 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

5 8

Change in coreboot[master]: tests: Add device/i2c-test test case
by Name of user not set (Code Review) May 4, 2020

May 4, 2020

Name of user not set #1002873 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40539 ) Change subject: tests: Add device/i2c-test test case ...................................................................... tests: Add device/i2c-test test case Add unit test for src/device/i2c.c module. This patch is also used as an example for incorporating Cmocka mocking feature (-wrap linker flag). Signed-off-by: Jan Dabros <jsd(a)semihalf.com> Change-Id: I2eeb565aacc724ae3b9f5c76ef4b98ef695416d6 --- A tests/device/Makefile.inc A tests/device/i2c-test.c 2 files changed, 203 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/40539/1 diff --git a/tests/device/Makefile.inc b/tests/device/Makefile.inc new file mode 100644 index 0000000..ec421af --- /dev/null +++ b/tests/device/Makefile.inc @@ -0,0 +1,18 @@ +## +## This file is part of the coreboot project. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +tests += i2c-test + +i2c-test-srcs += tests/device/i2c-test.c +i2c-test-srcs += src/device/i2c.c +i2c-test-mocks += platform_i2c_transfer diff --git a/tests/device/i2c-test.c b/tests/device/i2c-test.c new file mode 100644 index 0000000..6233f78 --- /dev/null +++ b/tests/device/i2c-test.c @@ -0,0 +1,185 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#include <device/i2c_simple.h> + +/* Simulate two i2c devices, both on bus 0, each with three uint8_t regs + * implemented. + */ +typedef struct { + uint8_t reg; + uint8_t data; +} i2c_ex_regs_t; + +typedef struct { + unsigned int bus; + uint8_t slave; + i2c_ex_regs_t regs[3]; +} i2c_ex_devs_t; + +i2c_ex_devs_t i2c_ex_devs[] = { + {0, 0xA, { /* bus, slave */ + {0x0, 0xB}, /* reg, data */ + {0x1, 0x6}, + {0x2, 0xF}, + } }, + {0, 0x3, { + {0x0, 0xDE}, + {0x1, 0xAD}, + {0x2, 0xBE}, + } }, +}; + +int __wrap_platform_i2c_transfer(unsigned int bus, struct i2c_msg *segments, + int count) +{ + int i; + int reg; + struct i2c_msg *tmp = segments; + i2c_ex_devs_t *i2c_dev = NULL; + + for (i = 0; i < count; i++, segments++) { + check_expected_ptr(segments->buf); + check_expected(segments->flags); + } + + reg = tmp->buf[0]; + + /* Find object for requested device */ + for (i = 0; i < ARRAY_SIZE(i2c_ex_devs); i++, i2c_dev++) + if (i2c_ex_devs[i].slave == tmp->slave) { + i2c_dev = &i2c_ex_devs[i]; + break; + } + + if (i2c_dev == NULL) + return -1; + + /* Write commands */ + if (tmp->len > 1) { + i2c_dev->regs[reg].data = tmp->buf[1]; + }; + + /* Read commands */ + for (i = 0; i < count; i++, tmp++) + if (tmp->flags & I2C_M_RD) { + *(tmp->buf) = i2c_dev->regs[reg].data; + }; +} + +static void mock_check_params(void) +{ + unsigned long int expected_flags[] = {0, I2C_M_RD, I2C_M_TEN, + I2C_M_RECV_LEN, I2C_M_NOSTART}; + + /* Flags should always be only within supported range */ + expect_in_set_count(__wrap_platform_i2c_transfer, segments->flags, + expected_flags, -1); + + expect_not_value_count(__wrap_platform_i2c_transfer, segments->buf, + NULL, -1); +} + +#define MASK 0x3 +#define SHIFT 0x1 + +static void i2c_read_field_test(void **state) +{ + int bus, slave, reg; + int i, j; + uint8_t buf; + + mock_check_params(); + + /* Read particular bits in all registers in all devices, then compare + * with expected value. + */ + for (i = 0; i < ARRAY_SIZE(i2c_ex_devs); i++) + for (j = 0; j < ARRAY_SIZE(i2c_ex_devs[0].regs); j++) { + i2c_read_field(i2c_ex_devs[i].bus, + i2c_ex_devs[i].slave, + i2c_ex_devs[i].regs[j].reg, + &buf, MASK, SHIFT); + assert_int_equal((i2c_ex_devs[i].regs[j].data & + (MASK << SHIFT)) >> SHIFT, buf); + }; + + /* Read whole registers */ + for (i = 0; i < ARRAY_SIZE(i2c_ex_devs); i++) + for (j = 0; j < ARRAY_SIZE(i2c_ex_devs[0].regs); j++) { + i2c_read_field(i2c_ex_devs[i].bus, + i2c_ex_devs[i].slave, + i2c_ex_devs[i].regs[j].reg, + &buf, 0xFF, 0); + assert_int_equal(i2c_ex_devs[i].regs[j].data, buf); + }; +} + +static void i2c_write_field_test(void **state) +{ + int bus, slave, reg; + int i, j; + uint8_t buf, tmp; + + mock_check_params(); + + /* Clear particular bits in all registers in all devices, then compare + * with expected value. + */ + for (i = 0; i < ARRAY_SIZE(i2c_ex_devs); i++) + for (j = 0; j < ARRAY_SIZE(i2c_ex_devs[0].regs); j++) { + buf = 0x0; + tmp = i2c_ex_devs[i].regs[j].data; + i2c_write_field(i2c_ex_devs[i].bus, + i2c_ex_devs[i].slave, + i2c_ex_devs[i].regs[j].reg, + buf, MASK, SHIFT); + assert_int_equal(i2c_ex_devs[i].regs[j].data, + (tmp & ~(MASK << SHIFT)) | (buf << SHIFT)); + i2c_read_field(i2c_ex_devs[i].bus, + i2c_ex_devs[i].slave, + i2c_ex_devs[i].regs[j].reg, + &buf, MASK, SHIFT); + }; + + /* Set all bits in all registers, this time verify using + * i2c_read_field() accessor. + */ + for (i = 0; i < ARRAY_SIZE(i2c_ex_devs); i++) + for (j = 0; j < ARRAY_SIZE(i2c_ex_devs[0].regs); j++) { + i2c_write_field(i2c_ex_devs[i].bus, + i2c_ex_devs[i].slave, + i2c_ex_devs[i].regs[j].reg, + 0xFF, 0xFF, 0); + i2c_read_field(i2c_ex_devs[i].bus, + i2c_ex_devs[i].slave, + i2c_ex_devs[i].regs[j].reg, + &buf, 0xFF, 0); + assert_int_equal(buf, 0xFF); + }; +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(i2c_read_field_test), + cmocka_unit_test(i2c_write_field_test) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/40539 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2eeb565aacc724ae3b9f5c76ef4b98ef695416d6 Gerrit-Change-Number: 40539 Gerrit-PatchSet: 1 Gerrit-Owner: Name of user not set #1002873 Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

5 17

Change in coreboot[master]: drivers/ocp/dmi: Add OCP_DMI driver for populating SMBIOS from IPMI F...
by Johnny Lin (Code Review) May 4, 2020

May 4, 2020

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40308 ) Change subject: drivers/ocp/dmi: Add OCP_DMI driver for populating SMBIOS from IPMI FRU data ...................................................................... Patch Set 26: (1 comment) https://review.coreboot.org/c/coreboot/+/40308/23/src/drivers/ocp/dmi/Kconf… File src/drivers/ocp/dmi/Kconfig: https://review.coreboot.org/c/coreboot/+/40308/23/src/drivers/ocp/dmi/Kconf… PS23, Line 14: config BMC_KCS_BASE > I noticed this so I added define BMC_KCS_BASE to ocp_dmi. […] For ramstage maybe I can add a global variable for it and sets it during ipmi_kcs_init() from dev->path.pnp.port, so all functions will use this global variable directly? -- To view, visit https://review.coreboot.org/c/coreboot/+/40308 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08c958dfad83216cd12545760a19d205efc2515b Gerrit-Change-Number: 40308 Gerrit-PatchSet: 26 Gerrit-Owner: Johnny Lin Gerrit-Reviewer: Andrey Petrov <anpetrov(a)fb.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jingle Hsu Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-Reviewer: Morgan Jang Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 May 2020 09:53:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Johnny Lin Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Patrick Georgi (Code Review) May 4, 2020

May 4, 2020

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... soc/intel/common/block/cse: Add boot partition related APIs In CSE Firmware Custom SKU, CSE region is logically divided into 2 boot partitions. These boot partitions are represented by BP1(RO), BP2(RW). With CSE Firmware Custom SKU, CSE can boot from either RO(BP1) or RW(BP2). The CSE Firmware Custom SKU layout appears as below: ------------- -------------------- --------------------- |CSE REGION | => | RO | RW | DATA | => | BP1 | BP2 | DATA | ------------- -------------------- --------------------- In order to support CSE FW update to RW region, below APIs help coreboot to get info about the boot partitions, and allows coreboot to set CSE to boot from required boot partition (either RO(BP1) or RW(BP2)). GET_BOOT_PARTITION_INFO - Provides info on available partitions in the CSE region. The API provides info on boot partitions like start/end offsets of a partition within CSE region, and their version and partition status. SET_BOOT_PARTITION_INFO - Sets CSE's next boot partition to boot from. With the HECI API, firmware can notify CSE to boot from RO(BP1) or RW(BP2) on next boot. As system having CSE Firmware Custom SKU, boots from RO(BP1) after G3, so coreboot sets CSE to boot from RW(BP2) in normal mode and further, coreboot ensure CSE to boot from whichever is selected boot partition if system is in recovery mode. BUG=b:145809764 TEST=Verified on hatch Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/35402 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Furquan Shaikh <furquan(a)google.com> --- M src/soc/intel/common/block/cse/Kconfig M src/soc/intel/common/block/cse/Makefile.inc A src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 4 files changed, 351 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved diff --git a/src/soc/intel/common/block/cse/Kconfig b/src/soc/intel/common/block/cse/Kconfig index 15de0b0..e566ddd 100644 --- a/src/soc/intel/common/block/cse/Kconfig +++ b/src/soc/intel/common/block/cse/Kconfig @@ -12,3 +12,10 @@ help Use this config to include common CSE block to make HECI function disable in SMM mode + +config SOC_INTEL_CSE_CUSTOM_SKU + bool + default n + depends on CHROMEOS + help + Enables CSE Custom SKU diff --git a/src/soc/intel/common/block/cse/Makefile.inc b/src/soc/intel/common/block/cse/Makefile.inc index 90f76d5..418b7a2 100644 --- a/src/soc/intel/common/block/cse/Makefile.inc +++ b/src/soc/intel/common/block/cse/Makefile.inc @@ -1,4 +1,5 @@ bootblock-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CSE) += cse.c romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CSE) += cse.c ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CSE) += cse.c +ramstage-$(CONFIG_SOC_INTEL_CSE_CUSTOM_SKU) += custom_bp.c smm-$(CONFIG_SOC_INTEL_COMMON_BLOCK_HECI_DISABLE_IN_SMM) += disable_heci.c diff --git a/src/soc/intel/common/block/cse/custom_bp.c b/src/soc/intel/common/block/cse/custom_bp.c new file mode 100644 index 0000000..653684a --- /dev/null +++ b/src/soc/intel/common/block/cse/custom_bp.c @@ -0,0 +1,329 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ +#include <bootstate.h> +#include <console/console.h> +#include <soc/intel/common/reset.h> +#include <intelblocks/cse.h> +#include <security/vboot/vboot_common.h> +#include <string.h> + +/* Converts bp index to boot partition string */ +#define GET_BP_STR(bp_index) (bp_index ? "RW" : "RO") + +/* + * CSE Firmware supports 3 boot partitions. For CSE Custom SKU, only 2 boot partitions are + * used and 3rd boot partition is set to BP_STATUS_PARTITION_NOT_PRESENT. + * CSE Custom SKU Image Layout: + * ------------- ------------------- --------------------- + * |CSE REGION | => | RO | RW | DATA | => | BP1 | BP2 | DATA | + * ------------- ------------------- --------------------- + */ +#define CSE_MAX_BOOT_PARTITIONS 3 + +/* CSE Custom SKU's valid bootable partition identifiers */ +enum boot_partition_id { + /* RO(BP1) contains recovery/minimal boot FW */ + RO = 0, + + /* RW(BP2) contains fully functional CSE Firmware */ + RW = 1 +}; + +/* + * Boot partition status. + * The status is returned in response to MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO cmd. + */ +enum bp_status { + /* This value is returned when a partition has no errors */ + BP_STATUS_SUCCESS = 0, + + /* + * This value is returned when a partition should be present based on layout, but it is + * not valid. + */ + BP_STATUS_GENERAL_FAILURE = 1, + + /* This value is returned when a partition is not present per initial image layout */ + BP_STATUS_PARTITION_NOT_PRESENT = 2, + +}; + +/* + * Boot Partition Info Flags + * The flags are returned in response to MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO cmd. + */ +enum bp_info_flags { + + /* Redundancy Enabled: It indicates CSE supports RO(BP1) and RW(BP2) regions */ + BP_INFO_REDUNDANCY_EN = 1 << 0, + + /* It indicates RO(BP1) supports Minimal Recovery Mode */ + BP_INFO_MIN_RECOV_MODE_EN = 1 << 1, + + /* + * Read-only Config Enabled: It indicates HW protection to CSE RO region is enabled. + * The option is relevant only if the BP_INFO_MIN_RECOV_MODE_EN flag is enabled. + */ + BP_INFO_READ_ONLY_CFG = 1 << 2, +}; + +/* Boot Partition FW Version */ +struct fw_version { + uint16_t major; + uint16_t minor; + uint16_t hotfix; + uint16_t build; +} __packed; + +/* CSE boot partition entry info */ +struct cse_bp_entry { + /* Boot partition version */ + struct fw_version fw_ver; + + /* Boot partition status */ + uint32_t status; + + /* Starting offset of the partition within CSE region */ + uint32_t start_offset; + + /* Ending offset of the partition within CSE region */ + uint32_t end_offset; + uint8_t reserved[12]; +} __packed; + +/* CSE boot partition info */ +struct cse_bp_info { + /* Number of boot partitions */ + uint8_t total_number_of_bp; + + /* Current boot partition */ + uint8_t current_bp; + + /* Next boot partition */ + uint8_t next_bp; + + /* Boot Partition Info Flags */ + uint8_t flags; + + /* Boot Partition Entry Info */ + struct cse_bp_entry bp_entries[CSE_MAX_BOOT_PARTITIONS]; +} __packed; + +struct get_bp_info_rsp { + struct mkhi_hdr hdr; + struct cse_bp_info bp_info; +} __packed; + +static uint8_t cse_get_current_bp(const struct cse_bp_info *cse_bp_info) +{ + return cse_bp_info->current_bp; +} + +static const struct cse_bp_entry *cse_get_bp_entry(enum boot_partition_id bp, + const struct cse_bp_info *cse_bp_info) +{ + return &cse_bp_info->bp_entries[bp]; +} + +static void cse_print_boot_partition_info(const struct cse_bp_info *cse_bp_info) +{ + const struct cse_bp_entry *cse_bp; + + printk(BIOS_DEBUG, "ME: Number of partitions = %d\n", cse_bp_info->total_number_of_bp); + printk(BIOS_DEBUG, "ME: Current partition = %s\n", GET_BP_STR(cse_bp_info->current_bp)); + printk(BIOS_DEBUG, "ME: Next partition = %s\n", GET_BP_STR(cse_bp_info->next_bp)); + printk(BIOS_DEBUG, "ME: Flags = 0x%x\n", cse_bp_info->flags); + + /* Log version info of RO & RW partitions */ + cse_bp = cse_get_bp_entry(RO, cse_bp_info); + printk(BIOS_DEBUG, "ME: %s version = %d.%d.%d.%d (Status=0x%x, Start=0x%x, End=0x%x)\n", + GET_BP_STR(RO), cse_bp->fw_ver.major, cse_bp->fw_ver.minor, + cse_bp->fw_ver.hotfix, cse_bp->fw_ver.build, + cse_bp->status, cse_bp->start_offset, + cse_bp->end_offset); + + cse_bp = cse_get_bp_entry(RW, cse_bp_info); + printk(BIOS_DEBUG, "ME: %s version = %d.%d.%d.%d (Status=0x%x, Start=0x%x, End=0x%x)\n", + GET_BP_STR(RW), cse_bp->fw_ver.major, cse_bp->fw_ver.minor, + cse_bp->fw_ver.hotfix, cse_bp->fw_ver.build, + cse_bp->status, cse_bp->start_offset, + cse_bp->end_offset); +} + +/* + * Checks prerequisites for MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO and + * MKHI_BUP_COMMON_SET_BOOT_PARTITION_INFO HECI commands. + * It allows execution of the Boot Partition commands in below scenarios: + * - When CSE boots from RW partition (COM: Normal and CWS: Normal) + * - When CSE boots from RO partition (COM: Soft Temp Disable and CWS: Normal) + * - After HMRFPO_ENABLE command is issued to CSE (COM: SECOVER_MEI_MSG and CWS: Normal) + */ +static bool cse_is_bp_cmd_info_possible(void) +{ + if (cse_is_hfs1_cws_normal()) { + if (cse_is_hfs1_com_normal()) + return true; + if (cse_is_hfs1_com_secover_mei_msg()) + return true; + if (cse_is_hfs1_com_soft_temp_disable()) + return true; + } + return false; +} + +static bool cse_get_bp_info(struct get_bp_info_rsp *bp_info_rsp) +{ + struct get_bp_info_req { + struct mkhi_hdr hdr; + uint8_t reserved[4]; + } __packed; + + struct get_bp_info_req info_req = { + .hdr.group_id = MKHI_GROUP_ID_BUP_COMMON, + .hdr.command = MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO, + .reserved = {0}, + }; + + if (!cse_is_bp_cmd_info_possible()) { + printk(BIOS_ERR, "cse_bp: CSE does not meet prerequisites\n"); + return false; + } + + size_t resp_size = sizeof(struct get_bp_info_rsp); + + if (!heci_send_receive(&info_req, sizeof(info_req), bp_info_rsp, &resp_size)) { + printk(BIOS_ERR, "cse_bp: Could not get partition info\n"); + return false; + } + + if (bp_info_rsp->hdr.result) { + printk(BIOS_ERR, "cse_bp: Get partition info resp failed: %d\n", + bp_info_rsp->hdr.result); + return false; + } + + cse_print_boot_partition_info(&bp_info_rsp->bp_info); + + return true; +} +/* + * It sends HECI command to notify CSE about its next boot partition. When coreboot wants + * CSE to boot from certain partition (BP1 <RO> or BP2 <RW>), then this command can be used. + * The CSE's valid bootable partitions are BP1(RO) and BP2(RW). + * This function must be used before EOP. + * Returns false on failure and true on success. + */ +static bool cse_set_next_boot_partition(enum boot_partition_id bp) +{ + struct set_boot_partition_info_req { + struct mkhi_hdr hdr; + uint8_t next_bp; + uint8_t reserved[3]; + } __packed; + + struct set_boot_partition_info_req switch_req = { + .hdr.group_id = MKHI_GROUP_ID_BUP_COMMON, + .hdr.command = MKHI_BUP_COMMON_SET_BOOT_PARTITION_INFO, + .next_bp = bp, + .reserved = {0}, + }; + + if (bp != RO && bp != RW) { + printk(BIOS_ERR, "cse_bp: Incorrect partition id(%d) is provided", bp); + return false; + } + + printk(BIOS_INFO, "cse_bp: Set Boot Partition Info Command (%s)\n", GET_BP_STR(bp)); + + if (!cse_is_bp_cmd_info_possible()) { + printk(BIOS_ERR, "cse_bp: CSE does not meet prerequisites\n"); + return false; + } + + struct mkhi_hdr switch_resp; + size_t sw_resp_sz = sizeof(struct mkhi_hdr); + + if (!heci_send_receive(&switch_req, sizeof(switch_req), &switch_resp, &sw_resp_sz)) + return false; + + if (switch_resp.result) { + printk(BIOS_ERR, "cse_bp: Set Boot Partition Info Response Failed: %d\n", + switch_resp.result); + return false; + } + + return true; +} + +static bool cse_boot_to_rw(const struct cse_bp_info *cse_bp_info) +{ + if (cse_get_current_bp(cse_bp_info) == RW) + return true; + + if (!cse_set_next_boot_partition(RW)) + return false; + + do_global_reset(); + + die("cse_bp: Failed to reset system\n"); + + /* Control never reaches here */ + return false; +} + +static bool cse_is_rw_status_valid(const struct cse_bp_info *cse_bp_info) +{ + const struct cse_bp_entry *rw_bp; + + /* RW(BP2) alone represents RW partition */ + rw_bp = cse_get_bp_entry(RW, cse_bp_info); + + if (rw_bp->status == BP_STATUS_PARTITION_NOT_PRESENT || + rw_bp->status == BP_STATUS_GENERAL_FAILURE) { + printk(BIOS_ERR, "cse_bp: RW BP (status:%u) is not valid\n", rw_bp->status); + return false; + } + return true; +} + +static bool cse_is_rw_info_valid(struct cse_bp_info *cse_bp_info) +{ + return cse_is_rw_status_valid(cse_bp_info); +} + +void cse_fw_sync(void *unused) +{ + static struct get_bp_info_rsp cse_bp_info; + + if (vboot_recovery_mode_enabled()) { + printk(BIOS_DEBUG, "cse_bp: Skip switching to RW in the recovery path\n"); + return; + } + + /* If CSE SKU type is not Custom, skip enabling CSE Custom SKU */ + if (!cse_is_hfs3_fw_sku_custom()) { + printk(BIOS_ERR, "cse_bp: Not a CSE Custom SKU\n"); + return; + } + + if (!cse_get_bp_info(&cse_bp_info)) { + printk(BIOS_ERR, "cse_bp: Failed to get CSE boot partition info\n"); + goto failed; + } + + + if (!cse_is_rw_info_valid(&cse_bp_info.bp_info)) { + printk(BIOS_ERR, "cse_bp: CSE RW partition is not valid\n"); + goto failed; + } + + if (!cse_boot_to_rw(&cse_bp_info.bp_info)) { + printk(BIOS_ERR, "cse_bp: Failed to switch to RW\n"); + goto failed; + } + return; +failed: + do_global_reset(); +} + +BOOT_STATE_INIT_ENTRY(BS_PRE_DEVICE, BS_ON_ENTRY, cse_fw_sync, NULL); diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index ead5d41..5cad63c 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -23,6 +23,7 @@ #define MKHI_GROUP_ID_CBM 0x0 #define MKHI_GROUP_ID_HMRFPO 0x5 #define MKHI_GROUP_ID_GEN 0xff +#define MKHI_GROUP_ID_BUP_COMMON 0xf0 /* Global Reset Command ID */ #define MKHI_CBM_GLOBAL_RESET_REQ 0xb @@ -37,6 +38,10 @@ /* Get Firmware Version Command Id */ #define MKHI_GEN_GET_FW_VERSION 0x2 +/* Boot partition info and set boot partition info command ids */ +#define MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO 0x1c +#define MKHI_BUP_COMMON_SET_BOOT_PARTITION_INFO 0x1d + /* ME Current Working States */ #define ME_HFS1_CWS_NORMAL 0x5 @@ -219,4 +224,13 @@ * Returns 0 on failure and 1 on success. */ uint8_t cse_wait_com_soft_temp_disable(void); + +/* + * The CSE Custom SKU supports notion of RO and RW boot partitions. The function will set + * CSE's boot partition as per Chrome OS boot modes. In normal mode, the function allows CSE to + * boot from RW and triggers recovery mode if CSE fails to jump to RW. + * In software triggered recovery mode, the function allows CSE to boot from whatever is + * currently selected partition. + */ +void cse_fw_sync(void *unused); #endif // SOC_INTEL_COMMON_CSE_H -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 82 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: security/vboot: Make vboot library is accessible to ramstage
by Sridhar Siricilla (Code Review) May 4, 2020

May 4, 2020

Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40562 ) Change subject: security/vboot: Make vboot library is accessible to ramstage ...................................................................... security/vboot: Make vboot library is accessible to ramstage Add changes to Makefile to make vboot helper functions accessible to ramstage. These functions help CSE firmware update module to trigger recovery mode for runtime errors. Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Change-Id: Id1ede45c4dffe90afcef210eabaa657cf92a9335 --- M src/security/vboot/Makefile.inc 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40562/1 diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index d1cc2da..d7c792a 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -115,6 +115,7 @@ else verstage-y += secdata_tpm.c romstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_tpm.c +ramstage-y += secdata_tpm.c endif ifneq ($(CONFIG_TPM1)$(CONFIG_TPM2),) @@ -126,6 +127,7 @@ ramstage-y += common.c postcar-y += common.c +ramstage-y += vboot_logic.c romstage-$(CONFIG_FSP2_0_USES_TPM_MRC_HASH) += mrc_cache_hash_tpm.c -- To view, visit https://review.coreboot.org/c/coreboot/+/40562 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id1ede45c4dffe90afcef210eabaa657cf92a9335 Gerrit-Change-Number: 40562 Gerrit-PatchSet: 1 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

8 30

Change in coreboot[master]: payloads/external/GRUB2: Makefile: fix checkout hint
by Patrick Georgi (Code Review) May 4, 2020

May 4, 2020

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40954 ) Change subject: payloads/external/GRUB2: Makefile: fix checkout hint ...................................................................... payloads/external/GRUB2: Makefile: fix checkout hint The git checkout hint introduced in cb:36343 does not get printed but executed instead. Escape the single-quotes to fix this. Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> Change-Id: I1277c3788a141b25cd9f22ec0476ee56b64aea4d Reviewed-on: https://review.coreboot.org/c/coreboot/+/40954 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Werner Zeh <werner.zeh(a)siemens.com> --- M payloads/external/GRUB2/Makefile 1 file changed, 2 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Werner Zeh: Looks good to me, approved diff --git a/payloads/external/GRUB2/Makefile b/payloads/external/GRUB2/Makefile index 4dfedbe..bb7b5f8 100644 --- a/payloads/external/GRUB2/Makefile +++ b/payloads/external/GRUB2/Makefile @@ -22,7 +22,8 @@ git -C $(project_dir) checkout -f $(TAG-y) else echo "WARNING: index/tree not clean, skipping update / force checkout." - echo " Checkout manually with `git -C $(project_dir) checkout -f`." + echo " Checkout manually with "\ + "\`git -C payloads/external/GRUB2/$(project_dir) checkout -f\`." endif grub2/build/config.h: $(CONFIG_DEP) | checkout -- To view, visit https://review.coreboot.org/c/coreboot/+/40954 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1277c3788a141b25cd9f22ec0476ee56b64aea4d Gerrit-Change-Number: 40954 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Denis Carikli <GNUtoo(a)no-log.org> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0