Re: [PATCH] x86, cpu: Fix X86_FEATURE_NOPL

[H. Peter Anvin]

On 10/04/2010 01:47 PM, Linus Torvalds wrote:
> On Mon, Oct 4, 2010 at 12:31 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
>>
>> ba0593bf553c450a03dbc5f8c1f0ff58b778a0c8 cleared the aforementioned
>> cpuid bit only on 32-bit due to various problems with Virtual PC. This
>> somehow got lost during the 32- + 64-bit merge so restore the feature
>> bit on 64-bit. For that, set it explicitly for non-constant arguments of
>> cpu_has(). Update comment for future reference.
> 
> I don't think this is right.
> 
> The cpu_has() logic depends not on x86-64, but on X86_P6_NOP.
> 

Actually, cpu_has() depends on:
#if defined(CONFIG_X86_P6_NOP) || defined(CONFIG_X86_64)

Obviously, if we *use* P6 NOPs they better be available on the
processor, but we also are pretty sure that every 64-bit processor
supports then

> Which has
> 
>         depends on X86_64
>         depends on (MCORE2 || MPENTIUM4 || MPSC)
> 
> as its config rules, not just X86_64.

Right; the top clause, of course, was added later, as we found out that
it was unsafe to ever use NOPL on 32 bits, because of Microsoft f*ckups.

CONFIG_X86_P6_NOP was intended to indicate that using NOPL is
*preferred*, whereas the CPUID bit -- cpu_has() -- was (and is) intended
to indicate that NOPL is *supported*, not necessarily preferred.

As such, the code I believe is technically correct for the current
situation (NOPL is always supported on 64 bits, never on 32 bits), but
as you quite correctly point out it is definitely more confusing than is
desirable; this is probably also reflected by the following code in
alternative.c:

static const unsigned char *const *__init_or_module find_nop_table(void)
{
        if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
            boot_cpu_has(X86_FEATURE_NOPL))
                return p6_nops;
        else
                return k8_nops;
}

The vendor check here is really ugly.

The only case where we need CONFIG_X86_P6_NOP as a compile-time check is
for the ASM_NOP* macros; for dynamic code we're of course better off
with a runtime check, but it would be better if that was part of the CPU
routines.  Perhaps X86_FEATURE_FAST_NOPL or something like that.

	-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/