Nico Huber submitted this change.
soc/intel/common: Prevent SMI storm when setting SPI WPD bit
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 <th3fanbus@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/50754
Reviewed-by: Michael Niewöhner <foss@mniewoehner.de>
Reviewed-by: Nico Huber <nico.h@gmx.de>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
---
M src/soc/intel/common/block/fast_spi/fast_spi.c
M src/soc/intel/common/block/fast_spi/fast_spi_def.h
M src/soc/intel/common/block/include/intelblocks/fast_spi.h
M src/soc/intel/common/block/smm/smihandler.c
4 files changed, 31 insertions(+), 0 deletions(-)
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 58e3ca2..a3481c6 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 @@
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 945feb0..4f79b75 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 3ba240b..d044256 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 @@
*/
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 a15bc7c..2c3364a 100644
--- a/src/soc/intel/common/block/smm/smihandler.c
+++ b/src/soc/intel/common/block/smm/smihandler.c
@@ -384,6 +384,18 @@
{
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;
To view, visit change 50754. To unsubscribe, or for help writing mail filters, visit settings.