Repository navigation
mpk: reenable MPK support with vendor string check - #7513
Merged
Merged
Conversation
In bytecodealliance#7446 I disabled MPK support temporarily due to failures in CI runs. Looking into this further in bytecodealliance#7445, I discovered that it is due to how `has_cpuid_bit_set` works on different x86 machines: Intel's `CPUID` instruction reports support for MPK in a certain leaf bit, AMD does it some other (unknown?) way. The CI problem boiled down to occasional runs on AMD machines that would fail with `SIGILL` because the AMD machine reported that it had MPK support when it really did not. This change fixes the issue by first checking if the CPU vendor string is `GenuineIntel` before inspecting the MPK `CPUID` leaf bit. Closes bytecodealliance#7445.
alexcrichton
approved these changes
Nov 9, 2023
abrown
enabled auto-merge
November 9, 2023 18:27
Closed
V0ldek
added a commit
to V0ldek/wasmtime
that referenced
this pull request
Oct 23, 2026
The check for `mpk::is_supported` explicitly checked for GenuineIntel, and then for the PKU bit on the cpuid message. This is misguided for two reasons: 1. Zen3 AMD also supports MPKs but wasmtime misses out on using them; 2. the OSPKE bit also has to be checked to actually be correct. Number 1. seems to have been done as an attempt to avoid CI crashes. In bytecodealliance#7445, bytecodealliance#7446 MPK was disabled and then reenabled for Intel only in PR bytecodealliance#7513. I believe that was actually a red herring and those crashes were caused by the second point. PKU only says if the CPU supports MPKs on the hardware level. However, the OS still needs to enable PKE. If it is not enabled, RDPKRU/WRPKRU raise #UD, which kills the process with SIGILL. An example is a VM that runs on a modern CPU as the hardware, but whose hypervisor has PKU disabled. This would actually fault on *any* vendor, Intel included. You can test it today on the old version by running wasmtime in QEMU with nopku passed (`-append "nopku"`), it will crash with the pooling allocator when it tries to set up PKs. In this commit we switch to checking both PKE and OSPKE, removing the vendor check. This is confirmed to work on modern AMD and Intel, and correctly recognises MPK when ran under QEMU: - with `-cpu host` and default kernel settings (supported), - with `-cpu host` but `nopku` in the kernel (unsupported), - with `-cpu host,-pku` (unsupported). We now have a test in tests/mpk.rs to ensure the result of the check agrees with actual capabilities the process has. Additionally, removed a stale test in `pkru::tests::check_read`, it was ignored since forever and the new mpk test supersedes it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #7446 I disabled MPK support temporarily due to failures in CI runs. Looking into this further in #7445, I discovered that it is due to how
has_cpuid_bit_setworks on different x86 machines: Intel'sCPUIDinstruction reports support for MPK in a certain leaf bit, AMD does it some other (unknown?) way. The CI problem boiled down to occasional runs on AMD machines that would fail withSIGILLbecause the AMD machine reported that it had MPK support when it really did not. This change fixes the issue by first checking if the CPU vendor string isGenuineIntelbefore inspecting the MPKCPUIDleaf bit.Closes #7445.