Vhost user blk basic config change

resources/seccomp/aarch64-unknown-linux-musl.json

@@ -428,6 +428,14 @@
             {
                 "syscall": "sched_yield",
                 "comment": "Used by the rust standard library in std::sync::mpmc. Firecracker uses mpsc channels from this module for inter-thread communication"
+            },
+            {
+                "syscall": "sendmsg",
+                "comment": "Used by vhost-user frontend to communicate with the backend"
+            },
+            {
+                "syscall": "recvmsg",
+                "comment": "Used by vhost-user frontend to read response from the backend"
             }
         ]
     },

resources/seccomp/x86_64-unknown-linux-musl.json

@@ -440,6 +440,14 @@
             {
                 "syscall": "sched_yield",
                 "comment": "Used by the rust standard library in std::sync::mpmc. Firecracker uses mpsc channels from this module for inter-thread communication"
+            },
+            {
+                "syscall": "sendmsg",
+                "comment": "Used by vhost-user frontend to communicate with the backend"
+            },
+            {
+                "syscall": "recvmsg",
+                "comment": "Used by vhost-user frontend to read response from the backend"
             }
         ]
     },

src/api_server/src/request/drive.rs

@@ -64,21 +64,6 @@ pub(crate) fn parse_patch_drive(
         ));
     }
 
-    // Validate request - we need to have at least one parameter set:
-    // - path_on_host
-    // - rate_limiter
-    if block_device_update_cfg.path_on_host.is_none()
-        && block_device_update_cfg.rate_limiter.is_none()
-    {
-        METRICS.patch_api_requests.drive_fails.inc();
-        return Err(Error::Generic(
-            StatusCode::BadRequest,
-            String::from(
-                "Please specify at least one property to patch: path_on_host, rate_limiter.",
-            ),
-        ));
-    }
-
     Ok(ParsedRequest::new_sync(VmmAction::UpdateBlockDevice(
         block_device_update_cfg,
     )))
@@ -117,12 +102,12 @@ mod tests {
         let res = parse_patch_drive(&Body::new(body), Some("1000"));
         assert!(res.is_err());
 
-        // PATCH with missing path_on_host field.
+        // PATCH with only drive_id field.
         let body = r#"{
-            "drive_id": "dummy_id"
+            "drive_id": "1000"
         }"#;
-        let res = parse_patch_drive(&Body::new(body), Some("dummy_id"));
-        assert!(res.is_err());
+        let res = parse_patch_drive(&Body::new(body), Some("1000"));
+        assert!(res.is_ok());
 
         // PATCH with missing drive_id field.
         let body = r#"{

src/vmm/src/devices/virtio/mmio.rs

@@ -246,14 +246,25 @@ impl MmioTransport {
                     0x34 => self.with_queue(0, |q| u32::from(q.get_max_size())),
                     0x44 => self.with_queue(0, |q| u32::from(q.ready)),
                     0x60 => {
-                        // For vhost-user backed devices we can only report
-                        // `VIRTIO_MMIO_INT_VRING` (bit 0 set) value, as any
-                        // changes to the interrupt status value cannot be propagated
-                        // back to FC from vhost-user-backend. This also means that backed should
-                        // not change device configuration as it also requires update to the
-                        // interrupt status (bit 1 set).
+                        // For vhost-user backed devices we need some additional
+                        // logic to differentiate between `VIRTIO_MMIO_INT_VRING`
+                        // and `VIRTIO_MMIO_INT_CONFIG` statuses.
+                        // Because backend cannot propagate any interrupt status
+                        // changes to the FC we always try to serve the `VIRTIO_MMIO_INT_VRING`
+                        // status. But in case when backend changes the configuration and
+                        // user triggers the manual notification, FC needs to send
+                        // `VIRTIO_MMIO_INT_CONFIG`. We know that for vhost-user devices the
+                        // interrupt status can only be 0 (no one set any bits) or
+                        // `VIRTIO_MMIO_INT_CONFIG`. Based on this knowledge we can simply
+                        // check if the current interrupt_status is equal to the
+                        // `VIRTIO_MMIO_INT_CONFIG` or not to understand if we need to send
+                        // `VIRTIO_MMIO_INT_CONFIG` or
+                        // `VIRTIO_MMIO_INT_VRING`.
+                        let is = self.interrupt_status.load(Ordering::SeqCst);
                         if !self.is_vhost_user {
-                            self.interrupt_status.load(Ordering::SeqCst)
+                            is
+                        } else if is == VIRTIO_MMIO_INT_CONFIG {
+                            VIRTIO_MMIO_INT_CONFIG
                         } else {
                             VIRTIO_MMIO_INT_VRING
                         }
@@ -539,10 +550,16 @@ pub(crate) mod tests {
         assert_eq!(read_le_u32(&buf[..]), 111);
 
         d.is_vhost_user = true;
-        d.interrupt_status.store(222, Ordering::SeqCst);
+        d.interrupt_status.store(0, Ordering::SeqCst);
         d.bus_read(0x60, &mut buf[..]);
         assert_eq!(read_le_u32(&buf[..]), VIRTIO_MMIO_INT_VRING);
 
+        d.is_vhost_user = true;
+        d.interrupt_status
+            .store(VIRTIO_MMIO_INT_CONFIG, Ordering::SeqCst);
+        d.bus_read(0x60, &mut buf[..]);
+        assert_eq!(read_le_u32(&buf[..]), VIRTIO_MMIO_INT_CONFIG);
+
         d.bus_read(0x70, &mut buf[..]);
         assert_eq!(read_le_u32(&buf[..]), 0);
 

src/vmm/src/devices/virtio/vhost_user_block/device.rs

@@ -17,7 +17,7 @@ use vhost::vhost_user::Frontend;
 
 use super::{VhostUserBlockError, NUM_QUEUES, QUEUE_SIZE};
 use crate::devices::virtio::block_common::CacheType;
-use crate::devices::virtio::device::{DeviceState, IrqTrigger, VirtioDevice};
+use crate::devices::virtio::device::{DeviceState, IrqTrigger, IrqType, VirtioDevice};
 use crate::devices::virtio::gen::virtio_blk::{
     VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_RO, VIRTIO_F_VERSION_1,
 };
@@ -181,7 +181,7 @@ impl<T: VhostUserHandleBackend> VhostUserBlockImpl<T> {
         // Get config from backend if CONFIG is acked or use empty buffer.
         let config_space =
             if acked_protocol_features & VhostUserProtocolFeatures::CONFIG.bits() != 0 {
-                // This buffer is read only. Ask vhost implementation why.
+                // This buffer is used for config size check in vhost crate.
                 let buffer = [0u8; BLOCK_CONFIG_SPACE_SIZE as usize];
                 let (_, new_config_space) = vu_handle
                     .vu
@@ -254,6 +254,32 @@ impl<T: VhostUserHandleBackend> VhostUserBlockImpl<T> {
             socket: self.vu_handle.socket_path.clone(),
         }
     }
+
+    pub fn config_update(&mut self) -> Result<(), VhostUserBlockError> {
+        let start_time = utils::time::get_time_us(utils::time::ClockType::Monotonic);
+
+        // This buffer is used for config size check in vhost crate.
+        let buffer = [0u8; BLOCK_CONFIG_SPACE_SIZE as usize];
+        let (_, new_config_space) = self
+            .vu_handle
+            .vu
+            .get_config(
+                0,
+                BLOCK_CONFIG_SPACE_SIZE,
+                VhostUserConfigFlags::WRITABLE,
+                &buffer,
+            )
+            .map_err(VhostUserBlockError::Vhost)?;
+        self.config_space = new_config_space;
+        self.irq_trigger
+            .trigger_irq(IrqType::Config)
+            .map_err(VhostUserBlockError::IrqTrigger)?;
+
+        let delta_us = utils::time::get_time_us(utils::time::ClockType::Monotonic) - start_time;
+        self.metrics.config_change_time_us.store(delta_us);
+
+        Ok(())
+    }
 }
 
 impl<T: VhostUserHandleBackend + Send + 'static> VirtioDevice for VhostUserBlockImpl<T> {
@@ -349,11 +375,13 @@ mod tests {
     #![allow(clippy::undocumented_unsafe_blocks)]
 
     use std::os::unix::net::UnixStream;
+    use std::sync::atomic::Ordering;
 
     use utils::tempfile::TempFile;
     use vhost::{VhostUserMemoryRegionInfo, VringConfigData};
 
     use super::*;
+    use crate::devices::virtio::mmio::VIRTIO_MMIO_INT_CONFIG;
     use crate::utilities::test_utils::create_tmp_socket;
     use crate::vstate::memory::{FileOffset, GuestAddress, GuestMemoryExtension};
 
@@ -626,6 +654,15 @@ mod tests {
         // Writing to the config does nothing
         vhost_block.write_config(0x69, &[0]);
         assert_eq!(vhost_block.config_space, vec![0x69, 0x69, 0x69]);
+
+        // Testing [`config_update`]
+        vhost_block.config_space = vec![];
+        vhost_block.config_update().unwrap();
+        assert_eq!(vhost_block.config_space, vec![0x69, 0x69, 0x69]);
+        assert_eq!(
+            vhost_block.interrupt_status().load(Ordering::SeqCst),
+            VIRTIO_MMIO_INT_CONFIG
+        );
     }
 
     #[test]

src/vmm/src/devices/virtio/vhost_user_metrics.rs

@@ -14,25 +14,28 @@
 //!     "cfg_fails": "SharedIncMetric",
 //!     "init_time_us": SharedStoreMetric,
 //!     "activate_time_us": SharedStoreMetric,
+//!     "config_change_time_us": SharedStoreMetric,
 //!  }
 //!  "vhost_user_{mod}_id1": {
 //!     "activate_fails": "SharedIncMetric",
 //!     "cfg_fails": "SharedIncMetric",
 //!     "init_time_us": SharedStoreMetric,
 //!     "activate_time_us": SharedStoreMetric,
+//!     "config_change_time_us": SharedStoreMetric,
 //!  }
 //!  ...
 //!  "vhost_user_{mod}_idN": {
 //!     "activate_fails": "SharedIncMetric",
 //!     "cfg_fails": "SharedIncMetric",
 //!     "init_time_us": SharedStoreMetric,
 //!     "activate_time_us": SharedStoreMetric,
+//!     "config_change_time_us": SharedStoreMetric,
 //!  }
 //! }
 //! ```
 //! Each `vhost_user` field in the example above is a serializable `VhostUserDeviceMetrics`
-//! structure collecting metrics such as `activate_fails`, `cfg_fails`, `init_time_us` and
-//! `activate_time_us` for the vhost_user device.
+//! structure collecting metrics such as `activate_fails`, `cfg_fails`, `init_time_us`,
+//! `activate_time_us` and `config_change_time_us` for the vhost_user device.
 //! For vhost-user block device having endpoint "/drives/drv0" the emitted metrics would be
 //! `vhost_user_block_drv0`.
 //! For vhost-user block device having endpoint "/drives/drvN" the emitted metrics would be
@@ -139,6 +142,8 @@ pub struct VhostUserDeviceMetrics {
     pub init_time_us: SharedStoreMetric,
     // Vhost-user activate time in microseconds.
     pub activate_time_us: SharedStoreMetric,
+    // Vhost-user config change time in microseconds.
+    pub config_change_time_us: SharedStoreMetric,
 }
 
 #[cfg(test)]

src/vmm/src/lib.rs

@@ -113,6 +113,7 @@
 use std::sync::{Arc, Barrier, Mutex};
 use std::time::Duration;
 
+use devices::virtio::vhost_user_block::device::VhostUserBlock;
 use event_manager::{EventManager as BaseEventManager, EventOps, Events, MutEventSubscriber};
 use seccompiler::BpfProgram;
 use snapshot::Persist;
@@ -655,6 +656,15 @@
             .map_err(VmmError::DeviceManager)
     }
 
+    /// Updates the rate limiter parameters for block device with `drive_id` id.
+    pub fn update_vhost_user_block_config(&mut self, drive_id: &str) -> Result<(), VmmError> {
+        self.mmio_device_manager
+            .with_virtio_device_with_id(TYPE_BLOCK, drive_id, |block: &mut VhostUserBlock| {
+                block.config_update().map_err(|err| format!("{:?}", err))
+            })
+            .map_err(VmmError::DeviceManager)
+    }
+
     /// Updates the rate limiter parameters for net device with `net_id` id.
     pub fn update_net_rate_limiters(
         &mut self,

src/vmm/src/rpc_interface.rs

@@ -811,6 +811,15 @@ impl RuntimeApiController {
         new_cfg: BlockDeviceUpdateConfig,
     ) -> Result<VmmData, VmmActionError> {
         let mut vmm = self.vmm.lock().expect("Poisoned lock");
+
+        // vhost-user-block updates
+        if new_cfg.path_on_host.is_none() && new_cfg.rate_limiter.is_none() {
+            vmm.update_vhost_user_block_config(&new_cfg.drive_id)
+                .map(|()| VmmData::Empty)
+                .map_err(DriveError::DeviceUpdate)?;
+        }
+
+        // virtio-block updates
         if let Some(new_path) = new_cfg.path_on_host {
             vmm.update_block_device_path(&new_cfg.drive_id, new_path)
                 .map(|()| VmmData::Empty)
@@ -1084,6 +1093,7 @@ mod tests {
         pub update_balloon_config_called: bool,
         pub update_balloon_stats_config_called: bool,
         pub update_block_device_path_called: bool,
+        pub update_block_device_vhost_user_config_called: bool,
         pub update_net_rate_limiters_called: bool,
         // when `true`, all self methods are forced to fail
         pub force_errors: bool,
@@ -1168,6 +1178,16 @@ mod tests {
             Ok(())
         }
 
+        pub fn update_vhost_user_block_config(&mut self, _: &str) -> Result<(), VmmError> {
+            if self.force_errors {
+                return Err(VmmError::DeviceManager(
+                    crate::device_manager::mmio::MmioError::InvalidDeviceType,
+                ));
+            }
+            self.update_block_device_vhost_user_config_called = true;
+            Ok(())
+        }
+
         pub fn update_net_rate_limiters(
             &mut self,
             _: &str,
@@ -1984,6 +2004,27 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_runtime_update_block_device_vhost_user_config() {
+        let req = VmmAction::UpdateBlockDevice(BlockDeviceUpdateConfig {
+            ..Default::default()
+        });
+        check_runtime_request(req, |result, vmm| {
+            assert_eq!(result, Ok(VmmData::Empty));
+            assert!(vmm.update_block_device_vhost_user_config_called)
+        });
+
+        let req = VmmAction::UpdateBlockDevice(BlockDeviceUpdateConfig {
+            ..Default::default()
+        });
+        check_runtime_request_err(
+            req,
+            VmmActionError::DriveConfig(DriveError::DeviceUpdate(VmmError::DeviceManager(
+                crate::device_manager::mmio::MmioError::InvalidDeviceType,
+            ))),
+        );
+    }
+
     #[test]
     fn test_runtime_update_net_rate_limiters() {
         let req = VmmAction::UpdateNetworkInterface(NetworkInterfaceUpdateConfig {

src/vmm/src/vmm_config/drive.rs

@@ -27,7 +27,7 @@ pub enum DriveError {
     CreateVhostUserBlockDevice(VhostUserBlockError),
     /// Cannot create RateLimiter: {0}
     CreateRateLimiter(io::Error),
-    /// Unable to patch the block device: {0}
+    /// Unable to patch the block device: {0} Please verify the request arguments.
     DeviceUpdate(VmmError),
     /// A root block device already exists!
     RootBlockDeviceAlreadyAdded,

tests/framework/microvm.py

@@ -704,13 +704,16 @@ def add_vhost_user_drive(
             cache_type=cache_type,
         )
 
-    def patch_drive(self, drive_id, file):
+    def patch_drive(self, drive_id, file=None):
         """Modify/patch an existing block device."""
-        self.api.drive.patch(
-            drive_id=drive_id,
-            path_on_host=self.create_jailed_resource(file.path),
-        )
-        self.disks[drive_id] = Path(file.path)
+        if file:
+            self.api.drive.patch(
+                drive_id=drive_id,
+                path_on_host=self.create_jailed_resource(file.path),
+            )
+            self.disks[drive_id] = Path(file.path)
+        else:
+            self.api.drive.patch(drive_id=drive_id)
 
     def add_net_iface(self, iface=None, api=True, **kwargs):
         """Add a network interface"""

tests/host_tools/metrics.py

@@ -446,6 +446,7 @@ def validate_fc_metrics(metrics):
                 "cfg_fails",
                 "init_time_us",
                 "activate_time_us",
+                "config_change_time_us",
             ]
             vhost_user_devices.append(metrics_name)