lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190425062126.GC40105@gmail.com> Date: Thu, 25 Apr 2019 08:21:26 +0200 From: Ingo Molnar <mingo@...nel.org> To: Fenghua Yu <fenghua.yu@...el.com> Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, Dave Hansen <dave.hansen@...el.com>, Ashok Raj <ashok.raj@...el.com>, Peter Zijlstra <peterz@...radead.org>, Ravi V Shankar <ravi.v.shankar@...el.com>, Xiaoyao Li <xiaoyao.li@...el.com>, Christopherson Sean J <sean.j.christopherson@...el.com>, Kalle Valo <kvalo@...eaurora.org>, Michael Chan <michael.chan@...adcom.com>, linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>, kvm@...r.kernel.org, netdev@...r.kernel.org, linux-wireless@...r.kernel.org Subject: Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register * Fenghua Yu <fenghua.yu@...el.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@...el.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
Powered by blists - more mailing lists