From fc9331b16a618bf1621e460493835bdb185924a1 Mon Sep 17 00:00:00 2001
From: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Date: Sat, 15 Jun 2024 21:49:33 +0200
Subject: [PATCH] virtio: Remove legacy transport support

Transitional virtio devices (which have both legacy and virtio 1.0
support) have been the default since QEMU 2.7.0, which was released
8 years ago. Basically every non-commercial Linux distro with an older
QEMU has been EOL already.

The legacy transport is also one of the few places where port I/O is
necessary, which is non-trivial to sandbox even once we have IOMMU
support. As it isn't possible to distinguish legacy and modern virtio
devices for pcid, this would mean that all virtio drivers have to be
started as privileged even if it turns out the modern transport is
supported by the VMM.
---
 virtio-core/src/arch/aarch64.rs     |   7 --
 virtio-core/src/arch/x86.rs         |  33 +----
 virtio-core/src/legacy_transport.rs | 187 ----------------------------
 virtio-core/src/lib.rs              |   3 -
 virtio-core/src/probe.rs            |  66 +++++-----
 virtio-core/src/transport.rs        |   2 +-
 6 files changed, 34 insertions(+), 264 deletions(-)
 delete mode 100644 virtio-core/src/legacy_transport.rs

diff --git a/virtio-core/src/arch/aarch64.rs b/virtio-core/src/arch/aarch64.rs
index 29e3b5855a..5d855258ca 100644
--- a/virtio-core/src/arch/aarch64.rs
+++ b/virtio-core/src/arch/aarch64.rs
@@ -7,10 +7,3 @@ use crate::{transport::Error, Device};
 pub fn enable_msix(pcid_handle: &mut PcidServerHandle) -> Result<File, Error> {
     unimplemented!("virtio_core: aarch64 enable_msix")
 }
-
-pub fn probe_legacy_port_transport(
-    pci_config: &SubdriverArguments,
-    pcid_handle: &mut PcidServerHandle,
-) -> Result<Device, Error> {
-    panic!("virtio-core: aarch64 doesn't support legacy port I/O")
-}
\ No newline at end of file
diff --git a/virtio-core/src/arch/x86.rs b/virtio-core/src/arch/x86.rs
index edb85ab942..ebea0f9aa7 100644
--- a/virtio-core/src/arch/x86.rs
+++ b/virtio-core/src/arch/x86.rs
@@ -1,4 +1,4 @@
-use crate::{legacy_transport::LegacyTransport, reinit, transport::Error, Device};
+use crate::transport::Error;
 
 use pcid_interface::irq_helpers::{allocate_single_interrupt_vector_for_msi, read_bsp_apic_id};
 use pcid_interface::msi::MsixTableEntry;
@@ -48,34 +48,3 @@ pub fn enable_msix(pcid_handle: &mut PciFunctionHandle) -> Result<File, Error> {
     log::info!("virtio: using MSI-X (interrupt_handle={interrupt_handle:?})");
     Ok(interrupt_handle)
 }
-
-pub fn probe_legacy_port_transport(
-    pci_config: &SubdriverArguments,
-    pcid_handle: &mut PciFunctionHandle,
-) -> Result<Device, Error> {
-    let port = pci_config.func.bars[0].expect_port();
-
-    common::acquire_port_io_rights().expect("virtio: failed to set I/O privilege level");
-    log::warn!("virtio: using legacy transport");
-
-    let transport = LegacyTransport::new(port);
-
-    // Setup interrupts.
-    let all_pci_features = pcid_handle.fetch_all_features()?;
-    let has_msix = all_pci_features.iter().any(|feature| feature.is_msix());
-
-    // According to the virtio specification, the device REQUIRED to support MSI-X.
-    assert!(has_msix, "virtio: device does not support MSI-X");
-    let irq_handle = enable_msix(pcid_handle)?;
-
-    let device = Device {
-        transport,
-        irq_handle,
-        device_space: core::ptr::null_mut(),
-    };
-
-    device.transport.reset();
-    reinit(&device)?;
-
-    Ok(device)
-}
diff --git a/virtio-core/src/legacy_transport.rs b/virtio-core/src/legacy_transport.rs
deleted file mode 100644
index 45341b103f..0000000000
--- a/virtio-core/src/legacy_transport.rs
+++ /dev/null
@@ -1,187 +0,0 @@
-use std::{sync::{Weak, atomic::{AtomicU16, Ordering}, Arc}, mem::size_of, fs::File};
-
-use common::dma::Dma;
-use syscall::{Pio, Io};
-
-use crate::{transport::{NotifyBell, Transport, Queue, Error, Available, Used, queue_part_sizes, spawn_irq_thread, Mem, Borrowed}, spec::{Descriptor, DeviceStatusFlags}};
-
-
-pub enum LegacyRegister {
-    DeviceFeatures = 0, // u32
-
-    QueueAddress = 8, // u32
-    QueueSize = 12,   // u16
-    QueueSelect = 14, // u16
-    QueueNotify = 16, // u16
-
-    DeviceStatus = 18, // u8
-
-    ConfigMsixVector = 20, // u16
-    QueueMsixVector = 22,  // u16
-}
-
-struct LegacyBell(Weak<LegacyTransport>);
-
-impl NotifyBell for LegacyBell {
-    #[inline]
-    fn ring(&self, queue_index: u16) {
-        let transport = self.0.upgrade().expect("bell: transport dropped");
-        transport.write::<u16>(LegacyRegister::QueueNotify, queue_index)
-    }
-}
-
-pub struct LegacyTransport(u16, AtomicU16, Weak<Self>);
-
-impl LegacyTransport {
-    pub(super) fn new(port: u16) -> Arc<Self> {
-        Arc::new_cyclic(|sref| Self(port, AtomicU16::new(0), sref.clone()))
-    }
-
-    unsafe fn read_raw<V>(&self, offset: usize) -> V
-    where
-        V: Sized + TryFrom<u64>,
-        <V as TryFrom<u64>>::Error: std::fmt::Debug,
-    {
-        let port = self.0 + offset as u16;
-
-        if size_of::<V>() == size_of::<u8>() {
-            V::try_from(Pio::<u8>::new(port).read() as u64).unwrap()
-        } else if size_of::<V>() == size_of::<u16>() {
-            V::try_from(Pio::<u16>::new(port).read() as u64).unwrap()
-        } else if size_of::<V>() == size_of::<u32>() {
-            V::try_from(Pio::<u32>::new(port).read() as u64).unwrap()
-        } else if size_of::<V>() == size_of::<u64>() {
-            let lower = Pio::<u32>::new(port).read() as u64;
-            let upper = Pio::<u32>::new(port + size_of::<u32>() as u16).read() as u64;
-
-            V::try_from(lower | (upper << 32)).unwrap()
-        } else {
-            unreachable!()
-        }
-    }
-
-    fn read<V>(&self, register: LegacyRegister) -> V
-    where
-        V: Sized + TryFrom<u64>,
-        <V as TryFrom<u64>>::Error: std::fmt::Debug,
-    {
-        unsafe { self.read_raw(register as usize) }
-    }
-
-    fn write<V>(&self, register: LegacyRegister, value: V)
-    where
-        V: Sized + TryInto<usize>,
-        <V as TryInto<usize>>::Error: std::fmt::Debug,
-    {
-        if size_of::<V>() == size_of::<u8>() {
-            Pio::<u8>::new(self.0 + register as u16).write(value.try_into().unwrap() as u8);
-        } else if size_of::<V>() == size_of::<u16>() {
-            Pio::<u16>::new(self.0 + register as u16).write(value.try_into().unwrap() as u16);
-        } else if size_of::<V>() == size_of::<u32>() {
-            Pio::<u32>::new(self.0 + register as u16).write(value.try_into().unwrap() as u32);
-        } else {
-            unreachable!()
-        }
-    }
-}
-
-impl Transport for LegacyTransport {
-    fn reset(&self) {
-        self.write(LegacyRegister::DeviceStatus, 0u8);
-
-        let status = self.read::<u8>(LegacyRegister::DeviceStatus);
-        assert_eq!(status, 0);
-    }
-
-    fn check_device_feature(&self, feature: u32) -> bool {
-        assert!(
-            feature < 32,
-            "virtio: cannot query feature {feature} on a legacy device"
-        );
-        self.read::<u32>(LegacyRegister::DeviceFeatures) & (1 << feature) == (1 << feature)
-    }
-
-    fn ack_driver_feature(&self, feature: u32) {
-        assert!(
-            feature < 32,
-            "virtio: cannot ack feature {feature} on a legacy device"
-        );
-
-        let current = self.read::<u32>(LegacyRegister::DeviceFeatures);
-        self.write::<u32>(LegacyRegister::DeviceFeatures, current | (1 << feature));
-    }
-
-    fn setup_queue(&self, vector: u16, irq_handle: &File) -> Result<Arc<Queue>, Error> {
-        let queue_index = self.1.fetch_add(1, Ordering::SeqCst);
-        self.write(LegacyRegister::QueueSelect, queue_index);
-
-        let queue_size = self.read::<u16>(LegacyRegister::QueueSize) as usize;
-        let (desc_size, avail_size, used_size) = queue_part_sizes(queue_size);
-
-        let descriptor = unsafe {
-            Dma::<[Descriptor]>::zeroed_slice(queue_size)?.assume_init()
-        };
-
-        let avail_addr = descriptor.physical() + desc_size;
-        let avail_virt = (descriptor.as_ptr() as usize) + desc_size;
-        let avail = unsafe { Available::from_raw(Mem::Borrowed(Borrowed::new(avail_addr, avail_virt, avail_size)), queue_size)? };
-
-        let used_addr = avail_addr + avail_size;
-        let used_virt = avail_virt + desc_size;
-        let used = unsafe { Used::from_raw(Mem::Borrowed(Borrowed::new(used_addr, used_virt, used_size)), queue_size)? };
-
-        self.write::<u16>(LegacyRegister::QueueMsixVector, vector);
-        self.write::<u32>(LegacyRegister::QueueAddress, (descriptor.physical() as u32) >> 12);
-
-        log::info!("virtio-core: enabled queue #{queue_index} (size={queue_size})");
-
-        let queue = Queue::new(
-            descriptor,
-            avail,
-            used,
-            LegacyBell(self.2.clone()),
-            queue_index,
-            vector,
-        );
-
-        spawn_irq_thread(irq_handle, &queue);
-        Ok(queue)
-    }
-
-    fn load_config(&self, offset: u8, size: u8) -> u64 {
-        // We always enable MSI-X. So, the device configuration space offset will
-        // always be 0x18.
-        //
-        // Checkout 4.1.4.8 Legacy Interfaces: A Note on PCI Device Layout
-        const DEVICE_SPACE_OFFSET: usize = 0x18;
-
-        let size = size as usize;
-        let offset = DEVICE_SPACE_OFFSET + offset as usize;
-
-        unsafe {
-            if size == size_of::<u8>() {
-                self.read_raw::<u8>(offset) as u64
-            } else if size == size_of::<u16>() {
-                self.read_raw::<u16>(offset) as u64
-            } else if size == size_of::<u32>() {
-                self.read_raw::<u32>(offset) as u64
-            } else if size == size_of::<u64>() {
-                self.read_raw::<u64>(offset) as u64
-            } else {
-                unreachable!()
-            }
-        }
-    }
-
-    fn insert_status(&self, status: DeviceStatusFlags) {
-        let old = self.read::<u8>(LegacyRegister::DeviceStatus);
-        self.write(LegacyRegister::DeviceStatus, old | status.bits());
-    }
-
-    fn reinit_queue(&self, _queue: Arc<Queue>) {
-        todo!()
-    }
-
-    // Legacy devices do not have the `FEATURES_OK` bit.
-    fn finalize_features(&self) {}
-}
diff --git a/virtio-core/src/lib.rs b/virtio-core/src/lib.rs
index 8dfd9a676f..817caaf913 100644
--- a/virtio-core/src/lib.rs
+++ b/virtio-core/src/lib.rs
@@ -14,8 +14,5 @@ mod arch;
 #[path="arch/x86.rs"]
 mod arch;
 
-#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
-mod legacy_transport;
-
 
 pub use probe::{probe_device, reinit, Device, MSIX_PRIMARY_VECTOR};
diff --git a/virtio-core/src/probe.rs b/virtio-core/src/probe.rs
index 984acd88d2..9ecdd01ab4 100644
--- a/virtio-core/src/probe.rs
+++ b/virtio-core/src/probe.rs
@@ -131,48 +131,46 @@ pub fn probe_device(pcid_handle: &mut PciFunctionHandle) -> Result<Device, Error
         log::trace!("virtio-core::device-probe: {capability:?}");
     }
 
-    if let (Some(common_addr), Some(device_addr), Some((notify_addr, notify_multiplier))) =
-        (common_addr, device_addr, notify_addr)
-    {
-        // FIXME this is explicitly allowed by the virtio specification to happen
-        assert!(
-            notify_multiplier != 0,
-            "virtio-core::device_probe: device uses the same Queue Notify addresses for all queues"
-        );
+    let common_addr = common_addr.expect("virtio common capability missing");
+    let device_addr = device_addr.expect("virtio device capability missing");
+    let (notify_addr, notify_multiplier) = notify_addr.expect("virtio notify capability missing");
 
-        let common = unsafe { &mut *(common_addr as *mut CommonCfg) };
-        let device_space = unsafe { &mut *(device_addr as *mut u8) };
+    // FIXME this is explicitly allowed by the virtio specification to happen
+    assert!(
+        notify_multiplier != 0,
+        "virtio-core::device_probe: device uses the same Queue Notify addresses for all queues"
+    );
 
-        let transport = StandardTransport::new(
-            common,
-            notify_addr as *const u8,
-            notify_multiplier,
-            device_space,
-        );
+    let common = unsafe { &mut *(common_addr as *mut CommonCfg) };
+    let device_space = unsafe { &mut *(device_addr as *mut u8) };
 
-        // Setup interrupts.
-        let all_pci_features = pcid_handle.fetch_all_features()?;
-        let has_msix = all_pci_features.iter().any(|feature| feature.is_msix());
+    let transport = StandardTransport::new(
+        common,
+        notify_addr as *const u8,
+        notify_multiplier,
+        device_space,
+    );
 
-        // According to the virtio specification, the device REQUIRED to support MSI-X.
-        assert!(has_msix, "virtio: device does not support MSI-X");
-        let irq_handle = crate::arch::enable_msix(pcid_handle)?;
+    // Setup interrupts.
+    let all_pci_features = pcid_handle.fetch_all_features()?;
+    let has_msix = all_pci_features.iter().any(|feature| feature.is_msix());
 
-        log::info!("virtio: using standard PCI transport");
+    // According to the virtio specification, the device REQUIRED to support MSI-X.
+    assert!(has_msix, "virtio: device does not support MSI-X");
+    let irq_handle = crate::arch::enable_msix(pcid_handle)?;
 
-        let device = Device {
-            transport,
-            device_space,
-            irq_handle,
-        };
+    log::info!("virtio: using standard PCI transport");
 
-        device.transport.reset();
-        reinit(&device)?;
+    let device = Device {
+        transport,
+        device_space,
+        irq_handle,
+    };
 
-        Ok(device)
-    } else {
-        crate::arch::probe_legacy_port_transport(&pci_config, pcid_handle)
-    }
+    device.transport.reset();
+    reinit(&device)?;
+
+    Ok(device)
 }
 
 pub fn reinit(device: &Device) -> Result<(), Error> {
diff --git a/virtio-core/src/transport.rs b/virtio-core/src/transport.rs
index 14150cb279..c679dd8699 100644
--- a/virtio-core/src/transport.rs
+++ b/virtio-core/src/transport.rs
@@ -463,7 +463,7 @@ pub trait Transport: Sync + Send {
     fn ack_driver_feature(&self, feature: u32);
 
     /// Finalizes the acknowledged features by setting the `FEATURES_OK` bit in the
-    /// device status flags. No-op on a legacy device.
+    /// device status flags.
     fn finalize_features(&self);
 
     /// Runs the device.