Fix Intel CPUID leaf 4 cache topology for SMT

lib/propolis/src/cpuid.rs

@@ -337,12 +337,18 @@ impl Specializer {
                         // processor)
                         subleaf.eax |= (num_vproc - 1) << 26;
 
-                        // Present L1 and L2 caches as per-thread, L3 is across
-                        // the whole VM.
+                        // L1/L2 shared by SMT siblings, L3 shared by whole VM.
+                        //
+                        // Per Intel SDM, EAX[25:14] is "Maximum number of
+                        // addressable IDs for logical processors sharing this
+                        // cache". Add one to get the actual count; the nearest
+                        // power of 2 >= that value gives the APIC ID mask width.
                         if level < 3 {
                             subleaf.eax &= !LEAF4_EAX_VCPU_MASK;
-                            // And leave that range 0: this means only one
-                            // vCPU shares the cache.
+                            if self.has_smt {
+                                // 2 logical processors share L1/L2 so (1 + 1) = 2
+                                subleaf.eax |= 1 << 14;
+                            }
                         } else {
                             subleaf.eax &= !LEAF4_EAX_VCPU_MASK;
                             let shifted_vcpu = (num_vcpu - 1) << 14;

phd-tests/tests/src/cpuid.rs

@@ -3,7 +3,6 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues};
-use itertools::Itertools;
 use phd_framework::{test_vm::MigrationTimeout, TestVm};
 use phd_testcase::*;
 use propolis_client::instance_spec::{CpuidEntry, InstanceSpecStatus};
@@ -168,11 +167,20 @@ struct LinuxGuestTopo<'a> {
     vm: &'a TestVm,
 }
 
+struct CacheInfo {
+    level: u8,
+    shared_cpu_list: String,
+}
+
 impl<'a> LinuxGuestTopo<'a> {
     fn cpu_stem(vcpu: u8) -> String {
         format!("/sys/devices/system/cpu/cpu{vcpu}/topology")
     }
 
+    fn cache_stem(vcpu: u8, index: u8) -> String {
+        format!("/sys/devices/system/cpu/cpu{vcpu}/cache/index{index}")
+    }
+
     async fn new(vm: &'a TestVm) -> Self {
         let this = Self { vm };
         // Expect Linux numbers CPUs as 0 through vCPU-1 (inclusive).
@@ -273,6 +281,57 @@ impl<'a> LinuxGuestTopo<'a> {
         }
         result.into_iter()
     }
+
+    /// Returns true if the guest has SMT enabled.
+    ///
+    /// This is determined by checking if cpu0's thread_siblings has more than
+    /// one bit set.
+    async fn has_smt(&self) -> bool {
+        let siblings = self
+            .vm
+            .run_shell_command(&format!(
+                "cat {}/thread_siblings",
+                Self::cpu_stem(0)
+            ))
+            .await
+            .expect("can get thread siblings");
+        let siblings = siblings.trim();
+
+        let value = u64::from_str_radix(siblings, 16)
+            .expect("thread_siblings should be valid hex");
+        value.count_ones() > 1
+    }
+
+    async fn cache_info(&self) -> Vec<CacheInfo> {
+        let mut result = Vec::new();
+        for index in 0u8.. {
+            let stem = Self::cache_stem(0, index);
+            let level_out = self
+                .vm
+                .run_shell_command(&format!("cat {stem}/level 2>/dev/null"))
+                .await
+                .expect("can run cat");
+
+            let level_out = level_out.trim();
+            if level_out.is_empty() || level_out.contains("file not found") {
+                break;
+            }
+
+            let level: u8 =
+                level_out.parse().expect("cache level parses");
+
+            let shared_cpu_list = self
+                .vm
+                .run_shell_command(&format!("cat {stem}/shared_cpu_list"))
+                .await
+                .expect("can read shared_cpu_list")
+                .trim()
+                .to_string();
+
+            result.push(CacheInfo { level, shared_cpu_list });
+        }
+        result
+    }
 }
 
 #[phd_testcase]
@@ -312,37 +371,57 @@ async fn guest_cpu_topo_test(ctx: &Framework) {
     // All cores should be in socket 0
     assert!(guest_topo.physical_package_ids().await.all(|item| item == 0));
 
-    // We currently number CPUs such that Linux numbers them as successive pairs
-    // of thread twins.
-    let siblings = guest_topo.thread_siblings().await;
-    for (idx, mut pair) in siblings.chunks(2).into_iter().enumerate() {
-        let lower = pair.next().expect("sibling pair has a pair of cores");
-        let upper = pair.next().expect("pairs have even numbers of cores");
-
-        // Each pair of siblings should see that they have the same siblings
-        assert_eq!(lower, upper);
-        // This character in the string should have a pair of bits for the
-        // current sibling pair under consideration.
-        let sibling_idx = idx / 4;
-        // And at that index, the character should be this hex digit
-        // (representing the pair of bits for these sibling threads). We can be
-        // looking either of the lower pairs (in which case the cores are 0b0011
-        // => 3), or the higher pairs (in which case the cores are 0b1100 => c)
-        let sibling_char = match idx % 4 {
-            0 => '3',
-            1 => '3',
-            2 => 'c',
-            3 => 'c',
-            o => {
-                panic!("bit index in hex digit is less than four? except {o}");
+    let has_smt = guest_topo.has_smt().await;
+
+    // The thread_siblings checks only make sense when SMT is enabled.
+    if has_smt {
+        // We currently number CPUs such that Linux numbers them as successive
+        // pairs of thread twins.
+        let siblings: Vec<_> = guest_topo.thread_siblings().await.collect();
+        for (idx, pair) in siblings.chunks(2).enumerate() {
+            assert!(pair.len() == 2, "expected even number of CPUs");
+            let lower = &pair[0];
+            let upper = &pair[1];
+
+            // Each pair of siblings should see that they have the same siblings
+            assert_eq!(lower, upper);
+            // Each hex digit represents 4 CPUs. With chunks(2), idx is the pair
+            // number: idx=0 is cpus 0-1, idx=1 is cpus 2-3, etc.
+            let sibling_idx = idx / 2;
+            let sibling_char = if idx % 2 == 0 { '3' } else { 'c' };
+            assert!(lower.chars().enumerate().all(|(i, ch)| {
+                if i != sibling_idx {
+                    ch == '0'
+                } else {
+                    ch == sibling_char
+                }
+            }));
+        }
+    }
+
+    // Check cache topology. With SMT, L1/L2 should be shared by SMT siblings
+    // while L3 is shared across all vCPUs.
+    let caches = guest_topo.cache_info().await;
+    let num_cpus = guest_topo.cpus().await;
+
+    for cache in &caches {
+        match cache.level {
+            1 | 2 => {
+                let expected = if has_smt { "0-1" } else { "0" };
+                assert_eq!(cache.shared_cpu_list, expected);
             }
-        };
-        assert!(lower.chars().enumerate().all(|(i, ch)| {
-            if i != sibling_idx {
-                ch == '0'
-            } else {
-                ch == sibling_char
+            3 => {
+                let expected = format!("0-{}", num_cpus - 1);
+                assert_eq!(cache.shared_cpu_list, expected);
+            }
+            other => {
+                panic!("unexpected cache level {other}");
             }
-        }));
+        }
     }
+
+    let l1_count = caches.iter().filter(|c| c.level == 1).count();
+    let l2_count = caches.iter().filter(|c| c.level == 2).count();
+    assert!(l1_count >= 2, "expected at least L1d and L1i caches");
+    assert!(l2_count >= 1, "expected at least one L2 cache");
 }