lkml.org 
[lkml]   [2022]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] x86/cpu: Start documenting what the X86_FEATURE_ flag testing macros do
From
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.

\
 
 \ /
  Last update: 2022-11-07 23:15    [W:0.066 / U:2.572 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site