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

[Fenghua Yu]

On Thu, Apr 25, 2019 at 08:21:26AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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.)

TEST_CTL is the MSR's exact name shown in Table 2-14 in the latest SDM.
https://software.intel.com/en-us/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4

So can I still use MSR_TEST_CTL here?

> 
>  - 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...

OK. Will change the bit and mask definitions.

Thanks.

-Fenghua