Re: [PATCH v6 3/7] x86/cpu, kvm: Move the LFENCE_RDTSC / LFENCE always serializing feature

[Borislav Petkov <bp@alien8.de>]

On Tue, Jan 10, 2023 at 04:46:39PM -0600, Kim Phillips wrote:
> The LFENCE_RDTSC / LFENCE always serializing feature was a scattered bit
> and open-coded for KVM in __do_cpuid_func().  Add it to its newly added
> CPUID leaf 0x80000021 EAX proper, and propagate it in kvm_set_cpu_caps()
> instead.
> 
> Also drop the bit description comments now it's more self-describing.
> 
> Whilst there, switch to using the more efficient cpu_feature_enabled()
> instead of static_cpu_has().
> 
> Signed-off-by: Kim Phillips <kim.phillips@amd.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 3 ++-
>  arch/x86/kvm/cpuid.c               | 9 ++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 0cd7b4afd528..79da8e492c0f 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -97,7 +97,7 @@
>  #define X86_FEATURE_SYSENTER32		( 3*32+15) /* "" sysenter in IA32 userspace */
>  #define X86_FEATURE_REP_GOOD		( 3*32+16) /* REP microcode works well */
>  #define X86_FEATURE_AMD_LBR_V2		( 3*32+17) /* AMD Last Branch Record Extension Version 2 */
> -#define X86_FEATURE_LFENCE_RDTSC	( 3*32+18) /* "" LFENCE synchronizes RDTSC */
> +/* FREE, was #define X86_FEATURE_LFENCE_RDTSC		( 3*32+18) "" LFENCE synchronizes RDTSC */
>  #define X86_FEATURE_ACC_POWER		( 3*32+19) /* AMD Accumulated Power Mechanism */
>  #define X86_FEATURE_NOPL		( 3*32+20) /* The NOPL (0F 1F) instructions */
>  #define X86_FEATURE_ALWAYS		( 3*32+21) /* "" Always-present feature */
> @@ -428,6 +428,7 @@
>  
>  /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
>  #define X86_FEATURE_NO_NESTED_DATA_BP	(20*32+ 0) /* "" AMD No Nested Data Breakpoints */
> +#define X86_FEATURE_LFENCE_RDTSC	(20*32+ 2) /* "" LFENCE always serializing / synchronizes RDTSC */

Hmm, a synthetic bit which gets replaced with a vendor one and then the other
vendors set it too. I don't see why that cannot work but we probably should be
careful here.

dhansen, am I missing an angle?

Also, X86_FEATURE_LFENCE_RDTSC gets set in init_amd() along with setting
DE_CFG[1]. I think you should check the new flag here first and avoid the
setting if that flag is set. Just for good measure - not that it changes
anything but still, it is cheap to do.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette