linux-kernel - Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE

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: <651504d9-f458-1d25-870d-b8c55061be45@intel.com>
Date:   Sat, 18 Apr 2020 12:15:57 +0800
From:   Xiaoyao Li <xiaoyao.li@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        "Luck, Tony" <tony.luck@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Ingo Molnar <mingo@...nel.org>, Fenghua Yu <fenghua.yu@...el.com>,
        Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Andy Lutomirski <luto@...nel.org>,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are
 not architectural

On 4/18/2020 5:07 AM, Thomas Gleixner wrote:
> Tony,
> 
> Thomas Gleixner <tglx@...utronix.de> writes:
>> "Luck, Tony" <tony.luck@...el.com> writes:
>>> Swings and roundabouts ... getting rid of the goto makes for
>>> deeper indentation. But if you really want to get rid of the
>>> goto, then your version is fine with me.
>>>
>>> Do you want me to spin it into v3?
>>
>> Nah. I tweak it myself.
> 
> as I fear that the infinite wisdom of HW folks will add yet another
> variant in the foreseeable future, I used a switch() right away and
> tweaked the comments a bit.
> 
> Can you have a look, please?
> 
> Thanks,
> 
>          tglx
> 
> 8<------------------
> From: Tony Luck <tony.luck@...el.com>
> Subject: x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural
> Date: Thu, 16 Apr 2020 13:57:53 -0700
> 
> From: Tony Luck <tony.luck@...el.com>
> 
> The Intel Software Developers' Manual erroneously listed bit 5 of the
> IA32_CORE_CAPABILITIES register as an architectural feature. It is not.
> 
> Features enumerated by IA32_CORE_CAPABILITIES are model specific and
> implementation details may vary in different cpu models. Thus it is only
> safe to trust features after checking the CPU model.
> 
> Icelake client and server models are known to implement the split lock
> detect feature even though they don't enumerate IA32_CORE_CAPABILITIES
> 
> Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Link: https://lkml.kernel.org/r/20200416205754.21177-3-tony.luck@intel.com
> 
> ---
>   arch/x86/kernel/cpu/intel.c |   45 ++++++++++++++++++++++++++++++--------------
>   1 file changed, 31 insertions(+), 14 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -1120,10 +1120,17 @@ void switch_to_sld(unsigned long tifn)
>   }
>   
>   /*
> - * The following processors have the split lock detection feature. But
> - * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
> - * be enumerated. Enable it by family and model matching on these
> - * processors.
> + * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they should
> + * only be trusted if it is confirmed that a CPU model implements a
> + * specific feature at a particular bit position.
> + *
> + * The possible driver data field values:
> + *
> + * - 0: CPU models that are known to have the per-core split-lock detection
> + *	feature even though they do not enumerate IA32_CORE_CAPABILITIES.
> + *
> + * - 1: CPU models which may enumerate IA32_CORE_CAPABILITIES and if so use
> + *      bit 5 to enumerate the per-core split-lock detection feature.

So now, it's tightly associated with CPU model, which makes it harder to 
expose this feature to guest. For guest, the CPU model can be configured 
to anything.

As suggested by Sean internally, we'd better use a KVM CPUID to expose 
it to guest, which makes it independent of CPU model.

Paolo, tglx,

What do you think?

>    */
>   static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
>   	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
> @@ -1133,19 +1140,29 @@ static const struct x86_cpu_id split_loc
>   
>   void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
>   {
> -	u64 ia32_core_caps = 0;
> +	const struct x86_cpu_id *m;
> +	u64 ia32_core_caps;
>   
> -	if (c->x86_vendor != X86_VENDOR_INTEL)
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
>   		return;
> -	if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
> -		/* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
> +
> +	m = x86_match_cpu(split_lock_cpu_ids);
> +	if (!m)
> +		return;
> +
> +	switch (m->driver_data) {
> +	case 0:
> +		break;
> +	case 1:
> +		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> +			return;
>   		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
> -	} else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> -		/* Enumerate split lock detection by family and model. */
> -		if (x86_match_cpu(split_lock_cpu_ids))
> -			ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
> +		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
> +			return;
> +		break;
> +	default:
> +		return;
>   	}
>   
> -	if (ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
> -		split_lock_setup();
> +	split_lock_setup();
>   }
>