Re: [PATCH v6 13/20] x86/split_lock: Enable split lock detection by default

[Thomas Gleixner]

On Wed, 3 Apr 2019, Fenghua Yu wrote:

> A split locked access locks bus and degrades overall memory access
> performance. When split lock detection feature is enumerated, enable
> the feature by default to find any split lock issue and then fix
> the issue.

Enabling the feature allows to find the issues, but does not automagically
fix them. Come on.

> +#define DISABLE_SPLIT_LOCK_DETECT 0
> +#define ENABLE_SPLIT_LOCK_DETECT  1

If those defines have a value at all, please start with the facility not
with functionality, i.e. AC_SPLIT_LOCK_ENABLE....

> +
> +static DEFINE_MUTEX(split_lock_detect_mutex);
> +static int split_lock_detect_val;

detect_val? What value is that? Its supposed to hold those magic defines
above. So something like

static unsigned int ac_split_lock_enable;

>  /*
>   * Just in case our CPU detection goes bad, or you have a weird system,
>   * allow a way to override the automatic disabling of MPX.
> @@ -161,10 +167,45 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
>  	return false;
>  }
>  
> +static u32 new_sp_test_ctl_val(u32 test_ctl_val)
> +{
> +	/* Change the split lock setting. */
> +	if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT)

That READ_ONCE() is required because?

> +		test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> +	else
> +		test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> +
> +	return test_ctl_val;
> +}

Aside of that do we really need a misnomed function which replaces the
simple inline code at the call site:

	rdmsr(l, h)
	l &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
	l |= ac_split_lock_enable << TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT;
	wrmrs(...)

or the even more simple

	if (ac_split_lock_enable)
		msr_set_bit(...)
	else
		msr_clear_nit(...)

Hmm?

> +
> +static inline void show_split_lock_detection_info(void)
> +{
> +	if (READ_ONCE(split_lock_detect_val))

That READ_ONCE() is required because?

> +		pr_info_once("x86/split_lock: split lock detection enabled\n");
> +	else
> +		pr_info_once("x86/split_lock: split lock detection disabled\n");

pr_fmt exists for a reason and having 'split lock' repeated several times
in the same line is not making it more readable.

> +}
> +
> +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		u32 l, h;
> +
> +		mutex_lock(&split_lock_detect_mutex);
> +		rdmsr(MSR_TEST_CTL, l, h);
> +		l = new_sp_test_ctl_val(l);
> +		wrmsr(MSR_TEST_CTL, l, h);
> +		show_split_lock_detection_info();
> +		mutex_unlock(&split_lock_detect_mutex);
> +	}
> +}
> +
>  static void early_init_intel(struct cpuinfo_x86 *c)
>  {
>  	u64 misc_enable;
>  
> +	init_split_lock_detect(c);

so we have in early boot:

	early_cpu_init()
	  early_identify_cpu()
	    this_cpu->c_early_init(c)
	      early_init_intel() {
	        init_split_lock_detect();
	      }	
            ....
            cpu_set_core_cap_bits(c)
	       set(FEATURE_SPLIT_LOCK)

I don't have to understand how init_split_lock_detect() will magically see
the feature bit which gets set afterwards, right? 


> +
>  	/* Unmask CPUID levels if masked: */
>  	if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
>  		if (msr_clear_bit(MSR_IA32_MISC_ENABLE,
> @@ -1032,6 +1073,7 @@ cpu_dev_register(intel_cpu_dev);
>  static void __init set_split_lock_detect(void)
>  {
>  	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
> +	split_lock_detect_val = 1;

Oh well. You add defines on top of the file and then you don't use them.

Thanks,

	tglx