use userspace bounce buffers if no swiotlb is configured

roypat · roypat · commit dfe97ece69fa · 2025-04-16T11:10:41.000+01:00
Mark vhost-user and async block engine as incompatible, as for
vhost-user-blk, we would need to communicate the need for bounce buffers
to the backend somehow, and for the async block engine we would need to
somehow keep the bounce buffers around until io_uring finishes requests
(which is not impossible, but complicated and not needed for now).

Signed-off-by: Patrick Roy &lt;roypat@amazon.co.uk&gt;
diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs
@@ -322,16 +322,24 @@ pub fn build_microvm_for_boot(
         &mut boot_cmdline,
         vm_resources.block.devices.iter(),
         event_manager,
+        secret_free,
     )?;
     attach_net_devices(
         &mut vmm,
         &mut boot_cmdline,
         vm_resources.net_builder.iter(),
         event_manager,
+        secret_free,
     )?;
 
     if let Some(unix_vsock) = vm_resources.vsock.get() {
-        attach_unixsock_vsock_device(&mut vmm, &mut boot_cmdline, unix_vsock, event_manager)?;
+        attach_unixsock_vsock_device(
+            &mut vmm,
+            &mut boot_cmdline,
+            unix_vsock,
+            event_manager,
+            secret_free,
+        )?;
     }
 
     if let Some(entropy) = vm_resources.entropy.get() {
@@ -708,14 +716,19 @@ fn attach_virtio_device<T: 'static + VirtioDevice + MutEventSubscriber + Debug>(
     device: Arc<Mutex<T>>,
     cmdline: &mut LoaderKernelCmdline,
     is_vhost_user: bool,
+    needs_bouncing: bool,
 ) -> Result<(), MmioError> {
     event_manager.add_subscriber(device.clone());
 
     // We have to enable swiotlb as part of the boot process, because the device objects
     // themselves are created when the corresponding PUT API calls are made, and at that
     // point we don't know yet whether swiotlb should be enabled or not.
     if vmm.vm.has_swiotlb() {
+        // If swiotlb was requested via the API, enable it independently of whether
+        // bouncing is actually required (e.g. of whether secret hiding is enabled)
         device.lock().unwrap().force_swiotlb();
+    } else if needs_bouncing {
+        device.lock().unwrap().force_userspace_bounce_buffers();
     }
 
     // The device mutex mustn't be locked here otherwise it will deadlock.
@@ -773,6 +786,7 @@ fn attach_entropy_device(
         entropy_device.clone(),
         cmdline,
         false,
+        false,
     )
 }
 
@@ -781,6 +795,7 @@ fn attach_block_devices<'a, I: Iterator<Item = &'a Arc<Mutex<Block>>> + Debug>(
     cmdline: &mut LoaderKernelCmdline,
     blocks: I,
     event_manager: &mut EventManager,
+    needs_bouncing: bool,
 ) -> Result<(), StartMicrovmError> {
     for block in blocks {
         let (id, is_vhost_user) = {
@@ -805,6 +820,7 @@ fn attach_block_devices<'a, I: Iterator<Item = &'a Arc<Mutex<Block>>> + Debug>(
             block.clone(),
             cmdline,
             is_vhost_user,
+            needs_bouncing,
         )?;
     }
     Ok(())
@@ -815,11 +831,20 @@ fn attach_net_devices<'a, I: Iterator<Item = &'a Arc<Mutex<Net>>> + Debug>(
     cmdline: &mut LoaderKernelCmdline,
     net_devices: I,
     event_manager: &mut EventManager,
+    needs_bouncing: bool,
 ) -> Result<(), StartMicrovmError> {
     for net_device in net_devices {
         let id = net_device.lock().expect("Poisoned lock").id().clone();
         // The device mutex mustn't be locked here otherwise it will deadlock.
-        attach_virtio_device(event_manager, vmm, id, net_device.clone(), cmdline, false)?;
+        attach_virtio_device(
+            event_manager,
+            vmm,
+            id,
+            net_device.clone(),
+            cmdline,
+            false,
+            needs_bouncing,
+        )?;
     }
     Ok(())
 }
@@ -829,10 +854,19 @@ fn attach_unixsock_vsock_device(
     cmdline: &mut LoaderKernelCmdline,
     unix_vsock: &Arc<Mutex<Vsock<VsockUnixBackend>>>,
     event_manager: &mut EventManager,
+    needs_bouncing: bool,
 ) -> Result<(), MmioError> {
     let id = String::from(unix_vsock.lock().expect("Poisoned lock").id());
     // The device mutex mustn't be locked here otherwise it will deadlock.
-    attach_virtio_device(event_manager, vmm, id, unix_vsock.clone(), cmdline, false)
+    attach_virtio_device(
+        event_manager,
+        vmm,
+        id,
+        unix_vsock.clone(),
+        cmdline,
+        false,
+        needs_bouncing,
+    )
 }
 
 fn attach_balloon_device(
@@ -843,7 +877,15 @@ fn attach_balloon_device(
 ) -> Result<(), MmioError> {
     let id = String::from(balloon.lock().expect("Poisoned lock").id());
     // The device mutex mustn't be locked here otherwise it will deadlock.
-    attach_virtio_device(event_manager, vmm, id, balloon.clone(), cmdline, false)
+    attach_virtio_device(
+        event_manager,
+        vmm,
+        id,
+        balloon.clone(),
+        cmdline,
+        false,
+        false,
+    )
 }
 
 // Adds `O_NONBLOCK` to the stdout flags.
@@ -1019,6 +1061,7 @@ pub(crate) mod tests {
             cmdline,
             block_dev_configs.devices.iter(),
             event_manager,
+            false,
         )
         .unwrap();
         block_files
@@ -1033,7 +1076,7 @@ pub(crate) mod tests {
         let mut net_builder = NetBuilder::new();
         net_builder.build(net_config).unwrap();
 
-        let res = attach_net_devices(vmm, cmdline, net_builder.iter(), event_manager);
+        let res = attach_net_devices(vmm, cmdline, net_builder.iter(), event_manager, false);
         res.unwrap();
     }
 
@@ -1054,7 +1097,7 @@ pub(crate) mod tests {
             Arc::new(Mutex::new(mmds)),
         );
 
-        attach_net_devices(vmm, cmdline, net_builder.iter(), event_manager).unwrap();
+        attach_net_devices(vmm, cmdline, net_builder.iter(), event_manager, false).unwrap();
     }
 
     pub(crate) fn insert_vsock_device(
@@ -1067,7 +1110,7 @@ pub(crate) mod tests {
         let vsock = VsockBuilder::create_unixsock_vsock(vsock_config).unwrap();
         let vsock = Arc::new(Mutex::new(vsock));
 
-        attach_unixsock_vsock_device(vmm, cmdline, &vsock, event_manager).unwrap();
+        attach_unixsock_vsock_device(vmm, cmdline, &vsock, event_manager, false).unwrap();
 
         assert!(
             vmm.mmio_device_manager
diff --git a/src/vmm/src/devices/virtio/block/vhost_user/device.rs b/src/vmm/src/devices/virtio/block/vhost_user/device.rs
@@ -302,6 +302,7 @@ impl<T: VhostUserHandleBackend + Send + 'static> VirtioDevice for VhostUserBlock
 
     fn force_userspace_bounce_buffers(&mut self) {
         // Nothing Firecracker can do about this, the backend would need to do the bouncing
+        panic!("vhost-user-blk is incompatible with userspace bounce buffers")
     }
 
     fn userspace_bounce_buffers(&self) -> bool {
diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs
@@ -586,7 +586,9 @@ impl VirtioDevice for VirtioBlock {
 
     fn force_userspace_bounce_buffers(&mut self) {
         match self.disk.file_engine {
-            FileEngine::Async(_) => panic!("No idea how this is supposed to work for io_uring"),
+            FileEngine::Async(_) => {
+                panic!("async engine is incompatible with userspace bounce buffers")
+            }
             FileEngine::Sync(ref mut engine) => engine.start_bouncing(),
         }
     }
diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs
@@ -6,7 +6,7 @@
 // found in the THIRD-PARTY file.
 
 use std::collections::VecDeque;
-use std::io::{ErrorKind, Read, Write};
+use std::io::{Read, Write};
 use std::mem::{self};
 use std::net::Ipv4Addr;
 use std::sync::{Arc, Mutex};
@@ -561,17 +561,12 @@ impl Net {
 
         let _metric = net_metrics.tap_write_agg.record_latency_metrics();
         match Self::write_tap(tap, frame_iovec, bb) {
-            Ok(written) if written == frame_iovec.len() as usize => {
+            Ok(_) => {
                 let len = u64::from(frame_iovec.len());
                 net_metrics.tx_bytes_count.add(len);
                 net_metrics.tx_packets_count.inc();
                 net_metrics.tx_count.inc();
             }
-            Ok(how_many) => error!(
-                "Failed to write full frame to tap! {}/{}",
-                how_many,
-                frame_iovec.len()
-            ),
             Err(err) => {
                 error!("Failed to write to tap: {:?}", err);
                 net_metrics.tap_write_fails.inc();
@@ -844,11 +839,10 @@ impl Net {
         };
 
         if self.userspace_bouncing {
-            let how_many = match self.tap.tap_file.read(self.userspace_buffer.as_mut_slice()) {
-                Err(ioe) if ioe.kind() == ErrorKind::WouldBlock => return Ok(0),
-                Err(err) => return Err(err),
-                Ok(read) => read,
-            };
+            let how_many = self
+                .tap
+                .tap_file
+                .read(self.userspace_buffer.as_mut_slice())?;
 
             assert!(how_many <= MAX_BUFFER_SIZE);
 
@@ -877,11 +871,7 @@ impl Net {
 
             Ok(how_many)
         } else {
-            match self.tap.read_iovec(slice) {
-                Err(ioe) if ioe.kind() == ErrorKind::WouldBlock => Ok(0),
-                Err(err) => Err(err),
-                Ok(read) => Ok(read),
-            }
+            self.tap.read_iovec(slice)
         }
     }
 
diff --git a/src/vmm/src/resources.rs b/src/vmm/src/resources.rs
@@ -10,6 +10,7 @@ use serde::{Deserialize, Serialize};
 
 use crate::cpu_config::templates::CustomCpuTemplate;
 use crate::device_manager::persist::SharedDeviceType;
+use crate::devices::virtio::block::device::Block;
 use crate::logger::info;
 use crate::mmds;
 use crate::mmds::data_store::{Mmds, MmdsVersion};
@@ -111,14 +112,6 @@ pub struct VmResources {
 }
 
 impl VmResources {
-    /// Whether this [`VmResources`] object contains any devices that require host kernel access
-    /// into guest memory.
-    pub fn has_any_io_devices(&self) -> bool {
-        !self.block.devices.is_empty()
-            || self.vsock.get().is_some()
-            || self.net_builder.iter().next().is_some()
-    }
-
     /// Configures Vmm resources as described by the `config_json` param.
     pub fn from_json(
         config_json: &str,
@@ -270,16 +263,6 @@ impl VmResources {
             return Err(MachineConfigError::IncompatibleBalloonSize);
         }
 
-        if self.has_any_io_devices()
-            && self.machine_config.mem_config.secret_free
-            && !self.swiotlb_used()
-        {
-            return Err(MachineConfigError::Incompatible(
-                "secret freedom",
-                "I/O without swiotlb",
-            ));
-        }
-
         if self.balloon.get().is_some() && updated.huge_pages != HugePageConfig::None {
             return Err(MachineConfigError::Incompatible(
                 "balloon device",
@@ -292,6 +275,22 @@ impl VmResources {
                 "secret freedom",
             ));
         }
+        if !self.swiotlb_used() && updated.mem_config.secret_free {
+            if self.vhost_user_devices_used() {
+                return Err(MachineConfigError::Incompatible(
+                    "vhost-user devices",
+                    "userspace bounce buffers",
+                ));
+            }
+
+            if self.async_block_engine_used() {
+                return Err(MachineConfigError::Incompatible(
+                    "async block engine",
+                    "userspace bounce buffers",
+                ));
+            }
+        }
+
         self.machine_config = updated;
 
         Ok(())
@@ -377,13 +376,18 @@ impl VmResources {
         &mut self,
         block_device_config: BlockDeviceConfig,
     ) -> Result<(), DriveError> {
-        if self.has_any_io_devices()
-            && self.machine_config.mem_config.secret_free
-            && !self.swiotlb_used()
-        {
-            return Err(DriveError::SecretFreeWithoutSwiotlb);
+        if self.machine_config.mem_config.secret_free && !self.swiotlb_used() {
+            if block_device_config.file_engine_type == Some(FileEngineType::Async) {
+                return Err(DriveError::IncompatibleWithUserspaceBounceBuffers(
+                    "async file engine",
+                ));
+            }
+            if block_device_config.socket.is_some() {
+                return Err(DriveError::IncompatibleWithUserspaceBounceBuffers(
+                    "vhost-user-blk",
+                ));
+            }
         }
-
         self.block.insert(block_device_config)
     }
 
@@ -392,26 +396,12 @@ impl VmResources {
         &mut self,
         body: NetworkInterfaceConfig,
     ) -> Result<(), NetworkInterfaceError> {
-        if self.has_any_io_devices()
-            && self.machine_config.mem_config.secret_free
-            && !self.swiotlb_used()
-        {
-            return Err(NetworkInterfaceError::SecretFreeWithoutSwiotlb);
-        }
-
         let _ = self.net_builder.build(body)?;
         Ok(())
     }
 
     /// Sets a vsock device to be attached when the VM starts.
     pub fn set_vsock_device(&mut self, config: VsockDeviceConfig) -> Result<(), VsockConfigError> {
-        if self.has_any_io_devices()
-            && self.machine_config.mem_config.secret_free
-            && !self.swiotlb_used()
-        {
-            return Err(VsockConfigError::SecretFreeWithoutSwiotlb);
-        }
-
         self.vsock.insert(config)
     }
 
@@ -505,6 +495,16 @@ impl VmResources {
             .any(|b| b.lock().expect("Poisoned lock").is_vhost_user())
     }
 
+    fn async_block_engine_used(&self) -> bool {
+        self.block
+            .devices
+            .iter()
+            .any(|b| match &*b.lock().unwrap() {
+                Block::Virtio(b) => b.file_engine_type() == FileEngineType::Async,
+                Block::VhostUser(_) => false,
+            })
+    }
+
     /// The size of the swiotlb region requested, in MiB
     #[cfg(target_arch = "aarch64")]
     pub fn swiotlb_size(&self) -> usize {
diff --git a/src/vmm/src/vmm_config/drive.rs b/src/vmm/src/vmm_config/drive.rs
@@ -24,8 +24,8 @@ pub enum DriveError {
     DeviceUpdate(VmmError),
     /// A root block device already exists!
     RootBlockDeviceAlreadyAdded,
-    /// A block device was added to a secret free VM that doesnt have a swiotlb region.
-    SecretFreeWithoutSwiotlb,
+    /// {0} is incompatible with userspace bounce buffers.
+    IncompatibleWithUserspaceBounceBuffers(&'static str),
 }
 
 /// Use this structure to set up the Block Device before booting the kernel.
diff --git a/src/vmm/src/vmm_config/net.rs b/src/vmm/src/vmm_config/net.rs
@@ -71,8 +71,6 @@ pub enum NetworkInterfaceError {
     GuestMacAddressInUse(String),
     /// Cannot open/create the tap device: {0}
     OpenTap(#[from] TapError),
-    /// A net device was added to a secret free VM that doesnt have a swiotlb region.
-    SecretFreeWithoutSwiotlb,
 }
 
 /// Builder for a list of network devices.
diff --git a/src/vmm/src/vmm_config/vsock.rs b/src/vmm/src/vmm_config/vsock.rs

Original file line number	Diff line number	Diff line change
`@@ -302,6 +302,7 @@ impl<T: VhostUserHandleBackend + Send + 'static> VirtioDevice for VhostUserBlock`
`302`	`302`
`303`	`303`	`fn force_userspace_bounce_buffers(&mut self) {`
`304`	`304`	`// Nothing Firecracker can do about this, the backend would need to do the bouncing`
	`305`	`+ panic!("vhost-user-blk is incompatible with userspace bounce buffers")`
`305`	`306`	`}`
`306`	`307`
`307`	`308`	`fn userspace_bounce_buffers(&self) -> bool {`
Original file line number	Diff line number	Diff line change
`@@ -586,7 +586,9 @@ impl VirtioDevice for VirtioBlock {`
`586`	`586`
`587`	`587`	`fn force_userspace_bounce_buffers(&mut self) {`
`588`	`588`	`match self.disk.file_engine {`
`589`		`- FileEngine::Async(_) => panic!("No idea how this is supposed to work for io_uring"),`
	`589`	`+ FileEngine::Async(_) => {`
	`590`	`+ panic!("async engine is incompatible with userspace bounce buffers")`
	`591`	`+ }`
`590`	`592`	`FileEngine::Sync(ref mut engine) => engine.start_bouncing(),`
`591`	`593`	`}`
`592`	`594`	`}`
Original file line number	Diff line number	Diff line change
`@@ -71,8 +71,6 @@ pub enum NetworkInterfaceError {`
`71`	`71`	`GuestMacAddressInUse(String),`
`72`	`72`	`/// Cannot open/create the tap device: {0}`
`73`	`73`	`OpenTap(#[from] TapError),`
`74`		`- /// A net device was added to a secret free VM that doesnt have a swiotlb region.`
`75`		`- SecretFreeWithoutSwiotlb,`
`76`	`74`	`}`
`77`	`75`
`78`	`76`	`/// Builder for a list of network devices.`