From 967753f0d89274b73665f7aafa06ef2d0eb168ab Mon Sep 17 00:00:00 2001 From: Angel Pons Date: Mon, 15 Feb 2021 17:44:09 +0100 Subject: [PATCH] soc/intel/common: Prevent SMI storm when setting SPI WPD bit MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit From Skylake/Sunrise Point onwards, there are two BIOS_CNTL registers: one on the LPC/eSPI PCI device, and another on the SPI PCI device. When the WPD bit changes from 0 to 1 and the LE bit is set, the PCH raises a TCO SMI with the BIOSWR_STS bit set. However, the BIOSWR_STS bit is not set when the TCO SMI comes from the SPI or eSPI controller instead, but a status bit in the BIOS_CNTL register gets set. If the SMI cause is not handled, another SMI will happen immediately after returning from the SMI handler, which results in a deadlock. Prevent deadlocks by clearing the SPI synchronous SMI status bit in the SMI handler. When SPI raises a synchronous SMI, the TCO_STS bit in the SMI_STS register is continously set until the SPI synchronous SMI status bit is cleared. To not risk missing any other TCO SMIs, do not clear the TCO_STS bit again in the same SMI handler invocation. If the TCO_STS bit remains set when returning from SMM, another SMI immediately happens and clears the TCO_STS bit, handling any pending events. SPI can also generate asynchronous SMIs when the WPD bit is cleared and one attempts to write to flash using SPI hardware sequencing. This patch does not account for SPI asynchronous SMIs, because they are disabled by default and cannot be enabled once the BIOS Interface Lock-Down bit in the BIOS_CNTL register has been set, which coreboot already does. These asynchronous SMIs set the SPI_STS bit of the SMI_STS register. Clearing the SPI asynchronous SMI source should be done inside the SPI_STS SMI handler, which is currently not implemented. All of this goes out of the scope of this patch, and is currently not necessary anyway. This patch does not handle eSPI because I cannot test it, and knowing if a board uses LPC or eSPI from common code is currently not possible, and this is beyond the scope of what this commit tries to achieve (fix SPI). Tested on HP 280 G2, no longer deadlocks when SMM BIOS write protection is on. Write protection will be enforced in a follow-up. Change-Id: Iec498674ae70f6590c33a6bf4967876268f2b0c8 Signed-off-by: Angel Pons Reviewed-on: https://review.coreboot.org/c/coreboot/+/50754 Reviewed-by: Michael Niewöhner Reviewed-by: Nico Huber Tested-by: build bot (Jenkins) --- src/soc/intel/common/block/fast_spi/fast_spi.c | 14 ++++++++++++++ src/soc/intel/common/block/fast_spi/fast_spi_def.h | 1 + src/soc/intel/common/block/include/intelblocks/fast_spi.h | 4 ++++ src/soc/intel/common/block/smm/smihandler.c | 12 ++++++++++++ 4 files changed, 31 insertions(+) diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index 58e3ca2c62..a3481c6fb5 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -387,6 +387,20 @@ void fast_spi_early_init(uintptr_t spi_base_address) fast_spi_init(); } +/* Clear SPI Synchronous SMI status bit and return its value. */ +bool fast_spi_clear_sync_smi_status(void) +{ + const uint32_t bios_cntl = pci_read_config32(PCH_DEV_SPI, SPI_BIOS_CONTROL); + const bool smi_asserted = bios_cntl & SPI_BIOS_CONTROL_SYNC_SS; + /* + * Do not unconditionally write 1 to clear SYNC_SS. Hardware could set + * SYNC_SS here (after we read but before we write SPI_BIOS_CONTROL), + * and the event would be lost when unconditionally clearing SYNC_SS. + */ + pci_write_config32(PCH_DEV_SPI, SPI_BIOS_CONTROL, bios_cntl); + return smi_asserted; +} + /* Read SPI Write Protect disable status. */ bool fast_spi_wpd_status(void) { diff --git a/src/soc/intel/common/block/fast_spi/fast_spi_def.h b/src/soc/intel/common/block/fast_spi/fast_spi_def.h index 945feb072d..4f79b75da7 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi_def.h +++ b/src/soc/intel/common/block/fast_spi/fast_spi_def.h @@ -17,6 +17,7 @@ #define SPI_BIOS_CONTROL_PREFETCH_ENABLE (1 << 3) #define SPI_BIOS_CONTROL_EISS (1 << 5) #define SPI_BIOS_CONTROL_BILD (1 << 7) +#define SPI_BIOS_CONTROL_SYNC_SS (1 << 8) #define SPI_BIOS_CONTROL_EXT_BIOS_ENABLE (1 << 27) #define SPI_BIOS_CONTROL_EXT_BIOS_LOCK_ENABLE (1 << 28) #define SPI_BIOS_CONTROL_EXT_BIOS_LIMIT(x) ((x) & ~(0xfff)) diff --git a/src/soc/intel/common/block/include/intelblocks/fast_spi.h b/src/soc/intel/common/block/include/intelblocks/fast_spi.h index 3ba240baf3..d0442566b3 100644 --- a/src/soc/intel/common/block/include/intelblocks/fast_spi.h +++ b/src/soc/intel/common/block/include/intelblocks/fast_spi.h @@ -65,6 +65,10 @@ void fast_spi_early_init(uintptr_t spi_base_address); */ extern const struct spi_ctrlr fast_spi_flash_ctrlr; /* + * Clear SPI Synchronous SMI status bit and return its value. + */ +bool fast_spi_clear_sync_smi_status(void); +/* * Read SPI Write protect disable bit. */ bool fast_spi_wpd_status(void); diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index a15bc7c8c5..2c3364ab06 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -384,6 +384,18 @@ void smihandler_southbridge_tco( { uint32_t tco_sts = pmc_clear_tco_status(); + /* + * SPI synchronous SMIs are TCO SMIs, but they do not have a status + * bit in the TCO_STS register. Furthermore, the TCO_STS bit in the + * SMI_STS register is continually set until the SMI handler clears + * the SPI synchronous SMI status bit in the SPI controller. To not + * risk missing any other TCO SMIs, do not clear the TCO_STS bit in + * this SMI handler invocation. If the TCO_STS bit remains set when + * returning from SMM, another SMI immediately happens which clears + * the TCO_STS bit and handles any pending events. + */ + fast_spi_clear_sync_smi_status(); + /* Any TCO event? */ if (!tco_sts) return; -- 2.11.4.GIT