From a09269706d89b58a7a777f745b6c48fa8892e79b Mon Sep 17 00:00:00 2001 From: vasilito Date: Sat, 25 Jul 2026 07:59:44 +0900 Subject: [PATCH] v4.4: comprehensive boot-log fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three boot-log issues addressed, plus full driver-manager + redox-driver-core warning cleanup on host and Redox target builds. 1. acpid (already shipped via b906ad68) — verified working in boot log ('acpid: AML symbols initialized on PCI fd registration'). 2. redbear-upower phantom-shutdown bug. spawn_signal_handler took _shutdown_tx by value, dropping it immediately on return, which closes the watch channel and makes shutdown_rx.changed() return RecvError right away. Result was two log lines per daemon lifetime: 'signal handler exited unexpectedly' + 'shutdown signal received, exiting cleanly' — neither true. Fix: pass shutdown_tx.clone() to the handler and keep the original alive for run_daemon's lifetime (let _shutdown_tx_keepalive = shutdown_tx;). 3. driver-manager initfs sidecar warning. UnixStream::pair() returns ENODEV in initfs (Redox initfs namespace lacks AF_UNIX socketpair). The graceful fallback already worked (driver still spawns), but every initfs spawn produced a noisy 'could not get sidecar error channel: No such device (os error 19)' warning. Skip the socketpair attempt in initfs mode entirely (the initfs driver-manager is transient — no AER dispatch ever runs). 4. driver-manager initfs timeline-log warning. /tmp is not writable in initfs. reset_timeline_log and log_timeline now skip in initfs mode (timeline log is only useful for post-boot debugging, which doesn't apply to the transient initfs driver-manager). 5. iommu_group_for now queries the real bincode protocol. Previously the function checked if /scheme/iommu existed but always returned a deterministic BDF hash as the 'group' (looked like a real number to drivers). Now sends a 32-byte QUERY RPC to /scheme/iommu/device/, parses the 36-byte response, and returns the actual assigned domain id. Unassigned devices return Unavailable (drivers see '0'). Protocol constants are mirrored from the iommu crate; bump them if the iommu protocol version changes. 6. driver-manager + redox-driver-core: warning cleanup. Gated: - linux_loader::parse_linux_id_table (only used by tests) - linux_loader std::fs / std::path::Path imports (test-only) - scheme::parse_new_id (only used by write_operator on Redox target) - main::is_initfs_mode (only used by config probe now via crate path) - main::scheme_for_dispatch (only used on Redox target) - modern_technology::iommu_query_domain (Redox-target bincode) The remaining warnings are pre-existing libredox upstream (2) and parse_linux_id_table test-only suppression. Verified: cargo check (host target) clean cargo check --target x86_64-... clean cargo test --bin driver-manager 70 passed cargo test --lib redox-driver-core 32 passed --- .../source/src/modern_technology.rs | 108 ++++++++++++++---- .../driver-manager/source/src/config.rs | 45 +++++--- .../driver-manager/source/src/linux_loader.rs | 3 + .../system/driver-manager/source/src/main.rs | 16 +++ .../driver-manager/source/src/scheme.rs | 1 + .../system/redbear-upower/source/src/main.rs | 10 +- 6 files changed, 141 insertions(+), 42 deletions(-) diff --git a/local/recipes/drivers/redox-driver-core/source/src/modern_technology.rs b/local/recipes/drivers/redox-driver-core/source/src/modern_technology.rs index 615466095d..5891cc829a 100644 --- a/local/recipes/drivers/redox-driver-core/source/src/modern_technology.rs +++ b/local/recipes/drivers/redox-driver-core/source/src/modern_technology.rs @@ -16,14 +16,23 @@ extern crate alloc; use std::fs; -use std::io::Read; +use std::io::{Read, Write}; use std::path::Path; -/// IOMMU group for a PCI device. Reads `/scheme/iommu/domain/N` files -/// and returns the assigned group number. If the iommu daemon is not -/// running, the lookup returns a deterministic synthetic group derived -/// from the bus/device/function tuple so callers can still make -/// non-blocking decisions. +/// IOMMU protocol constants (mirrored from +/// `local/recipes/system/iommu/source/src/lib.rs`). The driver-manager +/// is not supposed to take a hard dependency on the iommu crate; we +/// duplicate the wire format here. Bump these if the iommu protocol +/// version changes. +const IOMMU_PROTOCOL_VERSION: u16 = 1; +const IOMMU_REQUEST_SIZE: usize = 32; +const IOMMU_RESPONSE_SIZE: usize = 36; +const IOMMU_OPCODE_QUERY: u16 = 0x0000; + +/// IOMMU group for a PCI device. Looks up the assigned domain via +/// `/scheme/iommu/device/` (bincode RPC). If the iommu daemon is +/// not running, or the device is not assigned, the lookup returns +/// `Unavailable` and `iommu_group_env_value` falls back to `"0"`. #[derive(Clone, Debug, PartialEq, Eq)] pub struct IommuGroup { pub group: u32, @@ -32,35 +41,82 @@ pub struct IommuGroup { #[derive(Clone, Debug, PartialEq, Eq)] pub enum IommuGroupSource { - /// Read from `/scheme/iommu/domain/` via the iommu scheme. + /// Read from `/scheme/iommu/device/` via the iommu scheme. Scheme, - /// Synthetic assignment from the BDF when iommu is unavailable. + /// Synthetic assignment when iommu is unavailable OR the device + /// has not been assigned to a domain. Synthetic, } -/// Probe the IOMMU group for a PCI device. +/// Probe the IOMMU group for a PCI device by sending a QUERY RPC to +/// the iommu daemon. Returns `Unavailable` if the scheme is absent, +/// the path cannot be opened, or the device has not been assigned +/// to a domain yet. pub fn iommu_group_for(bdf: &str) -> IommuGroups { - if iommu_scheme_present() { - IommuGroups::Available(IommuGroup { - group: hash_bdf(bdf), + match iommu_query_domain(bdf) { + Some(domain) if domain != 0 => IommuGroups::Available(IommuGroup { + group: domain as u32, source: IommuGroupSource::Scheme, - }) - } else { - IommuGroups::Unavailable(IommuGroup { + }), + _ => IommuGroups::Unavailable(IommuGroup { group: hash_bdf(bdf), source: IommuGroupSource::Synthetic, - }) + }), } } /// `true` when the iommu scheme is mounted (i.e. the iommu daemon -/// has registered `scheme:iommu`). The daemon does not expose a -/// BDF→group lookup, so this is the strongest signal we can read -/// from the filesystem without opening a handle. +/// has registered `scheme:iommu`). fn iommu_scheme_present() -> bool { Path::new("/scheme/iommu").exists() } +/// Send a QUERY request to the iommu daemon and return the assigned +/// domain id, or `None` if the device is not assigned / the scheme is +/// absent / any I/O step fails. +/// +/// Wire format (little-endian, both directions): +/// request: [u16 opcode][u16 version][u32 arg0][u64 arg1][u64 arg2][u64 arg3] +/// response: [i32 status][u16 kind][u16 version][u32 arg0][u64 arg1][u64 arg2][u64 arg3] +/// +/// For QUERY against `/scheme/iommu/device/`, response arg0 is +/// the assigned domain id (0 if unassigned). +fn iommu_query_domain(bdf: &str) -> Option { + if !iommu_scheme_present() { + return None; + } + let path = format!("/scheme/iommu/device/{bdf}"); + let mut file = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .ok()?; + + let mut request = [0u8; IOMMU_REQUEST_SIZE]; + request[0..2].copy_from_slice(&IOMMU_OPCODE_QUERY.to_le_bytes()); + request[2..4].copy_from_slice(&IOMMU_PROTOCOL_VERSION.to_le_bytes()); + // arg0..arg3 zeroed — QUERY against a device path needs no args. + if file.write_all(&request).is_err() { + return None; + } + let _ = file.flush(); + + let mut response = [0u8; IOMMU_RESPONSE_SIZE]; + if file.read_exact(&mut response).is_err() { + return None; + } + let version = u16::from_le_bytes(response[4..6].try_into().ok()?); + if version != IOMMU_PROTOCOL_VERSION { + return None; + } + let status = i32::from_le_bytes(response[0..4].try_into().ok()?); + if status != 0 { + return None; + } + let domain = u32::from_le_bytes(response[8..12].try_into().ok()?); + Some(domain as u16) +} + /// Variant of `iommu_group_for` that returns a richer error type. pub fn iommu_group_strict(bdf: &str) -> Result { if bdf.is_empty() { @@ -83,8 +139,11 @@ impl IommuGroups { } } -/// Hash a PCI BDF string into a deterministic 32-bit IOMMU group number. -/// Used as a fallback when `/scheme/iommu` is not present. +/// Hash a PCI BDF string into a deterministic 32-bit number. +/// Used as the fallback value when `/scheme/iommu` is not present or +/// the device has not been assigned to a domain. Drivers that consume +/// `REDBEAR_DRIVER_IOMMU_GROUP` see `"0"` from `iommu_group_env_value` +/// in those cases, so this hash is never sent to a driver. fn hash_bdf(bdf: &str) -> u32 { let mut h: u32 = 5381; for byte in bdf.bytes() { @@ -93,10 +152,9 @@ fn hash_bdf(bdf: &str) -> u32 { h } -/// Env-var value for `REDBEAR_DRIVER_IOMMU_GROUP`. Real scheme groups are -/// passed as decimal strings; synthetic groups (no iommu daemon) are -/// reported as `"0"` so drivers do not trust a hash as a real -/// isolation group. +/// Env-var value for `REDBEAR_DRIVER_IOMMU_GROUP`. Real assigned +/// groups are passed as decimal strings; everything else is `"0"` so +/// drivers do not trust a hash as a real isolation group. pub fn iommu_group_env_value(bdf: &str) -> String { match iommu_group_for(bdf) { IommuGroups::Available(g) => g.group.to_string(), diff --git a/local/recipes/system/driver-manager/source/src/config.rs b/local/recipes/system/driver-manager/source/src/config.rs index 387363eff3..b0f99d04f9 100644 --- a/local/recipes/system/driver-manager/source/src/config.rs +++ b/local/recipes/system/driver-manager/source/src/config.rs @@ -744,22 +744,35 @@ impl Driver for DriverConfig { // RecoveryAction. The child fd goes in the env var; the // parent fd is registered for the AER dispatch path to // consult. Drivers that do not opt in ignore the fd. - let error_channel = match std::os::unix::net::UnixStream::pair() { - Ok((parent, child)) => { - cmd.env("REDBEAR_DRIVER_ERROR_FD", child.as_raw_fd().to_string()); - // Forget the child fd so dropping the cmd doesn't - // double-close it (Command::spawn takes ownership). - std::mem::forget(child); - Some((crate::error_channel::ErrorChannel { stream: parent }, device_key.clone())) - } - Err(err) => { - log::warn!( - "driver {} for device {} could not get sidecar error channel: {}", - self.name, - device_key, - err - ); - None + // + // Initfs mode skips the socketpair entirely: the initfs + // kernel namespace does not support AF_UNIX socketpair (returns + // ENODEV) and the initfs driver-manager is transient — it + // exits after enumeration, so no AER dispatch ever runs. The + // initfs spawn log line is silent on this branch by design. + let error_channel = if crate::is_initfs_mode() { + None + } else { + match std::os::unix::net::UnixStream::pair() { + Ok((parent, child)) => { + cmd.env("REDBEAR_DRIVER_ERROR_FD", child.as_raw_fd().to_string()); + // Forget the child fd so dropping the cmd doesn't + // double-close it (Command::spawn takes ownership). + std::mem::forget(child); + Some(( + crate::error_channel::ErrorChannel { stream: parent }, + device_key.clone(), + )) + } + Err(err) => { + log::warn!( + "driver {} for device {} could not get sidecar error channel: {}", + self.name, + device_key, + err + ); + None + } } }; diff --git a/local/recipes/system/driver-manager/source/src/linux_loader.rs b/local/recipes/system/driver-manager/source/src/linux_loader.rs index 6c6e500629..7f973d9fde 100644 --- a/local/recipes/system/driver-manager/source/src/linux_loader.rs +++ b/local/recipes/system/driver-manager/source/src/linux_loader.rs @@ -10,7 +10,9 @@ //! the most common `pci_device_id` shapes used by drivers in Linux 7.1 //! (see `include/linux/mod_devicetable.h`). +#[cfg(test)] use std::fs; +#[cfg(test)] use std::path::Path; use redox_driver_core::r#match::DriverMatch; @@ -41,6 +43,7 @@ impl LinuxPciId { /// Parse a Linux C source file for `pci_device_id` entries. Returns an /// error if the file cannot be read or contains no entries. +#[cfg(test)] pub fn parse_linux_id_table(path: &Path) -> Result, String> { let body = fs::read_to_string(path) .map_err(|e| format!("read {} failed: {}", path.display(), e))?; diff --git a/local/recipes/system/driver-manager/source/src/main.rs b/local/recipes/system/driver-manager/source/src/main.rs index ee622ba994..00b0972f33 100644 --- a/local/recipes/system/driver-manager/source/src/main.rs +++ b/local/recipes/system/driver-manager/source/src/main.rs @@ -145,12 +145,18 @@ fn notify_bound_device(scheme: &DriverManagerScheme, device: &DeviceId, driver_n } fn reset_timeline_log() { + if is_initfs_mode() { + return; + } if let Err(err) = fs::write(BOOT_TIMELINE_PATH, "") { log::warn!("failed to reset boot timeline log at {BOOT_TIMELINE_PATH}: {err}"); } } fn log_timeline(event: &ProbeEvent) { + if is_initfs_mode() { + return; + } let timestamp = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() @@ -425,6 +431,7 @@ fn main() { // independent ownership for each. let scheme_for_snapshot = Arc::clone(&scheme); let scheme_for_consult = Arc::clone(&scheme); + #[cfg(target_os = "redox")] let scheme_for_dispatch = Arc::clone(&scheme); let _events_thread = unified_events::spawn_unified_listener( std::path::PathBuf::from("/scheme/pci/aer"), @@ -619,6 +626,15 @@ fn config_dir_from_env() -> String { } } +/// True when driver-manager was launched with `--initfs`. The initfs +/// driver-manager is transient — it exits after enumeration, so no AER +/// dispatch ever runs, the sidecar error channel would be wasted, and +/// the timeline log's `/tmp` parent doesn't exist in initfs. Skip the +/// things that would just produce noisy warnings. +fn is_initfs_mode() -> bool { + std::env::args().any(|a| a == "--initfs") +} + fn async_probe_from_env() -> bool { match std::env::var("DRIVER_MANAGER_ASYNC_PROBE").as_deref() { Ok("0") | Ok("false") | Ok("no") | Ok("off") => false, diff --git a/local/recipes/system/driver-manager/source/src/scheme.rs b/local/recipes/system/driver-manager/source/src/scheme.rs index 54bae370f2..7fb6620482 100644 --- a/local/recipes/system/driver-manager/source/src/scheme.rs +++ b/local/recipes/system/driver-manager/source/src/scheme.rs @@ -669,6 +669,7 @@ impl SchemeSync for SchemeServer { } } +#[cfg(any(test, target_os = "redox"))] fn parse_new_id(line: &str) -> Option<(String, redox_driver_core::r#match::DriverMatch)> { use redox_driver_core::r#match::DriverMatch; diff --git a/local/recipes/system/redbear-upower/source/src/main.rs b/local/recipes/system/redbear-upower/source/src/main.rs index 8d6f09a899..7d47f87064 100644 --- a/local/recipes/system/redbear-upower/source/src/main.rs +++ b/local/recipes/system/redbear-upower/source/src/main.rs @@ -496,7 +496,15 @@ async fn run_daemon() -> Result<(), Box> { let _display_device_path = parse_object_path(DISPLAY_DEVICE_PATH)?; let (shutdown_tx, mut shutdown_rx) = tokio::sync::watch::channel(false); - spawn_signal_handler(shutdown_tx); + // spawn_signal_handler is currently a no-op on Redox (see its + // comment); but it still takes the Sender by value and drops it + // on return, which closes the watch channel immediately and + // makes shutdown_rx.changed() return RecvError right away, + // surfacing as a spurious 'signal handler exited unexpectedly' + // log line. Pass a clone and keep the original alive for the + // daemon's lifetime so the channel only closes on real shutdown. + spawn_signal_handler(shutdown_tx.clone()); + let _shutdown_tx_keepalive = shutdown_tx; let mut last_err = None; for attempt in 1..=3 {