refactor: Consolidate machine-config validation to fn update

Do all validation of the configuration provided to the `machine-config`
endpoint inside of the `update` method (e.g. right before it gets
applied), instead of splitting some of it into the Deserialization
routine.

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/firecracker/src/api_server/request/machine_configuration.rs

@@ -180,31 +180,24 @@ mod tests {
             parse_put_machine_config(&Body::new(body)).unwrap_err();
         }
 
-        // 5. Test that setting `smt: true` is successful on x86_64 while on aarch64, it is not.
+        // 5. Test that setting `smt: true` is successful
         let body = r#"{
             "vcpu_count": 8,
             "mem_size_mib": 1024,
             "smt": true,
             "track_dirty_pages": true
         }"#;
-        #[cfg(target_arch = "x86_64")]
-        {
-            let expected_config = MachineConfigUpdate {
-                vcpu_count: Some(8),
-                mem_size_mib: Some(1024),
-                smt: Some(true),
-                cpu_template: None,
-                track_dirty_pages: Some(true),
-            };
-            assert_eq!(
-                vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()),
-                VmmAction::UpdateVmConfiguration(expected_config)
-            );
-        }
-        #[cfg(target_arch = "aarch64")]
-        {
-            parse_put_machine_config(&Body::new(body)).unwrap_err();
-        }
+        let expected_config = MachineConfigUpdate {
+            vcpu_count: Some(8),
+            mem_size_mib: Some(1024),
+            smt: Some(true),
+            cpu_template: None,
+            track_dirty_pages: Some(true),
+        };
+        assert_eq!(
+            vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()),
+            VmmAction::UpdateVmConfiguration(expected_config)
+        );
     }
 
     #[test]

src/vmm/src/resources.rs

@@ -680,82 +680,6 @@ mod tests {
             _ => unreachable!(),
         }
 
-        // Invalid vCPU number.
-        json = format!(
-            r#"{{
-                    "boot-source": {{
-                        "kernel_image_path": "{}",
-                        "boot_args": "console=ttyS0 reboot=k panic=1 pci=off"
-                    }},
-                    "drives": [
-                        {{
-                            "drive_id": "rootfs",
-                            "path_on_host": "{}",
-                            "is_root_device": true,
-                            "is_read_only": false
-                        }}
-                    ],
-                    "machine-config": {{
-                        "vcpu_count": 0,
-                        "mem_size_mib": 1024
-                    }}
-            }}"#,
-            kernel_file.as_path().to_str().unwrap(),
-            rootfs_file.as_path().to_str().unwrap()
-        );
-
-        match VmResources::from_json(
-            json.as_str(),
-            &default_instance_info,
-            HTTP_MAX_PAYLOAD_SIZE,
-            None,
-        ) {
-            Err(ResourcesError::InvalidJson(_)) => (),
-            _ => unreachable!(),
-        }
-
-        // Valid config for x86 but invalid on aarch64 because smt is not available.
-        json = format!(
-            r#"{{
-                    "boot-source": {{
-                        "kernel_image_path": "{}",
-                        "boot_args": "console=ttyS0 reboot=k panic=1 pci=off"
-                    }},
-                    "drives": [
-                        {{
-                            "drive_id": "rootfs",
-                            "path_on_host": "{}",
-                            "is_root_device": true,
-                            "is_read_only": false
-                        }}
-                    ],
-                    "machine-config": {{
-                        "vcpu_count": 2,
-                        "mem_size_mib": 1024,
-                        "smt": true
-                    }}
-            }}"#,
-            kernel_file.as_path().to_str().unwrap(),
-            rootfs_file.as_path().to_str().unwrap()
-        );
-
-        #[cfg(target_arch = "x86_64")]
-        VmResources::from_json(
-            json.as_str(),
-            &default_instance_info,
-            HTTP_MAX_PAYLOAD_SIZE,
-            None,
-        )
-        .unwrap();
-        #[cfg(target_arch = "aarch64")]
-        VmResources::from_json(
-            json.as_str(),
-            &default_instance_info,
-            HTTP_MAX_PAYLOAD_SIZE,
-            None,
-        )
-        .unwrap_err();
-
         // Valid config for x86 but invalid on aarch64 since it uses cpu_template.
         json = format!(
             r#"{{
@@ -1330,7 +1254,7 @@ mod tests {
         let mut aux_vm_config = MachineConfigUpdate {
             vcpu_count: Some(32),
             mem_size_mib: Some(512),
-            smt: Some(true),
+            smt: Some(false),
             #[cfg(target_arch = "x86_64")]
             cpu_template: Some(StaticCpuTemplate::T2),
             #[cfg(target_arch = "aarch64")]
@@ -1359,7 +1283,25 @@ mod tests {
             vm_resources.update_vm_config(&aux_vm_config),
             Err(VmConfigError::InvalidVcpuCount)
         );
+
+        // Check that SMT is not supported on aarch64, and that on x86_64 enabling it requires vcpu
+        // count to be even.
+        aux_vm_config.smt = Some(true);
+        #[cfg(target_arch = "aarch64")]
+        assert_eq!(
+            vm_resources.update_vm_config(&aux_vm_config),
+            Err(VmConfigError::SmtNotSupported)
+        );
+        aux_vm_config.vcpu_count = Some(3);
+        #[cfg(target_arch = "x86_64")]
+        assert_eq!(
+            vm_resources.update_vm_config(&aux_vm_config),
+            Err(VmConfigError::InvalidVcpuCount)
+        );
         aux_vm_config.vcpu_count = Some(32);
+        #[cfg(target_arch = "x86_64")]
+        vm_resources.update_vm_config(&aux_vm_config).unwrap();
+        aux_vm_config.smt = Some(false);
 
         // Invalid mem_size_mib.
         aux_vm_config.mem_size_mib = Some(0);

src/vmm/src/vmm_config/machine_config.rs

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 use std::fmt::Debug;
 
-use serde::{de, Deserialize, Serialize};
+use serde::{Deserialize, Serialize};
 
 use crate::cpu_config::templates::{CpuTemplateType, CustomCpuTemplate, StaticCpuTemplate};
 
@@ -20,23 +20,25 @@ pub enum VmConfigError {
     IncompatibleBalloonSize,
     /// The memory size (MiB) is invalid.
     InvalidMemorySize,
-    /// The vCPU number is invalid! The vCPU number can only be 1 or an even number when SMT is enabled.
+    /// The number of vCPUs must be greater than 0, less than {MAX_SUPPORTED_VCPUS:} and must be 1 or an even number if SMT is enabled.
     InvalidVcpuCount,
     /// Could not get the configuration of the previously installed balloon device to validate the memory size.
     InvalidVmState,
+    /// Enabling simultaneous multithreading is not supported on aarch64.
+    #[cfg(target_arch = "aarch64")]
+    SmtNotSupported,
 }
 
 /// Struct used in PUT `/machine-config` API call.
 #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
 #[serde(deny_unknown_fields)]
 pub struct MachineConfig {
     /// Number of vcpu to start.
-    #[serde(deserialize_with = "deserialize_vcpu_num")]
     pub vcpu_count: u8,
     /// The memory size in MiB.
     pub mem_size_mib: usize,
     /// Enables or disabled SMT.
-    #[serde(default, deserialize_with = "deserialize_smt")]
+    #[serde(default)]
     pub smt: bool,
     /// A CPU template that it is used to filter the CPU features exposed to the guest.
     #[serde(default, skip_serializing_if = "Option::is_none")]
@@ -62,21 +64,13 @@ impl Default for MachineConfig {
 #[serde(deny_unknown_fields)]
 pub struct MachineConfigUpdate {
     /// Number of vcpu to start.
-    #[serde(
-        default,
-        skip_serializing_if = "Option::is_none",
-        deserialize_with = "deserialize_vcpu_num"
-    )]
+    #[serde(default, skip_serializing_if = "Option::is_none")]
     pub vcpu_count: Option<u8>,
     /// The memory size in MiB.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub mem_size_mib: Option<usize>,
     /// Enables or disabled SMT.
-    #[serde(
-        default,
-        skip_serializing_if = "Option::is_none",
-        deserialize_with = "deserialize_smt"
-    )]
+    #[serde(default, skip_serializing_if = "Option::is_none")]
     pub smt: Option<bool>,
     /// A CPU template that it is used to filter the CPU features exposed to the guest.
     #[serde(default, skip_serializing_if = "Option::is_none")]
@@ -146,7 +140,12 @@ impl VmConfig {
 
         let smt = update.smt.unwrap_or(self.smt);
 
-        if vcpu_count == 0 {
+        #[cfg(target_arch = "aarch64")]
+        if smt {
+            return Err(VmConfigError::SmtNotSupported);
+        }
+
+        if vcpu_count == 0 || vcpu_count > MAX_SUPPORTED_VCPUS {
             return Err(VmConfigError::InvalidVcpuCount);
         }
 
@@ -205,53 +204,3 @@ impl From<&VmConfig> for MachineConfig {
         }
     }
 }
-
-/// Deserialization function for the `vcpu_num` field in `MachineConfig` and `MachineConfigUpdate`.
-/// This is called only when `vcpu_num` is present in the JSON configuration.
-/// `T` can be either `u8` or `Option<u8>` which both support ordering if `vcpu_num` is
-/// present in the JSON.
-fn deserialize_vcpu_num<'de, D, T>(d: D) -> Result<T, D::Error>
-where
-    D: de::Deserializer<'de>,
-    T: Deserialize<'de> + PartialOrd + From<u8> + Debug,
-{
-    let val = T::deserialize(d)?;
-
-    if val > T::from(MAX_SUPPORTED_VCPUS) {
-        return Err(de::Error::invalid_value(
-            de::Unexpected::Other("vcpu_num"),
-            &"number of vCPUs exceeds the maximum limitation",
-        ));
-    }
-    if val < T::from(1) {
-        return Err(de::Error::invalid_value(
-            de::Unexpected::Other("vcpu_num"),
-            &"number of vCPUs should be larger than 0",
-        ));
-    }
-
-    Ok(val)
-}
-
-/// Deserialization function for the `smt` field in `MachineConfig` and `MachineConfigUpdate`.
-/// This is called only when `smt` is present in the JSON configuration.
-fn deserialize_smt<'de, D, T>(d: D) -> Result<T, D::Error>
-where
-    D: de::Deserializer<'de>,
-    T: Deserialize<'de> + PartialEq + From<bool> + Debug,
-{
-    let val = T::deserialize(d)?;
-
-    // If this function was called it means that `smt` was specified in
-    // the JSON. On aarch64 the only accepted value is `false` so throw an
-    // error if `true` was specified.
-    #[cfg(target_arch = "aarch64")]
-    if val == T::from(true) {
-        return Err(de::Error::invalid_value(
-            de::Unexpected::Other("smt"),
-            &"Enabling simultaneous multithreading is not supported on aarch64",
-        ));
-    }
-
-    Ok(val)
-}