Messages in this thread | | | Date | Mon, 7 Nov 2022 14:13:52 -0800 | Subject | Re: [PATCH] x86/cpu: Start documenting what the X86_FEATURE_ flag testing macros do | From | Dave Hansen <> |
| |
On 11/7/22 13:15, Borislav Petkov wrote: > /* > - * This macro is for detection of features which need kernel > - * infrastructure to be used. It may *not* directly test the CPU > - * itself. Use the cpu_has() family if you want true runtime > - * testing of CPU features, like in hypervisor code where you are > - * supporting a possible guest feature where host support for it > - * is not relevant. > + * This is the preferred macro to use when testing X86_FEATURE_ bits > + * support without the need to test on a particular CPU but rather > + * system-wide. It takes into account build-time disabled feature > + * support too. All those macros mirror the kernel's idea of enabled > + * CPU features and not necessarily what real, hardware CPUID bits are > + * set or clear. For that use tools/arch/x86/kcpuid/ and/or potentially > + * extend if it's feature list is lacking. > */ > #define cpu_feature_enabled(bit) \ > (__builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0 : static_cpu_has(bit))
Thanks for kicking this off! It's sorely needed.
This also makes me wonder if we should update the _static_cpu_has() comment:
* Static testing of CPU features. Used the same as boot_cpu_has(). It * statically patches the target code for additional performance. Use * static_cpu_has() only in fast paths, where every cycle counts. Which * means that the boot_cpu_has() variant is already fast enough for the * majority of cases and you should stick to using it as it is generally * only two instructions: a RIP-relative MOV and a TEST.
It seems to be mildly warning against using _static_cpu_has() indiscriminately. Should we tone that down a bit if we're recommending implicit use of static_cpu_has() via cpu_feature_enabled() everywhere?
I was also thinking that some longer-form stuff in Documentation/ might be a good idea, along with some examples. I'd be happy to follow this up with another patch that added Documentation/ like:
--
New processor features often have specific Kconfig options as well as enumeration in CPUID and/or and X86_FEATURE_* flags. In most cases, the feature must both be compiled in and have processor support, so checks for the feature might take this form:
void enable_foo(void) { if (!IS_ENABLED(CONFIG_X86_FOO)) return; if (!static_cpu_has(X86_FEATURE_FOO)) return;
... do some enabling here }
Or something equivalent with #ifdefs. The preferred form is:
void enable_foo(void) { if (!cpu_feature_enabled(X86_FEATURE_FOO)) return;
... do some enabling here }
plus adding X86_FEATURE_FOO to arch/x86/include/asm/disabled-features.h, like:
#ifdef CONFIG_X86_FOO # define DISABLE_FOO 0 #else # define DISABLE_FOO (1<<(X86_FEATURE_FOO & 31)) #endif
That form has two "hidden" optimizations: 1. Compile-time optimization: If the Kconfig option is disabled, cpu_feature_enabled() will evaluate at compile-time to 0. It can entirely replace an IS_ENABLED() check, or an #ifdef in most cases. 2. The conditional branch will be statically patched out using _static_cpu_has(). This allows the normal runtime code to execute without any conditional branches that might be mispredicted.
| |