[MODERATED] Re: [PATCH v7 04/10] TAAv7 4

On Mon, Oct 21, 2019 at 01:26:02PM -0700, speck for Pawan Gupta wrote:
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 6e0a3b43d027..999b85039128 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -988,4 +988,11 @@ enum mds_mitigations {
>  	MDS_MITIGATION_VMWERV,
>  };
>  
> +enum taa_mitigations {
> +	TAA_MITIGATION_OFF,
> +	TAA_MITIGATION_UCODE_NEEDED,
> +	TAA_MITIGATION_VERW,
> +	TAA_MITIGATION_TSX_DISABLE,

I would rename "TSX_DISABLE" to "TSX_DISABLED", because

 a) it matches the verb tense of "UCODE_NEEDED", and
 b) the past tense hopefully helps make it slightly clearer that TSX was
    disabled separately, not as part of the mitigation code itself.

> @@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
>  #undef pr_fmt
>  #define pr_fmt(fmt) fmt
>  
> -/* Update the static key controlling the MDS CPU buffer clear in idle */
> +/* Update the static key controlling the MDS and TAA CPU buffer clear in idle */
>  static void update_mds_branch_idle(void)
>  {
>  	/*
> @@ -775,8 +881,11 @@ static void update_mds_branch_idle(void)
>  	 * The other variants cannot be mitigated when SMT is enabled, so
>  	 * clearing the buffers on idle just to prevent the Store Buffer
>  	 * repartitioning leak would be a window dressing exercise.
> +	 *
> +	 * Apply idle buffer clearing to TAA affected CPUs also.
>  	 */
> -	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY))
> +	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY) &&
> +	    !boot_cpu_has_bug(X86_BUG_TAA))
>  		return;

Sorry, I was out for most of the last two weeks, so I think I left you
hanging on this.  For context, here's your answer to my previous
question about whether the X86_BUG_TAA check makes sense here:

> It does provide protection against the "store buffer" leak. But the
> other buffers(fill buffer and load port) are still SMT vulnerable. Do
> you prefer it removed?

Yes, please remove it, for the same reason we used for MDS.  There's not
much point in partially mitigating TAA here.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 885d4ac2111a..86f22c1e5912 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
>  	if (!cpu_matches(NO_SWAPGS))
>  		setup_force_cpu_bug(X86_BUG_SWAPGS);
>  
> +	/*
> +	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
> +	 *	- TSX is supported or
> +	 *	- TSX_CTRL is supported
> +	 *
> +	 * TSX_CTRL check is needed for cases when TSX could be disabled before
> +	 * the kernel boot e.g. kexec
> +	 * TSX_CTRL check alone is not sufficient for cases when the microcode
> +	 * update is not present or running as guest that don't get TSX_CTRL.
> +	 */
> +	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
> +	    (boot_cpu_has(X86_FEATURE_RTM) ||
> +	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
> +		setup_force_cpu_bug(X86_BUG_TAA);
> +

I'm finding this logic to be less than 100% convincing, or at least hard
to follow.  And it's different from most of the other bug checks in this
function.

Would the following work instead?

	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);

In other words I would presume all the NO_MDS CPUs listed in
'cpu_vuln_whitelist' are also immune to TAA.

-- 
Josh