kernel: migrate CPUID feature detection to cpufeature crate#1137
kernel: migrate CPUID feature detection to cpufeature crate#1137tanish111 wants to merge 1 commit into
Conversation
|
@luigix25 a test PR. |
bf4f59e to
7c95581
Compare
0e0b532 to
82d5c70
Compare
82d5c70 to
bc3cd74
Compare
luigix25
left a comment
There was a problem hiding this comment.
I think we should we drop cpuid from the SvsmPlatform trait entirely.
We still have code that is calling the "old" cpuid.
Please update the commit message as we are not doing anything related to the shadowstack.
bc3cd74 to
cff75e3
Compare
luigix25
left a comment
There was a problem hiding this comment.
Once again avoid unrelated changes, and split the PR into multiple commits as we said during the call.
Ideally one commit per feature.
One the last commit should drop the cpuid function from SvsmPlatform so that every commit compiles.
|
@luigix25 it's giving clippy errors with this code Can you check it once. |
CI runs Anyway, also if it's the case, it's unrelated so it should be in a separate commit/PR. |
|
@stefano-garzarella sorry clippy was not the culprit it's pre-commit hooks thanks @luigix25 for quick debug. |
f1fa0fe to
ce95c00
Compare
ce95c00 to
ac075bb
Compare
|
@tanish111 please could you explain more clearly in the commit and also in the PR description why we're doing this? (It's not at all clear to me, though I admit I haven't seen the changes; but since I don't understand the purpose of these changes, it's hard for me to follow) |
|
@stefano-garzarella The motivation is to stop maintaining hand-written CPUID checks in SVSM. Right now, each cpu feature is detected with custom leaf/register/bit logic, which is repetitive and easy to get wrong as we add more features. |
|
@tanish111 Thanks, I see now! This is exactly the information that the PR and commit descriptions should include. This makes it easier for reviewers or anyone who looks at the commit in the future to understand the reasoning behind a change. |
7f8446b to
350772c
Compare
|
CPUID leaf 0x4000_0001 (Hyper-V interface signature "Hv#1" in EAX) is defined by the Hyper-V TLFS but is not in x86-cpuid-db only 0x4000_0000 (vendor ID) is. So cpufeature has no generated descriptor for it, and we define it locally in hyperv until it can be added upstream. |
03386f9 to
53974cd
Compare
|
@luigix25 it's ready to review |
I missed all the initial discussion, maybe it got discussed already: |
|
@osteffenrh crate you mentioned only covers ~20 hardcoded crypto/SIMD features as booleans. It still lacks support for arbitrary fields like multi-bit PCID/address-width values and many more. Also it hardcodes raw CPUID execution, which doesn’t work for SNP guests that need to validate/source CPUID via the CPUID page rather than trusting the instruction directly, our CpuidBackend trait makes that pluggable. |
00xc
left a comment
There was a problem hiding this comment.
I don't understand why we need to remove features.rs, since it already contains some of the functions you're introducing with a new name (e.g. cpu_has_feat() -> has_cpuid_feature()). I'd rater have the existing functions updated to use the new backend you're introducing (with the proper CPUID leaf definitions instead of manually inserted ones).
Technically it does work since we handle CPUID in the #VC exception handler, but it's definitely undesirable to take an exception for no good reason. |
|
@00xc is there a specific reason we want to keep features.rs? |
|
Also open to ideas on how we can implement caching without having a central registry that need to be updated every time we want to use a new CPUID. One possible solution is to add cache at query time with a global lookup table. (For the first time it fills look-up table and on next call onwards use it to return cpuid) |
To minimize the overall diff size. The API you're replacing already exists.
I am fine with removing the caching. What I was suggesting is that we can keep the existing public API and only change what's behind that API.
That is all fine, we can rewrite the guts of |
That is what is what the existing code does. For this to work efficiently you need to find a way to generate the lookup table at compile time (that is what |
3ad3108 to
83e117d
Compare
The kernel maintained its own CPUID feature layer: a Feature enum, define_cpu_feats! macro, per-feature CpuFeat cache cells, and scattered leaf/register/bit constants at call sites. Every new feature meant updating the macro table, duplicating CPUID layout knowledge, and keeping that in sync with how each platform executes CPUID (native insn, SNP CPUID table, TDP VM exit). Adopt the shared cpufeature crate, generated from the X86-CPUID database, as the single source of truth for standard feature descriptors. Call sites pass cpufeature::leaves constants directly to cpu_has_feat() and cpu_get_feat() instead of a local Feature enum, so leaf numbers, registers, and bit positions are no longer hand-maintained in the kernel. Platform CPUID goes through CpuidBackend: native and TDP use the default backend; SNP routes architectural leaves through the CPUID table and hypervisor leaves through GHCB. platform::cpuid() takes a CpuidFeature descriptor, so feature checks and platform dispatch share one path. features.rs keeps only leaves outside the database (HYPERV_INTERFACE, PHYS_ADDR_SIZES). This removes about 110 lines of custom detection code, eliminates the macro-generatedd lookup table, and simplifies future feature additions by relying on external cpufeature crate instead of extending another local table. Signed-off-by: tanish111 <tanishdesai37@gmail.com>
83e117d to
dac11ce
Compare
|
@00xc I've updated the PR to keep the existing public API (cpu_has_feat / cpu_get_feat), but callers still need to change because the signature now takes &CpuidFeature constants from the crate instead of Feature. (The caller updates are mostly import + Feature::Foo → &X86_FEATURE_FOO; the behavioral logic stays the same.) An alternative would be to keep define_cpu_feats! and the Feature enum, with each variant mapping to a cpufeature constant internally that would avoid touching call sites. I don't think that's worth it: we'd be bookkeeping the same CPUID leaf/register/bit data twice, and readers would have to mentally map two names (Feature::Xsave vs X86_FEATURE_XSAVE) for the same thing. Passing the crate constants directly keeps a single source of truth. wdyt? Also PR is ready for review can you ptal? |
| if core::ptr::eq(feature, &HYPERV_INTERFACE) { | ||
| cpuid_field(feature) == Some(HYPERV_INTERFACE_SIGNATURE) | ||
| } else { | ||
| cpuid_field(feature) == Some(1) |
There was a problem hiding this comment.
Or maybe we can consider the feature present if it is Some and not zero.
cpuid_field(feature).is_some_and(|f| f != 0)There was a problem hiding this comment.
Yes this works as well
| use cpufeature::leaves::XSAVE_SZ; | ||
| use cpufeature::leaves::{ | ||
| X86_FEATURE_XMM, X86_FEATURE_XSAVE, X86_FEATURE_XSAVEOPT, XCR0_AVX, XCR0_SSE, XCR0_X87, | ||
| }; |
The kernel maintained its own CPUID feature layer: a Feature enum,
define_cpu_feats! macro, per-feature CpuFeat cache cells, and
scattered leaf/register/bit constants at call sites. Every new
feature meant updating the macro table, duplicating CPUID layout
knowledge, and keeping that in sync with how each platform
executes CPUID (native insn, SNP CPUID table, TDP VM exit).
Adopt the shared cpufeature crate, generated from the X86-CPUID
database, as the single source of truth for standard feature
descriptors. Call sites pass cpufeature::leaves constants directly
to cpu_has_feat() and cpu_get_feat() instead of a local Feature
enum, so leaf numbers, registers, and bit positions are no longer
hand-maintained in the kernel.
Platform CPUID goes through CpuidBackend: native and TDP use the
default backend; SNP routes architectural leaves through the CPUID
table and hypervisor leaves through GHCB. platform::cpuid() takes a
CpuidFeature descriptor, so feature checks and platform dispatch
share one path. features.rs keeps only leaves outside the database
(HYPERV_INTERFACE, PHYS_ADDR_SIZES).
This removes about 110 lines of custom detection code, eliminates
the macro-generatedd lookup table, and simplifies future feature
additions by relying on external cpufeature crate instead
of extending another local table.
cpufeature crate:- https://github.com/coconut-svsm/cpufeature