fix: update to cpu id validation during snapshot restoration

Previously `validate_cpu_vendor` and `validate_cpu_manufacturer_id`
checks were giving errors only when they could not obtain needed
ids from host or guest. The equality checks were only used
to print `info` or `error` logs.
Now both methods only print logs and can not error out.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Author: ShadowCurse

CHANGELOG.md

@@ -9,6 +9,9 @@
 - Updated deserialization of `bitmap` for custom CPU templates to allow usage
   of '_' as a separator.
 - Changed the strip feature of `cpu-template-helper` tool to operate bitwise.
+- Better logs during validation of CPU ID in snapshot restoration path. Also
+  Firecracker now does not fail if it can't get CPU ID from the host or
+  can't find CPU ID in the snapshot.
 
 ### Fixed
 

src/vmm/src/persist.rs

@@ -356,18 +356,6 @@ pub fn get_snapshot_data_version(
     Ok(data_version)
 }
 
-/// Error type for [`validate_cpu_vendor`].
-#[cfg(target_arch = "x86_64")]
-#[derive(Debug, thiserror::Error, PartialEq, Eq)]
-pub enum ValidateCpuVendorError {
-    /// Failed to read host vendor.
-    #[error("Failed to read host vendor: {0}")]
-    Host(#[from] crate::cpu_config::x86_64::cpuid::common::GetCpuidError),
-    /// Failed to read snapshot vendor.
-    #[error("Failed to read snapshot vendor")]
-    Snapshot,
-}
-
 /// Validates that snapshot CPU vendor matches the host CPU vendor.
 ///
 /// # Errors
@@ -376,38 +364,32 @@ pub enum ValidateCpuVendorError {
 /// - Failed to read host vendor.
 /// - Failed to read snapshot vendor.
 #[cfg(target_arch = "x86_64")]
-pub fn validate_cpu_vendor(microvm_state: &MicrovmState) -> Result<bool, ValidateCpuVendorError> {
-    let host_vendor_id = get_vendor_id_from_host()?;
-
-    let snapshot_vendor_id = microvm_state.vcpu_states[0]
-        .cpuid
-        .vendor_id()
-        .ok_or(ValidateCpuVendorError::Snapshot)?;
-
-    if host_vendor_id == snapshot_vendor_id {
-        info!("Snapshot CPU vendor id: {:?}", &snapshot_vendor_id);
-        Ok(true)
-    } else {
-        error!(
-            "Host CPU vendor id: {:?} differs from the snapshotted one: {:?}",
-            &host_vendor_id, &snapshot_vendor_id
-        );
-        Ok(false)
+pub fn validate_cpu_vendor(microvm_state: &MicrovmState) {
+    let host_vendor_id = get_vendor_id_from_host();
+    let snapshot_vendor_id = microvm_state.vcpu_states[0].cpuid.vendor_id();
+    match (host_vendor_id, snapshot_vendor_id) {
+        (Ok(host_id), Some(snapshot_id)) => {
+            info!("Host CPU vendor ID: {host_id:?}");
+            info!("Snapshot CPU vendor ID: {snapshot_id:?}");
+            if host_id != snapshot_id {
+                warn!("Host CPU vendor ID differs from the snapshotted one",);
+            }
+        }
+        (Ok(host_id), None) => {
+            info!("Host CPU vendor ID: {host_id:?}");
+            warn!("Snapshot CPU vendor ID: couldn't get from the snapshot");
+        }
+        (Err(_), Some(snapshot_id)) => {
+            warn!("Host CPU vendor ID: couldn't get from the host");
+            info!("Snapshot CPU vendor ID: {snapshot_id:?}");
+        }
+        (Err(_), None) => {
+            warn!("Host CPU vendor ID: couldn't get from the host");
+            warn!("Snapshot CPU vendor ID: couldn't get from the snapshot");
+        }
     }
 }
 
-/// Error type for [`validate_cpu_manufacturer_id`].
-#[cfg(target_arch = "aarch64")]
-#[derive(Debug, thiserror::Error, PartialEq, Eq)]
-pub enum ValidateCpuManufacturerIdError {
-    /// Failed to read host vendor.
-    #[error("Failed to get manufacturer ID from host: {0}")]
-    Host(String),
-    /// Failed to read host vendor.
-    #[error("Failed to get manufacturer ID from state: {0}")]
-    Snapshot(String),
-}
-
 /// Validate that Snapshot Manufacturer ID matches
 /// the one from the Host
 ///
@@ -418,27 +400,30 @@ pub enum ValidateCpuManufacturerIdError {
 /// - Failed to read host vendor.
 /// - Failed to read snapshot vendor.
 #[cfg(target_arch = "aarch64")]
-pub fn validate_cpu_manufacturer_id(
-    microvm_state: &MicrovmState,
-) -> Result<bool, ValidateCpuManufacturerIdError> {
-    let host_man_id = get_manufacturer_id_from_host()
-        .map_err(|err| ValidateCpuManufacturerIdError::Host(err.to_string()))?;
-
-    for state in &microvm_state.vcpu_states {
-        let state_man_id = get_manufacturer_id_from_state(&state.regs)
-            .map_err(|err| ValidateCpuManufacturerIdError::Snapshot(err.to_string()))?;
-
-        if host_man_id != state_man_id {
-            error!(
-                "Host CPU manufacturer ID: {} differs from snapshotted one: {}",
-                &host_man_id, &state_man_id
-            );
-            return Ok(false);
-        } else {
-            info!("Snapshot CPU manufacturer ID: {:?}", &state_man_id);
+pub fn validate_cpu_manufacturer_id(microvm_state: &MicrovmState) {
+    let host_cpu_id = get_manufacturer_id_from_host();
+    let snapshot_cpu_id = get_manufacturer_id_from_state(&microvm_state.vcpu_states[0].regs);
+    match (host_cpu_id, snapshot_cpu_id) {
+        (Ok(host_id), Ok(snapshot_id)) => {
+            info!("Host CPU manufacturer ID: {host_id:?}");
+            info!("Snapshot CPU manufacturer ID: {snapshot_id:?}");
+            if host_id != snapshot_id {
+                warn!("Host CPU manufacturer ID differs from the snapshotted one",);
+            }
+        }
+        (Ok(host_id), Err(_)) => {
+            info!("Host CPU manufacturer ID: {host_id:?}");
+            warn!("Snapshot CPU manufacturer ID: couldn't get from the snapshot");
+        }
+        (Err(_), Ok(snapshot_id)) => {
+            warn!("Host CPU manufacturer ID: couldn't get from the host");
+            info!("Snapshot CPU manufacturer ID: {snapshot_id:?}");
+        }
+        (Err(_), Err(_)) => {
+            warn!("Host CPU manufacturer ID: couldn't get from the host");
+            warn!("Snapshot CPU manufacturer ID: couldn't get from the snapshot");
         }
     }
-    Ok(true)
 }
 /// Error type for [`snapshot_state_sanity_check`].
 #[derive(Debug, thiserror::Error, PartialEq, Eq)]
@@ -449,14 +434,6 @@ pub enum SnapShotStateSanityCheckError {
     /// No memory region defined.
     #[error("No memory region defined.")]
     NoMemory,
-    /// Failed to validate vCPU vendor.
-    #[cfg(target_arch = "x86_64")]
-    #[error("Failed to validate vCPU vendor: {0}")]
-    ValidateCpuVendor(#[from] ValidateCpuVendorError),
-    /// Failed to validate vCPU manufacturer id.
-    #[error("Failed to validate vCPU manufacturer id: {0}")]
-    #[cfg(target_arch = "aarch64")]
-    ValidateCpuManufacturerId(#[from] ValidateCpuManufacturerIdError),
 }
 
 /// Performs sanity checks against the state file and returns specific errors.
@@ -478,9 +455,9 @@ pub fn snapshot_state_sanity_check(
     }
 
     #[cfg(target_arch = "x86_64")]
-    validate_cpu_vendor(microvm_state)?;
+    validate_cpu_vendor(microvm_state);
     #[cfg(target_arch = "aarch64")]
-    validate_cpu_manufacturer_id(microvm_state)?;
+    validate_cpu_manufacturer_id(microvm_state);
 
     Ok(())
 }

src/vmm/tests/integration_tests.rs

@@ -354,142 +354,3 @@ fn get_microvm_state_from_snapshot() -> MicrovmState {
     )
     .unwrap()
 }
-
-#[cfg(target_arch = "x86_64")]
-#[test]
-fn test_snapshot_cpu_vendor() {
-    use vmm::persist::validate_cpu_vendor;
-    let microvm_state = get_microvm_state_from_snapshot();
-
-    // Check if the snapshot created above passes validation since
-    // the snapshot was created locally.
-    assert!(validate_cpu_vendor(&microvm_state).is_ok());
-}
-
-#[cfg(target_arch = "x86_64")]
-#[test]
-fn test_snapshot_cpu_vendor_mismatch() {
-    use vmm::persist::validate_cpu_vendor;
-    let mut microvm_state = get_microvm_state_from_snapshot();
-
-    // Check if the snapshot created above passes validation since
-    // the snapshot was created locally.
-    assert_eq!(validate_cpu_vendor(&microvm_state), Ok(true));
-
-    // Modify the vendor id in CPUID.
-    for entry in microvm_state.vcpu_states[0].cpuid.as_mut_slice().iter_mut() {
-        if entry.function == 0 && entry.index == 0 {
-            // Fail if vendor id is NULL as this needs furhter investigation.
-            assert_ne!(entry.ebx, 0);
-            assert_ne!(entry.ecx, 0);
-            assert_ne!(entry.edx, 0);
-            entry.ebx = 0;
-            break;
-        }
-    }
-
-    // It succeeds in checking if the CPU vendor is valid, in this process it discovers the CPU
-    // vendor not valid.
-    assert_eq!(validate_cpu_vendor(&microvm_state), Ok(false));
-
-    // Negative test: remove the vendor id from cpuid.
-    for entry in microvm_state.vcpu_states[0].cpuid.as_mut_slice().iter_mut() {
-        if entry.function == 0 && entry.index == 0 {
-            entry.function = 1234;
-        }
-    }
-
-    // It succeeds in checking if the CPU vendor is valid, in this process it discovers the CPU
-    // vendor not valid.
-    assert_eq!(
-        validate_cpu_vendor(&microvm_state),
-        Err(vmm::persist::ValidateCpuVendorError::Snapshot)
-    );
-}
-
-#[cfg(target_arch = "x86_64")]
-#[test]
-fn test_snapshot_cpu_vendor_missing() {
-    use vmm::persist::validate_cpu_vendor;
-    let mut microvm_state = get_microvm_state_from_snapshot();
-
-    // Check if the snapshot created above passes validation since
-    // the snapshot was created locally.
-    assert!(validate_cpu_vendor(&microvm_state).is_ok());
-
-    // Negative test: remove the vendor id from cpuid.
-    for entry in microvm_state.vcpu_states[0].cpuid.as_mut_slice().iter_mut() {
-        if entry.function == 0 && entry.index == 0 {
-            entry.function = 1234;
-        }
-    }
-
-    // This must fail as the cpu vendor entry does not exist.
-    assert!(validate_cpu_vendor(&microvm_state).is_err());
-}
-
-#[cfg(target_arch = "aarch64")]
-#[test]
-fn test_snapshot_cpu_vendor() {
-    use vmm::persist::validate_cpu_manufacturer_id;
-
-    let microvm_state = get_microvm_state_from_snapshot();
-
-    // Check if the snapshot created above passes validation since
-    // the snapshot was created locally.
-    assert!(validate_cpu_manufacturer_id(&microvm_state).is_ok());
-}
-
-#[cfg(target_arch = "aarch64")]
-#[test]
-fn test_snapshot_cpu_vendor_missing() {
-    use vmm::arch::aarch64::regs::{Aarch64RegisterVec, MIDR_EL1};
-    use vmm::persist::{validate_cpu_manufacturer_id, ValidateCpuManufacturerIdError};
-
-    let mut microvm_state = get_microvm_state_from_snapshot();
-
-    // Check if the snapshot created above passes validation since
-    // the snapshot was created locally.
-    assert_eq!(validate_cpu_manufacturer_id(&microvm_state), Ok(true));
-
-    // Manufacturer id is stored in the MIDR_EL1 register. For this test we
-    // remove it from the state.
-    for state in microvm_state.vcpu_states.iter_mut() {
-        let mut new_regs = Aarch64RegisterVec::default();
-        // Removing MIDR_EL1 register.
-        for reg in state.regs.iter() {
-            if reg.id != MIDR_EL1 {
-                new_regs.push(reg);
-            }
-        }
-        state.regs = new_regs;
-    }
-    assert!(matches!(
-        validate_cpu_manufacturer_id(&microvm_state),
-        Err(ValidateCpuManufacturerIdError::Snapshot(_))
-    ));
-}
-
-#[cfg(target_arch = "aarch64")]
-#[test]
-fn test_snapshot_cpu_vendor_mismatch() {
-    use vmm::arch::aarch64::regs::MIDR_EL1;
-    use vmm::persist::validate_cpu_manufacturer_id;
-
-    let mut microvm_state = get_microvm_state_from_snapshot();
-
-    // Check if the snapshot created above passes validation since
-    // the snapshot was created locally.
-    assert_eq!(validate_cpu_manufacturer_id(&microvm_state), Ok(true));
-
-    // Change the MIDR_EL1 value from the VCPU states, to contain an
-    // invalid manufacturer ID
-    for state in microvm_state.vcpu_states.as_mut_slice().iter_mut() {
-        for mut reg in state.regs.iter_mut() {
-            if reg.id == MIDR_EL1 {
-                reg.set_value::<u64, 8>(0x710FD081);
-            }
-        }
-    }
-    assert_eq!(validate_cpu_manufacturer_id(&microvm_state), Ok(false));
-}