Skip to content

Commit 1061b1e

Browse files
authored
lib: don't send CPUID settings in vCPU device state payload (#848)
Remove the CPUID portion of the vCPU device state payload that gets sent during live migration. This payload is not currently constructed correctly due to a bug, but there's no need to fix it directly: a migration target's received instance spec will always contain a set of explicitly-applied CPUID settings from the source, even if the "original" instance spec that was used to start the VM contained no explicit CPUID values. This means there's no longer any need to transfer these settings in the device state phase. Upgrade PHD's cpuid_boot_test to detect this issue and verify the fix.
1 parent c0bb4cd commit 1061b1e

File tree

2 files changed

+77
-158
lines changed

2 files changed

+77
-158
lines changed

lib/propolis/src/vcpu.rs

Lines changed: 0 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -557,7 +557,6 @@ impl MigrateMulti for Vcpu {
557557
output.push(migrate::VcpuMsrsV1::read(self)?.into())?;
558558
output.push(migrate::FpuStateV1::read(self)?.into())?;
559559
output.push(migrate::LapicV1::read(self)?.into())?;
560-
output.push(migrate::CpuidV1::read(self)?.into())?;
561560

562561
// PMU was introduced in V18
563562
if bhyve_api::api_version()? >= ApiVersion::V18
@@ -582,7 +581,6 @@ impl MigrateMulti for Vcpu {
582581
let ms_regs: migrate::VcpuMsrsV1 = offer.take()?;
583582
let fpu: migrate::FpuStateV1 = offer.take()?;
584583
let lapic: migrate::LapicV1 = offer.take()?;
585-
let cpuid: migrate::CpuidV1 = offer.take()?;
586584

587585
run_state.write(self)?;
588586
gp_regs.write(self)?;
@@ -592,7 +590,6 @@ impl MigrateMulti for Vcpu {
592590
ms_regs.write(self)?;
593591
fpu.write(self)?;
594592
lapic.write(self)?;
595-
cpuid.write(self)?;
596593

597594
if let Ok(pmu_amd) = offer.take::<migrate::PmuAmdV1>() {
598595
pmu_amd.write(self)?;
@@ -610,8 +607,6 @@ pub mod migrate {
610607
use crate::migrate::*;
611608

612609
use bhyve_api::{vdi_field_entry_v1, vm_reg_name, ApiVersion};
613-
use cpuid_utils::CpuidSet;
614-
use propolis_types::{CpuidIdent, CpuidValues, CpuidVendor};
615610
use serde::{Deserialize, Serialize};
616611

617612
pub(super) trait VcpuReadWrite: Sized {
@@ -790,86 +785,6 @@ pub mod migrate {
790785
pub dcr_timer: u32,
791786
}
792787

793-
#[derive(Copy, Clone, Default, Deserialize, Serialize)]
794-
pub struct CpuidEntV1 {
795-
pub func: u32,
796-
pub idx: Option<u32>,
797-
pub data: [u32; 4],
798-
}
799-
impl From<CpuidEntV1> for (CpuidIdent, CpuidValues) {
800-
fn from(value: CpuidEntV1) -> Self {
801-
(
802-
CpuidIdent { leaf: value.func, subleaf: value.idx },
803-
CpuidValues {
804-
eax: value.data[0],
805-
ebx: value.data[1],
806-
ecx: value.data[2],
807-
edx: value.data[3],
808-
},
809-
)
810-
}
811-
}
812-
813-
#[derive(Copy, Clone, Deserialize, Serialize)]
814-
#[serde(rename_all = "lowercase")]
815-
pub enum CpuidVendorV1 {
816-
Amd,
817-
Intel,
818-
}
819-
impl From<CpuidVendor> for CpuidVendorV1 {
820-
fn from(value: CpuidVendor) -> Self {
821-
match value {
822-
CpuidVendor::Amd => Self::Amd,
823-
CpuidVendor::Intel => Self::Intel,
824-
}
825-
}
826-
}
827-
impl From<CpuidVendorV1> for CpuidVendor {
828-
fn from(value: CpuidVendorV1) -> Self {
829-
match value {
830-
CpuidVendorV1::Amd => Self::Amd,
831-
CpuidVendorV1::Intel => Self::Intel,
832-
}
833-
}
834-
}
835-
836-
#[derive(Clone, Deserialize, Serialize)]
837-
pub struct CpuidV1 {
838-
pub vendor: CpuidVendorV1,
839-
pub entries: Vec<CpuidEntV1>,
840-
}
841-
impl Schema<'_> for CpuidV1 {
842-
fn id() -> SchemaId {
843-
("bhyve-x86-cpuid", 1)
844-
}
845-
}
846-
impl From<CpuidSet> for CpuidV1 {
847-
fn from(value: CpuidSet) -> Self {
848-
let vendor = value.vendor().into();
849-
let entries: Vec<_> = value
850-
.iter()
851-
.map(|(k, v)| CpuidEntV1 {
852-
func: k.leaf,
853-
idx: k.subleaf,
854-
data: [v.eax, v.ebx, v.ecx, v.edx],
855-
})
856-
.collect();
857-
CpuidV1 { vendor, entries }
858-
}
859-
}
860-
impl From<CpuidV1> for CpuidSet {
861-
fn from(value: CpuidV1) -> Self {
862-
let mut set = CpuidSet::new(value.vendor.into());
863-
for item in value.entries {
864-
let (ident, value) = item.into();
865-
set.insert(ident, value).expect(
866-
"well-formed CpuidV1 entries have no subleaf conflicts",
867-
);
868-
}
869-
set
870-
}
871-
}
872-
873788
#[derive(Clone, Deserialize, Serialize)]
874789
pub struct PmuAmdV1 {
875790
pub evtsel: [u64; 6],
@@ -1427,26 +1342,6 @@ pub mod migrate {
14271342
Ok(())
14281343
}
14291344
}
1430-
impl VcpuReadWrite for CpuidV1 {
1431-
fn read(vcpu: &Vcpu) -> Result<Self> {
1432-
Ok(vcpu
1433-
.get_cpuid()
1434-
.map_err(|e| {
1435-
std::io::Error::new(
1436-
std::io::ErrorKind::InvalidData,
1437-
format!(
1438-
"error reading CPUID for vCPU {}: {}",
1439-
vcpu.id, e
1440-
),
1441-
)
1442-
})?
1443-
.into())
1444-
}
1445-
1446-
fn write(self, vcpu: &Vcpu) -> Result<()> {
1447-
vcpu.set_cpuid(self.into())
1448-
}
1449-
}
14501345
impl VcpuReadWrite for PmuAmdV1 {
14511346
fn read(vcpu: &Vcpu) -> Result<Self> {
14521347
let vdi = vcpu

phd-tests/tests/src/cpuid.rs

Lines changed: 77 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@
22
// License, v. 2.0. If a copy of the MPL was not distributed with this
33
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
44

5-
use cpuid_utils::{CpuidIdent, CpuidValues, CpuidVendor};
5+
use cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues};
6+
use phd_framework::{test_vm::MigrationTimeout, TestVm};
67
use phd_testcase::*;
78
use propolis_client::types::{
89
CpuidEntry, InstanceSpecStatus, VersionedInstanceSpec,
910
};
1011
use tracing::info;
12+
use uuid::Uuid;
1113

1214
fn cpuid_entry(
1315
leaf: u32,
@@ -47,44 +49,24 @@ async fn cpuid_instance_spec_round_trip_test(ctx: &Framework) {
4749
itertools::assert_equal(cpuid.entries, entries);
4850
}
4951

50-
#[phd_testcase]
51-
async fn cpuid_boot_test(ctx: &Framework) {
52-
use cpuid_utils::bits::*;
53-
54-
// This test verifies that plumbing a reasonably sensible-looking set of
55-
// CPUID values into the guest produces a bootable guest.
56-
//
57-
// The definition of "reasonably sensible" is derived from the suggested
58-
// virtual-Milan CPUID definitions in RFD 314. Those are in turn derived
59-
// from reading AMD's manuals and bhyve's source code to figure out what
60-
// host CPU features to hide from guests.
61-
//
62-
// To try to make this test at least somewhat host-agnostic, read all of the
63-
// host's CPUID leaves, then filter to just a handful of leaves that
64-
// advertise features to the guest (and that Linux guests will check for
65-
// during boot).
66-
let mut host_cpuid = cpuid_utils::host::query_complete(
67-
cpuid_utils::host::CpuidSource::BhyveDefault,
68-
)?;
69-
70-
info!(?host_cpuid, "read bhyve default CPUID");
71-
72-
// Linux guests expect to see at least a couple of leaves in the extended
73-
// CPUID range. These have vendor-specific meanings. This test only encodes
74-
// AMD's definitions, so skip the test if leaf 0 reports any other vendor.
75-
if host_cpuid.vendor() != CpuidVendor::Amd {
76-
phd_skip!("cpuid_boot_test can only run on AMD hosts");
77-
}
78-
79-
// This test works by injecting a fake brand string into extended leaves
80-
// 0x8000_0002-0x8000_0004 and seeing if the guest observes that string.
81-
//
52+
/// A synthetic brand string that can be injected into guest CPUID leaves
53+
/// 0x8000_0002-0x8000_0004.
54+
const BRAND_STRING: &[u8; 48] =
55+
b"Oxide Cloud Computer Company Cloud Computer\0\0\0\0\0";
56+
57+
/// Injects a fake CPU brand string into CPUID leaves 0x8000_0002-0x8000_0004.
58+
///
59+
/// # Panics
60+
///
61+
/// Panics if the input CPUID set does not include the brand string leaves.
62+
fn inject_brand_string(cpuid: &mut CpuidSet) {
8263
// The brand string leaves have been defined for long enough that they
8364
// should be present on virtually any host that's modern enough to run
84-
// Propolis and PHD. Fail the test (instead of skipping) if they're missing,
85-
// since that may indicate a latent bug in the `cpuid_utils` crate.
86-
let ext_leaf_0 = host_cpuid
87-
.get(CpuidIdent::leaf(EXTENDED_BASE_LEAF))
65+
// Propolis and PHD. Assert (instead of returning a "skipped" result) if
66+
// they're missing, since that may indicate a latent bug in the
67+
// `cpuid_utils` crate.
68+
let ext_leaf_0 = cpuid
69+
.get(CpuidIdent::leaf(cpuid_utils::bits::EXTENDED_BASE_LEAF))
8870
.expect("PHD-capable processors should have some extended leaves");
8971

9072
assert!(
@@ -94,11 +76,6 @@ async fn cpuid_boot_test(ctx: &Framework) {
9476
ext_leaf_0.eax
9577
);
9678

97-
// Reprogram the brand string leaves and see if the new string shows up in
98-
// the guest.
99-
const BRAND_STRING: &[u8; 48] =
100-
b"Oxide Cloud Computer Company Cloud Computer\0\0\0\0\0";
101-
10279
let chunks = BRAND_STRING.chunks_exact(4);
10380
let mut ext_leaf_2 = CpuidValues::default();
10481
let mut ext_leaf_3 = CpuidValues::default();
@@ -112,13 +89,38 @@ async fn cpuid_boot_test(ctx: &Framework) {
11289
*dst = u32::from_le_bytes(chunk.try_into().unwrap());
11390
}
11491

115-
host_cpuid.insert(CpuidIdent::leaf(0x8000_0002), ext_leaf_2).unwrap();
116-
host_cpuid.insert(CpuidIdent::leaf(0x8000_0003), ext_leaf_3).unwrap();
117-
host_cpuid.insert(CpuidIdent::leaf(0x8000_0004), ext_leaf_4).unwrap();
92+
cpuid.insert(CpuidIdent::leaf(0x8000_0002), ext_leaf_2).unwrap();
93+
cpuid.insert(CpuidIdent::leaf(0x8000_0003), ext_leaf_3).unwrap();
94+
cpuid.insert(CpuidIdent::leaf(0x8000_0004), ext_leaf_4).unwrap();
95+
}
96+
97+
/// Asserts that `/proc/cpuinfo` in the guest returns output that contains
98+
/// [`BRAND_STRING`].
99+
async fn verify_guest_brand_string(vm: &TestVm) -> anyhow::Result<()> {
100+
let cpuinfo = vm.run_shell_command("cat /proc/cpuinfo").await?;
101+
info!(cpuinfo, "/proc/cpuinfo output");
102+
assert!(cpuinfo.contains(
103+
std::str::from_utf8(BRAND_STRING).unwrap().trim_matches('\0')
104+
));
105+
106+
Ok(())
107+
}
118108

119-
// Try to boot a guest with the computed CPUID values. The modified brand
120-
// string should show up in /proc/cpuinfo.
121-
let mut cfg = ctx.vm_config_builder("cpuid_boot_test");
109+
/// Launches a test VM with a synthetic brand string injected into its CPUID
110+
/// leaves.
111+
async fn launch_cpuid_smoke_test_vm(
112+
ctx: &Framework,
113+
vm_name: &str,
114+
) -> anyhow::Result<TestVm> {
115+
let mut host_cpuid = cpuid_utils::host::query_complete(
116+
cpuid_utils::host::CpuidSource::BhyveDefault,
117+
)?;
118+
119+
info!(?host_cpuid, "read bhyve default CPUID");
120+
121+
inject_brand_string(&mut host_cpuid);
122+
123+
let mut cfg = ctx.vm_config_builder(vm_name);
122124
cfg.cpuid(
123125
host_cpuid
124126
.iter()
@@ -136,9 +138,31 @@ async fn cpuid_boot_test(ctx: &Framework) {
136138
vm.launch().await?;
137139
vm.wait_to_boot().await?;
138140

139-
let cpuinfo = vm.run_shell_command("cat /proc/cpuinfo").await?;
140-
info!(cpuinfo, "/proc/cpuinfo output");
141-
assert!(cpuinfo.contains(
142-
std::str::from_utf8(BRAND_STRING).unwrap().trim_matches('\0')
143-
));
141+
Ok(vm)
142+
}
143+
144+
#[phd_testcase]
145+
async fn cpuid_boot_test(ctx: &Framework) {
146+
let vm = launch_cpuid_smoke_test_vm(ctx, "cpuid_boot_test").await?;
147+
verify_guest_brand_string(&vm).await?;
148+
}
149+
150+
#[phd_testcase]
151+
async fn cpuid_migrate_smoke_test(ctx: &Framework) {
152+
let vm = launch_cpuid_smoke_test_vm(ctx, "cpuid_boot_test").await?;
153+
verify_guest_brand_string(&vm).await?;
154+
155+
// Migrate the VM and make sure the brand string setting persists.
156+
let mut target = ctx
157+
.spawn_successor_vm("cpuid_boot_test_migration_target", &vm, None)
158+
.await?;
159+
160+
target
161+
.migrate_from(&vm, Uuid::new_v4(), MigrationTimeout::default())
162+
.await?;
163+
164+
// Reset the target to force it to reread its CPU information.
165+
target.reset().await?;
166+
target.wait_to_boot().await?;
167+
verify_guest_brand_string(&target).await?;
144168
}

0 commit comments

Comments
 (0)