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

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

On Tue, Oct 22, 2019 at 06:51:12PM +0200, speck for Borislav Petkov wrote:
> On Mon, Oct 21, 2019 at 01:26:02PM -0700, speck for Pawan Gupta wrote:
> > @@ -268,6 +270,110 @@ static int __init mds_cmdline(char *str)
> >  }
> >  early_param("mds", mds_cmdline);
> >  
> > +#undef pr_fmt
> > +#define pr_fmt(fmt)	"TAA: " fmt
> > +
> > +/* Default mitigation for TAA-affected CPUs */
> > +static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW;
> > +static bool taa_nosmt __ro_after_init;
> > +
> > +static const char * const taa_strings[] = {
> > +	[TAA_MITIGATION_OFF]		= "Vulnerable",
> > +	[TAA_MITIGATION_UCODE_NEEDED]	= "Vulnerable: Clear CPU buffers attempted, no microcode",
> > +	[TAA_MITIGATION_VERW]		= "Mitigation: Clear CPU buffers",
> > +	[TAA_MITIGATION_TSX_DISABLE]	= "Mitigation: TSX disabled",
> > +};
> > +
> > +static void __init taa_select_mitigation(void)
> > +{
> > +	u64 ia32_cap = x86_read_arch_cap_msr();
> 
> Do that MSR access...
> 
> > +
> > +	if (!boot_cpu_has_bug(X86_BUG_TAA)) {
> > +		taa_mitigation = TAA_MITIGATION_OFF;
> > +		return;
> > +	}
> 
> ... here.

ok.

> > +
> > +	/*
> > +	 * As X86_BUG_TAA=1, TSX feature is supported by the hardware. If
> > +	 * TSX was disabled (X86_FEATURE_RTM=0) earlier during tsx_init().
> 
> That sentence is having trouble saying whatever it is trying to say. :)

I will fix it.


> > +	if (!boot_cpu_has(X86_FEATURE_RTM)) {
> > +		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
> > +		pr_info("%s\n", taa_strings[taa_mitigation]);
> > +		return;
> > +	}
> > +
> > +	/* All mitigations turned off from cmdline (mitigations=off) */
> > +	if (cpu_mitigations_off()) {
> > +		taa_mitigation = TAA_MITIGATION_OFF;
> > +		return;
> > +	}
> > +
> > +	/* TAA mitigation is turned off from cmdline (tsx_async_abort=off) */
> > +	if (taa_mitigation == TAA_MITIGATION_OFF) {
> 
> 		goto out;
> 
> and you slap an out label above the pr_info at the end of this function.

> 
> To show better what I mean, here's the whole function fixed up:
> 
> static void __init taa_select_mitigation(void)
> {
> 	u64 ia32_cap;
> 
> 	if (!boot_cpu_has(X86_FEATURE_RTM)) {
> 		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
> 		goto out;
> 	}

There is a small problem with this, it will set

	taa_mitigation = TAA_MITIGATION_TSX_DISABLE;

when there is no X86_BUG_TAA.

> 
> 	if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off()) {
> 		taa_mitigation = TAA_MITIGATION_OFF;
> 		goto out;
> 	}

goto will cause it to differ from other mitigations that don't print
individual mitigation status when mitigations are turned off globally
(when cpu_mitigations_off() is true).

> > +
> > +static int __init tsx_async_abort_cmdline(char *str)
> 
> That function name needs a verb: tsx_async_abort_parse_cmdline()

Ok.

> 
> > +{
> > +	if (!boot_cpu_has_bug(X86_BUG_TAA))
> > +		return 0;
> > +
> > +	if (!str)
> > +		return -EINVAL;
> > +
> > +	if (!strcmp(str, "off")) {
> > +		taa_mitigation = TAA_MITIGATION_OFF;
> > +	} else if (!strcmp(str, "full")) {
> > +		taa_mitigation = TAA_MITIGATION_VERW;
> > +	} else if (!strcmp(str, "full,nosmt")) {
> > +		taa_mitigation = TAA_MITIGATION_VERW;
> > +		taa_nosmt = true;
> > +	}
> > +
> > +	return 0;
> > +}
> > +early_param("tsx_async_abort", tsx_async_abort_cmdline);
> 
> Say what now?!
> 
> The previous patch added this:
> 
>         tsx=            [X86] Control Transactional Synchronization
>                         Extensions (TSX) feature in Intel processors that
>                         support TSX control.
> 
>                         This parameter controls the TSX feature. The options are:
> 
>                         on      - Enable TSX on the system.
>                         off     - Disable TSX on the system.
> 
> So what is the final name of command line option now?

We support two cmdline parameters:

	tsx=			To control TSX feature.
	tsx_async_abort=	To control the TAA mitigation.

> 
> > 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
> 
> s/supported/present/

Ok.

> 
> > +	 *
> > +	 * 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) ||
> 
> You have a @c parameter passed in, use it:
> 
> 	    cpu_has(c, X86_FEATURE_RTM)

Ok.

Thanks,
Pawan