diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index 15407a6..977317a 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -74,11 +74,22 @@ So this driver: previous transaction's data. The engine only ever clears GO, so that is the single permitted difference between the two words. -What is missing: no equivalent of Broadwell's TSODCNTL has been identified -on this part. Offset 0xe0, where Broadwell has it, reads as zero here. A -system with CLTT enabled is therefore expected to trip the interference -check rather than be arbitrated against. I would rather ship a driver that -detects the condition and refuses than one that quietly races. + - refuses to bind when the memory controller is polling the DIMM thermal + sensors itself. Intel documents SMBCNTL on the iMC channel function + (device 10 and 12, function 0, offsets 0xe88 and 0xe98) in the Xeon + Processor Scalable Family datasheet volume 2, reference 614073, section + 3.1.9. Its SMB_TSOD_POLL_EN bit is documented as mutually exclusive + with SPD command access, so it answers directly whether the engine is + in use. On the test system all four channels read 0xac000000: polling + disabled, TSOD_PRESENT mask empty, which also explains why no jc42 + device appears at 0x18-0x1f. + +This is deliberately weaker than what the Broadwell-E driver did. That one +switched the polling off for the duration of a transfer. Reading the bit +and declining is a smaller claim on hardware the driver does not own, and +it needs no write to a firmware-configured register. It covers the memory +controller only; SMM and a BMC remain out of reach, which is what the +opt-in is for. Relation to prior work ~~~~~~~~~~~~~~~~~~~~~~ @@ -119,6 +130,7 @@ Changes since v2 default-off gate, which was logging a driver-core error on every boot. - Add Documentation/i2c/busses/i2c-imc-skylake.rst. - Drop the ACPI dependency. + - Refuse to bind when the iMC reports TSOD polling enabled. Validation ~~~~~~~~~~ @@ -132,12 +144,35 @@ Validation per channel, the default-off gate, and 20 unload/reload cycles: 15 checks, 0 failures, no kernel warning or oops, no interference reported, every command word confirmed in the register. - - Config space of the PCU function sampled with the driver unloaded: no - register changed on its own, and TSOD_ACTIVE was clear on both channels, - so no firmware activity was observed on this system. + - Engine registers sampled every 2 s for one hour with the driver + unloaded: 1791 samples, 0 changes. No firmware activity was observed + on this system. + - SMBCNTL on all four iMC channels reads 0xac000000: TSOD polling + disabled, TSOD_PRESENT empty. + - Write transactions reach the bus: a byte write to an unpopulated + address returns -ENXIO with the engine reporting the address NACK, so + the write path is issued rather than suppressed. An end-to-end write + to a populated device is not part of this validation; the only + non-SPD device on the test system is an LED controller whose reads are + themselves unreliable, and SPD writes were out of scope. Still missing: an independent Tested-by, ideally on Skylake-X rather than -Cascade Lake-X, and suspend/resume testing with active I2C clients. +Cascade Lake-X, suspend/resume testing with active I2C clients, and an +end-to-end write to a populated device. + +One note on the register interface, since a reader with the datasheet will +ask. That document describes an SMBus interface on the iMC channel +function at 0xe80-0xe9c, with a different encoding from the one this driver +uses on PCU function 5 at 0x9c-0xb8; it does not describe function 5 at all, +covering device 30 functions 0 and 2 only. I tried the documented path on +this system: issuing SMBCMD with CMD_TRIGGER sets SMB_RDO and clears +SMB_BUSY, but returns 0x00 for a populated DIMM at 0x50, for a second one +at 0x52, and for an empty slot at 0x57 alike, with SMB_SBE never set. An +interface that cannot tell a populated slot from an empty one is not +reaching the bus here, while the path this driver uses returns the correct +SPD contents and a NACK for the empty slot. I may well be missing a setup +step; if someone knows what it is, I would rather use the documented +interface. Tool assistance ~~~~~~~~~~~~~~~ diff --git a/docs/submission/i2c-imc-skylake.rst b/docs/submission/i2c-imc-skylake.rst index 0ee63ad..e70fcef 100644 --- a/docs/submission/i2c-imc-skylake.rst +++ b/docs/submission/i2c-imc-skylake.rst @@ -48,7 +48,18 @@ Do this only on a system where concurrent firmware access has been excluded. Desktop and HEDT boards have no BMC, but CLTT may still be enabled by the firmware. -The driver reduces, but cannot remove, the risk: +The driver reduces, but cannot remove, the risk. + +At probe it reads SMBCNTL on every iMC channel function (device 10 and 12, +function 0; offsets 0xe88 and 0xe98, documented in the Intel Xeon Processor +Scalable Family datasheet volume 2, reference 614073, section 3.1.9). +``SMB_TSOD_POLL_EN`` in that register says whether the memory controller is +polling the DIMM thermal sensors on its own, which Intel documents as mutually +exclusive with SPD command access. If the bit is set on any channel the driver +refuses to bind: the engine is not ours to drive. This covers the memory +controller, not SMM or a BMC, which remain the reason for the opt-in. + +Per transfer: * every transaction waits for both channels to report idle before it touches any register; diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index 6d8fc39..c4df50e 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -57,6 +57,18 @@ /* PCI id of the Sky Lake-E PCU function carrying the SMBus engine */ #define PCU_DEVICE 0x2085 +/* + * The iMC channel function, device 10 and 12 function 0, carries the SMBus + * control register documented in the Intel Xeon Processor Scalable Family + * datasheet volume 2 (reference 614073) section 3.1.9. Only SMBCNTL is read + * here, and only to find out whether the hardware is polling the DIMM thermal + * sensors on its own. + */ +#define IMC_CHANNEL_DEVICE 0x2040 +#define IMC_SMBCNTL(ch) (0xE88 + (ch) * 0x10) +#define SMBCNTL_TSOD_POLL_EN BIT(8) +#define SMBCNTL_TSOD_PRESENT GENMASK(7, 0) + /* per-channel register offsets in the function's config space */ #define CH0_CTRL 0xB4 #define CH0_DATA 0x9C @@ -524,6 +536,51 @@ static int imc_resume(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume); +/* + * Refuse to share the engine with the memory controller's own thermal + * throttling. SMBCNTL.TSOD_POLL_EN tells us directly whether the iMC is + * issuing SMBus transactions of its own; Intel documents that SPD command + * access and TSOD polling are mutually exclusive, so a set bit means the + * engine is not ours to drive. This does not cover SMM or a BMC, which + * remain the reason for the allow_unsafe_access gate. + */ +static int imc_check_firmware_polling(struct pci_dev *pdev) +{ + struct pci_dev *imc = NULL; + unsigned int found = 0; + int i; + + while ((imc = pci_get_device(PCI_VENDOR_ID_INTEL, IMC_CHANNEL_DEVICE, + imc))) { + found++; + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) { + u32 cntl; + + if (pci_read_config_dword(imc, IMC_SMBCNTL(i), &cntl)) + continue; + + if (cntl & SMBCNTL_TSOD_POLL_EN) { + dev_err(&pdev->dev, + "%s ch%d has TSOD polling enabled (SMBCNTL 0x%08x); the memory controller is using the engine\n", + pci_name(imc), i, cntl); + pci_dev_put(imc); + return -EBUSY; + } + + dev_dbg(&pdev->dev, + "%s ch%d SMBCNTL 0x%08x, TSOD present mask 0x%02lx\n", + pci_name(imc), i, cntl, + cntl & SMBCNTL_TSOD_PRESENT); + } + } + + if (!found) + dev_warn(&pdev->dev, + "no iMC channel function found; cannot verify that TSOD polling is disabled\n"); + + return 0; +} + static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct imc_smbus *s; @@ -535,6 +592,10 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) return -ENODEV; } + ret = imc_check_firmware_polling(pdev); + if (ret) + return ret; + s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL); if (!s) return -ENOMEM; diff --git a/tools/probe-imc-documented.sh b/tools/probe-imc-documented.sh new file mode 100755 index 0000000..b61e904 --- /dev/null +++ b/tools/probe-imc-documented.sh @@ -0,0 +1,135 @@ +#!/usr/bin/env bash +# +# WRITES TO HARDWARE. Issues an SMBus read through the interface Intel +# documents, rather than the one this driver currently uses. +# +# Intel Xeon Processor Scalable Family datasheet vol. 2 (ref. 614073) chapter 3 +# puts the iMC SMBus interface on device 10 and 12, function 0: +# +# SMB_STAT_[0:1] 0xe80 0xe90 b31 RDO, b30 WOD, b29 SBE, b28 BUSY, +# b27:24 tsod_sa, b15:0 RDATA +# SMBCMD_[0:1] 0xe84 0xe94 b31 CMD_TRIGGER, b30 PNTR_SEL, +# b29 WORD_ACCESS, b28:27 rw, b26:24 SA, +# b23:16 BA, b15:0 WDATA +# SMBCNTL_[0:1] 0xe88 0xe98 b31:28 DTI (0xa EEPROM, 0x3 TSOD), +# b27 CKOVRD, b26 DIS_WRT, b10 SOFT_RST, +# b9 start_tsod_poll, b8 TSOD_POLL_EN, +# b7:0 TSOD_PRESENT +# +# The driver instead uses PCU function 5 at 0x9c-0xb8, which that document does +# not describe (it covers device 30 functions 0 and 2 only). This script reads +# a known SPD byte through the documented path so the two can be compared. +# +# Read transaction only, against an EEPROM that is certainly present. SMBCMD is +# restored on every exit path. SMBCNTL is not written at all. +# +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +SMB_STAT=(0xe80 0xe90) +SMB_CMD=(0xe84 0xe94) +SMB_CNTL=(0xe88 0xe98) + +CMD_TRIGGER=$((1 << 31)) +STAT_RDO=$((1 << 31)) +STAT_WOD=$((1 << 30)) +STAT_SBE=$((1 << 29)) +STAT_BUSY=$((1 << 28)) +CNTL_TSOD_POLL_EN=$((1 << 8)) + +SA=0 # slave address low bits; DTI 0xa | SA 0 selects 0x50 +BA=0x02 # SPD byte 2, DDR4 type code, expected 0x0c +CONFIRMED=false + +while (($#)); do + case $1 in + --i-understand) CONFIRMED=true; shift ;; + --sa) SA=$2; shift 2 ;; + --ba) BA=$2; shift 2 ;; + -h|--help) + sed -n '3,26p' "$0" + echo "usage: $0 --i-understand [--sa N] [--ba 0xNN]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +$CONFIRMED || { + echo "Issues an SMBus read from userspace. Re-run with --i-understand." >&2 + exit 2 +} + +require_root +require_cmd lspci setpci awk +driver_is_loaded && die "unload i2c-imc-skylake first" + +mapfile -t IMCS < <(lspci -Dn -d 8086:2040 | awk '{print $1}') +((${#IMCS[@]})) || die "no iMC channel device (8086:2040) found" + +probe_one() +{ + local bdf=$1 ch=$2 + local stat=${SMB_STAT[ch]} cmd=${SMB_CMD[ch]} cntl=${SMB_CNTL[ch]} + local cntl_val stat_val cmd_saved command i data + + cntl_val=$(read_reg "$bdf" "$cntl") + stat_val=$(read_reg "$bdf" "$stat") + cmd_saved=$(read_reg "$bdf" "$cmd") + + printf ' SMBCNTL=%s SMB_STAT=%s SMBCMD=%s\n' \ + "$cntl_val" "$stat_val" "$cmd_saved" + + if ((cntl_val & CNTL_TSOD_POLL_EN)); then + echo " SKIP: TSOD_POLL_EN is set, firmware owns this channel" + return 0 + fi + if ((stat_val & STAT_BUSY)); then + echo " SKIP: SMB_BUSY is set" + return 0 + fi + + # byte read: TRIGGER, PNTR_SEL=0, WORD=0, rw=00, SA, BA + command=$((CMD_TRIGGER | (SA << 24) | (BA << 16))) + printf ' issuing SMBCMD=0x%08x (read SA=%d BA=%s)\n' "$command" "$SA" "$BA" + setpci -s "$bdf" "$cmd.L=$(printf '%08x' "$command")" + + for i in $(seq 1 50); do + stat_val=$(read_reg "$bdf" "$stat") + ((stat_val & STAT_BUSY)) || break + sleep 0.005 + done + + setpci -s "$bdf" "$cmd.L=$(printf '%08x' "$cmd_saved")" + + data=$((stat_val & 0xff)) + printf ' SMB_STAT=%s RDO=%d WOD=%d SBE=%d BUSY=%d RDATA=0x%02x\n' \ + "$stat_val" \ + "$(((stat_val & STAT_RDO) != 0))" \ + "$(((stat_val & STAT_WOD) != 0))" \ + "$(((stat_val & STAT_SBE) != 0))" \ + "$(((stat_val & STAT_BUSY) != 0))" \ + "$data" + + if ((stat_val & STAT_SBE)); then + echo " -> SBE: the addressed device did not answer on this channel" + elif ((stat_val & STAT_RDO)); then + printf ' -> READ OK, data 0x%02x\n' "$data" + if [[ "$BA" == "0x02" ]] && ((data == 0x0c)); then + echo " matches the DDR4 type code, so the documented" + echo " interface reaches the same devices." + fi + else + echo " -> no completion: the interface did not act on the command" + fi +} + +for bdf in "${IMCS[@]}"; do + for ch in 0 1; do + echo "=== $bdf channel $ch ===" + probe_one "$bdf" "$ch" + echo + done +done