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

From: Fenghua Yu
Date: Thu Apr 25 2019 - 15:56:24 EST


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