-
Notifications
You must be signed in to change notification settings - Fork 290
s390x: z17 target feature detection #1832
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
base: master
Are you sure you want to change the base?
s390x: z17 target feature detection #1832
Conversation
d968db9
to
7bf8a59
Compare
/// These values are part of the platform-specific [asm/elf.h][kernel], and are a selection of the | ||
/// fields found in the [Facility Indications]. | ||
/// | ||
/// [Facility Indications]: https://www.ibm.com/support/pages/sites/default/files/2021-05/SA22-7871-10.pdf#page=63 |
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.
Where did you get the information about the facility bits that will be added in z17? (It is not included in the documentation at this link.)
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 can confirm that the z17 facility bit values are correct. The new version of the Principles of Operation including z17 features will be released shortly after the general availability of the new machine (which is next week).
7bf8a59
to
3809ead
Compare
enable_if_set(133, Feature::guarded_storage); | ||
enable_if_set(150, Feature::enhanced_sort); | ||
enable_if_set(151, Feature::deflate_conversion); | ||
// added in z17 |
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.
Do we need this comment? If so, should we add this information also for the vector-related facilities, and also specific machine information for pre-z17 machines?
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.
nah, this list doesn't really have structure to it, but it should be OK given the official docs.
enable_if_set(151, Feature::deflate_conversion); | ||
// added in z17 | ||
enable_if_set(84, Feature::miscellaneous_extensions_4); | ||
enable_if_set(86, Feature::message_security_assist_extension12); |
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 should really include all currently defined features here. What I see missing is the older variants of miscellaneous_extensions and messact_security_assist_extension. These all have defined facility bits.
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 can only add features here when they are also recognized by the compiler. I'll add them there, but that does not need to block this PR I think
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.
ah, I forgot that we did already add these to the compiler. Now they're in this PR too.
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.
Thanks! Looks good to me now.
3809ead
to
e8c0822
Compare
e8c0822
to
9ab93dc
Compare
tracking issue: rust-lang/rust#130869
I didn't add feature detection yet in #1826.
The implementation follows the information in rust-lang/rust#135413 (comment), in particular that
HWCAP
is used to check for thevector
feature because it needs kernel support, andstfle
is used for the other features because they only need hardware support.cc @uweigand @taiki-e