Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register

[Ingo Molnar <mingo@kernel.org>]

* Fenghua Yu <fenghua.yu@intel.com> wrote:

> Setting bit 29 in MSR TEST_CTL (0x33) enables split lock detection and
> clearing the bit disables split lock detection.
> 
> Define the MSR and the bit. The definitions will be used in enabling or
> disabling split lock detection.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/include/asm/msr-index.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index f65ef6f783d2..296eeb761ab6 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -39,6 +39,10 @@
>  
>  /* Intel MSRs. Some also available on other CPUs */
>  
> +#define MSR_TEST_CTL				0x00000033
> +#define TEST_CTL_SPLIT_LOCK_DETECT_SHIFT	29
> +#define TEST_CTL_SPLIT_LOCK_DETECT		BIT(29)

Three problems:

 - Is MSR_TEST_CTL is not really a canonical MSR name... A quick look at 
   msr-index reveals the prevailing nomenclature:

     dagon:~/tip> git grep -h 'define MSR' arch/x86/include/asm/msr-index.h | cut -d_ -f1-2 | sort -n | uniq -c | sort -n | tail -10
       8 #define MSR_K8
       8 #define MSR_MTRRfix4K
      12 #define MSR_CORE
      13 #define MSR_IDT
      14 #define MSR_K7
      16 #define MSR_PKG
      19 #define MSR_F15H
      33 #define MSR_AMD64
      83 #define MSR_P4
     163 #define MSR_IA32

   I.e. this shouldn't this be something like MSR_IA32_TEST_CTL - or this 
   the name the Intel SDM uses? (I haven't checked.)

 - The canonical way to define MSR capabilities is to use the MSR's name 
   as a prefix. I.e.:

        MSR_TEST_CTL
        MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT
        MSR_TEST_CTL_SPLIT_LOCK_DETECT
        etc.

   Instead of the random mixture of MSR_ prefixed and non-prefixed 
   MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT and 
   TEST_CTL_SPLIT_LOCK_DETECT names.

 - Finally, this is not how we define bits - the _SHIFT postfix is actively
   confusing as we usually denote _SHIFT values with something that is 
   used in a bit-shift operation, which this isn't. Instead the proper 
   scheme is to postfix the bit number with _BIT and the mask with _MASK, 
   i.e. something like:

     #define MSR_TEST_CTL				0x00000033
     #define MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT		29
     #define MSR_TEST_CTL_SPLIT_LOCK_DETECT		BIT(MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT)

Note how this cleans up actual usage:

+       msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+       this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);

-       msr_set_bit(MSR_TEST_CTL, MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT);
-       this_cpu_or(msr_test_ctl_cache, MSR_TEST_CTL_SPLIT_LOCK_DETECT);

Frankly, this kind of disorganized code in a v8 submission is *really* 
disappointing, it's not like it's hard to look up these patterns and 
practices in existing code...

Sigh.

Thanks,

	Ingo