lib: move cpuid::Set to cpuid_utils; prevent semantic subleaf conflic…

…ts (#782)

Move the functionality in `propolis::cpuid::Set` into `cpuid_utils`, then
tidy it up a bit by removing its `for_regs` and `into_inner` functions (the
former was dead code and the latter is no longer needed).

Rework `CpuidMap` to prevent callers from using both `CpuidIdent { leaf: x,
subleaf: None }` and `CpuidIdent { leaf: x, subleaf: Some(y) }` as keys in
the same map (does CPUID with eax = x ignore the value passed in ecx or
not?). This requires a fair bit of new code to handle insert/remove and
iteration. Add tests for these cases, including a property test to verify a
larger number of iteration patterns than we could hope to write by hand.

Finally, add a theory statement and some additional function documentation
to the `cpuid_utils` crate.

Cargo.toml

@@ -133,6 +133,7 @@ pin-project-lite = "0.2.13"
 proc-macro2 = "1.0"
 proc-macro-error = "1"
 progenitor = "0.8.0"
+proptest = "1.5.0"
 quote = "1.0"
 rand = "0.8"
 reqwest = { version = "0.12.0", default-features = false }

bin/propolis-server/src/lib/migrate/compat.rs

@@ -71,7 +71,7 @@ pub enum CpuidMismatch {
     Vendor { this: CpuidVendor, other: CpuidVendor },
 
     #[error(transparent)]
-    LeavesOrValues(#[from] cpuid_utils::CpuidMapMismatch),
+    LeavesOrValues(#[from] cpuid_utils::CpuidSetMismatch),
 }
 
 #[derive(Debug, Error)]
@@ -259,14 +259,14 @@ impl spec::Spec {
                 })
             }
             (Some(this), Some(other)) => {
-                if this.vendor != other.vendor {
+                if this.vendor() != other.vendor() {
                     return Err(CpuidMismatch::Vendor {
-                        this: this.vendor,
-                        other: other.vendor,
+                        this: this.vendor(),
+                        other: other.vendor(),
                     });
                 }
 
-                this.leaves_and_values_equivalent(other)?;
+                this.is_equivalent_to(other)?;
                 Ok(())
             }
         }
@@ -472,8 +472,7 @@ impl CompatCheck for PciPciBridge {
 
 #[cfg(test)]
 mod test {
-    use cpuid_utils::{CpuidIdent, CpuidValues};
-    use propolis::cpuid;
+    use cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues};
     use propolis_api_types::instance_spec::components::{
         backends::{
             CrucibleStorageBackend, FileStorageBackend, VirtioNetworkBackend,
@@ -842,62 +841,62 @@ mod test {
     fn compatible_cpuid() {
         let mut s1 = new_spec();
         let mut s2 = s1.clone();
-        let mut set1 = cpuid::Set::new(CpuidVendor::Intel);
-        let mut set2 = cpuid::Set::new(CpuidVendor::Intel);
+        let mut set1 = CpuidSet::new(CpuidVendor::Intel);
+        let mut set2 = CpuidSet::new(CpuidVendor::Intel);
 
         s1.cpuid = Some(set1.clone());
         s2.cpuid = Some(set2.clone());
         s1.is_migration_compatible(&s2).unwrap();
 
-        set1.insert(CpuidIdent::leaf(0x1337), CpuidValues::default());
-        set2.insert(CpuidIdent::leaf(0x1337), CpuidValues::default());
+        set1.insert(CpuidIdent::leaf(0x1337), CpuidValues::default()).unwrap();
+        set2.insert(CpuidIdent::leaf(0x1337), CpuidValues::default()).unwrap();
 
         s1.cpuid = Some(set1.clone());
         s2.cpuid = Some(set2.clone());
         s1.is_migration_compatible(&s2).unwrap();
 
         let values = CpuidValues { eax: 5, ebx: 6, ecx: 7, edx: 8 };
-        set1.insert(CpuidIdent::subleaf(3, 4), values);
-        set2.insert(CpuidIdent::subleaf(3, 4), values);
+        set1.insert(CpuidIdent::subleaf(3, 4), values).unwrap();
+        set2.insert(CpuidIdent::subleaf(3, 4), values).unwrap();
         s1.is_migration_compatible(&s2).unwrap();
     }
 
     #[test]
     fn cpuid_explicitness_mismatch() {
         let mut s1 = new_spec();
         let s2 = s1.clone();
-        s1.cpuid = Some(cpuid::Set::new(CpuidVendor::Intel));
+        s1.cpuid = Some(CpuidSet::new(CpuidVendor::Intel));
         assert!(s1.is_migration_compatible(&s2).is_err());
     }
 
     #[test]
     fn cpuid_vendor_mismatch() {
         let mut s1 = new_spec();
         let mut s2 = s1.clone();
-        s1.cpuid = Some(cpuid::Set::new(CpuidVendor::Intel));
-        s2.cpuid = Some(cpuid::Set::new(CpuidVendor::Amd));
+        s1.cpuid = Some(CpuidSet::new(CpuidVendor::Intel));
+        s2.cpuid = Some(CpuidSet::new(CpuidVendor::Amd));
         assert!(s1.is_migration_compatible(&s2).is_err());
     }
 
     #[test]
     fn cpuid_leaf_set_mismatch() {
         let mut s1 = new_spec();
         let mut s2 = s1.clone();
-        let mut set1 = cpuid::Set::new(CpuidVendor::Amd);
-        let mut set2 = cpuid::Set::new(CpuidVendor::Amd);
+        let mut set1 = CpuidSet::new(CpuidVendor::Amd);
+        let mut set2 = CpuidSet::new(CpuidVendor::Amd);
 
         // Give the first set an entry the second set doesn't have.
-        set1.insert(CpuidIdent::leaf(0), CpuidValues::default());
-        set1.insert(CpuidIdent::leaf(1), CpuidValues::default());
-        set2.insert(CpuidIdent::leaf(0), CpuidValues::default());
+        set1.insert(CpuidIdent::leaf(0), CpuidValues::default()).unwrap();
+        set1.insert(CpuidIdent::leaf(1), CpuidValues::default()).unwrap();
+        set2.insert(CpuidIdent::leaf(0), CpuidValues::default()).unwrap();
 
         s1.cpuid = Some(set1);
         s2.cpuid = Some(set2.clone());
         assert!(s1.is_migration_compatible(&s2).is_err());
 
         // Make the sets have the same number of entries, but with a difference
         // in which entries they have.
-        set2.insert(CpuidIdent::leaf(3), CpuidValues::default());
+        set2.insert(CpuidIdent::leaf(3), CpuidValues::default()).unwrap();
         s2.cpuid = Some(set2);
         assert!(s1.is_migration_compatible(&s2).is_err());
     }
@@ -906,13 +905,13 @@ mod test {
     fn cpuid_leaf_value_mismatch() {
         let mut s1 = new_spec();
         let mut s2 = s1.clone();
-        let mut set1 = cpuid::Set::new(CpuidVendor::Amd);
-        let mut set2 = cpuid::Set::new(CpuidVendor::Amd);
+        let mut set1 = CpuidSet::new(CpuidVendor::Amd);
+        let mut set2 = CpuidSet::new(CpuidVendor::Amd);
 
         let v1 = CpuidValues { eax: 4, ebx: 5, ecx: 6, edx: 7 };
         let v2 = CpuidValues { eax: 100, ebx: 200, ecx: 300, edx: 400 };
-        set1.insert(CpuidIdent::leaf(0), v1);
-        set2.insert(CpuidIdent::leaf(0), v2);
+        set1.insert(CpuidIdent::leaf(0), v1).unwrap();
+        set2.insert(CpuidIdent::leaf(0), v2).unwrap();
         s1.cpuid = Some(set1);
         s2.cpuid = Some(set2);
         assert!(s1.is_migration_compatible(&s2).is_err());
@@ -922,16 +921,16 @@ mod test {
     fn cpuid_leaf_subleaf_conflict() {
         let mut s1 = new_spec();
         let mut s2 = s1.clone();
-        let mut set1 = cpuid::Set::new(CpuidVendor::Amd);
-        let mut set2 = cpuid::Set::new(CpuidVendor::Amd);
+        let mut set1 = CpuidSet::new(CpuidVendor::Amd);
+        let mut set2 = CpuidSet::new(CpuidVendor::Amd);
 
         // Check that leaf 0 with no subleaf is not compatible with leaf 0 and a
         // subleaf of 0. These are semantically different: the former matches
         // leaf 0 with any subleaf value, while the latter technically matches
         // only leaf 0 and subleaf 0 (with leaf-specific behavior if a different
         // subleaf is specified).
-        set1.insert(CpuidIdent::leaf(0), CpuidValues::default());
-        set2.insert(CpuidIdent::subleaf(0, 0), CpuidValues::default());
+        set1.insert(CpuidIdent::leaf(0), CpuidValues::default()).unwrap();
+        set2.insert(CpuidIdent::subleaf(0, 0), CpuidValues::default()).unwrap();
         s1.cpuid = Some(set1);
         s2.cpuid = Some(set2);
         assert!(s1.is_migration_compatible(&s2).is_err());

bin/propolis-server/src/lib/spec/api_spec_v0.rs

@@ -10,7 +10,7 @@ use std::collections::HashMap;
 use propolis_api_types::instance_spec::{
     components::{
         backends::{DlpiNetworkBackend, VirtioNetworkBackend},
-        board::{Board as InstanceSpecBoard, Cpuid},
+        board::Board as InstanceSpecBoard,
         devices::{BootSettings, SerialPort as SerialPortDesc},
     },
     v0::{ComponentV0, InstanceSpecV0},
@@ -89,10 +89,7 @@ impl From<Spec> for InstanceSpecV0 {
             cpus: board.cpus,
             memory_mb: board.memory_mb,
             chipset: board.chipset,
-            cpuid: cpuid.map(|set| {
-                let (map, vendor) = set.into_inner();
-                Cpuid { entries: map.into(), vendor }
-            }),
+            cpuid: cpuid.map(|set| set.into_instance_spec_cpuid()),
         };
         let mut spec = InstanceSpecV0 { board, components: Default::default() };
 

bin/propolis-server/src/lib/spec/builder.rs

@@ -105,10 +105,10 @@ impl SpecBuilder {
                     .cpuid
                     .map(|cpuid| -> Result<_, CpuidMapConversionError> {
                         {
-                            Ok(propolis::cpuid::Set::new_from_map(
+                            Ok(cpuid_utils::CpuidSet::from_map(
                                 cpuid.entries.try_into()?,
                                 cpuid.vendor,
-                            ))
+                            )?)
                         }
                     })
                     .transpose()?,

bin/propolis-server/src/lib/spec/mod.rs

@@ -16,7 +16,7 @@
 
 use std::collections::HashMap;
 
-use propolis::cpuid::Set as CpuidSet;
+use cpuid_utils::CpuidSet;
 use propolis_api_types::instance_spec::{
     components::{
         backends::{

bin/propolis-standalone/src/config.rs

@@ -9,14 +9,14 @@ use std::str::FromStr;
 use std::sync::Arc;
 
 use anyhow::Context;
+use cpuid_utils::CpuidSet;
 use propolis_types::CpuidIdent;
 use propolis_types::CpuidValues;
 use propolis_types::CpuidVendor;
 use serde::{Deserialize, Serialize};
 
 use cpuid_profile_config::*;
 use propolis::block;
-use propolis::cpuid;
 use propolis::hw::pci::Bdf;
 
 use crate::cidata::build_cidata_be;
@@ -226,19 +226,19 @@ pub fn parse_bdf(v: &str) -> Option<Bdf> {
     }
 }
 
-pub fn parse_cpuid(config: &Config) -> anyhow::Result<Option<cpuid::Set>> {
+pub fn parse_cpuid(config: &Config) -> anyhow::Result<Option<CpuidSet>> {
     if let Some(profile) = config.cpuid_profile() {
         let vendor = match profile.vendor {
             CpuVendor::Amd => CpuidVendor::Amd,
             CpuVendor::Intel => CpuidVendor::Intel,
         };
-        let mut set = cpuid::Set::new(vendor);
+        let mut set = CpuidSet::new(vendor);
         let entries: Vec<CpuidEntry> = profile.try_into()?;
         for entry in entries {
             let conflict = set.insert(
                 CpuidIdent { leaf: entry.func, subleaf: entry.idx },
                 CpuidValues::from(entry.values),
-            );
+            )?;
 
             if conflict.is_some() {
                 anyhow::bail!(

bin/propolis-standalone/src/main.rs

@@ -1345,7 +1345,7 @@ fn setup_instance(
         } else {
             // An empty set will instruct the kernel to use the legacy
             // fallback behavior
-            propolis::cpuid::Set::new_host()
+            cpuid_utils::CpuidSet::new_host()
         };
         vcpu.set_cpuid(vcpu_profile)?;
         vcpu.set_default_capabs()?;

crates/cpuid-utils/Cargo.toml

@@ -6,9 +6,13 @@ edition = "2021"
 
 [dependencies]
 bitflags.workspace = true
+bhyve_api.workspace = true
 propolis_api_types = {workspace = true, optional = true}
 propolis_types.workspace = true
 thiserror.workspace = true
 
+[dev-dependencies]
+proptest.workspace = true
+
 [features]
 instance-spec = ["propolis_api_types"]