Re: [PATCH v3 05/35] x86/bugs: Restructure taa mitigation

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Wed, Jan 08, 2025 at 02:24:45PM -0600, David Kaplan wrote:
> @@ -400,48 +402,71 @@ static void __init taa_select_mitigation(void)
>  		return;
>  	}
>  
> -	if (cpu_mitigations_off()) {
> +	if (cpu_mitigations_off())
>  		taa_mitigation = TAA_MITIGATION_OFF;
> -		return;
> -	}
>  
>  	/*
>  	 * TAA mitigation via VERW is turned off if both
>  	 * tsx_async_abort=off and mds=off are specified.
> +	 *
> +	 * MDS mitigation will be checked in taa_update_mitigation().
>  	 */
> -	if (taa_mitigation == TAA_MITIGATION_OFF &&
> -	    mds_mitigation == MDS_MITIGATION_OFF)
> +	if (taa_mitigation == TAA_MITIGATION_OFF)
>  		return;

This check seems rather pointless, the only thing after this is the
TAA_MITIGATION_AUTO check.

>  
> -	if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> +	/* Microcode will be checked in taa_update_mitigation(). */
> +	if (taa_mitigation == TAA_MITIGATION_AUTO)
>  		taa_mitigation = TAA_MITIGATION_VERW;
> -	else
> -		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;

In the previous patch, MDS checks for ucode in both select and update,
which is overkill.  That should probably be done only in
mds_update_mitigation() to be consistent with how TAA does it here?

>  
> -	/*
> -	 * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and MDS_NO=1.
> -	 * A microcode update fixes this behavior to clear CPU buffers. It also
> -	 * adds support for MSR_IA32_TSX_CTRL which is enumerated by the
> -	 * ARCH_CAP_TSX_CTRL_MSR bit.
> -	 *
> -	 * On MDS_NO=1 CPUs if ARCH_CAP_TSX_CTRL_MSR is not set, microcode
> -	 * update is required.
> -	 */
> -	if ( (x86_arch_cap_msr & ARCH_CAP_MDS_NO) &&
> -	    !(x86_arch_cap_msr & ARCH_CAP_TSX_CTRL_MSR))
> -		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> +}

Extra whitespace here at the end of the function.

> +static void __init taa_update_mitigation(void)
> +{
> +	if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off())
> +		return;
> +
> +	if (verw_mitigation_enabled())
> +		taa_mitigation = TAA_MITIGATION_VERW;

This overwrites TAA_MITIGATION_TSX_DISABLED?

I think reporting TSX disabled here is more accurate than reporting
VERW, since the VERW is only done to mitigate the other vulns.

> +static void __init taa_apply_mitigation(void)
> +{
> +	if (taa_mitigation == TAA_MITIGATION_VERW ||
> +	    taa_mitigation == TAA_MITIGATION_UCODE_NEEDED) {
> +		/*
> +		 * TSX is enabled, select alternate mitigation for TAA which is
> +		 * the same as MDS. Enable MDS static branch to clear CPU buffers.
> +		 *
> +		 * For guests that can't determine whether the correct microcode is
> +		 * present on host, enable the mitigation for UCODE_NEEDED as well.
> +		 */
> +		setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> +
> +		if (taa_nosmt || cpu_mitigations_auto_nosmt())
> +			cpu_smt_disable(false);
> +	}
>  
> -	if (taa_nosmt || cpu_mitigations_auto_nosmt())
> -		cpu_smt_disable(false);
>  }

Another extra whitespace here at the end of the function.

-- 
Josh