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

On Wed, Sep 25, 2019 at 09:34:45PM -0500, speck for Josh Poimboeuf wrote:
> On Wed, Sep 25, 2019 at 06:13:05PM -0700, speck for Pawan Gupta wrote:
> > > > > > +static int __init tsx_cmdline(char *str)
> > > > > > +{
> > > > > > +	if (!str)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (!strcmp(str, "on"))
> > > > > > +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > > > > > +	else if (!strcmp(str, "off"))
> > > > > > +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +early_param("tsx", tsx_cmdline);
> > > > > 
> > > > > Hmm...  Let's say I have a non-TSX system.  I specify tsx=on.  This will
> > > > > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will
> > > > > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED.
> > > > > 
> > > > > I *think* this all works, but I'd love a comment or two about it.  Maybe
> > > > > even something that makes it clear that tsx_en/disable() are both only
> > > > > called when TSX *AND* TSX_CTRL_MSR are supported.
> > > > > 
> > > > > This is all kinda weird and confusing because we're doing this all
> > > > > without the X86_FEATURE bits for TSX.
> > > > 
> > > > If we add another X86_FEATURE bit it will have exact same state as
> > > > X86_FEATURE_RTM.  Do we really need this?
> > > 
> > > How about moving the tsx_init() call to early_init_intel()?  Then you'd
> > > be able to check the feature bit above.  And that might make the
> > > ordering a little easier to follow.
> > 
> > Sorry, I am failing to understand how it would make a difference.
> > early_init_intel() is also called from init_init().
> 
> 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.

> 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();
		[...]

> 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()?

Thanks,
Pawan