Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1537476292-28056-1-git-send-email-tedheadster@gmail.com>
In-Reply-To: <1537476292-28056-1-git-send-email-tedheadster@gmail.com>
From:   Andy Lutomirski <luto@amacapital.net>
Date:   Thu, 20 Sep 2018 14:01:00 -0700
Message-ID: <CALCETrX8J9-JG5kqppFLTZSnr0S=mGAmLqLcHzdtKmRNDXZ49w@mail.gmail.com>
Subject: Re: [PATCH] x86/cpu: Enable cpuid instruction on Cyrix 6x86/6x86L processors
To:     Matthew Whitehead <tedheadster@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 20, 2018 at 1:45 PM Matthew Whitehead <tedheadster@gmail.com> 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@gmail.com>
> ---
>  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