Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: read host CPUID values if the spec has none #844

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

gjcolombo
Copy link
Contributor

@gjcolombo gjcolombo commented Jan 28, 2025

The cpuid field in an instance spec's board is optional. If it is None, have propolis-server read bhyve's default CPUID values and insert them into the spec as though the caller had specified them. This has two benefits:

  1. propolis-server can set up CPUID values in the hypervisor leaf range without requiring callers to set CPUID values in the standard and extended ranges.
  2. When a VM migrates, the default CPUID settings from the source will transfer to the target, even if the target host's bhyve supplies different default CPUID values than the source host's.

Tests: cargo test, PHD, ran cpuid in a Debian 11 guest to check that the correct value appears in leaf 0x40000000 (and that other leaves have their expected values).

Fixes #835.

The `cpuid` field in an instance spec's board is optional. If it is
`None`, have propolis-server read bhyve's default CPUID values and
insert them into the spec as though the caller had specified them. This
has two benefits:

1. propolis-server can set up CPUID values in the hypervisor leaf range
   without requiring callers to set CPUID values in the standard and
   extended ranges.
2. When a VM migrates, the default CPUID settings from the source will
   transfer to the target, even if the target host's bhyve supplies
   different default CPUID values than the source host's.
@gjcolombo gjcolombo requested review from pfmooney and iximeow January 28, 2025 19:30
@gjcolombo
Copy link
Contributor Author

When a VM migrates, the default CPUID settings from the source will transfer to the target, even if the target host's bhyve supplies different default CPUID values than the source host's.

@iximeow reminded me in DMs that this property was already ensured by the state export/import logic for vCPUs, so this point rings hollow (and I'll remove it from the final commit description).

Putting realized CPUID values into the spec does still have the advantage that it will appear in VM initialization logs and in the output of the instance GET endpoint, which is a least a little bit nice for observability and setting up reproductions of badly-behaved VMs. But for the most part this change is useful because it provides a path to setting up enlightenment leaves without adding additional "okay but what other CPUID values do you want?" burden to the control plane.

Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, "we already do this for migration" aside, seems like a good change.

we also discussed in DMs that we'll be setting CPUID bits for each vCPU twice in the migration path (once initializing the CPU, then once applying the migrated state), which is all at once: linear in time with the number of vCPUs, probably applying mostly-redundant state on migration, and only different from the status quo in that we'll always have two CPUID sets to apply on migration instead of only sometimes.

i'd expect that to be on the order of single-digit milliseconds for a migration at worst, so it won't be too remarkable right now but.. would be nice to not do. on the other hand, having a bunch of conditional logic for "initialized via migration" and not would be much more unfortunate. so it's not even clear what to do with that if we wanted to.

@gjcolombo gjcolombo merged commit 33a0154 into master Jan 29, 2025
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/default-server-cpuid branch January 29, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

propolis-server should specify explicit CPUID values for all VMs, even if the spec contained none
2 participants