[MODERATED] Re: [PATCH v4 02/10] TAAv4 2

On Tue, Sep 03, 2019 at 02:12:32PM -0700, speck for Pawan Gupta wrote:
> From: Pawan Gupta <[email protected]>
> Subject: [PATCH v4 02/10] x86/tsx: Add TSX control initialization
> 
> Add a new file to host TSX feature controls.  TSX control has three

"Add a new file" makes it sound like a sysfs file rather than a .c file.

And it would probably be better to lead off with a description of the
*functional* change of the patch, rather than the not-so-interesting
aspect of adding a .c file.  For example, this patch disables TSX by
default.  That should also be reflected in the patch subject.

(This is of course pending the other discussion about what defaults make
sense.)

> valid states ENABLE, DISABLE and NOT_SUPPORTED.  TSX may be used on
> certain processors as part of a speculative side channel attack.

How is that relevant to this patch?

> Set the boot time default to DISABLE or NOT_SUPPORTED based on the
> presence of IA32_TSX_CTRL MSR.

This should say *why* it's changing the default to disabled, assuming
that's what we decide to do.

> +void tsx_init(struct cpuinfo_x86 *c)
> +{
> +	tsx_ctrl_check_support(c);
> +
> +	switch (tsx_ctrl_state) {
> +	case TSX_CTRL_DISABLE:
> +		tsx_disable();
> +		clear_cpu_cap(c, X86_FEATURE_RTM);
> +		setup_clear_cpu_cap(X86_FEATURE_RTM);

Can you clarify why setup_clear_cpu_cap() is needed?

AFAICT, setup_clear_cpu_cap() would be used for forcing a feature to be
cleared on all CPUs, but this function already calls clear_cpu_cap() for
every CPU anyway.

-- 
Josh