lkml.org 
[lkml]   [2019]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register

* 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

\
 
 \ /
  Last update: 2019-04-25 08:22    [W:0.152 / U:28.584 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site