netdev - Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190425194808.GB64477@romley-ivt3.sc.intel.com>
Date:   Thu, 25 Apr 2019 12:48:09 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Ingo Molnar <mingo@...nel.org>
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

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

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