Centralize vmm-data interface into bhyve_api

bin/propolis-standalone/src/snapshot.rs

@@ -22,7 +22,6 @@ use bhyve_api::{
     vdi_field_entry_v1, vdi_time_info_v1, ApiVersion, VAI_BOOT_HRTIME,
     VDC_VMM_ARCH, VDC_VMM_TIME,
 };
-use propolis::vmm::data as vmm_data;
 use propolis::{
     chardev::UDSock,
     common::{GuestAddr, GuestRegion},
@@ -439,12 +438,12 @@ pub struct VmGlobalState {
 
 fn export_global(hdl: &VmmHdl) -> std::io::Result<VmGlobalState> {
     if hdl.api_version()? > ApiVersion::V11.into() {
-        let info: vdi_time_info_v1 = vmm_data::read(hdl, -1, VDC_VMM_TIME, 1)?;
+        let info = hdl.data_op(VDC_VMM_TIME, 1).read::<vdi_time_info_v1>()?;
 
         Ok(VmGlobalState { boot_hrtime: info.vt_boot_hrtime })
     } else {
         let arch_entries: Vec<bhyve_api::vdi_field_entry_v1> =
-            vmm_data::read_many(hdl, -1, VDC_VMM_ARCH, 1)?;
+            hdl.data_op(VDC_VMM_ARCH, 1).read_all()?;
         let boot_ent = arch_entries
             .iter()
             .find(|ent| ent.vfe_ident == VAI_BOOT_HRTIME)
@@ -455,17 +454,17 @@ fn export_global(hdl: &VmmHdl) -> std::io::Result<VmGlobalState> {
 }
 fn import_global(hdl: &VmmHdl, state: &VmGlobalState) -> std::io::Result<()> {
     if hdl.api_version()? > ApiVersion::V11.into() {
-        let mut info: vdi_time_info_v1 =
-            vmm_data::read(hdl, -1, VDC_VMM_TIME, 1)?;
+        let mut info =
+            hdl.data_op(VDC_VMM_TIME, 1).read::<vdi_time_info_v1>()?;
 
         info.vt_boot_hrtime = state.boot_hrtime;
-        vmm_data::write(hdl, -1, VDC_VMM_TIME, 1, info)?;
+        hdl.data_op(VDC_VMM_TIME, 1).write(&info)?;
 
         Ok(())
     } else {
         let arch_entry =
             vdi_field_entry_v1::new(VAI_BOOT_HRTIME, state.boot_hrtime as u64);
-        vmm_data::write(hdl, -1, VDC_VMM_ARCH, 1, arch_entry)?;
+        hdl.data_op(VDC_VMM_ARCH, 1).write(&arch_entry)?;
         Ok(())
     }
 }

crates/bhyve-api/src/lib.rs

@@ -1,5 +1,6 @@
 use std::fs::{File, OpenOptions};
 use std::io::{Error, ErrorKind, Result};
+use std::mem::{size_of, size_of_val};
 use std::os::fd::*;
 use std::os::unix::fs::OpenOptionsExt;
 use std::path::PathBuf;
@@ -221,6 +222,12 @@ impl VmmFd {
         }
     }
 
+    /// Build a [`VmmDataOp`] with specified `class` and `version` to read or
+    /// write data from the in-kernel vmm.
+    pub fn data_op(&self, class: u16, version: u16) -> VmmDataOp<'_> {
+        VmmDataOp::new(self, class, version)
+    }
+
     /// Check VMM ioctl command against those known to not require any
     /// copyin/copyout to function.
     const fn ioctl_usize_safe(cmd: i32) -> bool {
@@ -241,6 +248,224 @@ impl AsRawFd for VmmFd {
     }
 }
 
+pub type VmmDataResult<T> = std::result::Result<T, VmmDataError>;
+
+/// Encompasses the configuration and context to perform a vmm-data operation
+/// (read or write) against an instance.  The `class` and `version` for the
+/// vmm-data operation are established with the parameters passed to
+/// [`VmmFd::data_op`].
+pub struct VmmDataOp<'a> {
+    fd: &'a VmmFd,
+    class: u16,
+    version: u16,
+    vcpuid: Option<i32>,
+}
+
+impl<'a> VmmDataOp<'a> {
+    pub fn new(fd: &'a VmmFd, class: u16, version: u16) -> Self {
+        Self { fd, class, version, vcpuid: None }
+    }
+}
+
+impl VmmDataOp<'_> {
+    /// Dictate that the vmm-data operation be performed in the context of a
+    /// specific vCPU, rather than against the VM as a whole.
+    pub fn for_vcpu(mut self, vcpuid: i32) -> Self {
+        self.vcpuid = Some(vcpuid);
+        self
+    }
+
+    /// Read item of data, returning the result.
+    pub fn read<T: Sized + Copy + Default>(self) -> VmmDataResult<T> {
+        let mut item = T::default();
+        self.do_read_single(
+            &mut item as *mut T as *mut libc::c_void,
+            size_of::<T>() as u32,
+            false,
+        )?;
+        Ok(item)
+    }
+
+    /// Read item of data into provided buffer
+    pub fn read_into<T: Sized>(self, data: &mut T) -> VmmDataResult<()> {
+        self.do_read_single(
+            data as *mut T as *mut libc::c_void,
+            size_of::<T>() as u32,
+            false,
+        )
+    }
+
+    /// Read item of data, specified by identifier existing in provided buffer
+    pub fn read_item<T: Sized>(self, data: &mut T) -> VmmDataResult<()> {
+        self.do_read_single(
+            data as *mut T as *mut libc::c_void,
+            size_of::<T>() as u32,
+            true,
+        )
+    }
+
+    fn do_read_single(
+        self,
+        data: *mut libc::c_void,
+        read_len: u32,
+        do_copyin: bool,
+    ) -> VmmDataResult<()> {
+        let mut xfer = self.xfer_base(read_len, data);
+
+        if do_copyin {
+            xfer.vdx_flags |= VDX_FLAG_READ_COPYIN;
+        }
+
+        let bytes_read = self.do_read(&mut xfer)?;
+        assert_eq!(bytes_read, read_len);
+        Ok(())
+    }
+
+    /// Read data items, specified by identifiers existing in provided buffer
+    pub fn read_many<T: Sized>(self, data: &mut [T]) -> VmmDataResult<()> {
+        let read_len = size_of_val(data) as u32;
+        let mut xfer =
+            self.xfer_base(read_len, data.as_mut_ptr() as *mut libc::c_void);
+
+        // When reading multiple items, it is expected that identifiers will be
+        // passed into the kernel to select the entries which will be read (as
+        // opposed to read-all, which is indiscriminate).
+        //
+        // As such, a copyin-before-read is implied to provide said identifiers.
+        xfer.vdx_flags |= VDX_FLAG_READ_COPYIN;
+
+        let bytes_read = self.do_read(&mut xfer)?;
+        assert_eq!(bytes_read, read_len);
+        Ok(())
+    }
+
+    /// Read all data items offered by this class/version
+    pub fn read_all<T: Sized>(self) -> VmmDataResult<Vec<T>> {
+        let mut xfer = self.xfer_base(0, std::ptr::null_mut());
+        let total_len = match self.do_read(&mut xfer) {
+            Err(VmmDataError::SpaceNeeded(sz)) => Ok(sz),
+            Err(e) => Err(e),
+            Ok(_) => panic!("unexpected success"),
+        }?;
+        let item_len = size_of::<T>() as u32;
+        assert!(total_len >= item_len, "item size exceeds total data size");
+
+        let item_count = total_len / item_len;
+        assert_eq!(
+            total_len,
+            item_count * item_len,
+            "per-item sizing does not match total data size"
+        );
+
+        let mut data: Vec<T> = Vec::with_capacity(item_count as usize);
+        let mut xfer =
+            self.xfer_base(total_len, data.as_mut_ptr() as *mut libc::c_void);
+
+        let bytes_read = self.do_read(&mut xfer)?;
+        assert!(bytes_read <= total_len);
+
+        // SAFETY: Data is populated by the ioctl
+        unsafe {
+            data.set_len((bytes_read / item_len) as usize);
+        }
+        Ok(data)
+    }
+
+    /// Write item of data
+    pub fn write<T: Sized>(self, data: &T) -> VmmDataResult<()> {
+        let write_len = size_of::<T>() as u32;
+        let mut xfer = self.xfer_base(
+            write_len,
+            data as *const T as *mut T as *mut libc::c_void,
+        );
+
+        let bytes_written = self.do_write(&mut xfer)?;
+        assert_eq!(bytes_written, write_len);
+        Ok(())
+    }
+
+    /// Write data items
+    pub fn write_many<T: Sized>(self, data: &[T]) -> VmmDataResult<()> {
+        let write_len = size_of_val(data) as u32;
+        let mut xfer = self
+            .xfer_base(write_len, data.as_ptr() as *mut T as *mut libc::c_void);
+
+        let bytes_written = self.do_write(&mut xfer)?;
+        assert_eq!(bytes_written, write_len);
+        Ok(())
+    }
+
+    /// Build a [`vm_data_xfer`] struct based on parameters established for this
+    /// data operation.
+    fn xfer_base(&self, len: u32, data: *mut libc::c_void) -> vm_data_xfer {
+        vm_data_xfer {
+            vdx_vcpuid: self.vcpuid.unwrap_or(-1),
+            vdx_class: self.class,
+            vdx_version: self.version,
+            vdx_len: len,
+            vdx_data: data,
+            ..Default::default()
+        }
+    }
+
+    fn do_read(
+        &self,
+        xfer: &mut vm_data_xfer,
+    ) -> std::result::Result<u32, VmmDataError> {
+        self.do_ioctl(VM_DATA_READ, xfer)
+    }
+
+    fn do_write(
+        &self,
+        xfer: &mut vm_data_xfer,
+    ) -> std::result::Result<u32, VmmDataError> {
+        // If logic is added to VM_DATA_WRITE which actually makes use of
+        // [`VDX_FLAG_WRITE_COPYOUT`], then the fact that [`write`] and
+        // [`write_many`] accept const references for the data input will need
+        // to be revisited.
+        self.do_ioctl(VM_DATA_WRITE, xfer)
+    }
+
+    /// Execute a vmm-data transfer, translating the ENOSPC error, if emitted
+    fn do_ioctl(
+        &self,
+        op: i32,
+        xfer: &mut vm_data_xfer,
+    ) -> std::result::Result<u32, VmmDataError> {
+        match unsafe { self.fd.ioctl(op, xfer) } {
+            Err(e) => match e.raw_os_error() {
+                Some(errno) if errno == libc::ENOSPC => {
+                    Err(VmmDataError::SpaceNeeded(xfer.vdx_result_len))
+                }
+                _ => Err(VmmDataError::IoError(e)),
+            },
+            Ok(_) => Ok(xfer.vdx_result_len),
+        }
+    }
+}
+
+#[derive(Debug)]
+pub enum VmmDataError {
+    IoError(Error),
+    SpaceNeeded(u32),
+}
+
+impl From<VmmDataError> for Error {
+    fn from(err: VmmDataError) -> Self {
+        match err {
+            VmmDataError::IoError(e) => e,
+            VmmDataError::SpaceNeeded(c) => {
+                // ErrorKind::StorageFull would more accurately match the underlying ENOSPC
+                // but that variant is unstable still
+                Error::new(
+                    ErrorKind::Other,
+                    format!("operation requires {} bytes", c),
+                )
+            }
+        }
+    }
+}
+
 /// Store a cached copy of the queried API version.  Negative values indicate an
 /// error occurred during query (and hold the corresponding negated `errno`).
 /// A positive value indicates the cached version, and should be less than

lib/propolis/src/hw/bhyve/atpic.rs

@@ -61,60 +61,61 @@ pub mod migrate {
         pub elc: u8,
         pub level: [u32; 8],
     }
-    impl AtPicV1 {
-        fn from_raw(inp: &bhyve_api::vdi_atpic_v1) -> Self {
-            Self {
-                chips: [
-                    AtPicChipV1::from_raw(&inp.va_chip[0]),
-                    AtPicChipV1::from_raw(&inp.va_chip[1]),
-                ],
-            }
+    impl From<bhyve_api::vdi_atpic_v1> for AtPicV1 {
+        fn from(value: bhyve_api::vdi_atpic_v1) -> Self {
+            Self { chips: [value.va_chip[0].into(), value.va_chip[1].into()] }
         }
-        fn to_raw(&self) -> bhyve_api::vdi_atpic_v1 {
-            bhyve_api::vdi_atpic_v1 {
-                va_chip: [self.chips[0].to_raw(), self.chips[1].to_raw()],
-            }
+    }
+    impl From<AtPicV1> for bhyve_api::vdi_atpic_v1 {
+        fn from(value: AtPicV1) -> Self {
+            Self { va_chip: [value.chips[0].into(), value.chips[1].into()] }
         }
     }
-    impl AtPicChipV1 {
-        fn from_raw(inp: &bhyve_api::vdi_atpic_chip_v1) -> Self {
+
+    impl From<bhyve_api::vdi_atpic_chip_v1> for AtPicChipV1 {
+        fn from(value: bhyve_api::vdi_atpic_chip_v1) -> Self {
             Self {
-                icw_state: inp.vac_icw_state,
-                status: inp.vac_status,
-                reg_irr: inp.vac_reg_irr,
-                reg_isr: inp.vac_reg_isr,
-                reg_imr: inp.vac_reg_imr,
-                irq_base: inp.vac_irq_base,
-                lowprio: inp.vac_lowprio,
-                elc: inp.vac_elc,
-                level: inp.vac_level,
+                icw_state: value.vac_icw_state,
+                status: value.vac_status,
+                reg_irr: value.vac_reg_irr,
+                reg_isr: value.vac_reg_isr,
+                reg_imr: value.vac_reg_imr,
+                irq_base: value.vac_irq_base,
+                lowprio: value.vac_lowprio,
+                elc: value.vac_elc,
+                level: value.vac_level,
             }
         }
-        fn to_raw(&self) -> bhyve_api::vdi_atpic_chip_v1 {
-            bhyve_api::vdi_atpic_chip_v1 {
-                vac_icw_state: self.icw_state,
-                vac_status: self.status,
-                vac_reg_irr: self.reg_irr,
-                vac_reg_isr: self.reg_isr,
-                vac_reg_imr: self.reg_imr,
-                vac_irq_base: self.irq_base,
-                vac_lowprio: self.lowprio,
-                vac_elc: self.elc,
-                vac_level: self.level,
+    }
+    impl From<AtPicChipV1> for bhyve_api::vdi_atpic_chip_v1 {
+        fn from(value: AtPicChipV1) -> Self {
+            Self {
+                vac_icw_state: value.icw_state,
+                vac_status: value.status,
+                vac_reg_irr: value.reg_irr,
+                vac_reg_isr: value.reg_isr,
+                vac_reg_imr: value.reg_imr,
+                vac_irq_base: value.irq_base,
+                vac_lowprio: value.lowprio,
+                vac_elc: value.elc,
+                vac_level: value.level,
             }
         }
     }
 
     impl AtPicV1 {
         pub(super) fn read(hdl: &vmm::VmmHdl) -> std::io::Result<Self> {
-            let vdi: bhyve_api::vdi_atpic_v1 =
-                vmm::data::read(hdl, -1, bhyve_api::VDC_ATPIC, 1)?;
+            let vdi = hdl
+                .data_op(bhyve_api::VDC_ATPIC, 1)
+                .read::<bhyve_api::vdi_atpic_v1>()?;
 
-            Ok(Self::from_raw(&vdi))
+            Ok(vdi.into())
         }
 
         pub(super) fn write(self, hdl: &vmm::VmmHdl) -> std::io::Result<()> {
-            vmm::data::write(hdl, -1, bhyve_api::VDC_ATPIC, 1, self.to_raw())?;
+            hdl.data_op(bhyve_api::VDC_ATPIC, 1)
+                .write::<bhyve_api::vdi_atpic_v1>(&self.into())?;
+
             Ok(())
         }
     }