Re: [PATCH] x86/cpu: Enable cpuid instruction on Cyrix 6x86/6x86L processors

From: Andy Lutomirski
Date: Thu Sep 20 2018 - 17:01:17 EST


On Thu, Sep 20, 2018 at 1:45 PM Matthew Whitehead <tedheadster@xxxxxxxxx> wrote:
>
> On power up, the cpuid instruction is disabled on Cyrix 6x86 and 6x86L
> processors and it needs to be enabled. There is code to do this, but it
> does not work because it uses the broken {set,get}Cx86_old() macros.
>
> There are comments in processor-cyrix.h advising you to _not_ make calls
> using the deprecated macros in this style:
>
> setCx86_old(CX86_CCR4, getCx86_old(CX86_CCR4) | 0x80);
>
> This is because it expands the macro into a non-functioning calling
> sequence. The new macros fix this problem, so we use them instead.
>
> We also need to forcibly enable X86_FEATURE_CPUID. This is because
> early_identify_cpu() does not know that the call to
> identify_cpu_without_cpuid() actually enables the cpuid instruction in the
> odd Cyrix case, and it turns X86_FEATURE_CPUID off after it has just been
> enabled on the processor.
>
> This was tested on actual Cyrix hardware.
>
> Signed-off-by: Matthew Whitehead <tedheadster@xxxxxxxxx>
> ---
> arch/x86/kernel/cpu/cyrix.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
> index 8949b7a..e603cc5 100644
> --- a/arch/x86/kernel/cpu/cyrix.c
> +++ b/arch/x86/kernel/cpu/cyrix.c
> @@ -437,10 +437,11 @@ static void cyrix_identify(struct cpuinfo_x86 *c)
> /* enable MAPEN */
> setCx86(CX86_CCR3, (ccr3 & 0x0f) | 0x10);
> /* enable cpuid */
> - setCx86_old(CX86_CCR4, getCx86_old(CX86_CCR4) | 0x80);
> + setCx86(CX86_CCR4, getCx86(CX86_CCR4) | 0x80);

Fine with me.

> /* disable MAPEN */
> setCx86(CX86_CCR3, ccr3);
> local_irq_restore(flags);
> + setup_force_cpu_cap(X86_FEATURE_CPUID);

I don't think this is right. This called from here:

/* cyrix could have cpuid enabled via c_identify()*/
if (have_cpuid_p()) {
cpu_detect(c);
get_cpu_vendor(c);
get_cpu_cap(c);
get_cpu_address_sizes(c);
setup_force_cpu_cap(X86_FEATURE_CPUID);

if (this_cpu->c_early_init)
this_cpu->c_early_init(c);

c->cpu_index = 0;
filter_cpuid_features(c, false);

if (this_cpu->c_bsp_init)
this_cpu->c_bsp_init(c);
} else {
identify_cpu_without_cpuid(c); <-- here
setup_clear_cpu_cap(X86_FEATURE_CPUID);
}

and it won't do what it should do. I would instead restructure the code:

if (!have_cpuid_p())
identify_cpu_without_cpuid(c);

/* cyrix could have cpuid enabled via c_identify() */
if (have_cpuid_p()) {
cpu_detect();
...
} else {
setup_clear_cpu_cal(X86_FEATURE_CPUID);
}

and then re-test, of course, to make sure that the Cyrix
identification code actually works when it gets re-invoked with CPUID
on.

--Andy