From 637717dad09538cc29d604a76016746d4ca28e36 Mon Sep 17 00:00:00 2001 From: Simone Date: Sun, 9 Aug 2026 09:48:04 +0200 Subject: [PATCH] i2c-imc-skylake: guard branch - ECAM accessors close the CF8/CFC SMI window A conf1 access is two I/O instructions (index on 0xCF8, data on 0xCFC) serialised against the kernel but not against SMM: an SMI landing between them, whose handler runs its own CF8/CFC dialogue without restoring the index, redirects our data phase to whatever register the firmware addressed last. The CMD write is covered by the landing check, but the DATA-register write has no readback, so its payload would land unnoticed in a foreign config register. Suspected (not proven) trigger of the ws1 poweroff hang: the wedge sits in the firmware's S5 path and the poison channel must be invisible to the driver, which this one is. Map the function's ECAM page from the ACPI MCFG table at probe and route every register touch through a single MMIO access, which no SMI can split. use_ecam=0 (load-time) restores the config-cycle baseline for A/B; MCFG lookup failure falls back automatically. --- VERSION | 2 +- i2c-imc-skylake.c | 99 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 97 insertions(+), 4 deletions(-) diff --git a/VERSION b/VERSION index a0b1e44..421092d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.0.4-guard1 +1.0.5-guard1 diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index 2598c69..9f533ec 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -44,6 +44,22 @@ * /sys/module/i2c_imc_skylake/parameters/protect_spd for deliberate * SPD maintenance. * + * - use_ecam (default on): reach the engine registers through a private + * ECAM (MMCONFIG) mapping instead of the kernel's legacy config + * cycles. For standard config space the kernel uses conf1: two I/O + * instructions, index on 0xCF8 and data on 0xCFC. pci_config_lock + * serialises that pair against the rest of the kernel but not against + * SMM: an SMI landing between the two, whose handler runs its own + * CF8/CFC dialogue without restoring the index, silently redirects + * the data phase to whatever register the firmware addressed last. + * A misdirected read only perturbs one poll iteration and the CMD + * write is covered by the landing check below, but the DATA-register + * write has no readback, so its payload would land unnoticed in a + * foreign config register. A single MMIO access cannot be split by + * an SMI, so the window does not exist on the ECAM path. Suspected + * (not proven) trigger of the ws1 poweroff hang. Falls back to + * config cycles when MCFG has no entry for the device. + * * Bus arbitration / concurrency with firmware: * The driver cannot coordinate with SMM, a BMC, or the iMC's own closed-loop * thermal throttling (CLTT), and the PCI ID alone does not prove that none of @@ -63,6 +79,7 @@ * to be arbitrated against. */ +#include #include #include #include @@ -183,6 +200,16 @@ module_param(protect_spd, bool, 0644); MODULE_PARM_DESC(protect_spd, "reject SMBus writes to SPD addresses 0x30-0x35 and 0x50-0x57 (default on)"); +/* + * Load-time only: the accessors pick their path from s->ecam, captured at + * probe. Flipping this at runtime would race in-flight transfers for no + * benefit; the A/B lever is a reboot away, like the pre-ECAM baseline. + */ +static bool use_ecam = true; +module_param(use_ecam, bool, 0444); +MODULE_PARM_DESC(use_ecam, + "reach the engine via an ECAM mapping instead of CF8/CFC config cycles (default on)"); + struct imc_chan { unsigned int cmd, data, stat; int idx; /* channel index 0 or 1 */ @@ -196,6 +223,8 @@ static const struct imc_chan imc_chans[2] = { /* one driver state object, shared by both per-channel adapters */ struct imc_smbus { struct pci_dev *pdev; /* config space carrying the engine */ + void __iomem *ecam; /* this function's config page, or NULL + to fall back to config cycles */ struct device *dev; /* &pdev->dev, for dev_*() logging */ struct mutex lock; /* serialises all SMBus transactions */ /* needed: both channels share one engine */ @@ -211,9 +240,52 @@ struct imc_xfer_state { }; /* - * Register accessors. A config-space failure is recorded in s->io_err and - * reported once, at the end of the transfer, so the transaction logic does not - * have to check a return value on every register touch. A failed read yields + * Locate this function's configuration page in the ECAM window described by + * the ACPI MCFG table and map it. The arch code keeps its own MCFG mapping + * for extended config space but exposes no way to route standard config + * space through it, hence the private lookup. Mapping the page a second + * time is harmless: it is device MMIO, not RAM. + */ +static void __iomem *imc_map_ecam(struct pci_dev *pdev) +{ + struct acpi_table_mcfg *mcfg; + struct acpi_mcfg_allocation *alloc; + void __iomem *map = NULL; + unsigned int bus = pdev->bus->number; + int i, n; + + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MCFG, 0, + (struct acpi_table_header **)&mcfg))) + return NULL; + + n = (mcfg->header.length - sizeof(*mcfg)) / sizeof(*alloc); + alloc = (struct acpi_mcfg_allocation *)(mcfg + 1); + for (i = 0; i < n; i++, alloc++) { + resource_size_t addr; + + if (alloc->pci_segment != pci_domain_nr(pdev->bus) || + bus < alloc->start_bus_number || + bus > alloc->end_bus_number) + continue; + + addr = alloc->address + + ((resource_size_t)(bus - alloc->start_bus_number) << 20) + + ((resource_size_t)pdev->devfn << 12); + map = devm_ioremap(&pdev->dev, addr, SZ_4K); + break; + } + + acpi_put_table(&mcfg->header); + return map; +} + +/* + * Register accessors. On the ECAM path a register touch is a single MMIO + * access; it has no failure channel, and the function sits on the CPU die, + * so it cannot vanish and produce the all-ones of a master abort. On the + * config-cycle fallback a failure is recorded in s->io_err and reported + * once, at the end of the transfer, so the transaction logic does not have + * to check a return value on every register touch. A failed read yields * all-ones, which no polling loop mistakes for a completed transaction. */ static u32 imc_reg(struct imc_smbus *s, unsigned int off) @@ -221,6 +293,9 @@ static u32 imc_reg(struct imc_smbus *s, unsigned int off) u32 val; int ret; + if (s->ecam) + return readl(s->ecam + off); + ret = pci_read_config_dword(s->pdev, off, &val); if (ret) { s->io_err = pcibios_err_to_errno(ret); @@ -234,6 +309,11 @@ static void imc_set(struct imc_smbus *s, unsigned int off, u32 val) { int ret; + if (s->ecam) { + writel(val, s->ecam + off); + return; + } + ret = pci_write_config_dword(s->pdev, off, val); if (ret) s->io_err = pcibios_err_to_errno(ret); @@ -837,6 +917,19 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (ret) return ret; + /* + * Before the first register touch, so that even the pristine-word + * snapshot below goes through the SMI-safe path. + */ + if (use_ecam) { + s->ecam = imc_map_ecam(pdev); + if (!s->ecam) + dev_warn(&pdev->dev, + "no usable MCFG entry; falling back to CF8/CFC config cycles\n"); + } + dev_dbg(&pdev->dev, "engine access via %s\n", + s->ecam ? "ECAM MMIO" : "CF8/CFC config cycles"); + /* * Pristine command words: the state the firmware left in the engine at * boot, captured before this driver's first transfer. They are handed