diff --git a/alioth/src/virtio/dev/dev.rs b/alioth/src/virtio/dev/dev.rs index c5ddf7fc..2839b55f 100644 --- a/alioth/src/virtio/dev/dev.rs +++ b/alioth/src/virtio/dev/dev.rs @@ -24,7 +24,7 @@ pub mod vsock; use std::fmt::Debug; use std::sync::Arc; -use std::sync::atomic::{AtomicU8, AtomicU16, AtomicU32}; +use std::sync::atomic::{AtomicU8, AtomicU16, AtomicU32, Ordering}; use std::thread::JoinHandle; use bitflags::Flags; @@ -84,6 +84,16 @@ pub struct Register { pub status: AtomicU8, } +impl Register { + pub fn get_driver_feature(&self) -> u128 { + let mut result = 0u128; + for (i, feature) in self.driver_feature.iter().enumerate() { + result |= (feature.load(Ordering::Acquire) as u128) << (i << 5); + } + result + } +} + const TOKEN_WARKER: u64 = 1 << 63; #[derive(Debug, Clone)] @@ -438,3 +448,7 @@ pub trait DevSpec { false } } + +#[cfg(test)] +#[path = "dev_test.rs"] +mod tests; diff --git a/alioth/src/virtio/dev/dev_test.rs b/alioth/src/virtio/dev/dev_test.rs new file mode 100644 index 00000000..677f5082 --- /dev/null +++ b/alioth/src/virtio/dev/dev_test.rs @@ -0,0 +1,34 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::sync::atomic::Ordering; + +use super::Register; + +#[test] +fn test_virtio_register_get_driver_feature() { + let reg = Register::default(); + assert_eq!(reg.get_driver_feature(), 0); + + reg.driver_feature[0].store(0x1234_5678, Ordering::Release); + reg.driver_feature[1].store(0x9abc_def0, Ordering::Release); + reg.driver_feature[2].store(0xfeed_cafe, Ordering::Release); + reg.driver_feature[3].store(0x0123_4567, Ordering::Release); + + let expected = (0x1234_5678u128) + | ((0x9abc_def0u128) << 32) + | ((0xfeed_cafeu128) << 64) + | ((0x0123_4567u128) << 96); + assert_eq!(reg.get_driver_feature(), expected); +} diff --git a/alioth/src/virtio/pci.rs b/alioth/src/virtio/pci.rs index 5a5b28a0..f83d6b3d 100644 --- a/alioth/src/virtio/pci.rs +++ b/alioth/src/virtio/pci.rs @@ -42,7 +42,7 @@ use crate::sync::notifier::Notifier; use crate::utils::{get_atomic_high32, get_atomic_low32, set_atomic_high32, set_atomic_low32}; use crate::virtio::dev::{Register, StartParam, VirtioDevice, WakeEvent}; use crate::virtio::queue::QueueReg; -use crate::virtio::{DevStatus, DeviceId, IrqSender, Result, error}; +use crate::virtio::{DevStatus, DeviceId, IrqSender, Result, VirtioFeature, error}; use crate::{consts, impl_mmio_for_zerocopy, mem}; const VIRTIO_MSI_NO_VECTOR: u16 = 0xffff; @@ -394,9 +394,18 @@ where reg.driver_feature_sel.store(val as u32, Ordering::Release); } VirtioCommonCfg::LAYOUT_DRIVER_FEATURE => { - let sel = reg.driver_feature_sel.load(Ordering::Acquire); - if let Some(feature) = reg.driver_feature.get(sel as usize) { - feature.store(val as u32, Ordering::Release); + let sel = reg.driver_feature_sel.load(Ordering::Acquire) as usize; + if let Some(feature) = reg.driver_feature.get(sel) { + let status = DevStatus::from_bits_retain(reg.status.load(Ordering::Acquire)); + if status.contains(DevStatus::FEATURES_OK) { + // VirtIO spec 1.4, Sec 3.1.1 + log::warn!( + "{}: cannot change driver features after FEATURES_OK", + self.name + ); + } else { + feature.store(val as u32 & reg.device_feature[sel], Ordering::Release); + } } else if val != 0 { log::error!("{}: unknown feature {val:#x} for sel {sel}", self.name); } @@ -418,26 +427,28 @@ where } } VirtioCommonCfg::LAYOUT_DEVICE_STATUS => { - let status = DevStatus::from_bits_truncate(val as u8); - let old = reg.status.swap(status.bits(), Ordering::AcqRel); - let old = DevStatus::from_bits_retain(old); - if (old ^ status).contains(DevStatus::DRIVER_OK) { - let event = if status.contains(DevStatus::DRIVER_OK) { - let mut feature = 0; - for (i, v) in reg.driver_feature.iter().enumerate() { - feature |= (v.load(Ordering::Acquire) as u128) << (i << 5); - } - let param = StartParam { - feature, - irq_sender: self.irq_sender.clone(), - ioeventfds: self.ioeventfds.clone(), - }; - WakeEvent::Start { param } - } else { - self.reset(); - WakeEvent::Reset + let status = DevStatus::from_bits_retain(val as u8); + let old = DevStatus::from_bits_retain(reg.status.load(Ordering::Acquire)); + // Allows multi-step transitions in a single MMIO write + if !(status.is_valid() && status.contains(old) || status.is_empty()) { + log::warn!("{}: invalid status change: {old:#x} -> {val:#x}", self.name); + return Ok(Action::None); + } + reg.status.store(status.bits(), Ordering::Release); + if status.is_empty() { + self.reset(); + if old.contains(DevStatus::DRIVER_OK) { + self.wake_up_dev(WakeEvent::Reset); + } + } else if !old.contains(DevStatus::DRIVER_OK) + && status.contains(DevStatus::DRIVER_OK) + { + let param = StartParam { + feature: reg.get_driver_feature(), + irq_sender: self.irq_sender.clone(), + ioeventfds: self.ioeventfds.clone(), }; - self.wake_up_dev(event); + self.wake_up_dev(WakeEvent::Start { param }); } } VirtioCommonCfg::LAYOUT_QUEUE_SELECT => { @@ -481,7 +492,14 @@ where VirtioCommonCfg::LAYOUT_QUEUE_DESC_LO => { let q_sel = reg.queue_sel.load(Ordering::Relaxed); if let Some(q) = self.queues.get(q_sel as usize) { - set_atomic_low32(&q.desc, val as u32) + if val.is_multiple_of(16) { + set_atomic_low32(&q.desc, val as u32) + } else { + log::warn!( + "{}: queue {q_sel}: unaligned descriptor area: {val:x}", + self.name + ) + } } } VirtioCommonCfg::LAYOUT_QUEUE_DESC_HI => { @@ -493,7 +511,20 @@ where VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO => { let q_sel = reg.queue_sel.load(Ordering::Relaxed); if let Some(q) = self.queues.get(q_sel as usize) { - set_atomic_low32(&q.driver, val as u32) + let feat = VirtioFeature::from_bits_retain(reg.get_driver_feature()); + let align = if feat.contains(VirtioFeature::RING_PACKED) { + 4 + } else { + 2 + }; + if val.is_multiple_of(align) { + set_atomic_low32(&q.driver, val as u32); + } else { + log::warn!( + "{}: queue {q_sel}: unaligned driver area: {val:x}", + self.name + ); + } } } VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_HI => { @@ -505,7 +536,14 @@ where VirtioCommonCfg::LAYOUT_QUEUE_DEVICE_LO => { let q_sel = reg.queue_sel.load(Ordering::Relaxed); if let Some(q) = self.queues.get(q_sel as usize) { - set_atomic_low32(&q.device, val as u32) + if val.is_multiple_of(4) { + set_atomic_low32(&q.device, val as u32) + } else { + log::warn!( + "{}: queue {q_sel}: unaligned device area: {val:x}", + self.name + ) + } } } VirtioCommonCfg::LAYOUT_QUEUE_DEVICE_HI => { diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index 9d5af6ea..ea3eec238 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::sync::Arc; -use std::sync::atomic::{AtomicU16, AtomicU64}; +use std::sync::atomic::{AtomicU16, AtomicU64, Ordering}; use assert_matches::assert_matches; use parking_lot::RwLock; @@ -24,42 +24,58 @@ use crate::pci::cap::MsixTableMmio; use crate::sync::notifier::Notifier; use crate::virtio::dev::{Register, WakeEvent}; use crate::virtio::pci::{ - PciIrqSender, VirtioCommonCfg, VirtioPciMsixVector, VirtioPciRegister, VirtioPciRegisterMmio, + PciIrqSender, VIRTIO_MSI_NO_VECTOR, VirtioCommonCfg, VirtioPciMsixVector, VirtioPciRegister, + VirtioPciRegisterMmio, }; use crate::virtio::queue::QueueReg; use crate::virtio::tests::FakeIoeventFd; +use crate::virtio::{DevStatus, VirtioFeature}; -#[test] -fn test_virtio_pci_queue_registers() { - let queues = Arc::new([QueueReg { - desc: AtomicU64::new(0x1122_3344_5566_7788), - driver: AtomicU64::new(0xaabb_ccdd_eeff_0011), - device: AtomicU64::new(0x0123_4567_89ab_cdef), - ..Default::default() - }]); +type TestMmio = VirtioPciRegisterMmio; +type TestWakeReceiver = flume::Receiver, FakeIoeventFd>>; + +fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver) { let (event_tx, event_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let msi_sender = TestMsiSender::default(); let msix_table = Arc::new(MsixTableMmio { entries: RwLock::new(vec![].into_boxed_slice()), }); + let num_queues = queues.len(); let irq_sender = Arc::new(PciIrqSender { msix_vector: VirtioPciMsixVector { - config: AtomicU16::new(0xffff), - queues: vec![AtomicU16::new(0xffff)], + config: AtomicU16::new(VIRTIO_MSI_NO_VECTOR), + queues: (0..num_queues) + .map(|_| AtomicU16::new(VIRTIO_MSI_NO_VECTOR)) + .collect(), }, msix_table, msi_sender, }); - let mmio = VirtioPciRegisterMmio::<_, FakeIoeventFd> { + let mmio = VirtioPciRegisterMmio { name: "test-virtio-pci".into(), - reg: Register::default(), - queues: queues.clone(), + reg: Register { + device_feature: [u32::MAX; 4], + ..Default::default() + }, + queues, irq_sender, ioeventfds: None, event_tx, notifier, }; + (mmio, event_rx) +} + +#[test] +fn test_virtio_pci_queue_registers() { + let queues = Arc::new([QueueReg { + desc: AtomicU64::new(0x1122_3344_5566_7788), + driver: AtomicU64::new(0xaabb_ccdd_eeff_0011), + device: AtomicU64::new(0x0123_4567_89ab_cdef), + ..Default::default() + }]); + let (mmio, event_rx) = create_test_mmio(queues.clone()); assert_matches!( mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DESC_LO.0 as u64, 4), @@ -160,3 +176,804 @@ fn test_virtio_pci_queue_registers() { ); assert!(event_rx.is_empty()); } + +#[test] +fn test_virtio_pci_device_status_valid_transitions() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx) = create_test_mmio(queues.clone()); + + // Initially status is 0 (empty) + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(0) + ); + + // Transition 0 -> ACK + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + DevStatus::ACK.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == DevStatus::ACK.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Transition ACK -> ACK | DRIVER + let ack_driver = DevStatus::ACK | DevStatus::DRIVER; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + ack_driver.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Set driver features + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0x1234_5678 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 1), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0x9abc_def0 + ), + Ok(Action::None) + ); + + // Transition ACK | DRIVER -> ACK | DRIVER | FEATURES_OK + let ack_driver_features = ack_driver | DevStatus::FEATURES_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + ack_driver_features.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver_features.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Transition ACK | DRIVER | FEATURES_OK -> ACK | DRIVER | FEATURES_OK | DRIVER_OK + let all_ok = ack_driver_features | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + all_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert_matches!( + event_rx.try_recv(), + Ok(WakeEvent::Start { param }) => { + assert_eq!(param.feature, ((0x9abc_def0u128) << 32) | 0x1234_5678); + } + ); + assert!(event_rx.is_empty()); + + // Rewriting same status is idempotent and does not send duplicate WakeEvent::Start + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + all_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Additional status flag: FAILED (contains old status) + let failed = all_ok | DevStatus::FAILED; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + failed.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == failed.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Additional status flag: NEEDS_RESET (contains old status) + let needs_reset = failed | DevStatus::NEEDS_RESET; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + needs_reset.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == needs_reset.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Enable queue and set MSI-X config vector + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_SELECT.0 as u64, 2, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_ENABLE.0 as u64, 2, 1), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_CONFIG_MSIX_VECTOR.0 as u64, 2, 0), + Ok(Action::None) + ); + assert!(queues[0].enabled.load(Ordering::Acquire)); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_CONFIG_MSIX_VECTOR.0 as u64, 2), + Ok(0) + ); + + // Reset device: write status = 0 + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(0) + ); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset)); + assert!(event_rx.is_empty()); + assert!(!queues[0].enabled.load(Ordering::Acquire)); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_CONFIG_MSIX_VECTOR.0 as u64, 2), + Ok(vector) if vector == VIRTIO_MSI_NO_VECTOR as u64 + ); +} + +#[test] +fn test_virtio_pci_device_status_invalid_transitions() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx) = create_test_mmio(queues); + + // Invalid transitions from empty state (skipping ACK or invalid combinations) + for invalid in [ + DevStatus::DRIVER, + DevStatus::FEATURES_OK, + DevStatus::DRIVER_OK, + DevStatus::ACK | DevStatus::FEATURES_OK, + DevStatus::ACK | DevStatus::DRIVER | DevStatus::DRIVER_OK, + ] { + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + invalid.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(0) + ); + assert!(event_rx.is_empty()); + } + + // Unknown status bits from empty state are ignored + for unknown in [0x10u64, 0x20, 0x30, 0xff] { + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1, unknown), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(0) + ); + assert!(event_rx.is_empty()); + } + + // Advance to ACK + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + DevStatus::ACK.bits() as u64 + ), + Ok(Action::None) + ); + + // Unknown status bits while in ACK state are ignored + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + DevStatus::ACK.bits() as u64 | 0x10 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == DevStatus::ACK.bits() as u64 + ); + + // Invalid transition: skipping DRIVER (writing ACK | FEATURES_OK) + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + (DevStatus::ACK | DevStatus::FEATURES_OK).bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == DevStatus::ACK.bits() as u64 + ); + + // Invalid transition: skipping DRIVER (writing ACK | DRIVER_OK) + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + (DevStatus::ACK | DevStatus::DRIVER_OK).bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == DevStatus::ACK.bits() as u64 + ); + + // Advance to ACK | DRIVER + let ack_driver = DevStatus::ACK | DevStatus::DRIVER; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + ack_driver.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver.bits() as u64 + ); + + // Invalid transition: clearing DRIVER bit (writing ACK only) without resetting to 0 + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + DevStatus::ACK.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Invalid transition: setting DRIVER_OK without retaining ACK | DRIVER + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + DevStatus::DRIVER_OK.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Invalid transition: skipping FEATURES_OK (writing ACK | DRIVER | DRIVER_OK) + let skipping_features_ok = ack_driver | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + skipping_features_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Advance to DRIVER_OK + let all_ok = ack_driver | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + all_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Start { .. })); + + // Invalid transition: clearing DRIVER_OK without resetting to 0 + let without_driver_ok = ack_driver | DevStatus::FEATURES_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + without_driver_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Invalid transition: clearing FEATURES_OK without resetting to 0 + let without_features_ok = ack_driver | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + without_features_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Invalid transition: clearing DRIVER without resetting to 0 + let without_driver = DevStatus::ACK | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + without_driver.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Add FAILED flag + let failed = all_ok | DevStatus::FAILED; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + failed.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == failed.bits() as u64 + ); + + // Invalid transition: clearing FAILED flag without resetting to 0 + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + all_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == failed.bits() as u64 + ); + assert!(event_rx.is_empty()); +} + +#[test] +fn test_virtio_pci_device_status_multistep_transition() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx) = create_test_mmio(queues); + + // Set driver features + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0x1234_5678 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 1), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0x9abc_def0 + ), + Ok(Action::None) + ); + + // Multi-step transition: 0 -> ACK | DRIVER | FEATURES_OK | DRIVER_OK in a single write + let all_ok = DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + all_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert_matches!( + event_rx.try_recv(), + Ok(WakeEvent::Start { param }) => { + assert_eq!(param.feature, ((0x9abc_def0u128) << 32) | 0x1234_5678); + } + ); + assert!(event_rx.is_empty()); + + // Reset to 0 + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1, 0), + Ok(Action::None) + ); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset)); + + // Multi-step transition: 0 -> ACK | DRIVER in a single write + let ack_driver = DevStatus::ACK | DevStatus::DRIVER; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + ack_driver.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == ack_driver.bits() as u64 + ); + assert!(event_rx.is_empty()); + + // Multi-step transition: ACK | DRIVER -> ACK | DRIVER | FEATURES_OK | DRIVER_OK in a single write + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + all_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(status) if status == all_ok.bits() as u64 + ); + assert_matches!( + event_rx.try_recv(), + Ok(WakeEvent::Start { param }) => { + assert_eq!(param.feature, ((0x9abc_def0u128) << 32) | 0x1234_5678); + } + ); + assert!(event_rx.is_empty()); +} + +#[test] +fn test_virtio_pci_device_status_reset_without_driver_ok() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx) = create_test_mmio(queues.clone()); + + // Set status to ACK | DRIVER + let ack_driver = DevStatus::ACK | DevStatus::DRIVER; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + ack_driver.bits() as u64 + ), + Ok(Action::None) + ); + + // Enable queue and set MSI-X config vector before reset + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_SELECT.0 as u64, 2, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_ENABLE.0 as u64, 2, 1), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_CONFIG_MSIX_VECTOR.0 as u64, 2, 0), + Ok(Action::None) + ); + assert!(queues[0].enabled.load(Ordering::Acquire)); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_CONFIG_MSIX_VECTOR.0 as u64, 2), + Ok(0) + ); + + // Reset status to 0 before DRIVER_OK is set + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, 1), + Ok(0) + ); + // No WakeEvent::Reset since DRIVER_OK was not set + assert!(event_rx.is_empty()); + // self.reset() should still disable queues and reset MSI-X vectors + assert!(!queues[0].enabled.load(Ordering::Acquire)); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_CONFIG_MSIX_VECTOR.0 as u64, 2), + Ok(vector) if vector == VIRTIO_MSI_NO_VECTOR as u64 + ); +} + +#[test] +fn test_virtio_pci_driver_features() { + let queues = Arc::new([QueueReg::default()]); + let (mut mmio, _event_rx) = create_test_mmio(queues); + mmio.reg.device_feature = [0x1234_5678, 0x0000_0005, 0, 0]; + + // Bank 0: only offered device features are accepted + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0xffff_ffff + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4), + Ok(0x1234_5678) + ); + + // Bank 1: only offered device features are accepted + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 1), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0xffff_ffff + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4), + Ok(0x0000_0005) + ); + + // Bank 2 (no device features offered): writes are masked to 0 + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 2), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + 0xffff_ffff + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4), + Ok(0) + ); + + // Out-of-bounds bank selection does not store and does not panic + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, + 4, + 10 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4, 0x1234), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4), + Ok(0) + ); + + // Set status to ACK | DRIVER | FEATURES_OK + let features_ok = DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DEVICE_STATUS.0 as u64, + 1, + features_ok.bits() as u64 + ), + Ok(Action::None) + ); + + // Feature writes after FEATURES_OK must be ignored + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4, 0), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, 4), + Ok(0x1234_5678) + ); +} + +#[test] +fn test_virtio_pci_queue_alignment() { + let queues = Arc::new([QueueReg { + desc: AtomicU64::new(0x1000_0000), + driver: AtomicU64::new(0x2000_0000), + device: AtomicU64::new(0x3000_0000), + ..Default::default() + }]); + let (mmio, _event_rx) = create_test_mmio(queues); + + // Select queue 0 + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_SELECT.0 as u64, 2, 0), + Ok(Action::None) + ); + + // LAYOUT_QUEUE_DESC_LO: must be 16-byte aligned + // Unaligned write (e.g. offset 8) should be ignored + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DESC_LO.0 as u64, 4, 0x1008), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DESC_LO.0 as u64, 4), + Ok(0x1000_0000) + ); + // Aligned write (16-byte aligned) should succeed + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DESC_LO.0 as u64, 4, 0x1010), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DESC_LO.0 as u64, 4), + Ok(0x1010) + ); + + // LAYOUT_QUEUE_DEVICE_LO: must be 4-byte aligned + // Unaligned write (e.g. 2) should be ignored + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DEVICE_LO.0 as u64, 4, 0x3002), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DEVICE_LO.0 as u64, 4), + Ok(0x3000_0000) + ); + // Aligned write (4-byte aligned) should succeed + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DEVICE_LO.0 as u64, 4, 0x3004), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DEVICE_LO.0 as u64, 4), + Ok(0x3004) + ); + + // LAYOUT_QUEUE_DRIVER_LO without RING_PACKED (split queue): + // 2-byte aligned is allowed, unaligned 1-byte is rejected + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4, 0x2001), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4), + Ok(0x2000_0000) + ); + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4, 0x2002), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4), + Ok(0x2002) + ); + + // Enable RING_PACKED in driver feature + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_DRIVER_FEATURE_SELECT.0 as u64, 4, 1), + Ok(Action::None) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::LAYOUT_DRIVER_FEATURE.0 as u64, + 4, + (VirtioFeature::RING_PACKED.bits() >> 32) as u64 + ), + Ok(Action::None) + ); + + // LAYOUT_QUEUE_DRIVER_LO with RING_PACKED: + // 2-byte aligned (not 4-byte aligned) must now be rejected + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4, 0x4002), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4), + Ok(0x2002) + ); + // 4-byte aligned is accepted + assert_matches!( + mmio.write(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4, 0x4004), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::LAYOUT_QUEUE_DRIVER_LO.0 as u64, 4), + Ok(0x4004) + ); +} diff --git a/alioth/src/virtio/virtio.rs b/alioth/src/virtio/virtio.rs index bbbbf08b..b307f788 100644 --- a/alioth/src/virtio/virtio.rs +++ b/alioth/src/virtio/virtio.rs @@ -29,6 +29,7 @@ use std::fmt::Debug; use std::os::fd::BorrowedFd; use std::path::Path; +use bitflags::Flags; use snafu::Snafu; use crate::errors::{DebugTrace, trace_error}; @@ -124,6 +125,23 @@ bitflags! { } } +impl DevStatus { + pub fn is_valid(&self) -> bool { + if self.contains_unknown_bits() { + return false; + } + + let status = *self + & (DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK); + + status == DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK + || status == DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK + || status == DevStatus::ACK | DevStatus::DRIVER + || status == DevStatus::ACK + || status == DevStatus::empty() + } +} + pub trait IrqSender: Send + Sync + Debug + 'static { fn queue_irq(&self, idx: u16); fn config_irq(&self); diff --git a/alioth/src/virtio/virtio_test.rs b/alioth/src/virtio/virtio_test.rs index f40bed2f..2d39addd 100644 --- a/alioth/src/virtio/virtio_test.rs +++ b/alioth/src/virtio/virtio_test.rs @@ -16,11 +16,12 @@ use std::os::fd::{AsFd, BorrowedFd}; use std::sync::atomic::{AtomicBool, AtomicU16, AtomicU64}; use flume::Sender; +use rstest::rstest; use crate::hv::IoeventFd; use crate::mem::mapped::{ArcMemPages, RamBus}; use crate::virtio::queue::{QUEUE_SIZE_MAX, QueueReg}; -use crate::virtio::{IrqSender, Result}; +use crate::virtio::{DevStatus, IrqSender, Result}; pub const QUEUE_SIZE: u16 = QUEUE_SIZE_MAX; const MEM_SIZE: usize = 2 << 20; @@ -88,3 +89,84 @@ impl AsFd for FakeIoeventFd { } impl IoeventFd for FakeIoeventFd {} + +#[rstest] +// Valid states in standard initialization sequence +#[case(DevStatus::empty(), true)] +#[case(DevStatus::ACK, true)] +#[case(DevStatus::ACK | DevStatus::DRIVER, true)] +#[case(DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK, true)] +#[case( + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK, + true, +)] +// Valid states with auxiliary flags (FAILED, NEEDS_RESET) +#[case(DevStatus::FAILED, true)] +#[case(DevStatus::ACK | DevStatus::FAILED, true)] +#[case(DevStatus::ACK | DevStatus::DRIVER | DevStatus::FAILED, true)] +#[case( + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::FAILED, + true, +)] +#[case( + DevStatus::ACK + | DevStatus::DRIVER + | DevStatus::FEATURES_OK + | DevStatus::DRIVER_OK + | DevStatus::FAILED, + true, +)] +#[case(DevStatus::NEEDS_RESET, true)] +#[case(DevStatus::ACK | DevStatus::NEEDS_RESET, true)] +#[case(DevStatus::ACK | DevStatus::DRIVER | DevStatus::NEEDS_RESET, true)] +#[case( + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::NEEDS_RESET, + true, +)] +#[case( + DevStatus::ACK + | DevStatus::DRIVER + | DevStatus::FEATURES_OK + | DevStatus::DRIVER_OK + | DevStatus::NEEDS_RESET, + true, +)] +#[case(DevStatus::FAILED | DevStatus::NEEDS_RESET, true)] +#[case( + DevStatus::ACK + | DevStatus::DRIVER + | DevStatus::FEATURES_OK + | DevStatus::DRIVER_OK + | DevStatus::FAILED + | DevStatus::NEEDS_RESET, + true, +)] +// Invalid states (skipping steps or isolated bits) +#[case(DevStatus::DRIVER, false)] +#[case(DevStatus::FEATURES_OK, false)] +#[case(DevStatus::DRIVER_OK, false)] +#[case(DevStatus::ACK | DevStatus::FEATURES_OK, false)] +#[case(DevStatus::ACK | DevStatus::DRIVER_OK, false)] +#[case(DevStatus::ACK | DevStatus::DRIVER | DevStatus::DRIVER_OK, false)] +#[case(DevStatus::DRIVER | DevStatus::FEATURES_OK, false)] +#[case(DevStatus::DRIVER | DevStatus::DRIVER_OK, false)] +#[case(DevStatus::FEATURES_OK | DevStatus::DRIVER_OK, false)] +#[case(DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK, false)] +// Invalid states with auxiliary flags +#[case(DevStatus::DRIVER | DevStatus::FAILED, false)] +#[case(DevStatus::FEATURES_OK | DevStatus::FAILED, false)] +#[case(DevStatus::DRIVER_OK | DevStatus::FAILED, false)] +#[case(DevStatus::ACK | DevStatus::FEATURES_OK | DevStatus::FAILED, false)] +#[case(DevStatus::ACK | DevStatus::DRIVER_OK | DevStatus::FAILED, false)] +#[case(DevStatus::ACK | DevStatus::DRIVER | DevStatus::DRIVER_OK | DevStatus::FAILED, false)] +#[case(DevStatus::DRIVER | DevStatus::NEEDS_RESET, false)] +#[case(DevStatus::FEATURES_OK | DevStatus::NEEDS_RESET, false)] +#[case(DevStatus::DRIVER_OK | DevStatus::NEEDS_RESET, false)] +#[case(DevStatus::ACK | DevStatus::FEATURES_OK | DevStatus::NEEDS_RESET, false)] +#[case(DevStatus::ACK | DevStatus::DRIVER_OK | DevStatus::NEEDS_RESET, false)] +#[case(DevStatus::ACK | DevStatus::DRIVER | DevStatus::DRIVER_OK | DevStatus::NEEDS_RESET, false)] +// Unknown bits +#[case(DevStatus(0xff), false)] +fn test_dev_status_is_valid(#[case] status: DevStatus, #[case] valid: bool) { + assert_eq!(status.is_valid(), valid); +}