Messages in this thread | | | Date | Thu, 25 Apr 2019 08:21:26 +0200 | From | Ingo Molnar <> | Subject | Re: [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
| |