coreboot-gerrit May 2021

coreboot-gerrit@coreboot.org

1 participants
6048 discussions

Change in coreboot[master]: soc/amd/common/block: Add support for configuring eSPI connection to ...
by Furquan Shaikh (Code Review) 20 Dec '21

20 Dec '21

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41272 ) Change subject: soc/amd/common/block: Add support for configuring eSPI connection to slave ...................................................................... soc/amd/common/block: Add support for configuring eSPI connection to slave This change adds a helper function espi_setup() which allows SoCs to configure connection to slave. Most of the configuration is dependent upon mainboard settings in espi_config done as part of the device tree. The general flow for setup involves the following steps: 1. Set initial configuration (lowest operating frequency and single mode). 2. Perform in-band reset and set initial configuration since the settings would be lost by the reset. 3. Read slave capabilities. 4. Set slave configuration based on mainboard settings. 5. Perform eSPI host controller configuration to match the slave configuration and set polarities for VW interrupts. 6. Perform VW channel setup and deassert PLTRST#. 7. Perform peripheral channel setup. 8. Perform OOB channel setup. 9. Perform flash channel setup. 10. Enable subtractive decoding if requested by mainboard. BUG=b:153675913 Signed-off-by: Furquan Shaikh <furquan(a)google.com> Change-Id: I872ec09cd92e9bb53f22e38d2773f3491355279e --- M src/soc/amd/common/block/include/amdblocks/espi.h M src/soc/amd/common/block/lpc/espi_util.c 2 files changed, 670 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/41272/1 diff --git a/src/soc/amd/common/block/include/amdblocks/espi.h b/src/soc/amd/common/block/include/amdblocks/espi.h index 69267f8..a77e49a 100644 --- a/src/soc/amd/common/block/include/amdblocks/espi.h +++ b/src/soc/amd/common/block/include/amdblocks/espi.h @@ -27,6 +27,46 @@ #define ESPI_GENERIC_MMIO_WIN_COUNT 4 #define ESPI_GENERIC_MMIO_MAX_WIN_SIZE 0x10000 +#define ESPI_SLAVE0_CONFIG 0x68 +#define ESPI_CRC_CHECKING_EN (1 << 31) +#define ESPI_ALERT_MODE (1 << 30) + +#define ESPI_IO_MODE_SHIFT 28 +#define ESPI_IO_MODE_MASK (0x3 << ESPI_IO_MODE_SHIFT) +#define ESPI_IO_MODE_VAL(x) ((x) << ESPI_IO_MODE_SHIFT) + +enum espi_io_mode { + ESPI_IO_MODE_SINGLE = ESPI_IO_MODE_VAL(0), + ESPI_IO_MODE_DUAL = ESPI_IO_MODE_VAL(1), + ESPI_IO_MODE_QUAD = ESPI_IO_MODE_VAL(2), +}; + +#define ESPI_OP_FREQ_SHIFT 25 +#define ESPI_OP_FREQ_MASK (0x7 << ESPI_OP_FREQ_SHIFT) +#define ESPI_OP_FREQ_VAL(x) ((x) << ESPI_OP_FREQ_SHIFT) + +enum espi_op_freq { + ESPI_OP_FREQ_16_MHZ = ESPI_OP_FREQ_VAL(0), + ESPI_OP_FREQ_33_MHZ = ESPI_OP_FREQ_VAL(1), + ESPI_OP_FREQ_66_MHZ = ESPI_OP_FREQ_VAL(2), +}; + +#define ESPI_PERIPH_CH_EN (1 << 3) +#define ESPI_VW_CH_EN (1 << 2) +#define ESPI_OOB_CH_EN (1 << 1) +#define ESPI_FLASH_CH_EN (1 << 0) + +/* + * Virtual wire interrupt polarity. If the interrupt is active level high or active falling + * edge, then controller expects its bit to be cleared in ESPI_RXVW_POLARITY whereas if the + * interrupt is active level low or active rising edge, then its bit needs to be set in + * ESPI_RXVW_POLARITY. + */ +#define ESPI_VW_IRQ_LEVEL_HIGH(x) (0 << (x)) +#define ESPI_VW_IRQ_LEVEL_LOW(x) (1 << (x)) +#define ESPI_VW_IRQ_EDGE_HIGH(x) (1 << (x)) +#define ESPI_VW_IRQ_EDGE_LOW(x) (0 << (x)) + struct espi_config { /* Bitmap for standard IO decodes. Use ESPI_DECODE_IO_* above. */ uint32_t std_io_decode_bitmap; @@ -35,6 +75,21 @@ uint16_t base; size_t size; } generic_io_range[ESPI_GENERIC_IO_WIN_COUNT]; + + /* Slave configuration parameters */ + enum espi_io_mode io_mode; + enum espi_op_freq op_freq_mhz; + + uint32_t crc_check_enable:1; + uint32_t dedicated_alert_pin:1; + uint32_t periph_ch_en:1; + uint32_t vw_ch_en:1; + uint32_t oob_ch_en:1; + uint32_t flash_ch_en:1; + uint32_t subtractive_decode:1; + + /* Use ESPI_VW_INT_* above */ + uint32_t vw_irq_polarity; }; /* @@ -61,4 +116,6 @@ */ void espi_update_static_bar(uintptr_t bar); +int espi_setup(void); + #endif /* __AMDBLOCKS_ESPI_H__ */ diff --git a/src/soc/amd/common/block/lpc/espi_util.c b/src/soc/amd/common/block/lpc/espi_util.c index 06737fa..96bad75 100644 --- a/src/soc/amd/common/block/lpc/espi_util.c +++ b/src/soc/amd/common/block/lpc/espi_util.c @@ -6,9 +6,11 @@ #include <amdblocks/lpc.h> #include <arch/mmio.h> #include <console/console.h> +#include <espi.h> #include <soc/pci_devs.h> #include <stddef.h> #include <stdint.h> +#include <timer.h> #include <types.h> static uintptr_t espi_bar; @@ -304,3 +306,614 @@ cfg->generic_io_range[i].size); } } + +#define ESPI_DN_TX_HDR0 0x00 +enum espi_cmd_type { + CMD_TYPE_SET_CONFIGURATION = 0, + CMD_TYPE_GET_CONFIGURATION = 1, + CMD_TYPE_IN_BAND_RESET = 2, + CMD_TYPE_PERIPHERAL = 4, + CMD_TYPE_VW = 5, + CMD_TYPE_OOB = 6, + CMD_TYPE_FLASH = 7, +}; + +#define ESPI_DN_TX_HDR1 0x04 +#define ESPI_DN_TX_HDR2 0x08 +#define ESPI_DN_TX_DATA 0x0c + +#define ESPI_MASTER_CAP 0x2c +#define ESPI_VW_MAX_SIZE_SHIFT 13 +#define ESPI_VW_MAX_SIZE_MASK (0x3f << ESPI_VW_MAX_SIZE_SHIFT) + +#define ESPI_GLOBAL_CONTROL_1 0x34 +#define ESPI_SUB_DECODE_SLV_SHIFT 3 +#define ESPI_SUB_DECODE_SLV_MASK (0x3 << ESPI_SUB_DECODE_SLV_SHIFT) +#define ESPI_SUB_DECODE_EN (1 << 2) + +#define SLAVE0_INT_STS 0x70 +#define ESPI_STATUS_DNCMD_COMPLETE (1 << 28) +#define ESPI_STATUS_NON_FATAL_ERROR (1 << 6) +#define ESPI_STATUS_FATAL_ERROR (1 << 5) +#define ESPI_STATUS_NO_RESPONSE (1 << 4) +#define ESPI_STATUS_CRC_ERR (1 << 2) +#define ESPI_STATUS_WAIT_TIMEOUT (1 << 1) +#define ESPI_STATUS_BUS_ERROR (1 << 0) + +#define ESPI_RXVW_POLARITY 0xac + +#define ESPI_CMD_TIMEOUT_US 100 +#define ESPI_CH_READY_TIMEOUT_US 1000 + +union espi_txhdr0 { + uint32_t val; + struct { + uint32_t type:3; + uint32_t cmd_sts:1; + uint32_t slave_sel:2; + uint32_t rsvd:2; + uint32_t hdata0:8; + uint32_t hdata1:8; + uint32_t hdata2:8; + }; +} __packed; + +union espi_txhdr1 { + uint32_t val; + struct { + uint32_t hdata3:8; + uint32_t hdata4:8; + uint32_t hdata5:8; + uint32_t hdata6:8; + }; +} __packed; + +union espi_txhdr2 { + uint32_t val; + struct { + uint32_t hdata7:8; + uint32_t rsvd:24; + }; +} __packed; + +union espi_txdata { + uint32_t val; + struct { + uint32_t byte0:8; + uint32_t byte1:8; + uint32_t byte2:8; + uint32_t byte3:8; + }; +} __packed; + +struct espi_cmd { + union espi_txhdr0 hdr0; + union espi_txhdr1 hdr1; + union espi_txhdr2 hdr2; + union espi_txdata data; +} __packed; + +/* Wait upto ESPI_CMD_TIMEOUT_US for hardware to clear DNCMD_STATUS bit. */ +static int espi_wait_ready(void) +{ + struct stopwatch sw; + union espi_txhdr0 hdr0; + + stopwatch_init_usecs_expire(&sw, ESPI_CMD_TIMEOUT_US); + do { + hdr0.val = espi_read32(ESPI_DN_TX_HDR0); + if (!hdr0.cmd_sts) + return 0; + } while (!stopwatch_expired(&sw)); + + return -1; +} + +/* Clear interrupt status register */ +static void espi_clear_status(void) +{ + uint32_t status = espi_read32(SLAVE0_INT_STS); + if (status) + espi_write32(SLAVE0_INT_STS, status); +} + +/* + * Wait upto ESPI_CMD_TIMEOUT_US for interrupt status register to update after sending a + * command. + */ +static uint32_t espi_check_status(void) +{ + struct stopwatch sw; + uint32_t status; + + stopwatch_init_usecs_expire(&sw, ESPI_CMD_TIMEOUT_US); + do { + status = espi_read32(SLAVE0_INT_STS); + if (status) + return status; + } while (!stopwatch_expired(&sw)); + + printk(BIOS_ERR, "Error: eSPI timed out waiting for status update.\n"); + + return 0; +} + +static void espi_show_failure(const struct espi_cmd *cmd, const char *str, uint32_t status) +{ + printk(BIOS_ERR, "eSPI cmd0-cmd2: %08x %08x %08x data: %08x.\n", + cmd->hdr0.val, cmd->hdr1.val, cmd->hdr2.val, cmd->data.val); + printk(BIOS_ERR, "%s (Status = 0x%x)\n", str, status); +} + +static int espi_send_command(const struct espi_cmd *cmd) +{ + uint32_t status; + + if (CONFIG(ESPI_DEBUG)) + printk(BIOS_ERR, "eSPI cmd0-cmd2: %08x %08x %08x data: %08x.\n", + cmd->hdr0.val, cmd->hdr1.val, cmd->hdr2.val, cmd->data.val); + + if (espi_wait_ready() == -1) { + espi_show_failure(cmd, "Error: eSPI was not ready to accept a command", 0); + return -1; + } + + espi_clear_status(); + + espi_write32(ESPI_DN_TX_HDR1, cmd->hdr1.val); + espi_write32(ESPI_DN_TX_HDR2, cmd->hdr2.val); + espi_write32(ESPI_DN_TX_DATA, cmd->data.val); + + /* Dword 0 must be last as this write triggers the transaction */ + espi_write32(ESPI_DN_TX_HDR0, cmd->hdr0.val); + + if (espi_wait_ready() == -1) { + espi_show_failure(cmd, + "Error: eSPI timed out waiting for command to complete", 0); + return -1; + } + + status = espi_check_status(); + + /* If command did not complete downstream, return error. */ + if (!(status & ESPI_STATUS_DNCMD_COMPLETE)) { + espi_show_failure(cmd, "Error: eSPI downstream command completion failure", + status); + return -1; + } + + if (status & ~ESPI_STATUS_DNCMD_COMPLETE) { + espi_show_failure(cmd, "Error: eSPI status register bits set", status); + return -1; + } + + return 0; +} + +static int espi_send_reset(void) +{ + struct espi_cmd cmd = { + .hdr0 = { + .type = CMD_TYPE_IN_BAND_RESET, + .cmd_sts = 1, + }, + }; + + return espi_send_command(&cmd); +} + +static int espi_send_pltrst_deassert(void) +{ + struct espi_cmd cmd = { + .hdr0 = { + .type = CMD_TYPE_VW, + .cmd_sts = 1, + .hdata0 = 0, /* 1 VW group */ + }, + .data = { + .byte0 = ESPI_VW_INDEX_SYSTEM_EVENT_3, + .byte1 = ESPI_VW_SIGNAL_HIGH(ESPI_VW_PLTRST), + }, + }; + + return espi_send_command(&cmd); +} + +/* + * In case of get configuration command, hdata0 contains bits 15:8 of the slave register address + * and hdata1 contains bits 7:0 of the slave register address. + */ +#define ESPI_CONFIGURATION_HDATA0(a) (((a) >> 8) & 0xff) +#define ESPI_CONFIGURATION_HDATA1(a) ((a) & 0xff) + +static int espi_get_configuration(uint16_t slave_reg_addr, uint32_t *config) +{ + struct espi_cmd cmd = { + .hdr0 = { + .type = CMD_TYPE_GET_CONFIGURATION, + .cmd_sts = 1, + .hdata0 = ESPI_CONFIGURATION_HDATA0(slave_reg_addr), + .hdata1 = ESPI_CONFIGURATION_HDATA1(slave_reg_addr), + }, + }; + + *config = 0; + + if (espi_send_command(&cmd)) + return -1; + + *config = espi_read32(ESPI_DN_TX_HDR1); + + if (CONFIG(ESPI_DEBUG)) + printk(BIOS_DEBUG, "Get configuration for slave register(0x%x): 0x%x\n", + slave_reg_addr, *config); + + return 0; +} + +static int espi_set_configuration(uint16_t slave_reg_addr, uint32_t config) +{ + struct espi_cmd cmd = { + .hdr0 = { + .type = CMD_TYPE_SET_CONFIGURATION, + .cmd_sts = 1, + .hdata0 = ESPI_CONFIGURATION_HDATA0(slave_reg_addr), + .hdata1 = ESPI_CONFIGURATION_HDATA1(slave_reg_addr), + }, + .hdr1 = { + .val = config, + }, + }; + + return espi_send_command(&cmd); +} + +static int espi_get_general_configuration(uint32_t *config) +{ + int ret = espi_get_configuration(ESPI_SLAVE_GENERAL_CFG, config); + if (ret == -1) + return -1; + + espi_show_slave_general_configuration(*config); + return 0; +} + +static void espi_set_io_mode_config(enum espi_io_mode mb_io_mode, uint32_t slave_caps, + uint32_t *slave_config, uint32_t *ctrlr_config) +{ + switch (mb_io_mode) { + case ESPI_IO_MODE_QUAD: + if (espi_slave_supports_quad_io(slave_caps)) { + *slave_config |= ESPI_SLAVE_IO_MODE_SEL_QUAD; + *ctrlr_config |= ESPI_IO_MODE_QUAD; + break; + } + /* Intentional fall-through */ + case ESPI_IO_MODE_DUAL: + if (espi_slave_supports_dual_io(slave_caps)) { + *slave_config |= ESPI_SLAVE_IO_MODE_SEL_DUAL; + *ctrlr_config |= ESPI_IO_MODE_DUAL; + break; + } + /* Intentional fall-through */ + case ESPI_IO_MODE_SINGLE: + /* Single I/O mode is always supported. */ + *slave_config |= ESPI_SLAVE_IO_MODE_SEL_SINGLE; + *ctrlr_config |= ESPI_IO_MODE_SINGLE; + break; + default: + die("No supported I/O modes!\n"); + } +} + +static void espi_set_op_freq_config(enum espi_op_freq mb_op_freq, uint32_t slave_caps, + uint32_t *slave_config, uint32_t *ctrlr_config) +{ + int slave_max_speed_mhz = espi_slave_max_speed_mhz_supported(slave_caps); + + switch (mb_op_freq) { + case ESPI_OP_FREQ_66_MHZ: + if (slave_max_speed_mhz >= 66) { + *slave_config |= ESPI_SLAVE_OP_FREQ_SEL_66_MHZ; + *ctrlr_config |= ESPI_OP_FREQ_66_MHZ; + break; + } + /* Intentional fall-through */ + case ESPI_OP_FREQ_33_MHZ: + if (slave_max_speed_mhz >= 33) { + *slave_config |= ESPI_SLAVE_OP_FREQ_SEL_33_MHZ; + *ctrlr_config |= ESPI_OP_FREQ_33_MHZ; + break; + } + /* Intentional fall-through */ + case ESPI_OP_FREQ_16_MHZ: + /* + * eSPI spec says the minimum frequency is 20MHz, but AMD datasheets support + * 16.7 Mhz. + */ + if (slave_max_speed_mhz > 0) { + *slave_config |= ESPI_SLAVE_OP_FREQ_SEL_20_MHZ; + *ctrlr_config |= ESPI_OP_FREQ_16_MHZ; + break; + } + /* Intentional fall-through */ + default: + die("No support Operating Frequency!\n"); + } +} + +static int espi_set_general_configuration(const struct espi_config *mb_cfg, uint32_t slave_caps) +{ + uint32_t slave_config = 0; + uint32_t ctrlr_config = 0; + + if (mb_cfg->crc_check_enable) { + slave_config |= ESPI_SLAVE_CRC_ENABLE; + ctrlr_config |= ESPI_CRC_CHECKING_EN; + } + + if (mb_cfg->dedicated_alert_pin) { + slave_config |= ESPI_SLAVE_ALERT_MODE_PIN; + ctrlr_config |= ESPI_ALERT_MODE; + } + + espi_set_io_mode_config(mb_cfg->io_mode, slave_caps, &slave_config, &ctrlr_config); + espi_set_op_freq_config(mb_cfg->op_freq_mhz, slave_caps, &slave_config, &ctrlr_config); + + if (CONFIG(ESPI_DEBUG)) + printk(BIOS_INFO, "Setting general configuration: slave: 0x%x controller: 0x%x\n", + slave_config, ctrlr_config); + + if (espi_set_configuration(ESPI_SLAVE_GENERAL_CFG, slave_config) == -1) + return -1; + + espi_write32(ESPI_SLAVE0_CONFIG, ctrlr_config); + return 0; +} + +static int espi_wait_channel_ready(uint16_t slave_reg_addr) +{ + struct stopwatch sw; + uint32_t config; + + stopwatch_init_usecs_expire(&sw, ESPI_CH_READY_TIMEOUT_US); + do { + espi_get_configuration(slave_reg_addr, &config); + if (espi_slave_is_channel_ready(config)) + return 0; + } while (!stopwatch_expired(&sw)); + + printk(BIOS_ERR, "Error: Channel is not ready after %d usec (slave addr: 0x%x)\n", + ESPI_CH_READY_TIMEOUT_US, slave_reg_addr); + return -1; + +} + +static void espi_enable_ctrlr_channel(uint32_t channel_en) +{ + uint32_t reg = espi_read32(ESPI_SLAVE0_CONFIG); + + reg |= channel_en; + + espi_write32(ESPI_SLAVE0_CONFIG, reg); +} + +static int espi_set_channel_configuration(uint32_t slave_config, uint32_t slave_reg_addr, + uint32_t ctrlr_enable) +{ + if (espi_set_configuration(slave_reg_addr, slave_config) == -1) + return -1; + + if (!(slave_config & ESPI_SLAVE_CHANNEL_ENABLE)) + return 0; + + if (espi_wait_channel_ready(slave_reg_addr) == -1) + return -1; + + espi_enable_ctrlr_channel(ctrlr_enable); + return 0; +} + +static int espi_setup_vw_channel(const struct espi_config *mb_cfg, uint32_t slave_caps) +{ + uint32_t slave_vw_caps; + uint32_t ctrlr_vw_caps; + uint32_t slave_vw_count_supp; + uint32_t ctrlr_vw_count_supp; + uint32_t use_vw_count; + uint32_t slave_config; + + if (!mb_cfg->vw_ch_en) + return 0; + + if (!espi_slave_supports_vw_channel(slave_caps)) { + printk(BIOS_ERR, "Error: eSPI slave doesn't support VW channel!\n"); + return -1; + } + + if (espi_get_configuration(ESPI_SLAVE_VW_CFG, &slave_vw_caps) == -1) + return -1; + + ctrlr_vw_caps = espi_read32(ESPI_MASTER_CAP); + ctrlr_vw_count_supp = (ctrlr_vw_caps & ESPI_VW_MAX_SIZE_MASK) >> ESPI_VW_MAX_SIZE_SHIFT; + + slave_vw_count_supp = espi_slave_get_vw_count_supp(slave_vw_caps); + use_vw_count = MIN(ctrlr_vw_count_supp, slave_vw_count_supp); + + slave_config = ESPI_SLAVE_CHANNEL_ENABLE | ESPI_SLAVE_VW_COUNT_SEL_VAL(use_vw_count); + if (espi_set_channel_configuration(slave_config, ESPI_SLAVE_VW_CFG, + ESPI_VW_CH_EN) == -1) + return -1; + + /* Now that VW channel is set up, deassert PLTRST# */ + return espi_send_pltrst_deassert(); +} + +static int espi_setup_periph_channel(const struct espi_config *mb_cfg, uint32_t slave_caps) +{ + uint32_t slave_config; + /* Peripheral channel requires BME bit to be set when enabling the channel. */ + const uint32_t slave_en_mask = ESPI_SLAVE_CHANNEL_READY | + ESPI_SLAVE_PERIPH_BUS_MASTER_ENABLE; + + if (espi_get_configuration(ESPI_SLAVE_PERIPH_CFG, &slave_config) == -1) + return -1; + + /* + * Peripheral channel is the only one which is enabled on reset. So, if the mainboard + * wants to disable it, set configuration to disable peripheral channel. It also + * requires that BME bit be cleared. + */ + if (mb_cfg->periph_ch_en) { + if (!espi_slave_supports_periph_channel(slave_caps)) { + printk(BIOS_ERR, "Error: eSPI slave doesn't support periph channel!\n"); + return -1; + } + slave_config |= slave_en_mask; + } else { + slave_config &= ~slave_en_mask; + } + + return espi_set_channel_configuration(slave_config, ESPI_SLAVE_PERIPH_CFG, + ESPI_PERIPH_CH_EN); +} + +static int espi_setup_oob_channel(const struct espi_config *mb_cfg, uint32_t slave_caps) +{ + uint32_t slave_config; + + if (!mb_cfg->oob_ch_en) + return 0; + + if (!espi_slave_supports_oob_channel(slave_caps)) { + printk(BIOS_ERR, "Error: eSPI slave doesn't support OOB channel!\n"); + return -1; + } + + if (espi_get_configuration(ESPI_SLAVE_OOB_CFG, &slave_config) == -1) + return -1; + + slave_config |= ESPI_SLAVE_CHANNEL_ENABLE; + + return espi_set_channel_configuration(slave_config, ESPI_SLAVE_OOB_CFG, + ESPI_OOB_CH_EN); +} + +static int espi_setup_flash_channel(const struct espi_config *mb_cfg, uint32_t slave_caps) +{ + uint32_t slave_config; + + if (!mb_cfg->flash_ch_en) + return 0; + + if (!espi_slave_supports_flash_channel(slave_caps)) { + printk(BIOS_ERR, "Error: eSPI slave doesn't support flash channel!\n"); + return -1; + } + + if (espi_get_configuration(ESPI_SLAVE_FLASH_CFG, &slave_config) == -1) + return -1; + + slave_config |= ESPI_SLAVE_CHANNEL_ENABLE; + + return espi_set_channel_configuration(slave_config, ESPI_SLAVE_FLASH_CFG, + ESPI_FLASH_CH_EN); +} + +static void espi_set_initial_config(const struct espi_config *mb_cfg) +{ + uint32_t espi_initial_mode = ESPI_OP_FREQ_16_MHZ | ESPI_IO_MODE_SINGLE; + + if (mb_cfg->dedicated_alert_pin) + espi_initial_mode |= ESPI_ALERT_MODE; + + espi_write32(ESPI_SLAVE0_CONFIG, espi_initial_mode); +} + +int espi_setup(void) +{ + uint32_t global_ctrl_reg; + uint32_t slave_caps; + const struct espi_config *cfg = espi_get_config(); + + /* + * Boot sequence: Step 1 + * Set correct initial configuration to talk to the slave: + * Set clock frequency to 16.7MHz and single IO mode. + */ + espi_set_initial_config(cfg); + + /* + * Boot sequence: Step 2 + * Send in-band reset + * The resets affects both host and slave devices, so set initial config again. + */ + if (espi_send_reset() == -1) { + printk(BIOS_ERR, "Error: In-band reset failed!\n"); + return -1; + } + espi_set_initial_config(cfg); + + /* + * Boot sequence: Step 3 + * Get configuration of slave device. + */ + if (espi_get_general_configuration(&slave_caps) == -1) { + printk(BIOS_ERR, "Error: Slave GET_CONFIGURATION failed!\n"); + return -1; + } + + /* + * Boot sequence: + * Step 4: Write slave device general config + * Step 5: Set host slave config + */ + if (espi_set_general_configuration(cfg, slave_caps) == -1) { + printk(BIOS_ERR, "Error: Slave SET_CONFIGURATION failed!\n"); + return -1; + } + + /* + * Setup polarity before enabling the VW channel so any interrupts + * received will have the correct polarity. + */ + espi_write32(ESPI_RXVW_POLARITY, cfg->vw_irq_polarity); + + /* + * Boot Sequences: Steps 6 - 9 + * Channel setup + */ + /* Set up VW first so we can deassert PLTRST#. */ + if (espi_setup_vw_channel(cfg, slave_caps) == -1) { + printk(BIOS_ERR, "Error: Setup VW channel failed!\n"); + return -1; + } + + if (espi_setup_periph_channel(cfg, slave_caps) == -1) { + printk(BIOS_ERR, "Error: Setup Periph channel failed!\n"); + return -1; + } + + if (espi_setup_oob_channel(cfg, slave_caps) == -1) { + printk(BIOS_ERR, "Error: Setup OOB channel failed!\n"); + return -1; + } + + if (espi_setup_flash_channel(cfg, slave_caps) == -1) { + printk(BIOS_ERR, "Error: Setup Flash channel failed!\n"); + return -1; + } + + /* Enable subtractive decode if configured */ + global_ctrl_reg = espi_read32(ESPI_GLOBAL_CONTROL_1); + if (cfg->subtractive_decode) { + global_ctrl_reg &= ~ESPI_SUB_DECODE_SLV_MASK; + global_ctrl_reg |= ESPI_SUB_DECODE_EN; + + } else { + global_ctrl_reg &= ~ESPI_SUB_DECODE_EN; + } + espi_write32(ESPI_GLOBAL_CONTROL_1, global_ctrl_reg); + + return 0; +} -- To view, visit https://review.coreboot.org/c/coreboot/+/41272 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I872ec09cd92e9bb53f22e38d2773f3491355279e Gerrit-Change-Number: 41272 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

6 24

Change in coreboot[master]: [NOTFORMERGE_YET]soc/intel/xeon_sp/cpx: Hook up microcode update
by Arthur Heymans (Code Review) 11 Dec '21

11 Dec '21

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49082 ) Change subject: [NOTFORMERGE_YET]soc/intel/xeon_sp/cpx: Hook up microcode update ...................................................................... [NOTFORMERGE_YET]soc/intel/xeon_sp/cpx: Hook up microcode update Update the Intel microcode repo to the microcode-20201118 tag. The CPX microcode updates hit the microcode repo as of 20201110 release. Some CPUs seem to have stability issues with these newer microcode updates: See https://review.coreboot.org/c/coreboot/+/47914 for discussion on that. So it might be better to wait until Intel fixes those. Change-Id: Ie1776925e7f6c1523d6b398a90949c5bbd471203 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M 3rdparty/intel-microcode M src/soc/intel/xeon_sp/cpx/Kconfig M src/soc/intel/xeon_sp/cpx/Makefile.inc 3 files changed, 3 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/49082/1 diff --git a/3rdparty/intel-microcode b/3rdparty/intel-microcode index 0e4288f..49bb67f 160000 --- a/3rdparty/intel-microcode +++ b/3rdparty/intel-microcode @@ -1 +1 @@ -Subproject commit 0e4288f81f806620c65f70ee2bcf94b69d574096 +Subproject commit 49bb67f32a2e3e631ba1a9a73da1c52e1cac7fd9 diff --git a/src/soc/intel/xeon_sp/cpx/Kconfig b/src/soc/intel/xeon_sp/cpx/Kconfig index 7b583cd..cd4384b 100644 --- a/src/soc/intel/xeon_sp/cpx/Kconfig +++ b/src/soc/intel/xeon_sp/cpx/Kconfig @@ -2,10 +2,6 @@ if SOC_INTEL_COOPERLAKE_SP -config SOC_SPECIFIC_OPTIONS - def_bool y - select MICROCODE_BLOB_NOT_HOOKED_UP - config FSP_HEADER_PATH string "Location of FSP headers" depends on MAINBOARD_USES_FSP2_0 diff --git a/src/soc/intel/xeon_sp/cpx/Makefile.inc b/src/soc/intel/xeon_sp/cpx/Makefile.inc index a5c6848..97c156c 100644 --- a/src/soc/intel/xeon_sp/cpx/Makefile.inc +++ b/src/soc/intel/xeon_sp/cpx/Makefile.inc @@ -19,4 +19,6 @@ CPPFLAGS_common += -I$(src)/soc/intel/xeon_sp/cpx/include -I$(src)/soc/intel/xeon_sp/cpx +cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-55-0b + endif ## CONFIG_SOC_INTEL_COOPERLAKE_SP -- To view, visit https://review.coreboot.org/c/coreboot/+/49082 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1776925e7f6c1523d6b398a90949c5bbd471203 Gerrit-Change-Number: 49082 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: cbfs: Add verification for RO CBFS master hash
by Julius Werner (Code Review) 08 Dec '21

08 Dec '21

Hello Patrick Georgi, Aaron Durbin, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/41120 to review the following change. Change subject: cbfs: Add verification for RO CBFS master hash ...................................................................... cbfs: Add verification for RO CBFS master hash This patch adds the first stage of the new CONFIG_CBFS_VERIFICATION feature. It's not useful to end-users in this stage so it cannot be selected in menuconfig (and should not be used other than for development) yet. With this patch coreboot can verify the master hash of the RO CBFS when it starts booting, but it does not verify individual files yet. Likewise, verifying RW CBFSes with vboot is not yet supported. Verification is bootstrapped from a "master hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS master hash and a separate hash for the FMAP which is required to find the primary CBFS. Both are verified on first use in the bootblock (and halt the system on failure). The CONFIG_TOCTOU_SAFETY option is also added for illustrative purposes to show some paths that need to be different when full protection against TOCTOU (time-of-check vs. time-of-use) attacks is desired. For normal verification it is sufficient to check the FMAP and the CBFS master hash only once in the bootblock -- for TOCTOU verification we do the same, but we need to be extra careful that we do not re-read the FMAP or any CBFS metadata in later stages. This is mostly achieved by depending on the CBFS metadata cache and FMAP cache features, but we allow for one edge case in case the RW CBFS metadata cache overflows (which may happen during an RW update and could otherwise no longer be fixed because mcache size is defined by RO code). This code is added to demonstrate design intent but won't really matter until RW CBFS verification can be supported. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/master_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/master_hash.h A src/lib/Kconfig.cbfs_verification M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c A src/lib/master_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c 13 files changed, 218 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/1 diff --git a/src/commonlib/bsd/include/commonlib/bsd/master_hash.h b/src/commonlib/bsd/include/commonlib/bsd/master_hash.h new file mode 100644 index 0000000..f8bad6c --- /dev/null +++ b/src/commonlib/bsd/include/commonlib/bsd/master_hash.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-only */ + +#ifndef _COMMONLIB_BSD_MASTER_HASH_H_ +#define _COMMONLIB_BSD_MASTER_HASH_H_ + +#include <stdint.h> +#include <vb2_sha.h> + +/* This structure is embedded somewhere in the (uncompressed) bootblock. */ +struct master_hash_anchor { + uint8_t magic[8]; + struct vb2_hash cbfs_hash; + /* NOTE: This is just reserving space. The FMAP hash is placed right after the CBFS + hash above, which may be shorter than the compile-time size of struct vb2_hash! */ + uint8_t reserved_space_for_fmap_hash[VB2_MAX_DIGEST_SIZE]; +} __packed; + +/* Always use this function to figure out the actual location of the FMAP hash. It always uses + the same algorithm as the CBFS hash. */ +static inline uint8_t *master_hash_anchor_fmap_hash(struct master_hash_anchor *anchor) +{ + return anchor->cbfs_hash.raw + vb2_digest_size(anchor->cbfs_hash.algo); +} + +/* Must *not* appear anywhere else in coreboot code (other than the anchor). */ +#define MASTER_HASH_ANCHOR_MAGIC "\xadMstHsh\x15" + +#endif /* _COMMONLIB_BSD_MASTER_HASH_H_ */ diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 7505fb2..710fd3b 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -25,6 +25,7 @@ #define CBMEM_ID_IGD_OPREGION 0x4f444749 #define CBMEM_ID_IMD_ROOT 0xff4017ff #define CBMEM_ID_IMD_SMALL 0x53a11439 +#define CBMEM_ID_MASTER_HASH 0x6873484D #define CBMEM_ID_MEMINFO 0x494D454D #define CBMEM_ID_MMA_DATA 0x4D4D4144 #define CBMEM_ID_MMC_STATUS 0x4d4d4353 diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 66cbe21..da3b80a 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -7,6 +7,7 @@ #include <commonlib/cbfs.h> #include <program_loading.h> #include <types.h> +#include <vb2_sha.h> /*********************************************** * Perform CBFS operations on the boot device. * @@ -65,4 +66,13 @@ */ const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro); +/* + * Builds the mcache (if |cbd->mcache| is set) and verifies the master hash (if + * |master_hash| is not NULL). If CB_CBFS_CACHE_FULL is returned, the mcache is + * incomplete but still valid and the master hash was still verified. Should be + * called once per *boot* (not once per stage) before the first CBFS access. + */ +cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd, + struct vb2_hash *master_hash); + #endif diff --git a/src/include/cbfs_glue.h b/src/include/cbfs_glue.h index 7c40714..7836e72 100644 --- a/src/include/cbfs_glue.h +++ b/src/include/cbfs_glue.h @@ -6,7 +6,7 @@ #include <commonlib/region.h> #include <console/console.h> -#define CBFS_ENABLE_HASHING 0 +#define CBFS_ENABLE_HASHING CONFIG(CBFS_VERIFICATION) #define ERROR(...) printk(BIOS_ERR, "CBFS ERROR: " __VA_ARGS__) #define LOG(...) printk(BIOS_ERR, "CBFS: " __VA_ARGS__) diff --git a/src/include/master_hash.h b/src/include/master_hash.h new file mode 100644 index 0000000..2ce1521 --- /dev/null +++ b/src/include/master_hash.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef _MASTER_HASH_H_ +#define _MASTER_HASH_H_ + +#include <commonlib/bsd/master_hash.h> + +vb2_error_t master_hash_verify_fmap(const void *fmap_base, size_t fmap_size); + +#if CONFIG(CBFS_VERIFICATION) +struct vb2_hash *master_hash_get(void); +#else +static inline struct vb2_hash *master_hash_get(void) { return NULL; } +#endif + +#endif diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification new file mode 100644 index 0000000..c11f1af --- /dev/null +++ b/src/lib/Kconfig.cbfs_verification @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-or-later +# +# This file is part of the coreboot project. +# +# This file is sourced from src/security/Kconfig for menuconfig convenience. + +#menu "CBFS verification" # TODO: enable once it works + +config CBFS_VERIFICATION + bool # TODO: make user selectable once it works + depends on !COMPRESS_BOOTBLOCK # TODO: figure out decompressor anchor + help + Work in progress. Do not use (yet). + +config TOCTOU_SAFETY + bool + depends on CBFS_VERIFICATION + depends on !NO_FMAP_CACHE + depends on !NO_CBFS_MCACHE + help + Work in progress. Not actually TOCTOU safe yet. Do not use. + + Design idea here is that mcache overflows in this mode are only legal + for the RW CBFS, because it's relatively easy to retrieve the RW + master hash from persistent vboot context at any time, but the RO + master hash is lost after the bootblock is unloaded. This avoids the + need to carry yet another piece forward through the stages. Mcache + overflows are mostly a concern for RW updates (if an update adds more + files than originally planned for), for the RO section it should + always be possible to dimension the mcache correctly beforehand, so + this should be an acceptable limitation. + +config CBFS_HASH_ALGO + int + default 1 if CBFS_HASH_SHA1 + default 2 if CBFS_HASH_SHA256 + default 3 if CBFS_HASH_SHA512 + +choice + prompt "--> hash type" + depends on CBFS_VERIFICATION + default CBFS_HASH_SHA256 + +config CBFS_HASH_SHA1 + bool "SHA-1" + +config CBFS_HASH_SHA256 + bool "SHA-256" + +config CBFS_HASH_SHA512 + bool "SHA-512" + +endchoice + +#endmenu diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 085f6b2..a031c01a 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -37,6 +37,7 @@ bootblock-y += cbfs.c bootblock-$(CONFIG_GENERIC_GPIO_LIB) += gpio.c bootblock-y += libgcc.c +bootblock-$(CONFIG_CBFS_VERIFICATION) += master_hash.c bootblock-$(CONFIG_GENERIC_UDELAY) += timer.c bootblock-$(CONFIG_COLLECT_TIMESTAMPS) += timestamp.c diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 201e705..992d104 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -11,6 +11,7 @@ #include <endian.h> #include <fmap.h> #include <lib.h> +#include <master_hash.h> #include <security/tpm/tspi/crtm.h> #include <security/vboot/vboot_common.h> #include <stdlib.h> @@ -30,8 +31,20 @@ if (!CONFIG(NO_CBFS_MCACHE)) err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size, name, mdata, &data_offset); - if (err == CB_CBFS_CACHE_FULL) - err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, NULL); + if (err == CB_CBFS_CACHE_FULL) { + struct vb2_hash *master_hash = NULL; + if (CONFIG(TOCTOU_SAFETY)) { + if (cbd != vboot_get_cbfs_boot_device()) { + printk(BIOS_ERR, + "ERROR: RO mcache overflow for %s breaks TOCTOU safety!\n", + name); + return CB_CBFS_CACHE_FULL; + } + /* TODO: set master_hash to RW master hash here. */ + } + err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, + master_hash); + } if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro && err == CB_CBFS_NOT_FOUND) { @@ -308,6 +321,26 @@ return 0; } +cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd, + struct vb2_hash *master_hash) +{ + /* If we have an mcache, mcache_build() will also check master hash. */ + if (!CONFIG(NO_CBFS_MCACHE) && cbd->mcache_size > 0) + return cbfs_mcache_build(&cbd->rdev, cbd->mcache, + cbd->mcache_size, master_hash); + + /* No mcache and no verification means we have nothing special to do. */ + if (!CONFIG(CBFS_VERIFICATION) || !master_hash) + return CB_SUCCESS; + + /* Verification only: use cbfs_walk() without a walker() function to + just run through the CBFS once, will return NOT_FOUND by default. */ + cb_err_t err = cbfs_walk(&cbd->rdev, NULL, NULL, master_hash, 0); + if (err == CB_CBFS_NOT_FOUND) + err = CB_SUCCESS; + return err; +} + const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro) { static struct cbfs_boot_device ro; @@ -328,7 +361,7 @@ return &ro; if (fmap_locate_area_as_rdev("COREBOOT", &ro.rdev)) - return NULL; + die("Cannot locate primary CBFS"); if (!CONFIG(NO_CBFS_MCACHE)) { const struct cbmem_entry *entry; @@ -341,12 +374,14 @@ ro.mcache_size = REGION_SIZE(cbfs_mcache) * (100 - CONFIG_CBFS_MCACHE_RW_PERCENTAGE) / 100; } - if (ENV_BOOTBLOCK) { - cb_err_t err = cbfs_mcache_build(&ro.rdev, ro.mcache, - ro.mcache_size, NULL); - if (err && err != CB_CBFS_CACHE_FULL) - die("Failed to build RO mcache"); - } + } + + if (ENV_BOOTBLOCK) { + cb_err_t err = cbfs_init_boot_device(&ro, master_hash_get()); + if (err == CB_CBFS_HASH_MISMATCH) + die("RO CBFS master hash verification failure"); + else if (err && err != CB_CBFS_CACHE_FULL) + die("RO CBFS initialization error: %d", err); } return &ro; diff --git a/src/lib/fmap.c b/src/lib/fmap.c index ecd23f6..197ad0e 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -5,6 +5,7 @@ #include <cbmem.h> #include <console/console.h> #include <fmap.h> +#include <master_hash.h> #include <stddef.h> #include <string.h> #include <symbols.h> @@ -28,9 +29,20 @@ return FMAP_OFFSET; } -static int check_signature(const struct fmap *fmap) +static int verify_fmap(const struct fmap *fmap) { - return memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature)); + if (memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature))) + return -1; + + static bool done = false; + if (!CONFIG(CBFS_VERIFICATION) || !ENV_BOOTBLOCK || done) + return 0; /* Only need to check hash in first stage. */ + + if (master_hash_verify_fmap(fmap, FMAP_SIZE) != VB2_SUCCESS) + return -1; + + done = true; + return 0; } static void report(const struct fmap *fmap) @@ -61,10 +73,12 @@ /* NOTE: This assumes that for all platforms running this code, the bootblock is the first stage and the bootblock will make at least one FMAP access (usually from finding CBFS). */ - if (!check_signature(fmap)) + if (!verify_fmap(fmap)) goto register_cache; printk(BIOS_ERR, "ERROR: FMAP cache corrupted?!\n"); + if (CONFIG(TOCTOU_SAFETY)) + die("TOCTOU safety relies on FMAP cache"); } /* In case we fail below, make sure the cache is invalid. */ @@ -78,7 +92,7 @@ /* memlayout statically guarantees that the FMAP_CACHE is big enough. */ if (rdev_readat(boot_rdev, fmap, FMAP_OFFSET, FMAP_SIZE) != FMAP_SIZE) return; - if (check_signature(fmap)) + if (verify_fmap(fmap)) return; report(fmap); @@ -109,8 +123,9 @@ if (fmap == NULL) return -1; - if (check_signature(fmap)) { - printk(BIOS_DEBUG, "No FMAP found at %zx offset.\n", offset); + if (verify_fmap(fmap)) { + printk(BIOS_ERR, "FMAP missing or corrupted at offset 0x%zx!\n", + offset); rdev_munmap(boot, fmap); return -1; } diff --git a/src/lib/master_hash.c b/src/lib/master_hash.c new file mode 100644 index 0000000..a4b5943 --- /dev/null +++ b/src/lib/master_hash.c @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <assert.h> +#include <cbmem.h> +#include <master_hash.h> +#include <symbols.h> + +__attribute__((used, section(".master_hash_anchor"))) +struct master_hash_anchor master_hash_anchor = { + .magic = MASTER_HASH_ANCHOR_MAGIC, + .cbfs_hash = { .algo = CONFIG_CBFS_HASH_ALGO } +}; + +struct vb2_hash *master_hash_get(void) +{ + return &master_hash_anchor.cbfs_hash; +} + +vb2_error_t master_hash_verify_fmap(const void *fmap_buffer, size_t fmap_size) +{ + struct vb2_hash hash = { .algo = master_hash_anchor.cbfs_hash.algo }; + memcpy(hash.raw, master_hash_anchor_fmap_hash(&master_hash_anchor), + vb2_digest_size(hash.algo)); + return vb2_hash_verify(fmap_buffer, fmap_size, &hash); +} diff --git a/src/lib/program.ld b/src/lib/program.ld index 6f096dc..cb753e8 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -28,6 +28,7 @@ CONFIG(ARCH_BOOTBLOCK_X86_64)) KEEP(*(.id)); #endif + KEEP(*(.master_hash_anchor)); *(.text); *(.text.*); diff --git a/src/security/Kconfig b/src/security/Kconfig index 65d2def..8987369 100644 --- a/src/security/Kconfig +++ b/src/security/Kconfig @@ -11,6 +11,10 @@ ## GNU General Public License for more details. ## +# These features are implemented in src/lib/cbfs.c, but they are security +# features so sort them in here for menuconfig. +source "src/lib/Kconfig.cbfs_verification" + source "src/security/vboot/Kconfig" source "src/security/tpm/Kconfig" source "src/security/memory/Kconfig" diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index e6ac5cf..f3b1c7c 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -25,18 +25,17 @@ int vboot_executed; -static void build_rw_mcache(void) +static void after_verstage(void) { - if (CONFIG(NO_CBFS_MCACHE)) - return; + vboot_executed = 1; /* Mark verstage execution complete. */ const struct cbfs_boot_device *cbd = vboot_get_cbfs_boot_device(); - if (!cbd) /* Don't build RW mcache in recovery mode. */ + if (!cbd) /* Can't initialize RW CBFS in recovery mode. */ return; - cb_err_t err = cbfs_mcache_build(&cbd->rdev, cbd->mcache, - cbd->mcache_size, NULL); - if (err && err != CB_CBFS_CACHE_FULL) - die("Failed to build RW mcache."); /* TODO: -> recovery? */ + + cb_err_t err = cbfs_init_boot_device(cbd, NULL); /* TODO: RW hash */ + if (err && err != CB_CBFS_CACHE_FULL) /* TODO: -> recovery? */ + die("RW CBFS initialization failure: %d", err); } void vboot_run_logic(void) @@ -44,8 +43,7 @@ if (verification_should_run()) { /* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */ verstage_main(); - vboot_executed = 1; - build_rw_mcache(); + after_verstage(); } else if (verstage_should_load()) { struct cbfsf file; struct prog verstage = @@ -72,8 +70,7 @@ if (!CONFIG(VBOOT_RETURN_FROM_VERSTAGE)) return; - vboot_executed = 1; - build_rw_mcache(); + after_verstage(); } } -- To view, visit https://review.coreboot.org/c/coreboot/+/41120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 Gerrit-Change-Number: 41120 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> 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

4 18

Change in coreboot[master]: soc/intel/tigerlake: Use Kconfig option per variant
by Felix Singer (Code Review) 05 Dec '21

05 Dec '21

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49055 ) Change subject: soc/intel/tigerlake: Use Kconfig option per variant ...................................................................... soc/intel/tigerlake: Use Kconfig option per variant Change-Id: I575f28c7eaa82ed5d52ba7c9328a52e76d71dc5b Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/Makefile.inc 5 files changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/49055/1 diff --git a/src/mainboard/google/deltaur/Kconfig b/src/mainboard/google/deltaur/Kconfig index 8d95849..0dfec23 100644 --- a/src/mainboard/google/deltaur/Kconfig +++ b/src/mainboard/google/deltaur/Kconfig @@ -14,7 +14,7 @@ select MAINBOARD_HAS_I2C_TPM_CR50 select MAINBOARD_HAS_TPM2 select MAINBOARD_USES_IFD_EC_REGION - select SOC_INTEL_TIGERLAKE + select SOC_INTEL_TIGERLAKE_CLIENT select SYSTEM_TYPE_LAPTOP select MAINBOARD_USES_IFD_GBE_REGION if BOARD_GOOGLE_DELTAN select SOC_INTEL_COMMON_BLOCK_HDA_VERB diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index c776ba1..a602932 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -30,7 +30,7 @@ select MAINBOARD_HAS_I2C_TPM_CR50 if BOARD_GOOGLE_VOLTEER2_TI50 select MAINBOARD_HAS_TPM2 select PCIEXP_HOTPLUG - select SOC_INTEL_TIGERLAKE + select SOC_INTEL_TIGERLAKE_CLIENT select HAVE_SPD_IN_CBFS if BOARD_GOOGLE_BASEBOARD_VOLTEER diff --git a/src/mainboard/intel/tglrvp/Kconfig b/src/mainboard/intel/tglrvp/Kconfig index 9df542d..8aedefc 100644 --- a/src/mainboard/intel/tglrvp/Kconfig +++ b/src/mainboard/intel/tglrvp/Kconfig @@ -13,7 +13,8 @@ select DRIVERS_INTEL_PMC select DRIVERS_USB_ACPI select DRIVERS_SPI_ACPI - select SOC_INTEL_TIGERLAKE + select SOC_INTEL_TIGERLAKE_IOT if BOARD_INTEL_TGLRVP_UP3 + select SOC_INTEL_TIGERLAKE_CLIENT if BOARD_INTEL_TGLRVP_UP4 select SOC_INTEL_COMMON_BLOCK_DTT select INTEL_LPSS_UART_FOR_CONSOLE select DRIVERS_INTEL_ISH diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index c7e10a9..0267252 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -1,9 +1,17 @@ -config SOC_INTEL_TIGERLAKE +config SOC_INTEL_TIGERLAKE_COMMON bool help Intel Tigerlake support -if SOC_INTEL_TIGERLAKE +config SOC_INTEL_TIGERLAKE_IOT + bool + select SOC_INTEL_TIGERLAKE_COMMON + +config SOC_INTEL_TIGERLAKE_CLIENT + bool + select SOC_INTEL_TIGERLAKE_COMMON + +if SOC_INTEL_TIGERLAKE_COMMON config CPU_SPECIFIC_OPTIONS def_bool y diff --git a/src/soc/intel/tigerlake/Makefile.inc b/src/soc/intel/tigerlake/Makefile.inc index 4a41812..d695a24 100644 --- a/src/soc/intel/tigerlake/Makefile.inc +++ b/src/soc/intel/tigerlake/Makefile.inc @@ -1,4 +1,4 @@ -ifeq ($(CONFIG_SOC_INTEL_TIGERLAKE),y) +ifeq ($(CONFIG_SOC_INTEL_TIGERLAKE_COMMON),y) subdirs-y += romstage subdirs-y += ../../../cpu/intel/microcode -- To view, visit https://review.coreboot.org/c/coreboot/+/49055 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I575f28c7eaa82ed5d52ba7c9328a52e76d71dc5b Gerrit-Change-Number: 49055 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: newchange

8 22

Change in coreboot[master]: soc/intel/*/Kconfig: Enable x86_64 Kconfig option
by Patrick Rudolph (Code Review) 30 Nov '21

30 Nov '21

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48244 ) Change subject: soc/intel/*/Kconfig: Enable x86_64 Kconfig option ...................................................................... soc/intel/*/Kconfig: Enable x86_64 Kconfig option Tested on Intel Skylake. Compiles and boots without issues. Used tianocore as payload and could boot into Ubuntu 20. Other platforms might not compile due to drivers not being ported to x86_64 or might not boot due to a yet unsupported boot flow. Untested: * FSP MPinit * FSP-T * SMM loader v2 * libgfxinit * FSP graphics init Change-Id: Id2919766acc2e11d81c279c23380a9e6fd50f0bb Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M src/soc/intel/alderlake/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/elkhartlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig 10 files changed, 24 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/48244/1 diff --git a/src/soc/intel/alderlake/Kconfig b/src/soc/intel/alderlake/Kconfig index d6db6a0..61d8ee4 100644 --- a/src/soc/intel/alderlake/Kconfig +++ b/src/soc/intel/alderlake/Kconfig @@ -8,7 +8,8 @@ config CPU_SPECIFIC_OPTIONS def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index 29e133c..0df396d 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -22,7 +22,8 @@ def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES select ACPI_NO_PCAT_8259 - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES # CPU specific options select CPU_INTEL_COMMON diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index f256405..07cdeb5 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -74,7 +74,8 @@ def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES select ACPI_NHLT - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 9023b58..6a95b4d 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -1,3 +1,9 @@ +config EXP_ARCH_X86_64 + bool "Experimental x86_64 support" + default n + help + This option will compile and run coreboot in x86_64 long mode. + config SOC_INTEL_COMMON_BLOCK_CPU bool default n diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig index 98c61ce..9ad2a70 100644 --- a/src/soc/intel/denverton_ns/Kconfig +++ b/src/soc/intel/denverton_ns/Kconfig @@ -13,7 +13,8 @@ config CPU_SPECIFIC_OPTIONS def_bool y - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select DEBUG_GPIO select SOC_INTEL_COMMON diff --git a/src/soc/intel/elkhartlake/Kconfig b/src/soc/intel/elkhartlake/Kconfig index 64680e8..4cfee7e 100644 --- a/src/soc/intel/elkhartlake/Kconfig +++ b/src/soc/intel/elkhartlake/Kconfig @@ -8,7 +8,8 @@ config CPU_SPECIFIC_OPTIONS def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 5fdfa2f..4dedbe7 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -8,7 +8,8 @@ config CPU_SPECIFIC_OPTIONS def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON diff --git a/src/soc/intel/jasperlake/Kconfig b/src/soc/intel/jasperlake/Kconfig index c23e3ce..683865a 100644 --- a/src/soc/intel/jasperlake/Kconfig +++ b/src/soc/intel/jasperlake/Kconfig @@ -8,7 +8,8 @@ config CPU_SPECIFIC_OPTIONS def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index c61304b..bb39571 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -19,7 +19,8 @@ def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES select ACPI_NHLT - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index b95b2f0..e4cd466 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -8,7 +8,8 @@ config CPU_SPECIFIC_OPTIONS def_bool y select ACPI_INTEL_HARDWARE_SLEEP_VALUES - select ARCH_ALL_STAGES_X86_32 + select ARCH_ALL_STAGES_X86_32 if !EXP_ARCH_X86_64 + select ARCH_ALL_STAGES_X86_64 if EXP_ARCH_X86_64 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select CPU_INTEL_COMMON -- To view, visit https://review.coreboot.org/c/coreboot/+/48244 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id2919766acc2e11d81c279c23380a9e6fd50f0bb Gerrit-Change-Number: 48244 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Mariusz Szafrański <mariuszx.szafranski(a)intel.com> Gerrit-Reviewer: Michal Motyl <michalx.motyl(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com> Gerrit-MessageType: newchange

9 29

Change in coreboot[master]: Documentation/lint: Use Super I/O instead of SuperIO
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39451 ) Change subject: Documentation/lint: Use Super I/O instead of SuperIO ...................................................................... Documentation/lint: Use Super I/O instead of SuperIO Change-Id: Idb16092b687ebffb319bc1908f08f350d612d36a Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- M Documentation/superio/nuvoton/nct5539d.md M util/lint/spelling.txt 2 files changed, 4 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/39451/1 diff --git a/Documentation/superio/nuvoton/nct5539d.md b/Documentation/superio/nuvoton/nct5539d.md index e91ebc3..009d329 100644 --- a/Documentation/superio/nuvoton/nct5539d.md +++ b/Documentation/superio/nuvoton/nct5539d.md @@ -1,9 +1,9 @@ -# NCT5539D SuperIO +# NCT5539D Super I/O -The SuperIO has the ID `0xd121` and the source can be found in +The Super I/O has the ID `0xd121` and the source can be found in `src/superio/nuvoton/nct5539d/`. ## For developers -The SuperIO generates ACPI using the +The Super I/O generates ACPI using the [SSDT generator for generic SuperIOs](../common/ssdt.md). diff --git a/util/lint/spelling.txt b/util/lint/spelling.txt index 1263144..f74abf1 100644 --- a/util/lint/spelling.txt +++ b/util/lint/spelling.txt @@ -16,6 +16,7 @@ FTBS||FTBFS POSIX-complient||POSIX-compliant READEME||README +SuperIO||Super I/O aaccessibility||accessibility aaccession||accession abailable||available -- To view, visit https://review.coreboot.org/c/coreboot/+/39451 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idb16092b687ebffb319bc1908f08f350d612d36a Gerrit-Change-Number: 39451 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 7

Change in coreboot[master]: util/testing: Build test all our tools
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49126 ) Change subject: util/testing: Build test all our tools ...................................................................... util/testing: Build test all our tools https://qa.coreboot.org/job/untested-coreboot-files reports a bunch of untouched Makefiles, so we never even attempt to build those tools. This commit currently doesn't build and serves as a reminder that there are tools to fix. Change-Id: Ib6378dbaef9787b3cd3e047068d01c73927fa73f Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- M util/testing/Makefile.inc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/49126/1 diff --git a/util/testing/Makefile.inc b/util/testing/Makefile.inc index da6db60..ad30514 100644 --- a/util/testing/Makefile.inc +++ b/util/testing/Makefile.inc @@ -61,7 +61,9 @@ intelvbttool \ nvramtool \ post \ -superiotool +superiotool \ +uio_usbdebug \ +vgabios TEST_PAYLOADLIST_INTERNAL= \ coreinfo \ -- To view, visit https://review.coreboot.org/c/coreboot/+/49126 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib6378dbaef9787b3cd3e047068d01c73927fa73f Gerrit-Change-Number: 49126 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: Documentation: Add proposal for firmware testing
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37643 ) Change subject: Documentation: Add proposal for firmware testing ...................................................................... Documentation: Add proposal for firmware testing Change-Id: Icb9380050f8ff1aa13ecbb501079e2556e43ca06 Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- A Documentation/technotes/2019-12-firmware-testing.md M Documentation/technotes/index.md 2 files changed, 176 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/37643/1 diff --git a/Documentation/technotes/2019-12-firmware-testing.md b/Documentation/technotes/2019-12-firmware-testing.md new file mode 100644 index 0000000..64838a7 --- /dev/null +++ b/Documentation/technotes/2019-12-firmware-testing.md @@ -0,0 +1,175 @@ +# Draft design of a firmware-aware integration test system + +## Problem statement + +Most full system test systems have assumptions baked in that make them +unsuitable for firmware level testing. For example, both Chrome OS test +runners assume that the DUT (Device Under Test) has an operational +kernel + network + sshd. That's a useful assumption when covering +userland testing, but not for firmware. Lava has its downsides as well +(TODO: as reported by Philipp, need details). + +This document outlines a system that, when implemented, helps +automatically test properties of firmware level code. It's _likely_ +also suitable for later stages (kernel and userland), but they're +not the main concern: there's enough tooling out there for them. + +## Collecting requirements + +This project doesn't consider a test system running only on the DUT +so it will require a test host that remains in control. Apart from +that, the system should be useful at all ends of the problem space: +hobbyists with little money to spare just the same as test labs +running automated continuous testing. + +Beyond DUT and test host, the system requires some way for them to +communicate, so we'll expect a bidirectional serial console connection +between them (although we don't care much about how this connection +looks like in practice). + +Such a bare-bone operation might require manual intervention directed +by the test host (e.g. the host asking the user to reboot the DUT when +it got stuck). It may also be augmented by additional circuitry that +allows automating these steps. + +These constraints ensure that a test suite can be executed "one-off" +by a developer with minimal hardware investment while also allowing +automated systems to execute the tests without user intervention. + +So these are + +*Requirement 1*: The system must be usable with minimal investment for +manual operation: a host, a DUT and a bidirectional serial connection +between them. + +and + +*Requirement 2*: The system must be able to drive additional controls +to support fully automated operation. + +There will be lots of different types of DUTs, and not all tests +apply to all of them: There's no battery on a desktop board, so no +need to test charging. A test for charging should be automatically +skipped on such a DUT without counting as failure. + +*Requirement 3*: The system must allow to specify DUTs and their +properties, as well as the properties that a test requires to be +meaningful. + +Tests should be self-contained. This means that they should start +from a flash cycle (that a tool like flashrom would optimize away to +a no-op if flash is still in the expected state) and power-on. + +*Requirement 4*: Tests always start with the required firmware image +to write and explicit power-on. + +Tests must be able to state that other tests must have succeeded before +them: A test that exercises boot loader options doesn't fail when the +boot process never gets to the options screen, it just shouldn't run. + +*Requirement 5*: Tests must be able to declare tests they depend on. + +State transitions should be well-defined: The DUT can move from the +power-off state to the power-on state. It can also fail to do so and +remain powered-off. These things need to be spelled out, together +with the consequences that arise from each transition: it's expected +that most transitions would be failures, but that should be noted down. + +And yet, this verbosity shouldn't mean that the tests become +unreadable. Ideally they could be used by a human operator as "regular +text" (with odd characters interspersed) outlining steps to execute +manually, without a test runner. + +*Requirement 6*: It must be simple to state "Initiate action, success +is W, X while failure is Y, Z". + +During the test, the host monitors the serial console and extracts +events from it. Such events could be, for example, "coreboot enters +ramstage". A test can expect events to appear on the stream and fail if +unexpected events appear or no events appear within a given deadline. + +The observed events are reported to the user. + +*Requirement 7*: There's an event parser looking for things that +happen on the DUT. + +The host can send characters to the DUT, for example in a boot loader +or on a terminal provided by an OS that eventually boots. + +*Requirement 8*: Test must be able to run an expect-like language to +drive complex interactions on the serial console. + +Some tests may be better run on the target system (e.g. to parse +out tables in memory, or otherwise inspect boot states). They should +have a minimal set of dependencies so they don't fail because the base +system changed slightly, be portable across architectures and operating +systems. The language should be popular and the implementation robust. + +*Requirement 9*: Implementation language must be popular, robust, +portable and operate with minimal dependencies. + +## Implementation proposal + +The test system will be implemented in Go: it's reasonably popular +(unlike Forth), robust (unlike shell scripts), its statically linked +binaries have minimal dependencies on the host environment (unlike +Python) and it supports multiple architectures and operating systems. + +The build creates a host side test runner (native OS/arch) and a +variable number of DUT test runners (for all supported DUT OS/arch +pairs) that are provided to the DUT on some storage medium (e.g. USB +stick). In a fully automated environment this might require switching +USB ports between host and DUT. + +There's a [liberally licensed implementation of expect][goexpect] that +could be integrated for handling interactive consoles. It supports +parallel execution which is useful to have a listener on the serial +console (when not driven by expect). + +[goexpect]: https://github.com/google/goexpect + +The test runner is called with the names of a set of tests to execute +(individually or as suites that tests can be assigned to) and the +type of DUT to work with. + +From this, it knows how to drive the DUT. The "manual operation" mode +would be just a special type of button driver that asks the user to +conduct some operation and potentially to press Enter. + +Tests are defined in go packages, with per-package set up and tear +down routines. The runner optimizes operations by telling the tear down +routine if the next test will be from the same package (so the amount +of tear down could be reduced). Setup isn't told about the previous +state but should assume a random state. It's only allowed to assume +that the tests it requires to execute earlier have passed. It can +still optimize based on things it measures such as flashrom not writing +unchanged blocks. + +### Flow + +``` + DUT name + Tests to execute +--------------+ + + +---->+Console driver| + | | +--------------+ + | +---->+Button driver | + | | +--------------+ + v v + +-----------+ +----------+ + |Test runner+--------+DUT object+<+ + +-----------+ +----------+ | + | executes |generates | + | v | + | sends+------------+ | control + +<-----+event stream| +----+ + | +------------+ | | + | | | + | +----------+ | + +------------->+Host tests| | + | +----------+ | + | | + | +--------------+ call +------------------+ + +------------->+Target tests +----------+Target test runner| + +--------------+ +------------------+ +``` + diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md index 7c231fc..0df2a1b 100644 --- a/Documentation/technotes/index.md +++ b/Documentation/technotes/index.md @@ -2,3 +2,4 @@ * [Dealing with Untrusted Input in SMM](2017-02-dealing-with-untrusted-input-in-smm.md) * [Rebuilding coreboot image generation](2015-11-rebuilding-coreboot-image-generation.md) +* [firmware test runner](2019-12-firmware-testing.md) -- To view, visit https://review.coreboot.org/c/coreboot/+/37643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb9380050f8ff1aa13ecbb501079e2556e43ca06 Gerrit-Change-Number: 37643 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

6 23

Change in ...coreboot[master]: libpayload: Enable -Wconversion
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32185 Change subject: libpayload: Enable -Wconversion ...................................................................... libpayload: Enable -Wconversion Together with -Werror this ensures that no new conversion related issues can appear. BUG=b:111443775 BRANCH=none TEST=make junit.xml has no failed builds Change-Id: Idc37c50cae90d283bd6ec240dd7de3546485a9a9 Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- M payloads/libpayload/Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/32185/1 diff --git a/payloads/libpayload/Makefile.inc b/payloads/libpayload/Makefile.inc index 7787762..ceac4ca 100644 --- a/payloads/libpayload/Makefile.inc +++ b/payloads/libpayload/Makefile.inc @@ -64,7 +64,7 @@ CFLAGS += -ffunction-sections -fdata-sections CFLAGS += -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes CFLAGS += -Wwrite-strings -Wredundant-decls -Wno-trigraphs -CFLAGS += -Wstrict-aliasing -Wshadow -Werror +CFLAGS += -Wstrict-aliasing -Wshadow -Werror -Wconversion $(obj)/libpayload-config.h: $(KCONFIG_AUTOHEADER) cmp $@ $< 2>/dev/null || cp $< $@ -- To view, visit https://review.coreboot.org/c/coreboot/+/32185 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idc37c50cae90d283bd6ec240dd7de3546485a9a9 Gerrit-Change-Number: 32185 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

2 4

Change in ...coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Hello Julien Viard de Galbert, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/31072 to review the following change. Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib ...................................................................... soc/intel/denverton_ns: Rewrite pmutil using pmclib Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751 Signed-off-by: Julien Viard de Galbert <jviarddegalbert(a)online.net> --- M src/soc/intel/denverton_ns/include/soc/pm.h M src/soc/intel/denverton_ns/pmutil.c M src/soc/intel/denverton_ns/smihandler.c M src/soc/intel/denverton_ns/smm.c 4 files changed, 33 insertions(+), 184 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/31072/1 diff --git a/src/soc/intel/denverton_ns/include/soc/pm.h b/src/soc/intel/denverton_ns/include/soc/pm.h index 09ce158..0aab0e2 100644 --- a/src/soc/intel/denverton_ns/include/soc/pm.h +++ b/src/soc/intel/denverton_ns/include/soc/pm.h @@ -47,18 +47,6 @@ struct chipset_power_state *fill_power_state(void); /* Power Management Utility Functions. */ -uint32_t clear_smi_status(void); -uint16_t clear_pm1_status(void); -uint32_t clear_tco_status(void); -uint32_t clear_gpe_status(void); void clear_pmc_status(void); -void enable_smi(uint32_t mask); -void disable_smi(uint32_t mask); -void enable_pm1(uint16_t events); -void enable_pm1_control(uint32_t mask); -void disable_pm1_control(uint32_t mask); -void enable_gpe(uint32_t mask); -void disable_gpe(uint32_t mask); -void disable_all_gpe(void); #endif /* _DENVERTON_NS_PM_H_ */ diff --git a/src/soc/intel/denverton_ns/pmutil.c b/src/soc/intel/denverton_ns/pmutil.c index ccf0d95..344f8b9 100644 --- a/src/soc/intel/denverton_ns/pmutil.c +++ b/src/soc/intel/denverton_ns/pmutil.c @@ -19,29 +19,12 @@ #include <arch/io.h> #include <console/console.h> +#include <intelblocks/pmclib.h> #include <soc/iomap.h> #include <soc/soc_util.h> #include <soc/pm.h> -static void print_num_status_bits(int num_bits, uint32_t status, - const char *const bit_names[]) -{ - int i; - - if (!status) - return; - - for (i = num_bits - 1; i >= 0; i--) { - if (status & (1 << i)) { - if (bit_names[i]) - printk(BIOS_DEBUG, "%s ", bit_names[i]); - else - printk(BIOS_DEBUG, "BIT%d ", i); - } - } -} - -static uint32_t print_smi_status(uint32_t smi_sts) +const char *const *soc_smi_sts_array(size_t *a) { static const char *const smi_sts_bits[] = { [2] = "BIOS", @@ -67,93 +50,11 @@ [31] = "LEGACY_USB3", }; - if (!smi_sts) - return 0; - - printk(BIOS_DEBUG, "SMI_STS: "); - print_num_status_bits(ARRAY_SIZE(smi_sts_bits), smi_sts, smi_sts_bits); - printk(BIOS_DEBUG, "\n"); - - return smi_sts; + *a = ARRAY_SIZE(smi_sts_bits); + return smi_sts_bits; } -static uint32_t reset_smi_status(void) -{ - uint16_t pmbase = get_pmbase(); - uint32_t smi_sts = inl((uint16_t)(pmbase + SMI_STS)); - outl(smi_sts, (uint16_t)(pmbase + SMI_STS)); - return smi_sts; -} - -uint32_t clear_smi_status(void) { return print_smi_status(reset_smi_status()); } - -void enable_smi(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t smi_en = inl((uint16_t)(pmbase + SMI_EN)); - smi_en |= mask; - outl(smi_en, (uint16_t)(pmbase + SMI_EN)); -} - -void disable_smi(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t smi_en = inl((uint16_t)(pmbase + SMI_EN)); - smi_en &= ~mask; - outl(smi_en, (uint16_t)(pmbase + SMI_EN)); -} - -void enable_pm1_control(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t pm1_cnt = inl((uint16_t)(pmbase + PM1_CNT)); - pm1_cnt |= mask; - outl(pm1_cnt, (uint16_t)(pmbase + PM1_CNT)); -} - -void disable_pm1_control(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t pm1_cnt = inl((uint16_t)(pmbase + PM1_CNT)); - pm1_cnt &= ~mask; - outl(pm1_cnt, (uint16_t)(pmbase + PM1_CNT)); -} - -static uint16_t reset_pm1_status(void) -{ - uint16_t pmbase = get_pmbase(); - uint16_t pm1_sts = inw((uint16_t)(pmbase + PM1_STS)); - outw(pm1_sts, (uint16_t)(pmbase + PM1_STS)); - return pm1_sts; -} - -static uint16_t print_pm1_status(uint16_t pm1_sts) -{ - static const char *const pm1_sts_bits[] = { - [0] = "TMROF", [4] = "BM", [5] = "GBL", - [8] = "PWRBTN", [10] = "RTC", [11] = "PRBTNOR", - [15] = "WAK", - }; - - if (!pm1_sts) - return 0; - - printk(BIOS_SPEW, "PM1_STS: "); - print_num_status_bits(ARRAY_SIZE(pm1_sts_bits), pm1_sts, pm1_sts_bits); - printk(BIOS_SPEW, "\n"); - - return pm1_sts; -} - -uint16_t clear_pm1_status(void) { return print_pm1_status(reset_pm1_status()); } - -void enable_pm1(uint16_t events) -{ - uint16_t pmbase = get_pmbase(); - outw(events, (uint16_t)(pmbase + PM1_EN)); -} - -static uint32_t print_tco_status(uint32_t tco_sts) +const char *const *soc_tco_sts_array(size_t *a) { static const char *const tco_sts_bits[] = { [0] = "NMI2SMI", [1] = "OS_TCO_SMI", @@ -164,17 +65,11 @@ [17] = "SECOND_TO", [20] = "SMLINK_SLV_SMI", }; - if (!tco_sts) - return 0; - - printk(BIOS_DEBUG, "TCO_STS: "); - print_num_status_bits(ARRAY_SIZE(tco_sts_bits), tco_sts, tco_sts_bits); - printk(BIOS_DEBUG, "\n"); - - return tco_sts; + *a = ARRAY_SIZE(tco_sts_bits); + return tco_sts_bits; } -static uint32_t reset_tco_status(void) +uint32_t soc_reset_tco_status(void) { uint16_t tcobase = get_tcobase(); uint32_t tco_sts = inl((uint16_t)(tcobase + TCO1_STS)); @@ -184,35 +79,7 @@ return tco_sts & tco_en; } -uint32_t clear_tco_status(void) { return print_tco_status(reset_tco_status()); } - -void enable_gpe(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t gpe0_en = inl((uint16_t)(pmbase + GPE0_EN(GPE_STD))); - gpe0_en |= mask; - outl(gpe0_en, (uint16_t)(pmbase + GPE0_EN(GPE_STD))); -} - -void disable_gpe(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t gpe0_en = inl((uint16_t)(pmbase + GPE0_EN(GPE_STD))); - gpe0_en &= ~mask; - outl(gpe0_en, (uint16_t)(pmbase + GPE0_EN(GPE_STD))); -} - -void disable_all_gpe(void) { disable_gpe(~0); } - -static uint32_t reset_gpe_status(void) -{ - uint16_t pmbase = get_pmbase(); - uint32_t gpe_sts = inl((uint16_t)(pmbase + GPE0_STS(GPE_STD))); - outl(gpe_sts, (uint16_t)(pmbase + GPE0_STS(GPE_STD))); - return gpe_sts; -} - -static uint32_t print_gpe_sts(uint32_t gpe_sts) +const char *const *soc_std_gpe_sts_array(size_t *a) { static const char *const gpe_sts_bits[] = { [0] = "GPIO_0", [1] = "GPIO_1", @@ -233,16 +100,8 @@ [30] = "GPIO_30", [31] = "GPIO_31", }; - if (!gpe_sts) - return gpe_sts; - - printk(BIOS_DEBUG, "GPE0a_STS: "); - print_num_status_bits(ARRAY_SIZE(gpe_sts_bits), gpe_sts, gpe_sts_bits); - printk(BIOS_DEBUG, "\n"); - - return gpe_sts; + *a = ARRAY_SIZE(gpe_sts_bits); + return gpe_sts_bits; } -uint32_t clear_gpe_status(void) { return print_gpe_sts(reset_gpe_status()); } - void clear_pmc_status(void) { /* TODO */ } diff --git a/src/soc/intel/denverton_ns/smihandler.c b/src/soc/intel/denverton_ns/smihandler.c index 85f99d7..e1e7bc4 100644 --- a/src/soc/intel/denverton_ns/smihandler.c +++ b/src/soc/intel/denverton_ns/smihandler.c @@ -25,6 +25,7 @@ #include <device/pci_def.h> #include <elog.h> #include <intelblocks/fast_spi.h> +#include <intelblocks/pmclib.h> #include <spi-generic.h> #include <soc/iomap.h> #include <soc/soc_util.h> @@ -52,7 +53,7 @@ return 0; } -void southbridge_smi_set_eos(void) { enable_smi(EOS); } +void southbridge_smi_set_eos(void) { pmc_enable_smi(EOS); } global_nvs_t *smm_get_gnvs(void) { return gnvs; } @@ -98,7 +99,7 @@ uint16_t pmbase = get_pmbase(); /* First, disable further SMIs */ - disable_smi(SLP_SMI_EN); + pmc_disable_smi(SLP_SMI_EN); /* Figure out SLP_TYP */ reg32 = inl((uint16_t)(pmbase + PM1_CNT)); @@ -131,7 +132,7 @@ printk(BIOS_DEBUG, "SMI#: Entering S5 (Soft Power off)\n"); /* Disable all GPE */ - disable_all_gpe(); + pmc_disable_all_gpe(); /* also iterates over all bridges on bus 0 */ busmaster_disable_on_bus(0); @@ -145,7 +146,7 @@ * event again. We need to set BIT13 (SLP_EN) though to make the * sleep happen. */ - enable_pm1_control(SLP_EN); + pmc_enable_pm1_control(SLP_EN); /* Make sure to stop executing code here for S3/S4/S5 */ if (slp_typ > 1) @@ -158,7 +159,7 @@ reg32 = inl((uint16_t)(pmbase + PM1_CNT)); if (reg32 & SCI_EN) { /* The OS is not an ACPI OS, so we set the state to S0 */ - disable_pm1_control(SLP_EN | SLP_TYP); + pmc_disable_pm1_control(SLP_EN | SLP_TYP); } } @@ -237,11 +238,11 @@ printk(BIOS_DEBUG, "P-state control\n"); break; case APM_CNT_ACPI_DISABLE: - disable_pm1_control(SCI_EN); + pmc_disable_pm1_control(SCI_EN); printk(BIOS_DEBUG, "SMI#: ACPI disabled.\n"); break; case APM_CNT_ACPI_ENABLE: - enable_pm1_control(SCI_EN); + pmc_enable_pm1_control(SCI_EN); printk(BIOS_DEBUG, "SMI#: ACPI enabled.\n"); break; case APM_CNT_FINALIZE: @@ -268,23 +269,23 @@ static void southbridge_smi_pm1(void) { - uint16_t pm1_sts = clear_pm1_status(); + uint16_t pm1_sts = pmc_clear_pm1_status(); /* While OSPM is not active, poweroff immediately * on a power button event. */ if (pm1_sts & PWRBTN_STS) { // power button pressed - disable_pm1_control(-1UL); - enable_pm1_control(SLP_EN | (SLP_TYP_S5 << SLP_TYP_SHIFT)); + pmc_disable_pm1_control(-1UL); + pmc_enable_pm1_control(SLP_EN | (SLP_TYP_S5 << SLP_TYP_SHIFT)); } } -static void southbridge_smi_gpe0(void) { clear_gpe_status(); } +static void southbridge_smi_gpe0(void) { pmc_clear_all_gpe_status(); } static void southbridge_smi_tco(void) { - uint32_t tco_sts = clear_tco_status(); + uint32_t tco_sts = pmc_clear_tco_status(); /* Any TCO event? */ if (!tco_sts) @@ -354,7 +355,7 @@ /* We need to clear the SMI status registers, or we won't see what's * happening in the following calls. */ - smi_sts = clear_smi_status(); + smi_sts = pmc_clear_smi_status(); /* Call SMI sub handler for each of the status bits */ for (i = 0; i < ARRAY_SIZE(southbridge_smi); i++) { diff --git a/src/soc/intel/denverton_ns/smm.c b/src/soc/intel/denverton_ns/smm.c index 9d3fa75..65d2499 100644 --- a/src/soc/intel/denverton_ns/smm.c +++ b/src/soc/intel/denverton_ns/smm.c @@ -23,6 +23,7 @@ #include <cpu/x86/smm.h> #include <string.h> +#include <intelblocks/pmclib.h> #include <soc/iomap.h> #include <soc/soc_util.h> #include <soc/pm.h> @@ -48,10 +49,10 @@ } /* Dump and clear status registers */ - clear_smi_status(); - clear_pm1_status(); - clear_tco_status(); - clear_gpe_status(); + pmc_clear_smi_status(); + pmc_clear_pm1_status(); + pmc_clear_tco_status(); + pmc_clear_all_gpe_status(); clear_pmc_status(); } @@ -60,8 +61,8 @@ printk(BIOS_DEBUG, "Enabling SMIs.\n"); /* Configure events Disable pcie wake. */ - enable_pm1(PWRBTN_EN | GBL_EN | PCIEXPWAK_DIS); - disable_gpe(PME_B0_EN); + pmc_enable_pm1(PWRBTN_EN | GBL_EN | PCIEXPWAK_DIS); + pmc_disable_std_gpe(PME_B0_EN); /* Enable SMI generation: * - on APMC writes (io 0xb2) @@ -71,7 +72,7 @@ * - on TCO events * - on microcontroller writes (io 0x62/0x66) */ - enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS); + pmc_enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS); } void smm_setup_structures(void *gnvs, void *tcg, void *smi1) -- To view, visit https://review.coreboot.org/c/coreboot/+/31072 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751 Gerrit-Change-Number: 31072 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Julien Viard de Galbert <coreboot-review-ju(a)vdg.name> Gerrit-MessageType: newchange

5 8

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2021