Edit CPUID Normalization to account for possibility of hotplugging

JamesC1305 · JamesC1305 · commit 4df4135acfe3 · 2024-08-07T09:52:13.000Z
Changes some of the CPUID leaves to account for the possibility of
hotplugged vCPUs. Previously, the maximum APIC address space and L3
cache were only shared between boot_cpus. Whereas, now we want this to
be extended for the maximum number of CPUs that can be added,
MAX_SUPPORTED_VCPUS.

Signed-off-by: James Curtis &lt;jxcurtis@amazon.co.uk&gt;
diff --git a/src/vmm/src/cpu_config/x86_64/cpuid/amd/normalize.rs b/src/vmm/src/cpu_config/x86_64/cpuid/amd/normalize.rs
@@ -9,6 +9,7 @@ use crate::cpu_config::x86_64::cpuid::{
     cpuid, cpuid_count, CpuidEntry, CpuidKey, CpuidRegisters, CpuidTrait, KvmCpuidFlags,
     MissingBrandStringLeaves, BRAND_STRING_LENGTH, VENDOR_ID_AMD,
 };
+use crate::vmm_config::machine_config::MAX_SUPPORTED_VCPUS;
 
 /// Error type for [`super::AmdCpuid::normalize`].
 #[allow(clippy::module_name_repetitions)]
@@ -107,7 +108,7 @@ impl super::AmdCpuid {
         self.update_largest_extended_fn_entry()?;
         self.update_extended_feature_fn_entry()?;
         self.update_amd_feature_entry(cpu_count)?;
-        self.update_extended_cache_topology_entry(cpu_count, cpus_per_core)?;
+        self.update_extended_cache_topology_entry(cpus_per_core)?;
         self.update_extended_apic_id_entry(cpu_index, cpus_per_core)?;
         self.update_brand_string_entry()?;
 
@@ -268,7 +269,6 @@ impl super::AmdCpuid {
     #[allow(clippy::unwrap_in_result, clippy::unwrap_used)]
     fn update_extended_cache_topology_entry(
         &mut self,
-        cpu_count: u8,
         cpus_per_core: u8,
     ) -> Result<(), ExtendedCacheTopologyError> {
         for i in 0.. {
@@ -312,7 +312,7 @@ impl super::AmdCpuid {
                     // L3 Cache
                     // The L3 cache is shared among all the logical threads
                     3 => {
-                        let sub = cpu_count
+                        let sub = MAX_SUPPORTED_VCPUS
                             .checked_sub(1)
                             .ok_or(ExtendedCacheTopologyError::NumSharingCacheOverflow)?;
                         set_range(&mut subleaf.result.eax, 14..26, u32::from(sub))
diff --git a/src/vmm/src/cpu_config/x86_64/cpuid/intel/normalize.rs b/src/vmm/src/cpu_config/x86_64/cpuid/intel/normalize.rs
@@ -8,6 +8,7 @@ use crate::cpu_config::x86_64::cpuid::{
     host_brand_string, CpuidKey, CpuidRegisters, CpuidTrait, MissingBrandStringLeaves,
     BRAND_STRING_LENGTH,
 };
+use crate::vmm_config::machine_config::MAX_SUPPORTED_VCPUS;
 
 /// Error type for [`super::IntelCpuid::normalize`].
 #[derive(Debug, thiserror::Error, displaydoc::Display, Eq, PartialEq)]
@@ -62,14 +63,10 @@ impl super::IntelCpuid {
     #[inline]
     pub fn normalize(
         &mut self,
-        // The index of the current logical CPU in the range [0..cpu_count].
-        _cpu_index: u8,
-        // The total number of logical CPUs.
-        cpu_count: u8,
         // The number of logical CPUs per core.
         cpus_per_core: u8,
     ) -> Result<(), NormalizeCpuidError> {
-        self.update_deterministic_cache_entry(cpu_count, cpus_per_core)?;
+        self.update_deterministic_cache_entry(cpus_per_core)?;
         self.update_power_management_entry()?;
         self.update_extended_feature_flags_entry()?;
         self.update_performance_monitoring_entry()?;
@@ -82,7 +79,6 @@ impl super::IntelCpuid {
     #[allow(clippy::unwrap_in_result)]
     fn update_deterministic_cache_entry(
         &mut self,
-        cpu_count: u8,
         cpus_per_core: u8,
     ) -> Result<(), DeterministicCacheError> {
         for i in 0.. {
@@ -125,7 +121,7 @@ impl super::IntelCpuid {
                     // The L3 cache is shared among all the logical threads
                     3 => {
                         let sub = u32::from(
-                            cpu_count
+                            MAX_SUPPORTED_VCPUS
                                 .checked_sub(1)
                                 .ok_or(DeterministicCacheError::MaxCpusPerCoreUnderflow)?,
                         );
@@ -137,7 +133,7 @@ impl super::IntelCpuid {
 
                 // We know `cpus_per_core !=0` therefore this is always safe.
                 #[allow(clippy::unwrap_used)]
-                let cores = cpu_count.checked_div(cpus_per_core).unwrap();
+                let cores = MAX_SUPPORTED_VCPUS.checked_div(cpus_per_core).unwrap();
 
                 // Maximum number of addressable IDs for processor cores in the physical package.
                 // - Add one to the return value to get the result.
diff --git a/src/vmm/src/cpu_config/x86_64/cpuid/normalize.rs b/src/vmm/src/cpu_config/x86_64/cpuid/normalize.rs
@@ -4,6 +4,7 @@
 use crate::cpu_config::x86_64::cpuid::{
     cpuid, CpuidEntry, CpuidKey, CpuidRegisters, CpuidTrait, KvmCpuidFlags,
 };
+use crate::vmm_config::machine_config::MAX_SUPPORTED_VCPUS;
 
 /// Error type for [`super::Cpuid::normalize`].
 #[allow(clippy::module_name_repetitions)]
@@ -182,15 +183,15 @@ impl super::Cpuid {
             .checked_shl(u32::from(cpu_bits))
             .ok_or(NormalizeCpuidError::CpuBits(cpu_bits))?;
         self.update_vendor_id()?;
-        self.update_feature_info_entry(cpu_index, cpu_count)?;
-        self.update_extended_topology_entry(cpu_index, cpu_count, cpu_bits, cpus_per_core)?;
+        self.update_feature_info_entry(cpu_index)?;
+        self.update_extended_topology_entry(cpu_index, cpu_bits, cpus_per_core)?;
         self.update_extended_cache_features()?;
 
         // Apply manufacturer specific modifications.
         match self {
             // Apply Intel specific modifications.
             Self::Intel(intel_cpuid) => {
-                intel_cpuid.normalize(cpu_index, cpu_count, cpus_per_core)?;
+                intel_cpuid.normalize(cpus_per_core)?;
             }
             // Apply AMD specific modifications.
             Self::Amd(amd_cpuid) => amd_cpuid.normalize(cpu_index, cpu_count, cpus_per_core)?,
@@ -216,11 +217,7 @@ impl super::Cpuid {
     }
 
     // Update feature information entry
-    fn update_feature_info_entry(
-        &mut self,
-        cpu_index: u8,
-        cpu_count: u8,
-    ) -> Result<(), FeatureInformationError> {
+    fn update_feature_info_entry(&mut self, cpu_index: u8) -> Result<(), FeatureInformationError> {
         // Flush a cache line size.
         const EBX_CLFLUSH_CACHELINE: u32 = 8;
 
@@ -268,7 +265,7 @@ impl super::Cpuid {
             .map_err(FeatureInformationError::Clflush)?;
 
         let max_cpus_per_package = u32::from(
-            get_max_cpus_per_package(cpu_count)
+            get_max_cpus_per_package(MAX_SUPPORTED_VCPUS)
                 .map_err(FeatureInformationError::GetMaxCpusPerPackage)?,
         );
 
@@ -294,7 +291,7 @@ impl super::Cpuid {
         // A value of 1 for HTT indicates the value in CPUID.1.EBX[23:16]
         // (the Maximum number of addressable IDs for logical processors in this package)
         // is valid for the package
-        set_bit(&mut leaf_1.result.edx, 28, cpu_count > 1);
+        set_bit(&mut leaf_1.result.edx, 28, true);
 
         Ok(())
     }
@@ -303,7 +300,6 @@ impl super::Cpuid {
     fn update_extended_topology_entry(
         &mut self,
         cpu_index: u8,
-        cpu_count: u8,
         cpu_bits: u8,
         cpus_per_core: u8,
     ) -> Result<(), ExtendedTopologyError> {
@@ -408,8 +404,12 @@ impl super::Cpuid {
                         set_range(&mut subleaf.result.eax, 0..5, LEAFBH_INDEX1_APICID)
                             .map_err(ExtendedTopologyError::ApicId)?;
 
-                        set_range(&mut subleaf.result.ebx, 0..16, u32::from(cpu_count))
-                            .map_err(ExtendedTopologyError::LogicalProcessors)?;
+                        set_range(
+                            &mut subleaf.result.ebx,
+                            0..16,
+                            u32::from(MAX_SUPPORTED_VCPUS),
+                        )
+                        .map_err(ExtendedTopologyError::LogicalProcessors)?;
 
                         // We expect here as this is an extremely rare case that is unlikely to ever
                         // occur. It would require manual editing of the CPUID structure to push
@@ -577,12 +577,7 @@ mod tests {
                 },
             },
         )])));
-        let result = intel_cpuid.update_extended_topology_entry(
-            cpu_index,
-            cpu_count,
-            cpu_bits,
-            cpus_per_core,
-        );
+        let result = intel_cpuid.update_extended_topology_entry(cpu_index, cpu_bits, cpus_per_core);
         result.unwrap();
         assert!(intel_cpuid.inner().contains_key(&CpuidKey {
             leaf: 0xb,
@@ -605,8 +600,7 @@ mod tests {
                 },
             },
         )])));
-        let result =
-            amd_cpuid.update_extended_topology_entry(cpu_index, cpu_count, cpu_bits, cpus_per_core);
+        let result = amd_cpuid.update_extended_topology_entry(cpu_index, cpu_bits, cpus_per_core);
         result.unwrap();
         assert!(amd_cpuid.inner().contains_key(&CpuidKey {
             leaf: 0xb,
diff --git a/tests/framework/defs.py b/tests/framework/defs.py
@@ -39,3 +39,5 @@
     IMG_DIR = LOCAL_BUILD_PATH / "img"
 
 ARTIFACT_DIR = IMG_DIR / platform.machine()
+
+MAX_SUPPORTED_VCPUS = 32
diff --git a/tests/integration_tests/functional/test_cpu_features.py b/tests/integration_tests/functional/test_cpu_features.py
@@ -19,7 +19,7 @@
 
 import framework.utils_cpuid as cpuid_utils
 from framework import utils
-from framework.defs import SUPPORTED_HOST_KERNELS
+from framework.defs import MAX_SUPPORTED_VCPUS, SUPPORTED_HOST_KERNELS
 from framework.properties import global_props
 from framework.utils_cpu_templates import SUPPORTED_CPU_TEMPLATES
 
@@ -38,12 +38,12 @@ def clean_and_mkdir(dir_path):
     os.makedirs(dir_path)
 
 
-def _check_cpuid_x86(test_microvm, expected_cpu_count, expected_htt):
+def _check_cpuid_x86(test_microvm):
     expected_cpu_features = {
-        "maximum IDs for CPUs in pkg": f"{expected_cpu_count:#x} ({expected_cpu_count})",
+        "maximum IDs for CPUs in pkg": f"{MAX_SUPPORTED_VCPUS:#x} ({MAX_SUPPORTED_VCPUS})",
         "CLFLUSH line size": "0x8 (8)",
         "hypervisor guest status": "true",
-        "hyper-threading / multi-core supported": expected_htt,
+        "hyper-threading / multi-core supported": "true",
     }
 
     cpuid_utils.check_guest_cpuid_output(
@@ -116,7 +116,7 @@ def test_cpuid(uvm_plain_any, num_vcpus, htt):
     vm.basic_config(vcpu_count=num_vcpus, smt=htt)
     vm.add_net_iface()
     vm.start()
-    _check_cpuid_x86(vm, num_vcpus, "true" if num_vcpus > 1 else "false")
+    _check_cpuid_x86(vm)
 
 
 @pytest.mark.skipif(PLATFORM != "x86_64", reason="CPUID is only supported on x86_64.")
diff --git a/tests/integration_tests/functional/test_topology.py b/tests/integration_tests/functional/test_topology.py
@@ -8,6 +8,7 @@
 import pytest
 
 import framework.utils_cpuid as utils
+from framework.defs import MAX_SUPPORTED_VCPUS
 from framework.properties import global_props
 
 TOPOLOGY_STR = {1: "0", 2: "0,1", 16: "0-15"}
@@ -31,15 +32,13 @@ def _check_cpu_topology(
     )
 
 
-def _check_cache_topology_x86(
-    test_microvm, num_vcpus_on_lvl_1_cache, num_vcpus_on_lvl_3_cache
-):
+def _check_cache_topology_x86(test_microvm, num_vcpus_on_lvl_1_cache):
     vm = test_microvm
     expected_lvl_1_str = "{} ({})".format(
         hex(num_vcpus_on_lvl_1_cache), num_vcpus_on_lvl_1_cache
     )
     expected_lvl_3_str = "{} ({})".format(
-        hex(num_vcpus_on_lvl_3_cache), num_vcpus_on_lvl_3_cache
+        hex(MAX_SUPPORTED_VCPUS - 1), MAX_SUPPORTED_VCPUS - 1
     )
 
     cpu_vendor = utils.get_cpu_vendor()
@@ -181,7 +180,7 @@ def test_cache_topology(uvm_plain_any, num_vcpus, htt):
     vm.add_net_iface()
     vm.start()
     if PLATFORM == "x86_64":
-        _check_cache_topology_x86(vm, 1 if htt and num_vcpus > 1 else 0, num_vcpus - 1)
+        _check_cache_topology_x86(vm, 1 if htt and num_vcpus > 1 else 0)
     elif PLATFORM == "aarch64":
         _check_cache_topology_arm(vm, num_vcpus, global_props.host_linux_version_tpl)
     else:
