From 93ed767388c4fab11af8a98ad33fdbeac4098b0c Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Mon, 14 Oct 2024 10:13:17 -0700 Subject: [PATCH] lib: move cpuid::Set to cpuid_utils; prevent semantic subleaf conflicts (#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.lock | 73 ++ Cargo.toml | 1 + bin/propolis-server/src/lib/migrate/compat.rs | 59 +- .../src/lib/spec/api_spec_v0.rs | 7 +- bin/propolis-server/src/lib/spec/builder.rs | 4 +- bin/propolis-server/src/lib/spec/mod.rs | 2 +- bin/propolis-standalone/src/config.rs | 8 +- bin/propolis-standalone/src/main.rs | 2 +- crates/cpuid-utils/Cargo.toml | 4 + crates/cpuid-utils/src/instance_spec.rs | 42 +- crates/cpuid-utils/src/lib.rs | 782 +++++++++++++++++- lib/propolis/src/cpuid.rs | 145 +--- lib/propolis/src/vcpu.rs | 83 +- phd-tests/tests/src/cpuid.rs | 26 +- 14 files changed, 963 insertions(+), 275 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e19d8e2f..04fd2b4c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -359,6 +359,21 @@ dependencies = [ "serde", ] +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bit_field" version = "0.10.2" @@ -756,9 +771,11 @@ dependencies = [ name = "cpuid_utils" version = "0.0.0" dependencies = [ + "bhyve_api 0.0.0", "bitflags 2.6.0", "propolis_api_types", "propolis_types", + "proptest", "thiserror", ] @@ -4455,6 +4472,26 @@ dependencies = [ "serde_test", ] +[[package]] +name = "proptest" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4c2511913b88df1637da85cc8d96ec8e43a3f8bb8ccb71ee1ac240d6f3df58d" +dependencies = [ + "bit-set", + "bit-vec", + "bitflags 2.6.0", + "lazy_static", + "num-traits", + "rand", + "rand_chacha", + "rand_xorshift", + "regex-syntax 0.8.4", + "rusty-fork", + "tempfile", + "unarray", +] + [[package]] name = "quick-error" version = "1.2.3" @@ -4554,6 +4591,15 @@ dependencies = [ "getrandom", ] +[[package]] +name = "rand_xorshift" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d25bf25ec5ae4a3f1b92f929810509a2f53d7dca2f50b794ff57e3face536c8f" +dependencies = [ + "rand_core", +] + [[package]] name = "rayon" version = "1.10.0" @@ -5016,6 +5062,18 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "rusty-fork" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb3dcc6e454c328bb824492db107ab7c0ae8fcffe4ad210136ef014458c1bc4f" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.18" @@ -6457,6 +6515,12 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed646292ffc8188ef8ea4d1e0e0150fb15a5c2e12ad9b8fc191ae7a8a7f3c4b9" +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicode-bidi" version = "0.3.15" @@ -6770,6 +6834,15 @@ dependencies = [ "utf8parse", ] +[[package]] +name = "wait-timeout" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f200f5b12eb75f8c1ed65abd4b2db8a6e1b138a20de009dacee265a2498f3f6" +dependencies = [ + "libc", +] + [[package]] name = "waitgroup" version = "0.1.2" diff --git a/Cargo.toml b/Cargo.toml index 16ba87ac..feb79ee4 100644 --- a/Cargo.toml +++ b/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 } diff --git a/bin/propolis-server/src/lib/migrate/compat.rs b/bin/propolis-server/src/lib/migrate/compat.rs index c82e8f49..a2d7fbfe 100644 --- a/bin/propolis-server/src/lib/migrate/compat.rs +++ b/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,23 +841,23 @@ 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(); } @@ -866,7 +865,7 @@ mod 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()); } @@ -874,8 +873,8 @@ mod 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()); } @@ -883,13 +882,13 @@ mod 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()); @@ -897,7 +896,7 @@ mod test { // 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()); diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 5cfd15c7..1bceaa04 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/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 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() }; diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index de699688..75a3f8ec 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/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()?, diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index a8fb4f88..41445355 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/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::{ diff --git a/bin/propolis-standalone/src/config.rs b/bin/propolis-standalone/src/config.rs index 38248e16..3631edde 100644 --- a/bin/propolis-standalone/src/config.rs +++ b/bin/propolis-standalone/src/config.rs @@ -9,6 +9,7 @@ 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; @@ -16,7 +17,6 @@ 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 { } } -pub fn parse_cpuid(config: &Config) -> anyhow::Result> { +pub fn parse_cpuid(config: &Config) -> anyhow::Result> { 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 = 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!( diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs index 987aba8b..10fa8968 100644 --- a/bin/propolis-standalone/src/main.rs +++ b/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()?; diff --git a/crates/cpuid-utils/Cargo.toml b/crates/cpuid-utils/Cargo.toml index ca789afb..096eb5bb 100644 --- a/crates/cpuid-utils/Cargo.toml +++ b/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"] diff --git a/crates/cpuid-utils/src/instance_spec.rs b/crates/cpuid-utils/src/instance_spec.rs index 063734c5..0c507fa5 100644 --- a/crates/cpuid-utils/src/instance_spec.rs +++ b/crates/cpuid-utils/src/instance_spec.rs @@ -6,28 +6,18 @@ use super::*; -use propolis_api_types::instance_spec::components::board::CpuidEntry; -use thiserror::Error; +use propolis_api_types::instance_spec::components::board::{Cpuid, CpuidEntry}; -/// An error that can occur when converting a list of CPUID entries in an -/// instance spec into a [`CpuidMap`]. -#[derive(Debug, Error)] -pub enum CpuidMapConversionError { - #[error("duplicate leaf and subleaf ({0:x}, {1:?})")] - DuplicateLeaf(u32, Option), - - #[error("leaf {0:x} specified subleaf 0 both explicitly and as None")] - SubleafZeroAliased(u32), - - #[error("leaf {0:x} not in standard or extended range")] - LeafOutOfRange(u32), +impl super::CpuidSet { + pub fn into_instance_spec_cpuid(self) -> Cpuid { + Cpuid { entries: self.map.into(), vendor: self.vendor } + } } impl From for Vec { fn from(value: CpuidMap) -> Self { value - .0 - .into_iter() + .iter() .map( |( CpuidIdent { leaf, subleaf }, @@ -59,7 +49,7 @@ impl TryFrom> for CpuidMap { fn try_from( value: Vec, ) -> Result { - let mut map = BTreeMap::new(); + let mut map = Self::default(); for CpuidEntry { leaf, subleaf, eax, ebx, ecx, edx } in value.into_iter() { @@ -69,25 +59,11 @@ impl TryFrom> for CpuidMap { return Err(CpuidMapConversionError::LeafOutOfRange(leaf)); } - if subleaf.is_none() - && map.contains_key(&CpuidIdent::subleaf(leaf, 0)) - { - return Err(CpuidMapConversionError::SubleafZeroAliased(leaf)); - } - - if let Some(0) = subleaf { - if map.contains_key(&CpuidIdent::leaf(leaf)) { - return Err(CpuidMapConversionError::SubleafZeroAliased( - leaf, - )); - } - } - if map .insert( CpuidIdent { leaf, subleaf }, CpuidValues { eax, ebx, ecx, edx }, - ) + )? .is_some() { return Err(CpuidMapConversionError::DuplicateLeaf( @@ -96,7 +72,7 @@ impl TryFrom> for CpuidMap { } } - Ok(Self(map)) + Ok(map) } } diff --git a/crates/cpuid-utils/src/lib.rs b/crates/cpuid-utils/src/lib.rs index 312d091d..8faaf1d3 100644 --- a/crates/cpuid-utils/src/lib.rs +++ b/crates/cpuid-utils/src/lib.rs @@ -4,6 +4,43 @@ //! Utility functions and types for working with CPUID values. //! +//! # The `CPUID` instruction +//! +//! The x86 CPUID instruction returns information about the executing processor. +//! This information can range from a manufacturer ID to a model number to the +//! processor's supported feature set to the calling logical processor's APIC +//! ID. +//! +//! CPUID takes as input a "leaf" or "function" value, passed in the eax +//! register, which determines what information the processor should return. +//! Some leaves accept a "subleaf" or "index" value, specified in ecx, that +//! further qualifies the information class supplied in eax. For example, leaf 4 +//! returns information about a processor's cache topology; it uses the subleaf +//! value as an index that identifies a particular type and level of cache. +//! +//! The CPUID leaf space is divided into "standard" and "extended" regions. +//! Leaves in the standard region (0 to 0xFFFF) have architecturally-defined +//! semantics. Leaves in the extended region (0x80000000 to 0x8000FFFF) have +//! vendor-specific semantics. +//! +//! `propolis-server` can accept a list of CPUID leaf/subleaf/value tuples as +//! part of an instance specification. If a client supplies one, Propolis will +//! initialize each vCPU so that CPUID instructions executed there will return +//! the supplied values, possibly with some adjustments (substituting vCPU +//! numbers into those leaves that return them, adjusting CPU topology leaves +//! based on other settings in the spec, etc.). +//! +//! This module implements two collections that Propolis components can use to +//! hold CPUID information: +//! +//! - [`CpuidMap`] maps from CPUID leaf and subleaf pairs to their associated +//! values, taking care to ensure that a single leaf is marked either as +//! ignoring or honoring the subleaf number, but not both. +//! - [`CpuidSet`] pairs a `CpuidMap` with a [`CpuidVendor`] that Propolis can +//! use to interpret the values of the map leaves in the extended region. +//! +//! # The `instance-spec` feature +//! //! If this crate is built with the `instance-spec` feature, this module //! includes mechanisms for converting from instance spec CPUID entries to and //! from the CPUID map types in the crate. This is feature-gated so that the @@ -11,7 +48,10 @@ //! `propolis-api-types`. use std::{ - collections::{BTreeMap, BTreeSet}, + collections::{ + btree_map::{self, Entry}, + BTreeMap, BTreeSet, + }, ops::RangeInclusive, }; @@ -23,55 +63,501 @@ pub mod bits; #[cfg(feature = "instance-spec")] mod instance_spec; -#[cfg(feature = "instance-spec")] -pub use instance_spec::*; +type CpuidSubleafMap = BTreeMap; +type CpuidMapInsertResult = Result, SubleafInsertConflict>; -/// A map from CPUID leaves to CPUID values. +/// Denotes the presence or absence of subleaves for a given CPUID leaf. +#[derive(Clone, Debug, PartialEq, Eq)] +enum Subleaves { + /// This leaf is configured to have subleaves, whose values are stored in + /// the inner map. + Present(CpuidSubleafMap), + + /// This leaf does not have subleaves. + Absent(CpuidValues), +} + +/// [`CpuidMap`]'s insert functions return this error if a request to insert a +/// new value would produce a leaf that has both per-subleaf values and a +/// no-subleaf value. +#[derive(Debug, Error)] +pub enum SubleafInsertConflict { + #[error("leaf {0:x} has entries with subleaves")] + SubleavesAlreadyPresent(u32), + + #[error("leaf {0:x} has an entry with no subleaves")] + SubleavesAlreadyAbsent(u32), +} + +/// A mapping from CPUID leaf/subleaf pairs to CPUID return values. This struct +/// allows each registered leaf either to have or not to have subleaf values, +/// but not both at once. +/// +/// Some CPUID leaves completely ignore the subleaf value passed in ecx. Others +/// pay attention to it and have their own per-leaf semantics that govern what +/// happens if the supplied ecx value is out of the function's expected range. +/// Tracking CPUID values in a simple `BTreeMap` from [`CpuidIdent`] to +/// [`CpuidValues`] allows a single leaf to use both options at once, because +/// `CpuidIdent { leaf: x, subleaf: None }` is not the same as `CpuidIdent { +/// leaf: x, subleaf: Some(y) }`. To avoid this kind of semantic confusion, this +/// type's `insert` method returns a `Result` that indicates whether inserting +/// the requested leaf/subleaf identifier would produce a semantic conflict +/// between a subleaf-bearing and subleaf-free entry. +/// +/// This structure allows "holes" in a leaf's subleaf IDs; that is, the +/// structure permits `leaf: 0, subleaf: Some(0)` and `subleaf: Some(2)` to +/// appear in a map where `subleaf: Some(1)` is absent. This is mostly for +/// simplicity (it saves the map type from having to check for discontiguous +/// subleaf domains); the existing subleaf-having CPUID leaves in the Intel and +/// AMD manuals all specify contiguous subleaf domains, and a client who +/// specifies a discontiguous subleaf set may find itself with unhappy guest +/// operating systems. #[derive(Clone, Debug, Default)] -pub struct CpuidMap(pub BTreeMap); +pub struct CpuidMap(BTreeMap); + +impl CpuidMap { + /// Retrieves the values associated with the supplied `ident`, or `None` if + /// the identifier is not present in the map. + pub fn get(&self, ident: CpuidIdent) -> Option<&CpuidValues> { + match ident.subleaf { + None => self.0.get(&ident.leaf).map(|ent| match ent { + Subleaves::Present(_) => None, + Subleaves::Absent(val) => Some(val), + }), + Some(sl) => self.0.get(&ident.leaf).map(|ent| match ent { + Subleaves::Absent(_) => None, + Subleaves::Present(sl_map) => sl_map.get(&sl), + }), + } + .flatten() + } + + /// Retrieves a mutable reference to the values associated with the supplied + /// `ident`, or `None` if the identifier is not present in the map. + pub fn get_mut(&mut self, ident: CpuidIdent) -> Option<&mut CpuidValues> { + match ident.subleaf { + None => self.0.get_mut(&ident.leaf).map(|ent| match ent { + Subleaves::Present(_) => None, + Subleaves::Absent(val) => Some(val), + }), + Some(sl) => self.0.get_mut(&ident.leaf).map(|ent| match ent { + Subleaves::Absent(_) => None, + Subleaves::Present(sl_map) => sl_map.get_mut(&sl), + }), + } + .flatten() + } + + fn insert_leaf_no_subleaf( + &mut self, + leaf: u32, + values: CpuidValues, + ) -> CpuidMapInsertResult { + match self.0.entry(leaf) { + Entry::Vacant(e) => { + e.insert(Subleaves::Absent(values)); + Ok(None) + } + Entry::Occupied(mut e) => match e.get_mut() { + Subleaves::Present(_) => { + Err(SubleafInsertConflict::SubleavesAlreadyPresent(leaf)) + } + Subleaves::Absent(v) => Ok(Some(std::mem::replace(v, values))), + }, + } + } + + fn insert_leaf_subleaf( + &mut self, + leaf: u32, + subleaf: u32, + values: CpuidValues, + ) -> CpuidMapInsertResult { + match self.0.entry(leaf) { + Entry::Vacant(e) => { + e.insert(Subleaves::Present( + [(subleaf, values)].into_iter().collect(), + )); + Ok(None) + } + Entry::Occupied(mut e) => match e.get_mut() { + Subleaves::Absent(_) => { + Err(SubleafInsertConflict::SubleavesAlreadyAbsent(leaf)) + } + Subleaves::Present(sl_map) => { + Ok(sl_map.insert(subleaf, values)) + } + }, + } + } + + /// Inserts the supplied (`ident`, `values`) pair into the map. + /// + /// # Return value + /// + /// - `Ok(None)` if the supplied leaf/subleaf pair was not present in the + /// map. + /// - `Ok(Some)` if the supplied leaf/subleaf pair was present in the map. + /// The wrapped value is the previous value set for this pair, which is + /// replaced by the supplied `values`. + /// - `Err` if the insert would cause the selected leaf to have one entry + /// with no subleaf and one entry with a subleaf. + pub fn insert( + &mut self, + ident: CpuidIdent, + values: CpuidValues, + ) -> CpuidMapInsertResult { + match ident.subleaf { + Some(sl) => self.insert_leaf_subleaf(ident.leaf, sl, values), + None => self.insert_leaf_no_subleaf(ident.leaf, values), + } + } + + /// Removes the entry with the supplied `ident` from the map if it is + /// present, returning its value. + /// + /// If `ident.subleaf` is `None`, this routine will only match leaf entries + /// that don't specify per-subleaf values. + pub fn remove(&mut self, ident: CpuidIdent) -> Option { + // If the leaf isn't present there's nothing to return. + let Entry::Occupied(mut entry) = self.0.entry(ident.leaf) else { + return None; + }; + + let (val, remove_leaf) = { + match (ident.subleaf, entry.get_mut()) { + // The caller didn't supply a subleaf, and this leaf doesn't + // have a subleaf map. Yank the leaf entry and return the + // associated value. (This can't be done inline because the + // entry is mutably borrowed.) + (None, Subleaves::Absent(val)) => (*val, true), + // The caller didn't supply a subleaf, but the leaf has one, so + // the keys don't match. + (None, Subleaves::Present(_)) => { + return None; + } + // The caller supplied a subleaf, but the leaf doesn't use + // subleaves, so the keys don't match. + (Some(_), Subleaves::Absent(_)) => { + return None; + } + // The caller supplied a subleaf, and this leaf has subleaf + // data. If the requested subleaf is in the leaf's subleaf map, + // remove the corresponding subleaf entry. If this empties the + // subleaf map, also clean up the leaf entry. + (Some(subleaf), Subleaves::Present(sl_map)) => { + let val = sl_map.remove(&subleaf)?; + (val, sl_map.is_empty()) + } + } + }; + + if remove_leaf { + entry.remove_entry(); + } + + Some(val) + } + + /// Removes all data for the supplied `leaf`. If the leaf has subleaves, all + /// their entries are removed. + pub fn remove_leaf(&mut self, leaf: u32) { + self.0.remove(&leaf); + } + + /// Passes each leaf number in the map to `f` and removes any leaf entries + /// for which `f` returns `false`. If a removed leaf has subleaves, all + /// their entries are removed. + // + // This function can be made to consider subleaves by changing `f` to take a + // `CpuidIdent` and then writing the call to `retain` with a `match`: + // + // - If the leaf has `Subleaves::Absent`, pass the leaf ID through to `f` + // directly and return the result to `retain`. + // - If the leaf has `Subleaves::Present`, call `retain` on the subleaf map, + // passing each subleaf to `f`, then return `!map.is_empty()` to the outer + // call to `retain` (i.e. keep the leaf entry if the subleaf map still has + // entries in it). + // + // The cost of doing this is that the function now needs to visit every + // subleaf entry in the map, even if the caller doesn't care about subleaf + // IDs. So it may be better to break this out into a separate function. + pub fn retain_by_leaf(&mut self, mut f: F) + where + F: FnMut(u32) -> bool, + { + self.0.retain(|leaf, _| f(*leaf)); + } + + /// Clears the entire map. + pub fn clear(&mut self) { + self.0.clear(); + } + + /// Returns `true` if the map has no entries. + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + /// Returns the total number of leaf/subleaf/value tuples in the map. + pub fn len(&self) -> usize { + let mut len = 0; + for sl in self.0.values() { + match sl { + Subleaves::Absent(_) => len += 1, + Subleaves::Present(sl_map) => len += sl_map.len(), + } + } + + len + } + + /// Returns an iterator over the ([`CpuidIdent`], [`CpuidValues`]) pairs in + /// the map. + pub fn iter(&self) -> CpuidMapIterator { + CpuidMapIterator::new(self) + } +} + +/// An iterator over a [`CpuidMap`]'s leaf/subleaf/value tuples. +pub struct CpuidMapIterator<'a> { + leaf_iter: btree_map::Iter<'a, u32, Subleaves>, + subleaf_iter: Option<(u32, btree_map::Iter<'a, u32, CpuidValues>)>, +} + +impl<'a> CpuidMapIterator<'a> { + fn new(map: &'a CpuidMap) -> Self { + Self { leaf_iter: map.0.iter(), subleaf_iter: None } + } +} + +impl Iterator for CpuidMapIterator<'_> { + type Item = (CpuidIdent, CpuidValues); + + fn next(&mut self) -> Option { + // If a subleaf iteration is in progress, try to advance that iterator. + // If that produces another subleaf value, return it. Otherwise, clear + // the subleaf iterator and move to the next leaf. + if let Some((leaf, subleaf_iter)) = &mut self.subleaf_iter { + if let Some((subleaf, value)) = subleaf_iter.next() { + return Some((CpuidIdent::subleaf(*leaf, *subleaf), *value)); + }; + + self.subleaf_iter = None; + } + + // Advance the leaf iterator. If there are no more leaves to iterate, + // the entire iteration is over. + let (leaf, subleaves) = self.leaf_iter.next()?; + + // Iteration has moved to a new leaf. Consider whether it has any + // subleaves. If it doesn't, simply return the leaf and value. + // Otherwise, start iterating over subleaves. + match subleaves { + Subleaves::Absent(val) => Some((CpuidIdent::leaf(*leaf), *val)), + Subleaves::Present(sl_map) => { + let mut subleaf_iter = sl_map.iter(); + + // This invariant is upheld by insert/remove. + let (subleaf, value) = subleaf_iter + .next() + .expect("subleaf maps always have at least one entry"); + + // Stash the iterator along with the leaf value it + // corresponds to so that future iterations can return + // the entire leaf/subleaf pair. + self.subleaf_iter = Some((*leaf, subleaf_iter)); + Some((CpuidIdent::subleaf(*leaf, *subleaf), *value)) + } + } + } +} + +/// A map from CPUID leaves to CPUID values that includes a vendor ID. Callers +/// can use the vendor ID to interpret the meanings of any extended leaves +/// present in the map. +#[derive(Clone, Debug)] +pub struct CpuidSet { + map: CpuidMap, + vendor: CpuidVendor, +} + +impl Default for CpuidSet { + /// Equivalent to [`Self::new_host`]. + fn default() -> Self { + Self::new_host() + } +} +/// A discrepancy between two [`CpuidSet`]s. #[derive(Debug, Error)] -pub enum CpuidMapMismatch { +pub enum CpuidSetMismatch { + /// The sets have different CPU vendors. + #[error("CPUID set has mismatched vendors (self: {this}, other: {other})")] + Vendor { this: CpuidVendor, other: CpuidVendor }, + + /// The two sets have different leaves or subleaves. The payload contains + /// all of the leaf identifiers that were present in one set but not the + /// other. #[error("CPUID leaves not found in both sets ({0:?})")] - MismatchedLeaves(Vec), + LeafSet(Vec), + /// The two sets disagree on the values to return for one or more + /// leaf/subleaf pairs. The payload contains the leaf/subleaf ID and values + /// for all such pairs. #[error("CPUID leaves have different values in different sets ({0:?})")] - MismatchedValues(Vec<(CpuidIdent, CpuidValues, CpuidValues)>), + Values(Vec<(CpuidIdent, CpuidValues, CpuidValues)>), } -impl CpuidMap { +impl CpuidSet { + /// Creates an empty `CpuidSet` with the supplied `vendor`. + pub fn new(vendor: CpuidVendor) -> Self { + Self { map: CpuidMap::default(), vendor } + } + + /// Creates a new `CpuidSet` with the supplied initial leaf/value `map` and + /// `vendor`. + pub fn from_map( + map: CpuidMap, + vendor: CpuidVendor, + ) -> Result { + Ok(Self { map, vendor }) + } + + /// Yields this set's vendor. + pub fn vendor(&self) -> CpuidVendor { + self.vendor + } + + /// Executes the CPUID instruction on the current machine to determine its + /// processor vendor, then creates an empty `CpuidSet` with that vendor. + /// + /// # Panics + /// + /// Panics if the host is not an Intel or AMD CPU (leaf 0 ebx/ecx/edx + /// contain something other than "GenuineIntel" or "AuthenticAMD"). + pub fn new_host() -> Self { + let vendor = CpuidVendor::try_from(host_query(CpuidIdent::leaf(0))) + .expect("host CPU should be from recognized vendor"); + Self::new(vendor) + } + + /// See [`CpuidMap::insert`]. + pub fn insert( + &mut self, + ident: CpuidIdent, + values: CpuidValues, + ) -> CpuidMapInsertResult { + self.map.insert(ident, values) + } + + /// See [`CpuidMap::get`]. + pub fn get(&self, ident: CpuidIdent) -> Option<&CpuidValues> { + self.map.get(ident) + } + + /// See [`CpuidMap::get_mut`]. + pub fn get_mut(&mut self, ident: CpuidIdent) -> Option<&mut CpuidValues> { + self.map.get_mut(ident) + } + + /// See [`CpuidMap::remove_leaf`]. + pub fn remove_leaf(&mut self, leaf: u32) { + self.map.remove_leaf(leaf); + } + + /// See [`CpuidMap::is_empty`]. + pub fn is_empty(&self) -> bool { + self.map.is_empty() + } + + /// See [`CpuidMap::iter`]. + pub fn iter(&self) -> CpuidMapIterator { + self.map.iter() + } + /// Returns `Ok` if `self` is equivalent to `other`; if not, returns a - /// [`CpuidMapMismatch`] describing the difference between the maps. - pub fn is_equivalent(&self, other: &Self) -> Result<(), CpuidMapMismatch> { - let this_set: BTreeSet<_> = self.0.keys().collect(); - let other_set: BTreeSet<_> = other.0.keys().collect(); + /// [`CpuidMapMismatch`] describing the first observed difference between + /// the two sets. + pub fn is_equivalent_to( + &self, + other: &Self, + ) -> Result<(), CpuidSetMismatch> { + if self.vendor != other.vendor { + return Err(CpuidSetMismatch::Vendor { + this: self.vendor, + other: other.vendor, + }); + } + + let this_set: BTreeSet<_> = + self.map.iter().map(|(ident, _)| ident).collect(); + let other_set: BTreeSet<_> = + other.map.iter().map(|(ident, _)| ident).collect(); let diff = this_set.symmetric_difference(&other_set); - let diff: Vec = diff.copied().copied().collect(); + let diff: Vec = diff.copied().collect(); if !diff.is_empty() { - return Err(CpuidMapMismatch::MismatchedLeaves(diff)); + return Err(CpuidSetMismatch::LeafSet(diff)); } let mut mismatches = vec![]; - for (this_leaf, this_value) in self.0.iter() { + for (this_leaf, this_value) in self.map.iter() { let other_value = other - .0 + .map .get(this_leaf) .expect("key sets were already found to be equal"); - if this_value != other_value { - mismatches.push((*this_leaf, *this_value, *other_value)); + if this_value != *other_value { + mismatches.push((this_leaf, this_value, *other_value)); } } if !mismatches.is_empty() { - Err(CpuidMapMismatch::MismatchedValues(mismatches)) + Err(CpuidSetMismatch::Values(mismatches)) } else { Ok(()) } } } +impl From for Vec { + fn from(value: CpuidSet) -> Self { + let mut out = Vec::with_capacity(value.map.len()); + out.extend(value.map.iter().map(|(ident, leaf)| { + let vce_flags = match ident.subleaf.as_ref() { + Some(_) => bhyve_api::VCE_FLAG_MATCH_INDEX, + None => 0, + }; + bhyve_api::vcpu_cpuid_entry { + vce_function: ident.leaf, + vce_index: ident.subleaf.unwrap_or(0), + vce_flags, + vce_eax: leaf.eax, + vce_ebx: leaf.ebx, + vce_ecx: leaf.ecx, + vce_edx: leaf.edx, + ..Default::default() + } + })); + out + } +} + +/// An error that can occur when converting a list of CPUID entries in an +/// instance spec into a [`CpuidMap`]. +#[derive(Debug, Error)] +pub enum CpuidMapConversionError { + #[error("duplicate leaf and subleaf ({0:x}, {1:?})")] + DuplicateLeaf(u32, Option), + + #[error("leaf {0:x} not in standard or extended range")] + LeafOutOfRange(u32), + + #[error(transparent)] + SubleafConflict(#[from] SubleafInsertConflict), +} + /// The range of standard, architecturally-defined CPUID leaves. pub const STANDARD_LEAVES: RangeInclusive = 0..=0xFFFF; @@ -100,25 +586,269 @@ pub fn host_query(leaf: CpuidIdent) -> CpuidValues { /// /// Panics if the target architecture is not x86-64. pub fn host_query_all() -> CpuidMap { - let mut map = BTreeMap::new(); + let mut map = CpuidMap::default(); let leaf_0 = CpuidIdent::leaf(*STANDARD_LEAVES.start()); let leaf_0_values = host_query(leaf_0); - map.insert(leaf_0, leaf_0_values); + map.insert(leaf_0, leaf_0_values).unwrap(); for l in (STANDARD_LEAVES.start() + 1)..=leaf_0_values.eax { let leaf = CpuidIdent::leaf(l); - map.insert(leaf, host_query(leaf)); + map.insert(leaf, host_query(leaf)).unwrap(); } // This needs to be done by hand because the `__get_cpuid_max` intrinsic // only returns the maximum standard leaf. let ext_leaf_0 = CpuidIdent::leaf(*EXTENDED_LEAVES.start()); let ext_leaf_0_values = host_query(ext_leaf_0); - map.insert(ext_leaf_0, ext_leaf_0_values); + map.insert(ext_leaf_0, ext_leaf_0_values).unwrap(); for l in (EXTENDED_LEAVES.start() + 1)..=ext_leaf_0_values.eax { let leaf = CpuidIdent::leaf(l); - map.insert(leaf, host_query(leaf)); + map.insert(leaf, host_query(leaf)).unwrap(); + } + + map +} + +#[cfg(test)] +mod test { + use proptest::prelude::*; + + use super::*; + + #[test] + fn insert_leaf_then_subleaf_fails() { + let mut map = CpuidMap::default(); + assert_eq!( + map.insert(CpuidIdent::leaf(0), CpuidValues::default()).unwrap(), + None + ); + + map.insert(CpuidIdent::subleaf(0, 0), CpuidValues::default()) + .unwrap_err(); } - CpuidMap(map) + #[test] + fn insert_subleaf_then_leaf_fails() { + let mut map = CpuidMap::default(); + assert_eq!( + map.insert(CpuidIdent::subleaf(0, 0), CpuidValues::default()) + .unwrap(), + None + ); + + map.insert(CpuidIdent::leaf(0), CpuidValues::default()).unwrap_err(); + } + + #[test] + fn insert_leaf_then_remove_then_insert_subleaf() { + let mut map = CpuidMap::default(); + let values = CpuidValues { eax: 1, ebx: 2, ecx: 3, edx: 4 }; + assert_eq!(map.insert(CpuidIdent::leaf(0), values).unwrap(), None); + + map.insert(CpuidIdent::subleaf(0, 1), values).unwrap_err(); + assert_eq!(map.remove(CpuidIdent::leaf(0)).unwrap(), values); + assert_eq!( + map.insert(CpuidIdent::subleaf(0, 1), CpuidValues::default()) + .unwrap(), + None + ); + } + + #[test] + fn insert_multiple_subleaves() { + let mut map = CpuidMap::default(); + let values = CpuidValues { eax: 1, ebx: 2, ecx: 3, edx: 4 }; + for subleaf in 0..10 { + assert_eq!( + map.insert(CpuidIdent::subleaf(0, subleaf), values).unwrap(), + None + ); + } + } + + #[test] + fn leaf_subleaf_removal() { + let mut map = CpuidMap::default(); + + // Add some leaves (0-2) with no subleaves. + for leaf in 0..3 { + assert_eq!( + map.insert(CpuidIdent::leaf(leaf), CpuidValues::default()) + .unwrap(), + None + ); + } + + // Add some leaves (3-5) with subleaves. + for leaf in 3..6 { + for subleaf in 6..9 { + assert_eq!( + map.insert( + CpuidIdent::subleaf(leaf, subleaf), + CpuidValues { + eax: leaf, + ebx: subleaf, + ..Default::default() + } + ) + .unwrap(), + None + ); + } + } + + // One entry for each of leaves 0-2 and three entries each for 3-5. + let mut len = map.len(); + assert_eq!(len, 3 + (3 * 3)); + + // Manually remove all of leaf 5's subleaves. + for subleaf in 6..9 { + assert_eq!( + map.remove(CpuidIdent::subleaf(5, subleaf)).unwrap(), + CpuidValues { eax: 5, ebx: subleaf, ..Default::default() } + ); + + len -= 1; + assert_eq!(map.len(), len); + } + + // Leaf 5 should no longer be in the map in any of its guises. + assert_eq!(map.get(CpuidIdent::leaf(5)), None); + for subleaf in 6..9 { + assert_eq!(map.get(CpuidIdent::subleaf(5, subleaf)), None); + } + + // Removing leaves 3 and 4 without specifying their subleaves should + // fail. + assert_eq!(map.remove(CpuidIdent::leaf(3)), None); + assert_eq!(map.remove(CpuidIdent::leaf(4)), None); + assert_eq!(map.len(), len); + + // Remove leaf 3 and its subleaves via `remove_leaf`. + map.remove_leaf(3); + len -= 3; + assert_eq!(map.len(), len); + + // Remove leaf 4 via `retain_by_leaf`. + map.retain_by_leaf(|leaf| leaf != 4); + len -= 3; + assert_eq!(map.len(), len); + + // Removing leaf 0, subleaf 0 should fail. + assert_eq!(map.remove(CpuidIdent::subleaf(0, 0)), None); + + // Remove leaves 0-2 by their IDs. + for leaf in 0..3 { + assert!(map.remove(CpuidIdent::leaf(leaf)).is_some()); + len -= 1; + assert_eq!(map.len(), len); + } + + assert_eq!(len, 0); + assert!(map.is_empty()); + } + + #[derive(Debug)] + enum MapEntry { + Leaf(u32, CpuidValues), + Subleaves(u32, Vec<(u32, CpuidValues)>), + } + + /// Produces a random CPUID leaf entry. Each entry has an leaf number in + /// [0..8) and one of the following values: + /// + /// - One of sixteen random [`CpuidValues`] values + /// - One subleaf with index [0..5) and one of two values + /// - Two such subleaves + /// - Three such subleaves + /// + /// There are (16 + 10 + 100 + 1000) = 1,126 possible values for each leaf, + /// for a total of 9,008 possible leaf entries. + fn map_entry_strategy() -> impl Strategy { + const MAX_LEAF: u32 = 8; + prop_oneof![ + (0..MAX_LEAF, prop::array::uniform4(0..2u32)).prop_map( + |(leaf, value_arr)| { MapEntry::Leaf(leaf, value_arr.into()) } + ), + ( + 0..MAX_LEAF, + prop::collection::vec(0..5u32, 1..=3), + proptest::bool::ANY + ) + .prop_map(|(leaf, subleaves, set_value)| { + let value = if set_value { + CpuidValues { eax: 1, ebx: 2, ecx: 3, edx: 4 } + } else { + CpuidValues::default() + }; + let subleaves = subleaves + .into_iter() + .map(|subleaf| (subleaf, value)) + .collect(); + MapEntry::Subleaves(leaf, subleaves) + }) + ] + } + + proptest! { + /// Verifies that a [`CpuidMapIterator`] visits all of the leaf and + /// subleaf entries in a map and does so in the expected order. + /// + /// proptest will generate a set of 3-8 leaves for each test according + /// to the strategy defined in [`map_entry_strategy`]. + #[test] + fn map_iteration_order( + entries in prop::collection::vec(map_entry_strategy(), 3..=8) + ) { + let mut map = CpuidMap::default(); + let mut _expected_len = 0; + + // Insert all of the entries into the map. The input array may have + // some duplicates and may assign both a no-subleaf and a subleaf + // value to a single leaf; ignore all of the resulting errors and + // substitutions. + for entry in entries { + match entry { + MapEntry::Leaf(leaf, values) => { + if let Ok(None) = map.insert( + CpuidIdent::leaf(leaf), + values + ) { + _expected_len += 1; + } + } + MapEntry::Subleaves(leaf, subleaves) => { + for (subleaf, values) in subleaves { + if let Ok(None) = map.insert( + CpuidIdent::subleaf(leaf, subleaf), + values + ) { + _expected_len += 1; + } + } + } + } + } + + assert_eq!(map.len(), _expected_len); + + // The iterator should visit leaves in order and return subleaves + // before the next leaf. This happens to be the ordering provided by + // `CpuidIdent`'s `Ord` implementation, so it suffices just to + // compare identifiers directly. + let mut _observed_len = 0; + let output: Vec<(CpuidIdent, CpuidValues)> = map.iter().collect(); + for (first, second) in + output.as_slice().windows(2).map(|sl| (sl[0].0, sl[1].0)) { + assert!(first < second, "first: {first:?}, second: {second:?}"); + _observed_len += 1; + } + + // The `windows(2)` iterator will not count the last entry (it has + // no successor), so the actual observed length is one more than the + // number of observed iterations. (Note that by construction the map + // is not empty, so there is always a last entry.) + assert_eq!(_observed_len + 1, _expected_len); + } + } } diff --git a/lib/propolis/src/cpuid.rs b/lib/propolis/src/cpuid.rs index 3323872e..3a29c481 100644 --- a/lib/propolis/src/cpuid.rs +++ b/lib/propolis/src/cpuid.rs @@ -11,113 +11,7 @@ use std::num::NonZeroU8; use std::ops::Bound; use bhyve_api::vcpu_cpuid_entry; -use cpuid_utils::{CpuidIdent, CpuidMap, CpuidValues, CpuidVendor}; - -/// Set of cpuid leafs -#[derive(Clone, Debug)] -pub struct Set { - map: CpuidMap, - pub vendor: CpuidVendor, -} - -impl Default for Set { - fn default() -> Self { - Self::new_host() - } -} - -impl Set { - pub fn new(vendor: CpuidVendor) -> Self { - Set { map: CpuidMap::default(), vendor } - } - - pub fn new_host() -> Self { - let vendor = - CpuidVendor::try_from(cpuid_utils::host_query(CpuidIdent::leaf(0))) - .expect("host CPU should be from recognized vendor"); - Self::new(vendor) - } - - pub fn new_from_map(map: CpuidMap, vendor: CpuidVendor) -> Self { - Self { map, vendor } - } - - pub fn insert( - &mut self, - ident: CpuidIdent, - entry: CpuidValues, - ) -> Option { - self.map.0.insert(ident, entry) - } - pub fn remove(&mut self, ident: CpuidIdent) -> Option { - self.map.0.remove(&ident) - } - pub fn remove_all(&mut self, func: u32) { - self.map.0.retain(|ident, _val| ident.leaf != func); - } - pub fn get(&self, ident: CpuidIdent) -> Option<&CpuidValues> { - self.map.0.get(&ident) - } - pub fn get_mut(&mut self, ident: CpuidIdent) -> Option<&mut CpuidValues> { - self.map.0.get_mut(&ident) - } - pub fn is_empty(&self) -> bool { - self.map.0.is_empty() - } - pub fn len(&self) -> usize { - self.map.0.len() - } - - pub fn iter(&self) -> Iter { - Iter(self.map.0.iter()) - } - - pub fn for_regs(&self, eax: u32, ecx: u32) -> Option { - if let Some(ent) = self.map.0.get(&CpuidIdent::subleaf(eax, ecx)) { - // Exact match - Some(*ent) - } else if let Some(ent) = self.map.0.get(&CpuidIdent::leaf(eax)) { - // Function-only match - Some(*ent) - } else { - None - } - } - - pub fn leaves_and_values_equivalent( - &self, - other: &Self, - ) -> Result<(), cpuid_utils::CpuidMapMismatch> { - self.map.is_equivalent(&other.map) - } - - pub fn into_inner(self) -> (CpuidMap, CpuidVendor) { - (self.map, self.vendor) - } -} - -impl From for Vec { - fn from(value: Set) -> Self { - let mut out = Vec::with_capacity(value.map.0.len()); - out.extend(value.map.0.iter().map(|(ident, leaf)| { - let vce_flags = match ident.subleaf.as_ref() { - Some(_) => bhyve_api::VCE_FLAG_MATCH_INDEX, - None => 0, - }; - bhyve_api::vcpu_cpuid_entry { - vce_function: ident.leaf, - vce_index: ident.subleaf.unwrap_or(0), - vce_flags, - vce_eax: leaf.eax, - vce_ebx: leaf.ebx, - vce_ecx: leaf.ecx, - vce_edx: leaf.edx, - ..Default::default() - } - })); - out - } -} +use cpuid_utils::{CpuidIdent, CpuidMap, CpuidSet, CpuidValues, CpuidVendor}; /// Convert a [vcpu_cpuid_entry] into an ([CpuidLeaf], /// [CpuidValues]) tuple, suitable for insertion into a [Set]. @@ -143,17 +37,6 @@ pub fn from_raw( ) } -pub struct Iter<'a>( - std::collections::btree_map::Iter<'a, CpuidIdent, CpuidValues>, -); -impl<'a> Iterator for Iter<'a> { - type Item = (&'a CpuidIdent, &'a CpuidValues); - - fn next(&mut self) -> Option { - self.0.next() - } -} - #[derive(Debug, thiserror::Error)] pub enum SpecializeError { #[error("unsupported cache level")] @@ -171,7 +54,6 @@ pub struct Specializer { has_smt: bool, num_vcpu: Option, vcpuid: Option, - vendor_kind: Option, cpu_topo_populate: BTreeSet, cpu_topo_clear: BTreeSet, do_cache_topo: bool, @@ -192,11 +74,6 @@ impl Specializer { Self { vcpuid: Some(vcpuid), ..self } } - /// Specify CPU vendor - pub fn with_vendor(self, vendor: CpuidVendor) -> Self { - Self { vendor_kind: Some(vendor), ..self } - } - /// Specify CPU topology types to render into the specialized [Set] /// /// Without basic information such as the number of vCPUs (set by @@ -237,12 +114,11 @@ impl Specializer { /// Given the attributes and modifiers specified in this [Specializer], /// render an updated [Set] reflecting those data. - pub fn execute(self, mut set: Set) -> Result { - // Use vendor override if provided, or else the existing one - if let Some(vendor) = self.vendor_kind { - set.vendor = vendor; - } - match set.vendor { + pub fn execute( + self, + mut set: CpuidSet, + ) -> Result { + match set.vendor() { CpuidVendor::Amd => { if self.do_cache_topo && self.num_vcpu.is_some() { self.fix_amd_cache_topo(&mut set)?; @@ -278,7 +154,10 @@ impl Specializer { Ok(set) } - fn fix_amd_cache_topo(&self, set: &mut Set) -> Result<(), SpecializeError> { + fn fix_amd_cache_topo( + &self, + set: &mut CpuidSet, + ) -> Result<(), SpecializeError> { assert!(self.do_cache_topo); let num = self.num_vcpu.unwrap().get(); for ecx in 0..u32::MAX { @@ -314,11 +193,11 @@ impl Specializer { } Ok(()) } - fn fix_cpu_topo(&self, set: &mut Set) -> Result<(), SpecializeError> { + fn fix_cpu_topo(&self, set: &mut CpuidSet) -> Result<(), SpecializeError> { for topo in self.cpu_topo_populate.union(&self.cpu_topo_clear) { // Nuke any existing info in order to potentially override it let leaf = *topo as u32; - set.remove_all(leaf); + set.remove_leaf(leaf); if !self.cpu_topo_populate.contains(topo) { continue; diff --git a/lib/propolis/src/vcpu.rs b/lib/propolis/src/vcpu.rs index b2411da5..21fed44b 100644 --- a/lib/propolis/src/vcpu.rs +++ b/lib/propolis/src/vcpu.rs @@ -4,7 +4,7 @@ //! Virtual CPU functionality. -use std::io::{Error, ErrorKind, Result}; +use std::io::Result; use std::sync::Arc; use crate::common::Lifecycle; @@ -15,7 +15,9 @@ use crate::mmio::MmioBus; use crate::pio::PioBus; use crate::tasks; use crate::vmm::VmmHdl; +use cpuid_utils::{CpuidMapConversionError, CpuidSet}; use migrate::VcpuReadWrite; +use thiserror::Error; use bhyve_api::ApiVersion; use propolis_types::{CpuidIdent, CpuidVendor}; @@ -33,6 +35,18 @@ pub const MAXCPU: usize = bhyve_api::VM_MAXCPU; #[cfg(feature = "omicron-build")] pub const MAXCPU: usize = 64; +#[derive(Debug, Error)] +pub enum GetCpuidError { + #[error("failed to read CPUID values from bhyve")] + ReadIoctlFailed(#[source] std::io::Error), + + #[error("failed to build a map of CPUID entries")] + MapConversion(#[from] CpuidMapConversionError), + + #[error("unsupported CPUID vendor string: {0}")] + UnsupportedVendor(&'static str), +} + /// A handle to a virtual CPU. pub struct Vcpu { hdl: Arc, @@ -147,7 +161,7 @@ impl Vcpu { /// /// If `values` contains no cpuid entries, then legacy emulation handling /// will be used. - pub fn set_cpuid(&self, values: cpuid::Set) -> Result<()> { + pub fn set_cpuid(&self, values: CpuidSet) -> Result<()> { let mut config = bhyve_api::vm_vcpu_cpuid_config { vvcc_vcpuid: self.id, ..Default::default() @@ -158,7 +172,7 @@ impl Vcpu { self.hdl.ioctl(bhyve_api::VM_SET_CPUID, &mut config)?; } } else { - if values.vendor.is_intel() { + if values.vendor().is_intel() { config.vvcc_flags |= bhyve_api::VCC_FLAG_INTEL_FALLBACK; } let mut entries: Vec = values.into(); @@ -175,9 +189,9 @@ impl Vcpu { /// Query the configured (in-kernel) `cpuid` emulation state for this vCPU. /// - /// If legacy cpuid handling is configured, the resulting [Set](cpuid::Set) + /// If legacy cpuid handling is configured, the resulting [Set](CpuidSet) /// will contain no entries. - pub fn get_cpuid(&self) -> Result { + pub fn get_cpuid(&self) -> std::result::Result { let mut config = bhyve_api::vm_vcpu_cpuid_config { vvcc_vcpuid: self.id, vvcc_nent: 0, @@ -199,13 +213,16 @@ impl Vcpu { Ok(0) } Err(e) => Err(e), - }?; + } + .map_err(GetCpuidError::ReadIoctlFailed)?; let mut entries = Vec::with_capacity(count as usize); entries.fill(bhyve_api::vcpu_cpuid_entry::default()); config.vvcc_entries = entries.as_mut_ptr() as *mut libc::c_void; unsafe { - self.hdl.ioctl(bhyve_api::VM_GET_CPUID, &mut config)?; + self.hdl + .ioctl(bhyve_api::VM_GET_CPUID, &mut config) + .map_err(GetCpuidError::ReadIoctlFailed)?; } if config.vvcc_flags & bhyve_api::VCC_FLAG_LEGACY_HANDLING != 0 { @@ -216,28 +233,29 @@ impl Vcpu { let vendor = CpuidVendor::try_from(cpuid_utils::host_query( CpuidIdent::leaf(0), )) - .map_err(|e| Error::new(ErrorKind::InvalidData, e.to_string()))?; + .map_err(GetCpuidError::UnsupportedVendor)?; - return Ok(cpuid::Set::new(vendor)); + return Ok(CpuidSet::new(vendor)); } let intel_fallback = config.vvcc_flags & bhyve_api::VCC_FLAG_INTEL_FALLBACK != 0; - let mut set = cpuid::Set::new(match intel_fallback { + let mut set = CpuidSet::new(match intel_fallback { true => CpuidVendor::Intel, false => CpuidVendor::Amd, }); for entry in entries { let (ident, value) = cpuid::from_raw(entry); - let conflict = set.insert(ident, value); + let conflict = set + .insert(ident, value) + .map_err(CpuidMapConversionError::SubleafConflict)?; + if conflict.is_some() { - return Err(Error::new( - ErrorKind::InvalidData, - format!( - "conflicting entry at eax:{:x} ecx:{:x?})", - ident.leaf, ident.subleaf - ), - )); + return Err(CpuidMapConversionError::DuplicateLeaf( + ident.leaf, + ident.subleaf, + ) + .into()); } } Ok(set) @@ -501,10 +519,10 @@ pub mod migrate { use std::{convert::TryInto, io}; use super::Vcpu; - use crate::cpuid; use crate::migrate::*; use bhyve_api::{vdi_field_entry_v1, vm_reg_name, ApiVersion}; + use cpuid_utils::CpuidSet; use propolis_types::{CpuidIdent, CpuidValues, CpuidVendor}; use serde::{Deserialize, Serialize}; @@ -737,9 +755,9 @@ pub mod migrate { ("bhyve-x86-cpuid", 1) } } - impl From for CpuidV1 { - fn from(value: cpuid::Set) -> Self { - let vendor = value.vendor.into(); + impl From for CpuidV1 { + fn from(value: CpuidSet) -> Self { + let vendor = value.vendor().into(); let entries: Vec<_> = value .iter() .map(|(k, v)| CpuidEntV1 { @@ -751,12 +769,14 @@ pub mod migrate { CpuidV1 { vendor, entries } } } - impl From for cpuid::Set { + impl From for CpuidSet { fn from(value: CpuidV1) -> Self { - let mut set = cpuid::Set::new(value.vendor.into()); + let mut set = CpuidSet::new(value.vendor.into()); for item in value.entries { let (ident, value) = item.into(); - set.insert(ident, value); + set.insert(ident, value).expect( + "well-formed CpuidV1 entries have no subleaf conflicts", + ); } set } @@ -1297,7 +1317,18 @@ pub mod migrate { } impl VcpuReadWrite for CpuidV1 { fn read(vcpu: &Vcpu) -> Result { - Ok(vcpu.get_cpuid()?.into()) + Ok(vcpu + .get_cpuid() + .map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "error reading CPUID for vCPU {}: {}", + vcpu.id, e + ), + ) + })? + .into()) } fn write(self, vcpu: &Vcpu) -> Result<()> { diff --git a/phd-tests/tests/src/cpuid.rs b/phd-tests/tests/src/cpuid.rs index 3cebf473..3ec77376 100644 --- a/phd-tests/tests/src/cpuid.rs +++ b/phd-tests/tests/src/cpuid.rs @@ -58,10 +58,10 @@ async fn cpuid_boot_test(ctx: &Framework) { // host's CPUID leaves, then filter to just a handful of leaves that // advertise features to the guest (and that Linux guests will check for // during boot). - let mut host_cpuid = cpuid_utils::host_query_all().0; + let mut host_cpuid = cpuid_utils::host_query_all(); info!(?host_cpuid, "read host CPUID"); let leaf_0_values = *host_cpuid - .get(&CpuidIdent::leaf(0)) + .get(CpuidIdent::leaf(0)) .expect("host CPUID leaf 0 should always be present"); // Linux guests expect to see at least a couple of leaves in the extended @@ -82,13 +82,12 @@ async fn cpuid_boot_test(ctx: &Framework) { // Keep only standard leaves up to 7 and the first two extended leaves. // Extended leaves 2 through 4 will be overwritten with a fake brand string // (see below). - host_cpuid.retain(|leaf, _values| { - leaf.leaf <= 0x7 - || (leaf.leaf >= 0x8000_0000 && leaf.leaf <= 0x8000_0001) + host_cpuid.retain_by_leaf(|leaf| { + leaf <= 0x7 || (0x8000_0000..=0x8000_0001).contains(&leaf) }); // Report that leaf 7 is the last available standard leaf. - host_cpuid.get_mut(&CpuidIdent::leaf(0)).unwrap().eax = 7; + host_cpuid.get_mut(CpuidIdent::leaf(0)).unwrap().eax = 7; // Mask off bits that a minimum-viable Oxide platform doesn't support or // can't (or won't) expose to guests. See RFD 314 for further discussion of @@ -97,11 +96,11 @@ async fn cpuid_boot_test(ctx: &Framework) { // These are masks (and not assignments) so that, if the host processor // doesn't support a feature contained in an `ALL_FLAGS` value, the // feature will not be enabled in the guest. - let leaf_1 = host_cpuid.get_mut(&CpuidIdent::leaf(1)).unwrap(); + let leaf_1 = host_cpuid.get_mut(CpuidIdent::leaf(1)).unwrap(); leaf_1.ecx &= (Leaf1Ecx::ALL_FLAGS & !(Leaf1Ecx::OSXSAVE | Leaf1Ecx::MONITOR)).bits(); leaf_1.edx &= Leaf1Edx::ALL_FLAGS.bits(); - let leaf_7 = host_cpuid.get_mut(&CpuidIdent::leaf(7)).unwrap(); + let leaf_7 = host_cpuid.get_mut(CpuidIdent::leaf(7)).unwrap(); leaf_7.eax = 0; leaf_7.ebx &= (Leaf7Sub0Ebx::ALL_FLAGS & !(Leaf7Sub0Ebx::PQM @@ -114,10 +113,9 @@ async fn cpuid_boot_test(ctx: &Framework) { // Report that leaf 0x8000_0004 is the last extended leaf and clean up // feature bits in leaf 0x8000_0001. - host_cpuid.get_mut(&CpuidIdent::leaf(0x8000_0000)).unwrap().eax = + host_cpuid.get_mut(CpuidIdent::leaf(0x8000_0000)).unwrap().eax = 0x8000_0004; - let ext_leaf_1 = - host_cpuid.get_mut(&CpuidIdent::leaf(0x8000_0001)).unwrap(); + let ext_leaf_1 = host_cpuid.get_mut(CpuidIdent::leaf(0x8000_0001)).unwrap(); ext_leaf_1.ecx &= (AmdExtLeaf1Ecx::ALT_MOV_CR8 | AmdExtLeaf1Ecx::ABM | AmdExtLeaf1Ecx::SSE4A @@ -147,9 +145,9 @@ async fn cpuid_boot_test(ctx: &Framework) { *dst = u32::from_le_bytes(chunk.try_into().unwrap()); } - host_cpuid.insert(CpuidIdent::leaf(0x8000_0002), ext_leaf_2); - host_cpuid.insert(CpuidIdent::leaf(0x8000_0003), ext_leaf_3); - host_cpuid.insert(CpuidIdent::leaf(0x8000_0004), ext_leaf_4); + host_cpuid.insert(CpuidIdent::leaf(0x8000_0002), ext_leaf_2).unwrap(); + host_cpuid.insert(CpuidIdent::leaf(0x8000_0003), ext_leaf_3).unwrap(); + host_cpuid.insert(CpuidIdent::leaf(0x8000_0004), ext_leaf_4).unwrap(); // Try to boot a guest with the computed CPUID values. The modified brand // string should show up in /proc/cpuinfo.