linux-kernel - Re: [PATCH] x86/tsx: Use MSR_TSX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgqToxbGQluNHABF@zn.tnic>
Date:   Mon, 14 Feb 2022 18:38:43 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>, linux-kernel@...r.kernel.org,
        antonio.gomez.iglesias@...ux.intel.com, neelima.krishnan@...el.com
Subject: Re: [PATCH] x86/tsx: Use MSR_TSX_CTRL to clear CPUID bits

On Wed, Feb 09, 2022 at 01:04:36PM -0800, Pawan Gupta wrote:
> tsx_clear_cpuid() uses MSR_TSX_FORCE_ABORT to clear CPUID.RTM and
> CPUID.HLE. Not all CPUs support MSR_TSX_FORCE_ABORT, alternatively use
> MSR_IA32_TSX_CTRL when supported.
> 
> Fixes: 293649307ef9 ("x86/tsx: Clear CPUID bits when TSX always force aborts")
> Reported-by: kernel test robot <lkp@...el.com>
> Tested-by: Neelima Krishnan <neelima.krishnan@...el.com>
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>

<--- I'm assuming this needs to be

Cc: <stable@...r.kernel.org>

?

> @@ -106,13 +110,11 @@ void __init tsx_init(void)
>  	int ret;
>  
>  	/*
> -	 * Hardware will always abort a TSX transaction if both CPUID bits
> -	 * RTM_ALWAYS_ABORT and TSX_FORCE_ABORT are set. In this case, it is
> -	 * better not to enumerate CPUID.RTM and CPUID.HLE bits. Clear them
> -	 * here.
> +	 * Hardware will always abort a TSX transaction when CPUID
> +	 * RTM_ALWAYS_ABORT is set. In this case, it is better not to enumerate
> +	 * CPUID.RTM and CPUID.HLE bits. Clear them here.
>  	 */
> -	if (boot_cpu_has(X86_FEATURE_RTM_ALWAYS_ABORT) &&
> -	    boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
> +	if (boot_cpu_has(X86_FEATURE_RTM_ALWAYS_ABORT)) {

So you test here X86_FEATURE_RTM_ALWAYS_ABORT and tsx_clear_cpuid()
tests it again. Simplify I guess.

>  		tsx_ctrl_state = TSX_CTRL_RTM_ALWAYS_ABORT;
>  		tsx_clear_cpuid();
>  		setup_clear_cpu_cap(X86_FEATURE_RTM);

Also, here you clear X86_FEATURE_RTM and X86_FEATURE_HLE but the other
caller of tsx_clear_cpuid() - init_intel() - doesn't clear those bits.
Why?

IOW, can we concentrate the whole tsx_clear_cpuid() logic inside it and
have callers only call it unconditionally. Then the decision whether
to disable TSX and clear bits will happen all solely in that function
instead of being spread around the boot code and being inconsistent.

Hmmm?

-- 
Regards/Gruss,
    Boris.

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