-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable Arm64 RDM on Windows #109493
base: main
Are you sure you want to change the base?
Enable Arm64 RDM on Windows #109493
Conversation
CC @dotnet/jit-contrib Doesn't look like the bot notified anyone on this one. I couldn't get a clean CI run, but none of the failures are related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me.
I finally found enough documentation to support enabling RDM by implication
@a74nh - can you double check this?
This is a reasonable assumption to make. However, I think you can go a bit further. This change will miss some cases - you'll disable For that reason, I'd recommend you also enable You'll still miss some cases, but I think that's the best you can do given the available outputs in windows. |
Actually, scratch that. For any given feature, it's possible that either now or in the future it is backported to an earlier architecture revision due to a particular customer requirement.
Definitely do not this this! The correct and safe solution here is that the windows API needs expanding to include more features. The full list is much bigger and this issue is only going to occur again for other features. |
In the unlikely event this actually happened and such a device happened to run Windows and someone tried to run a .NET app using RDM on that device, we do still have the
That would be ideal, but with no movement from the Windows side in over 4 years, I don't know what else we can do. This is broken on hardware (including Microsoft-branded) that exists and runs Windows today, so a workaround seems reasonable. The main issue here is that the config knobs can disable an ISA but can't forcibly enable it, meaning there's no way to use the hardware we know is present. |
@jkotas, @kunalspathak. Anyone we can reach out to and ensure such config knobs are exposed? Unlike x64, there is no user-mode instruction (like
This isn't a knob that's really meant for use in production scenarios and certainly not by typical end users of applications. Rather its something that is primarily meant for testing scenarios, so that developers can validate their fallback paths instead. I don't think any scenario where we emit invalid instructions by default for regular code is going to be viable for the shipping product. |
Done (both of you are on cc line) |
If it helps, we might be able to provide some code could be used by the kernel to do the detection of all the features. |
Had an internal conversation that basically boils down to Windows doesn't have any plans to expose a FEAT_RDM flag at this point in time. Rather instead we can encode this essentially as This will exclude Windows 10, but that is going out of support next year in October 2025; prior to when .NET 10 will ship. CC. @jkotas |
More context: Rdm is supported on all Arm hardware supported by Windows 11 today and it is not expected to change in future. |
ef44d3e
to
91ed568
Compare
src/native/minipal/cpufeatures.c
Outdated
@@ -475,6 +503,12 @@ int minipal_getcpufeatures(void) | |||
// FP and SIMD support are enabled by default | |||
result |= ARM64IntrinsicConstants_AdvSimd; | |||
|
|||
// RDM does not have an IsProcessorFeaturePresent flag, but it is a requirement for Windows 11 | |||
if (IsWindows11OrGreater()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use https://github.com/search?q=repo%3Adotnet%2Fruntime%20IsWindowsVersionOrGreate&type=code for this purpose in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsWindowsVersionOrGreater
also lies about Windows version and says 6.2 without a manifest. Using it will return invalid values with custom hosts.
(Do note that in some cases even RtlGetVersion
lies about the Windows version and returns 6.2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsWindowsVersionOrGreater also lies about Windows version and says 6.2 without a manifest. Using it will return invalid values with custom hosts.
This is intentional on the part of Windows and is covered under the Remarks
section of the deprecated GetVersion
/GetVersionEx
functions and the recommended VersionHelper
replacements: https://learn.microsoft.com/en-us/windows/win32/api/versionhelpers/
Applications must be explicitly manifested for Windows 10+, this manifest is likewise required for functions to use newer behaviors, for some (but not all) newer exports to be available, etc.
This is also required to ensure that user-selected comapt settings are respected, among other considerations. Trying to manually determine the actual underlying OS version is never recommended and liable to cause undefined or other problematic behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas, notably the managed OperationgSystem.IsWindowsVersionAtLeast
check looks to use Environment.OSVersion.Version
which is currently populated via RtlGetVersion
This does not correctly respect the manifest file and will mismatch from the underlying GetVersionEx
call and the corresponding IsWindowsVersionOrGreater
calls that are currently recommended for use.
Is that known/intentional? It seems like it should be unintentional and like it could cause various bugs/issues, especially where application behaviors may not match up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that known/intentional? It seems like it should be unintentional and like it could cause various bugs/issues, especially where application behaviors may not match up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should fix the manifests for Windows. I was trying to point out that the Windows developers at large try to avoid the OS version shimming since it does not work well for them.
Once that manifest file exists; then the question of using RtlGetVersion vs IsWindowsVersionOrGreater essentially goes away
It does not go away completely since manifests do not work well for plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not go away completely since manifests do not work well for plugins.
Not sure I understand this?
Plugins and libraries in general are at the mercy of the application as it decides the OS context under which code runs. If the app doesn't include a manifest then IsWindowsVersionOrGreater correctly reports that the OS Context under which it is running and things like RDM or other functionality detailed above may not be available. As far as the app/library/plugin is concerned, unless it intentionally digs deeper, it's running on 6.2 -- Windows 8.
If the app does include a manifest, then the same APIs reports the lower of the greatest listed OS that is supported and the actual underlying OS being run on. This is the general target that the code should consider that it is running against and so everything continues to work.
A plugin or library could "require" that they run on a particular version or later and there isn't a way for them to force an application to use such a context; but they can add a relevant DllMain (C/C++) or Module Initializer/Static Constructor (.NET) which validates this and fails without -- most often, however, its better/easier to just not light up on the optional functionality. -- .NET assemblies also have the option to use SupportedOSPlatform
and/or TargetFramework
(net8.0-windows10.0.22000.0
as an example) to further help enforce such usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the OS compat shims only isolate you from OS-version specific compat problems that were decided to be important enough to be shimmed. Many real-world OS-version specific compat issues are not shimmed and you are on your own. If you have to know the actual OS version to implement OS version specific workaround, you have to be able to get around the version lie shims.
The case study close to home is .NET Framework. Some OS version checks in .NET Framework had to get the real OS version and avoid version lie shims. Things would be broken otherwise. You can see some of that code in the initial .NET Core payload that included .NET Framework specifics:
runtime/src/coreclr/src/vm/ceemain.cpp
Lines 4940 to 4944 in 2024b63
// GetOSVersion - Gets the real OS version bypassing the OS compatibility shim | |
// Mscoree.dll resides in System32 dir and is always excluded from compat shim. | |
// This function calls mscoree!shim function via mscoreei ICLRRuntimeHostInternal interface | |
// to get the OS version. We do not do this PAL or coreclr..we direclty call the OS | |
// in that case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted this back to the original implication and added a big comment.
Given the oldest processor supported on Windows 11 is based on the Arm Cortex-A76 core (and its small core sibling Cortex-A55), which implements both RDM and DP, I don't believe there is any chance of either false positive or false negative in any supported scenario.
I also believe this implication is, for all intents and purposes, identical to the implication that "we are running on Windows 11, therefore we have hardware that meets the minimum requirements for Windows 11" but without the surprising difficulty of answering the question "are we really running on Windows 11?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did one last round of reading and wanted to add one final context note:
There is a single processor model on the Windows 11 support list that might implement RDM but not DP: the Snapdragon 850. That model has Cortex-A75 large cores and Cortex-A55 small cores. I don't have one to test, but docs seem to indicate that the A75 does not implement DP, while the A55 does. More commonly, the A55 appears as the small core alongside the Cortex-A76, which also supports both RDM and DP. The A76/A55 combination is what's in the Surface X 2019 (Microsoft SQ1) model, Snapdragon 7c, and the Broadcom SoC used in Raspberry Pi 5.
Windows 11 does not actually require Armv8.1 until 24H2, according to https://www.neowin.net/news/first-generation-windows-on-arm-pcs-will-not-be-able-to-run-windows-11-24h2/. There are reports that people have been running Windows 11 on unsupported devices with older builds and that 24H2 actually fails to boot.
So the DP->RDM implication includes a possible false negative on one supported processor model, while the Windows11->RDM implication includes possible false positives in unsupported configurations on older builds.
False negatives being preferable to false positives, I believe the implementation here is still right, but I can update the comment if it's worth churning the CI to get the exception noted there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #39094
On Windows, we currently never enable RDM intrinsics because
IsProcessorFeaturePresent
does not have a flag for the Armv8.1 RDM optional extension. This results inRdm.IsSupported
returning different values on Windows vs Linux on the same hardware (realistically, anything newer than 2016).I finally found enough documentation to support enabling RDM by implication. The docs for Armv8.1 extensions include the following:
Since we require a baseline of AdvSIMD support, we may assume that if we have an Armv8.1 processor, it has RDM support. This leads to a second problem, which is that there is also no
IsProcessorFeaturePresent
flag for Armv8.1 baseline. However, from the generic docs for Armv8 extensions:We may assume that the presence of any Armv8.2 optional feature means we have at least Armv8.1. And because
IsProcessorFeaturePresent
does have a flag for the Armv8.2 DP optional extension, we may safely assume that its presence also implies RDM.