[X86][APX] Update the CPUID check logics for APX.#124624
[X86][APX] Update the CPUID check logics for APX.#124624Ruihan-Yin wants to merge 2 commits intodotnet:mainfrom
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
Updates x86/x64 APX feature detection in minipal_getcpufeatures to incorporate the new APX-related CPUID bit from the updated Intel APX documentation.
Changes:
- Renames the existing APX CPUID bit comment to
APX_Fand adds a follow-up CPUID query (leaf0x29) forAPX_NCI_NDD_NF. - Requires both
APX_FandCPUID.(EAX=29H,ECX=0):EBX[0]to be present before settingXArchIntrinsicConstants_Apx. - Re-issues
CPUID.(EAX=07H,ECX=1H)after the new leaf0x29query to restore state for subsequent checks.
| // As documented in the "Intel® Advanced Performance Extensions (Intel® APX) Software Developer’s Manual" revision 7.0 | ||
| // APX_F (CPUID.(EAX=07H,ECX=1H):EDX[bit 21]) indicates the basic support for APX, and | ||
| // APX_NCI_NDD_NF (CPUID.(EAX=29H,ECX=0H):EBX[bit 0]) indicates support for the NCI/NDD/NF features. | ||
| // As of now, both bits are required to turn on APX in JIT. | ||
| result |= XArchIntrinsicConstants_Apx; |
There was a problem hiding this comment.
The new logic only sets XArchIntrinsicConstants_Apx when both APX_F and CPUID.(EAX=29H,ECX=0H):EBX[0] are set, and the comment says “both bits are required to turn on APX in JIT.” This appears to contradict the PR description stating JIT behavior should not depend on the presence of the new bit (and that APX_F is sufficient). Either update the implementation to preserve the APX_F-only behavior (treating the new bit as informational/for future gating), or update the PR description (and any callers’ expectations) to reflect the new requirement.
src/native/minipal/cpufeatures.c
Outdated
| if ((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) // Apx_NCI_NDD_NF | ||
| { | ||
| // As documented in the "Intel® Advanced Performance Extensions (Intel® APX) Software Developer’s Manual" revision 7.0 | ||
| // APX_F (CPUID.(EAX=07H,ECX=1H):EDX[bit 21]) indicates the basic support for APX, and |
There was a problem hiding this comment.
There’s trailing whitespace at the end of the // ... basic support for APX, and comment line. Please remove the trailing space to avoid whitespace-only diffs and potential formatting/style checks.
| // APX_F (CPUID.(EAX=07H,ECX=1H):EDX[bit 21]) indicates the basic support for APX, and | |
| // APX_F (CPUID.(EAX=07H,ECX=1H):EDX[bit 21]) indicates the basic support for APX, and |
| __cpuidex(cpuidInfo, 0x00000007, 0x00000001); | ||
|
|
There was a problem hiding this comment.
The extra __cpuidex(cpuidInfo, 0x00000007, 0x00000001) here is only needed because the APX leaf 0x29 query reuses/clobbers cpuidInfo. Consider using a separate local buffer for CPUID(0x29,0) (or saving/restoring the leaf 7 values) to avoid the additional CPUID call and make the flow less brittle.
src/native/minipal/cpufeatures.c
Outdated
| if (IsApxEnabled() && apxStateSupport()) | ||
| { | ||
| if ((cpuidInfo[CPUID_EDX] & (1 << 21)) != 0) // Apx | ||
| if ((cpuidInfo[CPUID_EDX] & (1 << 21)) != 0) // Apx_F |
There was a problem hiding this comment.
__cpuidex(cpuidInfo, 0x00000029, 0x00000000) is executed without checking maxCpuId >= 0x29. The rest of this function consistently guards CPUID leaves (e.g., 0x07 and 0x24), so this can read an unsupported leaf (undefined results) and may incorrectly disable/enable APX under CPUID filtering/virtualization. Add a maxCpuId gate for leaf 0x29 (and decide what the fallback behavior should be when the leaf isn’t present).
| if ((cpuidInfo[CPUID_EDX] & (1 << 21)) != 0) // Apx_F | |
| if (((cpuidInfo[CPUID_EDX] & (1 << 21)) != 0) && // Apx_F | |
| (maxCpuId >= 0x00000029)) |
|
Build Analysis is green, PR is ready for review. |
| if (((cpuidInfo[CPUID_EDX] & (1 << 21)) != 0) && maxCpuId >= 0x29) // Apx_F | ||
| { | ||
| result |= XArchIntrinsicConstants_Apx; | ||
| __cpuidex(cpuidInfo, 0x00000029, 0x00000000); | ||
| if ((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) // Apx_NCI_NDD_NF | ||
| { |
There was a problem hiding this comment.
This changes APX enablement semantics: XArchIntrinsicConstants_Apx is now only set when both APX_F is present and CPUID leaf 0x29 bit EBX[0] is set (and maxCpuId >= 0x29). The PR description says the new bit is informational and should not change JIT behavior based on its presence; if that’s still the intent, consider continuing to set Apx based on APX_F + OS state support and using the new leaf/bit only to record/validate the extra capability (or guard it behind a separate flag).
|
Failures look irrelevant Hi @kg @tannergooding , could you please take a look at the PR :) |
This PR is to update the new APX CPUID bits as documented in the specification:
This PR is to introduce the new CPUID bit only, and it is not intended to change the JIT behavior based on the presence of the new bit. (
APX_Fbit will guarantee the newAPX_NDD_NCI_NFbit on Intel processors as documented above). All the features and optimizations will be available as long asAPX_Fbit is set.