MIME-Version: 1.0
In-Reply-To: <20161120111917.pw3alolx4fksfwbv@pd.tnic>
References: <70eac6639f23df8be5fe03fa1984aedd5d40077a.1479598603.git.luto@kernel.org>
 <20161120111917.pw3alolx4fksfwbv@pd.tnic>
From: Andy Lutomirski <luto@amacapital.net>
Date: Sun, 20 Nov 2016 08:22:25 -0800
Message-ID: <CALCETrXMZ3eMwG6Up0kox=jQy4r-bg1RUhbqGNuZB7bUpR2QKQ@mail.gmail.com>
Subject: Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing
To: Borislav Petkov <bp@alien8.de>
Cc: Matthew Whitehead <tedheadster@gmail.com>,
        Brian Gerst <brgerst@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        X86 ML <x86@kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3796
Lines: 100

On Nov 20, 2016 3:19 AM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Sat, Nov 19, 2016 at 03:37:30PM -0800, Andy Lutomirski wrote:
> > Linux will have all kinds of sporadic problems on systems that don't
> > have the CPUID instruction unless CONFIG_M486=y.  In particular,
> > sync_core() will explode.
>
> Btw, I think we should do something like this, in addition:
>
> ---
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 50425dd7e113..ee9de769941f 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -105,6 +105,7 @@
>  #define X86_FEATURE_AMD_DCM     ( 3*32+27) /* multi-node processor */
>  #define X86_FEATURE_APERFMPERF ( 3*32+28) /* APERFMPERF */
>  #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 state */
> +#define X86_FEATURE_CPUID      ( 3*32+31) /* CPU has CPUID instruction itself */
>
>  /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
>  #define X86_FEATURE_XMM3       ( 4*32+ 0) /* "pni" SSE-3 */
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 1f6a92903b09..63aa4842c0ae 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -602,34 +602,21 @@ static __always_inline void cpu_relax(void)
>         rep_nop();
>  }
>
> -/* Stop speculative execution and prefetching of modified code. */
> +/*
> + * Stop speculative execution and prefetching of modified code. CPUID is a
> + * barrier to speculative execution. Prefetched instructions are automatically
> + * invalidated when modified.
> + */
>  static inline void sync_core(void)
>  {
>         int tmp;
>
> -#ifdef CONFIG_M486
> -       /*
> -        * Do a CPUID if available, otherwise do a jump.  The jump
> -        * can conveniently enough be the jump around CPUID.
> -        */
> -       asm volatile("cmpl %2,%1\n\t"
> -                    "jl 1f\n\t"
> -                    "cpuid\n"
> -                    "1:"
> -                    : "=a" (tmp)
> -                    : "rm" (boot_cpu_data.cpuid_level), "ri" (0), "0" (1)
> -                    : "ebx", "ecx", "edx", "memory");
> -#else
> -       /*
> -        * CPUID is a barrier to speculative execution.
> -        * Prefetched instructions are automatically
> -        * invalidated when modified.
> -        */
> -       asm volatile("cpuid"
> -                    : "=a" (tmp)
> -                    : "0" (1)
> -                    : "ebx", "ecx", "edx", "memory");
> -#endif
> +       /* Do a CPUID if available, otherwise do a forward jump. */
> +       alternative_io("jmp 1f\n\t1:", "cpuid",
> +                       X86_FEATURE_CPUID,
> +                       "=a" (tmp),
> +                       "0" (1)
> +                       : "ebx", "ecx", "edx", "memory");
>  }

This makes me nervous: don't some CPUs actually need the cpuid
instruction when patching alternatives?  And with this approach, we
won't have the cpuid instruction there until after patching.

Why not change this function entirely:

write_cr2(0);

CR2 should be available on all 32-bit CPUs.  It clobbers fewer
registers.  More usefully, CPUID causes an exit when running under
most hypervisors, and that's quite slow.  The only case I can think of
where CPUID should be faster than MOV to CR2 is on Xen PV before Ivy
Bridge, and I'm not sure I care about performance there.

(On Xen PV, it will do a hypercall instead, but the hypercall should
be good enough to serialize, too.)

Or we could do it dynamically:

bt $X86_FEATURE_CPUID, CPU_FLAGS(boot_cpu_data) # or whatever -- I
think we need to add an asm offset
jnc 1f # here's our jump
cpuid
1:

It's not like CPUID is fast enough that avoiding a predictable branch
will help measurably.

--Andy