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

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

On Wed, Sep 25, 2019 at 05:13:19PM -0500, speck for Josh Poimboeuf wrote:
> 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;

Below is the order in which tsx_ctrl_state can be modified:
1. Compile time
2. Cmdline parsing (early_param())
3. TSX_CTRL MSR support check (tsx_ctrl_check_support())

Here we need to handle tsx=on case as well. If we set tsx_ctrl_state to
ENABLE/DISABLE in tsx_ctrl_check_support() we could override what user
asked for via cmdline. Unless we keep another copy of tsx_ctrl_state,
which might add more complexity.

I believe tsx_ctrl_check_support() should only change state to
NOT_SUPPORTED when TSX_CTRL MSR is not supported.

Thanks,
Pawan