Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 14 Feb 2022 18:38:43 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@linux.intel.com>,
        Tony Luck <tony.luck@intel.com>, linux-kernel@vger.kernel.org,
        antonio.gomez.iglesias@linux.intel.com, neelima.krishnan@intel.com
Subject: Re: [PATCH] x86/tsx: Use MSR_TSX_CTRL to clear CPUID bits
Message-ID: <YgqToxbGQluNHABF@zn.tnic>
References: <5bd785a1d6ea0b572250add0c6617b4504bc24d1.1644440311.git.pawan.kumar.gupta@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <5bd785a1d6ea0b572250add0c6617b4504bc24d1.1644440311.git.pawan.kumar.gupta@linux.intel.com>
Precedence: bulk

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@intel.com>
> Tested-by: Neelima Krishnan <neelima.krishnan@intel.com>
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>

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

Cc: <stable@vger.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