Re: [PATCH] x86/cpu: Start documenting what the X86_FEATURE_ flag testing macros do

[Dave Hansen <dave.hansen@intel.com>]

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.