[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <50b2113d-d6a8-ab36-028d-b78c41142c18@intel.com>
Date: Mon, 7 Nov 2022 14:13:52 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Borislav Petkov <bp@...en8.de>, X86 ML <x86@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/cpu: Start documenting what the X86_FEATURE_ flag
testing macros do
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.
Powered by blists - more mailing lists