[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

On Thu, Sep 26, 2019 at 12:15:55AM -0700, speck for Pawan Gupta wrote:
> > Regardless, shouldn't tsx_init() at least be called before
> > cpu_set_bug_bits(), so that X86_BUG_TAA can get set appropriately?
> 
> X86_BUG_TAA should be set even when tsx_init() disables TSX. This is to
> indicate that the CPU has the bug but was mitigated by disabling TSX.
> That is why we are calling cpu_set_bug_bits() first to set X86_BUG_TAA
> and then disabling TSX in tsx_init(). Moreover cpu_set_bug_bits() is
> called only by the boot-cpu, and tsx_init() is called per-cpu to disable
> TSX on each CPU.

Hm, I guess that makes sense.  But what if TSX enumeration is disabled
due to a previous kexec?  Then the CPUID feature bit (X86_FEATURE_RTM)
won't be set, and thus X86_BUG_TAA won't get set, right?

> > In that case, instead of early_param(), tsx_init() could just use
> > cmdline_find_option() to find the 'tsx=' option.  That would also have
> > the advantage of making it much easier to untangle the initialization
> > order.
> 
> Thanks for the suggestion. Does this look okay?
> 
> static enum tsx_ctrl_states {
> 	TSX_CTRL_ENABLE,
> 	TSX_CTRL_DISABLE,
> 	TSX_CTRL_NOT_SUPPORTED,
> } tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
> 
> static bool tsx_ctrl_is_supported(void)
> {
> 	u64 ia32_cap = read_ia32_arch_cap();
> 
> 	/*
> 	 * TSX is controlled via MSR_IA32_TSX_CTRL.  However,
> 	 * support for this MSR is enumerated by ARCH_CAP_TSX_MSR bit
> 	 * in MSR_IA32_ARCH_CAPABILITIES.
> 	 */
> 	return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR);
> }
> 
> void tsx_init(struct cpuinfo_x86 *c)
> {
> 	char arg[20];
> 	int ret;
> 
> 	/* return if TSX_CTRL is not supported */
> 	if (!tsx_ctrl_is_supported())
> 		return;
> 
> 	ret = cmdline_find_option(boot_command_line, "tsx", arg, sizeof(arg));
> 	if (ret >= 0) {
> 		if (!strcmp(arg, "on"))
> 			tsx_ctrl_state = TSX_CTRL_ENABLE;
> 		else if (!strcmp(arg, "off"))
> 			tsx_ctrl_state = TSX_CTRL_DISABLE;
> 		else
> 			pr_info("tsx: invalid option, defaulting to off\n");
> 	}
> 
> 	/*
> 	 * If user provided an invalid option or tsx= is not provided on cmdline,
> 	 * default to TSX_CTRL_DISABLE. This is because on certain processors
> 	 * TSX may be used as part of a speculative side channel attack.
> 	 */
> 	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
> 		tsx_ctrl_state = TSX_CTRL_DISABLE;
> 
> 	switch (tsx_ctrl_state) {
> 	case TSX_CTRL_DISABLE:
> 		tsx_disable();
> 		[...]

Yeah, that does look better to me.

> > If early_init_intel() isn't the right spot, then maybe
> > early_identify_cpu().
> 
> early_identify_cpu() is called for other vendors as well, I think
> init_intel() is the ideal place for calling an Intel specific function.
> Why do we want to move tsx_init() from init_intel()?

IIUC, the fact that the X86_FEATURE_RTM enumeration can be removed
before kexec means that tsx_init() needs to run early, so that it can
properly detect the enumeration so that X86_BUG_TAA can get set
properly.

Since tsx_init() can clear X86_FEATURE_RTM even though the CPU supports
it, cpu_set_bug_bits() could check TSX_CTRL_NOT_SUPPORTED instead of
X86_FEATURE_RTM, like:

	if (cpu_has_tsx() && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);


where cpu_has_tsx() is just !TSX_CTRL_NOT_SUPPORTED.

-- 
Josh