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

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Sep 06, 2019 at 02:07:12PM -0700, speck for Dave Hansen wrote:
> On 9/4/19 12:43 AM, speck for Pawan Gupta wrote:
> > On Wed, Sep 04, 2019 at 07:54:06AM +0200, speck for Greg KH wrote:
> >>> +static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> >>> +{
> >>> +	u64 ia32_cap = 0;
> >>> +
> >>> +	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
> >>> +		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> >>> +
> >>> +	if (!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
> >>> +		tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
> >> Why isn't this if statement under the if statement above?
> > Because we still have to set tsx_ctrl_state when cpu doesn't support
> > X86_FEATURE_ARCH_CAPABILITIES.
> 
> FWIW, I struggled to read this the first time I read it, too.  It might
> make sense to break it up like this:
> 
> u64 read_ia32_arch_cap(void)
> {
> 	u64 ia32_cap = 0;
> 
> 	/* Leave the MSR set to all 0's when not supported */
> 	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
> 		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> 
> 	return ia32_cap;
> }
> 
> You could even replace the other places that we read the MSR.  They have
> the same pattern.  Then do:
> 
> static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> {
> 	u64 ia32_cap = read_ia32_arch_cap();
> 
> 	if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)
> 		tsx_ctrl_state = TSX_CTRL_ENABLE;
> }
> 
> which takes tsx_ctrl_state out of the 'disable' mode.

I think it would be even clearer if the compile-time default were
changed to TSX_CTRL_NOT_SUPPORTED.

Then the logic would be less confusing.  For example the reader of the
code doesn't have to remember that ia32_cap is initialized to zero.  And
IMO, "not supported" conceptually makes the most sense as a compile-time
default anyway.

static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
{
	u64 ia32_cap;

	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES)) {
		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);

		if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)
			tsx_ctrl_state = TSX_CTRL_DISABLE;
	}
}

-- 
Josh