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

lib: move cpuid::Set to cpuid_utils; prevent semantic subleaf conflicts #782

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

gjcolombo
Copy link
Contributor

@gjcolombo gjcolombo commented Oct 5, 2024

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?).1 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.

Footnotes

  1. In practice the behavior depends on the order in which the conflicting entries appear in the vcpu_cpuid_entry array that Propolis passes to bhyve (the first one to match an incoming CPUID request wins); see bhyve's cpuid_find_entry.

@gjcolombo gjcolombo marked this pull request as ready for review October 6, 2024 06:01
@gjcolombo gjcolombo requested review from hawkw and iximeow October 7, 2024 20:13
@gjcolombo gjcolombo force-pushed the gjcolombo/one-cpuid-map branch 2 times, most recently from f37ff55 to 9a9605c Compare October 8, 2024 20:41
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Very nice! I had a very small suggestions and questions, but nothing big.

crates/cpuid-utils/src/lib.rs Show resolved Hide resolved
crates/cpuid-utils/src/lib.rs Show resolved Hide resolved
crates/cpuid-utils/src/lib.rs Outdated Show resolved Hide resolved
crates/cpuid-utils/src/lib.rs Show resolved Hide resolved
crates/cpuid-utils/src/lib.rs Outdated Show resolved Hide resolved
lib/propolis/src/vcpu.rs Outdated Show resolved Hide resolved
lib/propolis/src/vcpu.rs Outdated Show resolved Hide resolved
crates/cpuid-utils/src/lib.rs Show resolved Hide resolved
Base automatically changed from gjcolombo/plumb-cpuid to master October 8, 2024 23:39
@gjcolombo gjcolombo force-pushed the gjcolombo/one-cpuid-map branch from 9a9605c to 79204d2 Compare October 11, 2024 00:54
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.

i echo the proptest enthusiasm and appreciation for nice docs here 🙏

crates/cpuid-utils/src/lib.rs Show resolved Hide resolved
@gjcolombo
Copy link
Contributor Author

Thanks as always for the reviews! Will merge once CI is green.

@gjcolombo gjcolombo merged commit 93ed767 into master Oct 14, 2024
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/one-cpuid-map branch October 14, 2024 17:13
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.

3 participants