Improved error typing

Signed-off-by: Jonathan Woollett-Light <[email protected]

src/arch/Cargo.toml

@@ -14,7 +14,9 @@ versionize = ">=0.1.6"
 versionize_derive = ">=0.1.3"
 vm-fdt = "0.1.0"
 derive_more = { version = "0.99.17", default-features = false, features = ["from"] }
+thiserror = "1.0.32"
 bitflags = ">=1.0.4"
+vmm-sys-util = ">=0.8.0"
 
 arch_gen = { path = "../arch_gen" }
 logger = { path = "../logger" }

src/arch/src/x86_64/interrupts.rs

@@ -9,11 +9,13 @@ use kvm_bindings::kvm_lapic_state;
 use kvm_ioctls::VcpuFd;
 use utils::byte_order;
 /// Errors thrown while configuring the LAPIC.
-#[derive(Debug)]
+#[derive(Debug, thiserror::Error)]
 pub enum Error {
     /// Failure in retrieving the LAPIC configuration.
+    #[error("Failure in retrieving the LAPIC configuration: {0}")]
     GetLapic(kvm_ioctls::Error),
     /// Failure in modifying the LAPIC configuration.
+    #[error("Failure in modifying the LAPIC configuration: {0}")]
     SetLapic(kvm_ioctls::Error),
 }
 type Result<T> = std::result::Result<T, Error>;

src/arch/src/x86_64/msr.rs

@@ -282,18 +282,42 @@ pub fn create_boot_msr_entries() -> Vec<kvm_msr_entry> {
     ]
 }
 
+/// Error type for [`set_msrs`].
+#[derive(Debug, thiserror::Error)]
+pub enum SetMSRsError {
+    /// Could not create [`vmm_sys_util::fam::FamStructWrapper`] for MSRs.
+    #[error("Could not create `vmm_sys_util::fam::FamStructWrapper` for MSRs")]
+    CreateMSRs(utils::fam::Error),
+    /// Setting MSRs resulted in an error.
+    #[error("Setting MSRs resulted in an error: {0}")]
+    SetModelSpecificRegisters(#[from] kvm_ioctls::Error),
+    /// Not all given MSRs where set.
+    #[error("Not all given MSRs where set.")]
+    SetModelSpecificRegistersCount,
+}
+
 /// Configure Model Specific Registers (MSRs) required to boot Linux for a given x86_64 vCPU.
 ///
 /// # Arguments
 ///
 /// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
-pub fn set_msrs(vcpu: &VcpuFd, msr_entries: &[kvm_msr_entry]) -> Result<()> {
-    let msrs = Msrs::from_entries(&msr_entries).map_err(Error::FamError)?;
+///
+/// # Errors
+///
+/// When:
+/// - Failed when trying to create [`vmm_sys_util::fam::FamStructWrapper`] for MSRs.
+/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_msrs`] errors.
+/// - When [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_msrs`] failed to write all MSRs entries.
+pub fn set_msrs(
+    vcpu: &VcpuFd,
+    msr_entries: &[kvm_msr_entry],
+) -> std::result::Result<(), SetMSRsError> {
+    let msrs = Msrs::from_entries(&msr_entries).map_err(SetMSRsError::CreateMSRs)?;
     vcpu.set_msrs(&msrs)
-        .map_err(Error::SetModelSpecificRegisters)
+        .map_err(SetMSRsError::SetModelSpecificRegisters)
         .and_then(|msrs_written| {
             if msrs_written as u32 != msrs.as_fam_struct_ref().nmsrs {
-                Err(Error::SetModelSpecificRegistersCount)
+                Err(SetMSRsError::SetModelSpecificRegistersCount)
             } else {
                 Ok(())
             }

src/arch/src/x86_64/regs.rs

@@ -5,7 +5,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the THIRD-PARTY file.
 
-use std::mem;
+use std::{fmt, mem};
 
 use kvm_bindings::{kvm_fpu, kvm_regs, kvm_sregs};
 use kvm_ioctls::VcpuFd;
@@ -19,42 +19,76 @@ const PDPTE_START: u64 = 0xa000;
 const PDE_START: u64 = 0xb000;
 
 /// Errors thrown while setting up x86_64 registers.
-#[derive(Debug)]
+#[derive(Debug, thiserror::Error)]
 pub enum Error {
     /// Failed to get SREGs for this CPU.
+    #[error("Failed to get SREGs for this CPU: {0}")]
     GetStatusRegisters(kvm_ioctls::Error),
     /// Failed to set base registers for this CPU.
+    #[error("Failed to set base registers for this CPU: {0}")]
     SetBaseRegisters(kvm_ioctls::Error),
     /// Failed to configure the FPU.
+    #[error("Failed to configure the FPU: {0}")]
     SetFPURegisters(kvm_ioctls::Error),
     /// Failed to set SREGs for this CPU.
+    #[error("Failed to set SREGs for this CPU: {0}")]
     SetStatusRegisters(kvm_ioctls::Error),
     /// Writing the GDT to RAM failed.
+    #[error("Writing the GDT to RAM failed.")]
     WriteGDT,
     /// Writing the IDT to RAM failed.
+    #[error("Writing the IDT to RAM failed")]
     WriteIDT,
     /// Writing PDPTE to RAM failed.
+    #[error("WritePDPTEAddress")]
     WritePDPTEAddress,
     /// Writing PDE to RAM failed.
+    #[error("WritePDEAddress")]
     WritePDEAddress,
     /// Writing PML4 to RAM failed.
+    #[error("WritePML4Address")]
     WritePML4Address,
 }
 type Result<T> = std::result::Result<T, Error>;
 
+/// Error types for [`setup_fpu`].
+#[derive(Debug, derive_more::From)]
+pub struct SetupFpuError(vmm_sys_util::errno::Error);
+impl std::error::Error for SetupFpuError {}
+impl fmt::Display for SetupFpuError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "Failed to setup FPU: {}", self.0)
+    }
+}
+
 /// Configure Floating-Point Unit (FPU) registers for a given CPU.
 ///
 /// # Arguments
 ///
 /// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
-pub fn setup_fpu(vcpu: &VcpuFd) -> Result<()> {
+///
+/// # Errors
+///
+/// When [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_fpu`] errors.
+pub fn setup_fpu(vcpu: &VcpuFd) -> std::result::Result<(), SetupFpuError> {
     let fpu: kvm_fpu = kvm_fpu {
         fcw: 0x37f,
         mxcsr: 0x1f80,
         ..Default::default()
     };
 
-    vcpu.set_fpu(&fpu).map_err(Error::SetFPURegisters)
+    let res = vcpu.set_fpu(&fpu)?;
+    Ok(res)
+}
+
+/// Error type of [`setup_regs`].
+#[derive(Debug, derive_more::From)]
+pub struct SetupRegistersError(vmm_sys_util::errno::Error);
+impl std::error::Error for SetupRegistersError {}
+impl fmt::Display for SetupRegistersError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "Failed to setup registers:{}", self.0)
+    }
 }
 
 /// Configure base registers for a given CPU.
@@ -63,7 +97,11 @@ pub fn setup_fpu(vcpu: &VcpuFd) -> Result<()> {
 ///
 /// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
 /// * `boot_ip` - Starting instruction pointer.
-pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> Result<()> {
+///
+/// # Errors
+///
+/// When [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_regs`] errors.
+pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> std::result::Result<(), SetupRegistersError> {
     let regs: kvm_regs = kvm_regs {
         rflags: 0x0000_0000_0000_0002u64,
         rip: boot_ip,
@@ -79,7 +117,25 @@ pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> Result<()> {
         ..Default::default()
     };
 
-    vcpu.set_regs(&regs).map_err(Error::SetBaseRegisters)
+    let res = vcpu.set_regs(&regs)?;
+    Ok(res)
+}
+
+/// Error type for [`setup_sregs`].
+#[derive(Debug, thiserror::Error)]
+pub enum SetupSegmentRegistersError {
+    /// Failed to get segment registers
+    #[error("Failed to get segment registers: {0}")]
+    GetSegmentRegisters(#[from] vmm_sys_util::errno::Error),
+    /// Failed to configure segments and segment registers
+    #[error("Failed to configure segments and segment registers: {0}")]
+    ConfigureSegmentsAndSegmentRegisters(Error),
+    /// Failed to setup page tables
+    #[error("Failed to setup page tables: {0}")]
+    SetupPageTables(Error),
+    /// Failed to set segment registers
+    #[error("Failed to set segment registers: {0}")]
+    SetSegmentRegisters(vmm_sys_util::errno::Error),
 }
 
 /// Configures the segment registers and system page tables for a given CPU.
@@ -88,13 +144,28 @@ pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> Result<()> {
 ///
 /// * `mem` - The memory that will be passed to the guest.
 /// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
-pub fn setup_sregs(mem: &GuestMemoryMmap, vcpu: &VcpuFd) -> Result<()> {
-    let mut sregs: kvm_sregs = vcpu.get_sregs().map_err(Error::GetStatusRegisters)?;
-
-    configure_segments_and_sregs(mem, &mut sregs)?;
-    setup_page_tables(mem, &mut sregs)?; // TODO(dgreid) - Can this be done once per system instead?
-
-    vcpu.set_sregs(&sregs).map_err(Error::SetStatusRegisters)
+///
+/// # Errors
+///
+/// When:
+/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::get_sregs`] errors.
+/// - [`configure_segments_and_sregs`] errors.
+/// - [`setup_page_tables`] errors
+/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_sregs`] errors.
+pub fn setup_sregs(
+    mem: &GuestMemoryMmap,
+    vcpu: &VcpuFd,
+) -> std::result::Result<(), SetupSegmentRegistersError> {
+    let mut sregs: kvm_sregs = vcpu
+        .get_sregs()
+        .map_err(SetupSegmentRegistersError::GetSegmentRegisters)?;
+
+    configure_segments_and_sregs(mem, &mut sregs)
+        .map_err(SetupSegmentRegistersError::ConfigureSegmentsAndSegmentRegisters)?;
+    setup_page_tables(mem, &mut sregs).map_err(SetupSegmentRegistersError::SetupPageTables)?; // TODO(dgreid) - Can this be done once per system instead?
+
+    vcpu.set_sregs(&sregs)
+        .map_err(SetupSegmentRegistersError::SetSegmentRegisters)
 }
 
 const BOOT_GDT_OFFSET: u64 = 0x500;

src/bit-fields-macros/src/lib.rs

@@ -446,8 +446,8 @@ pub fn bitfield(item: proc_macro::TokenStream) -> proc_macro::TokenStream {
                             #[cfg(feature = "field_map")]
                             write!(
                                 &mut field_setting_hashmap,
-                                "map.insert(String::from(\"{field_ident}\"),{struct_data_type}::\
-                                 from(&bit_field.{field_ident}));",
+                                "map.insert(String::from(\"{field_ident}\"),\
+                                 {struct_data_type}::from(&bit_field.{field_ident}));",
                                 struct_data_type = struct_data_type,
                                 field_ident = field_ident
                             )

src/cpuid/Cargo.toml

@@ -9,10 +9,12 @@ license = "Apache-2.0"
 kvm-bindings = { version = ">=0.5.0", features = ["fam-wrappers"] }
 kvm-ioctls = ">=0.9.0"
 derive_more = { version = "0.99.17", default-features = false, features = ["from"] }
+thiserror = "1.0.32"
 
 utils = { path = "../utils"}
 arch = { path = "../arch" }
 
+vmm-sys-util = ">=0.8.0"
 bit-fields = { path = "../bit-fields", features = ["serde_feature"] }
 phf = { version = "0.10", features = ["macros"] }
 serde = { version="1.0.138", features=["derive"] }

src/cpuid/benches/benchmark.rs

@@ -1,6 +1,7 @@
-use criterion::{black_box, criterion_group, criterion_main, Criterion,BenchmarkId};
-use cpuid::{RawCpuid,Cpuid,AmdCpuid,IntelCpuid};
-use std::convert::{From,TryFrom};
+use std::convert::{From, TryFrom};
+
+use cpuid::{AmdCpuid, Cpuid, IntelCpuid, RawCpuid};
+use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion};
 
 pub fn conversions(c: &mut Criterion) {
     let kvm = kvm_ioctls::Kvm::new().unwrap();
@@ -20,14 +21,22 @@ pub fn conversions(c: &mut Criterion) {
     c.bench_function("cpuid.clone()", |b| b.iter(|| cpuid.clone()));
 
     c.bench_function("kvm->raw", |b| b.iter(|| RawCpuid::from(kvm_cpuid.clone())));
-    c.bench_function("raw->kvm", |b| b.iter(|| kvm_bindings::CpuId::from(raw_cpuid.clone())));
-    c.bench_function("raw->cpuid", |b| b.iter(|| Cpuid::try_from(raw_cpuid.clone())));
+    c.bench_function("raw->kvm", |b| {
+        b.iter(|| kvm_bindings::CpuId::from(raw_cpuid.clone()))
+    });
+    c.bench_function("raw->cpuid", |b| {
+        b.iter(|| Cpuid::try_from(raw_cpuid.clone()))
+    });
     c.bench_function("cpuid->raw", |b| b.iter(|| RawCpuid::from(cpuid.clone())));
     c.bench_function("amd->raw", |b| b.iter(|| RawCpuid::from(amd_cpuid.clone())));
     c.bench_function("raw->amd", |b| b.iter(|| AmdCpuid::from(raw_cpuid.clone())));
-    c.bench_function("intel->raw", |b| b.iter(|| RawCpuid::from(intel_cpuid.clone())));
-    c.bench_function("raw->intel", |b| b.iter(|| IntelCpuid::from(raw_cpuid.clone())));
+    c.bench_function("intel->raw", |b| {
+        b.iter(|| RawCpuid::from(intel_cpuid.clone()))
+    });
+    c.bench_function("raw->intel", |b| {
+        b.iter(|| IntelCpuid::from(raw_cpuid.clone()))
+    });
 }
 
 criterion_group!(benches, conversions);
-criterion_main!(benches);
+criterion_main!(benches);

src/cpuid/src/brand_string.rs

@@ -139,6 +139,7 @@ impl BrandString {
     /// Returns the given register value for the given CPUID leaf.
     ///
     /// `leaf` must be between 0x80000002 and 0x80000004.
+    #[cfg(test)]
     #[inline]
     pub fn get_reg_for_leaf(&self, leaf: u32, reg: Reg) -> u32 {
         // It's ok not to validate parameters here, leaf and reg should