diff --git a/issues/qemus-invalidate-iommu-all-leaves-every-function-untranslated.md b/issues/qemus-invalidate-iommu-all-leaves-every-function-untranslated.md new file mode 100644 index 0000000000..d8645da3bc --- /dev/null +++ b/issues/qemus-invalidate-iommu-all-leaves-every-function-untranslated.md @@ -0,0 +1,33 @@ +--- +status: open +kind: tooling +opened: 2026-10-12 +--- + +# QEMU's `INVALIDATE_IOMMU_ALL` leaves every function untranslated + +AMD 48882 §2.4.8: once `INVALIDATE_IOMMU_ALL` completes, "all IOMMU caches +are empty", so the unit reads every device table entry afresh. QEMU 11.1.1's +`amd-iommu` does not. With `dma-remap=on` a function's address space starts +on `iommu_nodma`, which passes its DMA untranslated, and moves to +translation only in `amdvi_update_addr_translation_mode`, whose one caller is +`amdvi_inval_devtab_entry`. `amdvi_inval_all` resets the IOTLB, notifies the +interrupt caches and replays the address spaces that have notifiers, and +never reads an entry's mode (`hw/i386/amd_iommu.c` at `c3d48b7d`, +`:1249-1313` and `:1332-1351`). + +So under `iommu-firmware-left`, which invalidates with one +`INVALIDATE_IOMMU_ALL` (`kernel/src/arch/x86_64/amdvi/mod.rs`, `leave_on`), +QEMU passes every function's DMA through a unit whose every entry blocks it. +`amdvi_firmware_left`'s second boot binds the stick whether or not the +switch-off runs: its negative control reds only on the register half, and the +DMA half is held by the AMD bench's `iommu_firmware_left` row alone, in +whatever run the orchestrator stages. + +Owner: the orchestrator's AMD laptop track +(`issues/an-amd-vi-machine-isolates-no-device.md`), whose translation stage +is the next code to rest on what QEMU does on this command. + +**Exit:** under the QEMU `build/ci/qemu-version` names, `amdvi_firmware_left` +with `switch_off` skipping every field reds on `usb-storage: 1 device(s)`, +and the test's doc says that boot's stick never binds. diff --git a/kernel/src/arch/x86_64/amdvi/mod.rs b/kernel/src/arch/x86_64/amdvi/mod.rs index 71870b046d..f669e23569 100644 --- a/kernel/src/arch/x86_64/amdvi/mod.rs +++ b/kernel/src/arch/x86_64/amdvi/mod.rs @@ -88,7 +88,7 @@ pub fn init(rsdp_addr: u64) -> bool { if let Some(regs) = present(units, &unit) { #[cfg(feature = "boot-actuators")] if crate::actuator::iommu_firmware_left() { - leave_on(regs, &ivrs, &unit); + leave_on(regs); } switch_off(units, &unit, regs); off += 1; @@ -261,15 +261,14 @@ fn switch_off(index: usize, unit: &Unit, regs: Mmio) { /// protecting memory before the operating system runs may leave it — every /// requester's device table entry translating through a table that maps /// nothing, so every DMA is blocked, with its command buffer and event log -/// running. The entry of every requester the unit's IVHD names is -/// invalidated, which is what a unit reads a changed entry on, and a -/// completion wait says each was. +/// running. One `INVALIDATE_IOMMU_ALL` empties every cache the unit holds, +/// so it reads every entry as written, and a completion wait says it did. /// /// The tables are leaked, as firmware's are its own: the unit reads them /// until it is switched off, and freed they could read back as an entry that /// passes DMA untranslated. #[cfg(feature = "boot-actuators")] -fn leave_on(regs: Mmio, ivrs: &Ivrs, unit: &Unit) { +fn leave_on(regs: Mmio) { use crate::mm::{pmm, DirectMap}; const DEVICE_TABLE: u64 = 0x0000; @@ -281,17 +280,26 @@ fn leave_on(regs: Mmio, ivrs: &Ivrs, unit: &Unit) { const EVENT_TAIL: u64 = 0x2018; /// 2^8 entries of 16 bytes, 4 KiB, in bits 59:56 of either base. const LOG_LEN: u64 = 8 << 56; - const RING: usize = 1 << 8; const DEVICE_IDS: u64 = 1 << 16; const ENTRY: u64 = 32; /// `V`, `TV`, and a four-level table: `Mode` 100b at 11:9; `IR` and `IW` /// clear. const BLOCKED: u64 = 1 | (1 << 1) | (4 << 9); - const INVALIDATE_ENTRY: u64 = 2 << 60; + /// `IASup`, set where the unit takes `INVALIDATE_IOMMU_ALL`. + const IA_SUP: u64 = 1 << 6; + /// The opcode alone: every other bit of the command is reserved. + const INVALIDATE_IOMMU_ALL: u64 = 8 << 60; /// The opcode, and `S`: store the second word at the address. const COMPLETION_WAIT: u64 = (1 << 60) | 1; const DONE: u64 = 0x600d_f00d; + let efr = regs.read_u64(EXTENDED_FEATURES); + assert!( + efr & IA_SUP != 0, + "amdvi: the unit's EFR {efr:#018x} has IASup clear, so it takes no INVALIDATE_IOMMU_ALL and the actuator \ + cannot make it read the entries it leaves" + ); + let window = |phys: u64, bytes: u64| { // SAFETY: `phys` is inside a page `pmm::alloc_page` handed out and // this function leaks, which the direct map covers for the machine's life. @@ -318,31 +326,12 @@ fn leave_on(regs: Mmio, ivrs: &Ivrs, unit: &Unit) { } regs.write_u64(CONTROL, regs.read_u64(CONTROL) | CMD_BUF_EN | EVENT_LOG_EN | IOMMU_EN); - let commands = window(commands_at, RING as u64 * 16); - let mut tail = 0; - let mut push = |low: u64, high: u64| { - // A ring holds one entry fewer than it has. - assert!(tail < (RING as u64 - 1) * 16, "amdvi: the unit's requesters overflow the actuator's {RING}-entry ring"); - commands.write_u64(tail, low); - commands.write_u64(tail + 8, high); - tail += 16; - }; - for entry in ivrs.devices(unit) { - let (first, last) = match entry.expect("amdvi: the actuator's unit has a device list it can walk") { - DeviceEntry::All { .. } => (0, u16::MAX), - DeviceEntry::Select { id, .. } | DeviceEntry::Extended { id, .. } | DeviceEntry::Hid { id, .. } => (id, id), - DeviceEntry::Range { first, last, .. } | DeviceEntry::ExtendedRange { first, last, .. } => (first, last), - DeviceEntry::Alias { used, .. } - | DeviceEntry::AliasRange { used, .. } - | DeviceEntry::Special { used, .. } => (used, used), - DeviceEntry::Other(_) => continue, - }; - for id in first..=last { - push(INVALIDATE_ENTRY | u64::from(id), 0); - } + let commands = [INVALIDATE_IOMMU_ALL, 0, COMPLETION_WAIT | done_at, DONE]; + let ring = window(commands_at, commands.len() as u64 * 8); + for (word, value) in (0..).zip(commands) { + ring.write_u64(word * 8, value); } - push(COMPLETION_WAIT | done_at, DONE); - regs.write_u64(COMMAND_TAIL, tail); + regs.write_u64(COMMAND_TAIL, commands.len() as u64 * 8); let done = window(done_at, 8); spin_until( || done.read_u64(0) == DONE, diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index 3a6dff7725..44a66384f4 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -440,15 +440,26 @@ const NO_DOMAIN: &str = "iommu: no domain of its own for a device: no unit on th /// which only DMA the unit lets through can do. const STICK_BOUND: &str = "usb-storage: 1 device(s)"; +/// What the actuator leaves on an AMD-Vi unit, in the kernel's words for +/// `unit`: one line per field the hand-over switches off by its own write. +pub(crate) fn amdvi_left_on(unit: &str) -> [String; 3] { + ["CmdBufEn", "EventLogEn", "IommuEn"] + .map(|field| format!("iommu: amdvi {unit} was handed over with {field} on; it goes off")) +} + /// **A unit an AMD machine's firmware left on is switched off, and the boot /// stick's DMA reaches memory.** QEMU's `amd-iommu` is the only AMD-Vi unit -/// in reach: the T14 is an Intel machine, and the hand-over's register -/// sequence and the DMA it frees are neither of them a host test's. Two -/// boots: the shipped kernel finds a unit nothing left on and switches +/// the merge queue runs: the T14 is an Intel machine, and the hand-over's +/// register sequence and the DMA it frees are neither of them a host test's. +/// Two boots: the shipped kernel finds a unit nothing left on and switches /// nothing; the test kernel's `iommu-firmware-left` leaves the unit blocking /// every device's DMA with its command buffer and event log running, and /// each goes off by its own write before the stick is driven. With the -/// switch-off reverted that boot's stick never binds. +/// switch-off reverted that boot reds on the fields' lines and on `control` +/// reading back zero, and its stick still binds: QEMU 11.1.1 moves no +/// function to translation on `INVALIDATE_IOMMU_ALL` +/// (`issues/qemus-invalidate-iommu-all-leaves-every-function-untranslated.md`), +/// so the AMD bench's `iommu_firmware_left` row holds the DMA half. pub fn amdvi_firmware_left(test_config: &Path) -> Result<(), String> { let profile = Profile::MetalAmdVi; let argv = qemu::profile_argv(&BootOptions { profile, ..Default::default() }); @@ -474,12 +485,12 @@ pub fn amdvi_firmware_left(test_config: &Path) -> Result<(), String> { log.must_not_say("no DMAR or IVRS table")?; let handed = log.must_say("iommu: amdvi unit0 handed over control=")?; if left { - // `CmdBufEn`, `EventLogEn` and `IommuEn`, as the actuator wrote them. + // [`amdvi_left_on`]'s fields, as the actuator wrote them. if !handed.contains("control=0x0000000000001005") { return Err(format!("the actuator's unit was handed over as {handed:?}")); } - for field in ["CmdBufEn", "EventLogEn", "IommuEn"] { - log.must_say(&format!("iommu: amdvi unit0 was handed over with {field} on; it goes off"))?; + for line in amdvi_left_on("unit0") { + log.must_say(&line)?; } for field in ["EventIntEn", "GALogEn", "GAIntEn", "PPRLogEn", "PPRIntEn"] { log.must_not_say(&format!("with {field} on"))?; diff --git a/tests/toyos.rs b/tests/toyos.rs index 4bb3bc94a3..da5cbc2ec3 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -992,9 +992,10 @@ const METAL: &[(&str, metal::Metal)] = &[ }, ), ( - // A unit handed over translating, remapping and queueing, on real - // silicon: each field goes off by its own write before the unit is - // programmed, and the boot goes on. + // A unit handed over translating, remapping and queueing, or on an + // AMD-Vi machine blocking every DMA with its command buffer and event + // log running, on real silicon: each field goes off by its own write + // before the unit is programmed or left off, and the boot goes on. "iommu_firmware_left", metal::Metal { arms: SELFTESTS, judge: |b| iommu_firmware_left(b[0].kernel().text()) }, ), @@ -5213,8 +5214,11 @@ fn input_merge_ok(log: &str) -> Result<(), String> { Ok(()) } -/// Every unit this kernel went on to program said it was handed over with what -/// the actuator left on. +/// Every unit this kernel went on to program, and every AMD-Vi unit it +/// switched off, said it was handed over with what the actuator left on. +/// +/// An AMD-Vi unit's lines are on the boot stick only because the switch-off +/// let the stick's DMA through the unit the actuator left blocking it. fn iommu_firmware_left(log: &str) -> Result<(), String> { let mut units = 0; for line in log.lines().filter(|l| l.contains(" translating gsts=")) { @@ -5242,13 +5246,32 @@ fn iommu_firmware_left(log: &str) -> Result<(), String> { } units += 1; } + let amdvi = log + .lines() + .filter_map(|l| l.split_once("iommu: amdvi ")?.1.split_once(" handed over control=")) + .map(|(unit, _)| unit); + for unit in amdvi { + for line in common::iommu::amdvi_left_on(unit) { + if !log.contains(&line) { + return Err(format!( + "amdvi {unit} never said {line:?}, so the state the actuator left was \ + never switched off:\n{log}" + )); + } + } + let off = format!("iommu: amdvi {unit} @"); + if !log.lines().any(|l| l.contains(&off) && l.contains(" switched off control=")) { + return Err(format!("amdvi {unit} was handed over and never said it was switched off:\n{log}")); + } + units += 1; + } if units == 0 { - return Err(format!("no unit was programmed, so nothing was handed over:\n{log}")); + return Err(format!("no unit was programmed or switched off, so nothing was handed over:\n{log}")); } let passed = log.matches("handed over with compatibility-format pass-through on").count(); eprintln!( - " [iommu-firmware-left] {units} unit(s) programmed after switching off what they were \ - handed over with; {passed} of them reported compatibility format passed" + " [iommu-firmware-left] {units} unit(s) switched off what they were handed over with; \ + {passed} of them reported compatibility format passed" ); Ok(()) }