[MODERATED] Re: [PATCH v6 9/9] TAAv6 9

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

On Thu, Oct 10, 2019 at 08:54:12AM +0200, speck for Greg KH wrote:
> > +static DEFINE_MUTEX(tsx_mutex);
> 
> I still don't know what this is trying to "protect".  Please at least
> document it so I have a chance to review it...

I will add these comments to the code:

	/*
	 * Protect tsx_ctrl_state and TSX update on_each_cpu() from concurrent
	 * writers.
	 *
	 *  - Serialize TSX_CTRL MSR writes across all CPUs when there are
	 *    concurrent sysfs requests. on_each_cpu() callback execution
	 *    order on other CPUs can be different for multiple calls to
	 *    on_each_cpu(). For conflicting concurrent sysfs requests the
	 *    lock ensures all CPUs have updated the TSX_CTRL MSR before the
	 *    next call to on_each_cpu().
	 *  - Serialize tsx_ctrl_state update so that it doesn't get out of
	 *    sync with TSX_CTRL MSR.
	 *  - Serialize update to taa_mitigation.
	 */

> > +/* Take tsx_mutex lock and update tsx_ctrl_state when calling this function */
> > +static void tsx_update_on_each_cpu(bool val)
> > +{
> > +	get_online_cpus();
> > +	on_each_cpu(tsx_update_this_cpu, (void *)val, 1);
> > +	put_online_cpus();
> > +}
> 
> Why take the lock?  This is only called in one place.

So that TSX_CTRL MSR state stays consistent across all CPUs between
multiple on_each_cpu() calls. Otherwise overlapping conflicting TSX_CTRL MSR
writes could end up in some CPUs with TSX enabled and others with TSX
disabled.

> > +ssize_t hw_tx_mem_show(struct device *dev, struct device_attribute *attr,
> > +		       char *buf)
> > +{
> > +	return sprintf(buf, "%d\n", tsx_ctrl_state == TSX_CTRL_ENABLE ? 1 : 0);
> > +}
> > +
> > +ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
> > +			const char *buf, size_t count)
> > +{
> > +	enum tsx_ctrl_states requested_state;
> > +	ssize_t ret;
> > +	bool val;
> > +
> > +	ret = kstrtobool(buf, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&tsx_mutex);
> > +
> > +	if (val)
> > +		requested_state = TSX_CTRL_ENABLE;
> > +	else
> > +		requested_state = TSX_CTRL_DISABLE;
> 
> Why is lock grabbed above and not here?

Yes, can be moved here.

Thanks,
Pawan