Re: [PATCH] x86/cpu/centaur: Disable X86_FEATURE_FSGSBASE on Zhaoxin C4600

[Dave Hansen]

> --- /dev/null
> +++ b/arch/x86/include/asm/zhaoxin.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_ZHAOXIN_H
> +#define _ASM_X86_ZHAOXIN_H
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/microcode.h>
> +
> +#define    ZHAOXIN_MODEL_ZXC    VFM_MAKE(X86_VENDOR_ZHAOXIN, 6, 25)
> +#define    CENTAUR_MODEL_ZXC    VFM_MAKE(X86_VENDOR_CENTAUR, 6, 15)
> +
> +struct x86_cpu_id naughty_list[] = {
> +    X86_MATCH_VFM_STEPS(ZHAOXIN_MODEL_ZXC, 0, 3, 0),
> +    X86_MATCH_VFM_STEPS(CENTAUR_MODEL_ZXC, 14, 15, 0),
> +    {}
> +};

Hi Tony,

This is headed in the right direction, in a way.

However, I think you might have missed a few things. Did you notice that
this structure is in a .h file? We generally don't define data
structures and variables in header files. You might want to take a quick
look around the tree.

Then, go try and #include this header in two different places. See what
happens.

> +void check_fsgsbase_bugs(void);
> +
> +void check_fsgsbase_bugs(void)
> +{

Generally, compiler warnings are good things. They tell you that you've
done something wrong. Simply throwing code in to silence them isn't a
great practice.

Remember the compiler warning you got without the function declaration?
That was there to tell you that something is wrong. You placed
definitions in a header, not declarations.

But, adding a declaration before the definition made the compiler quiet.

> +    u32 chip_pf, dummy, fixed_ucode;

This is whitespace damaged, btw.

I also prefer one variable per line

	u32 fixed_ucode;
	u32 chip_pf;
	u32 dummy;

> +    if (!cpu_feature_enabled(X86_FEATURE_FSGSBASE))
> +        return;
> +
> +    if (!x86_match_cpu(naughty_list))
> +        return;

Heh, also I was joking about 'naughty_list'. It would be best to give it
a good symbolic, meaningful name.

> +    native_rdmsr(MSR_ZHAOXIN_MFGID, dummy, chip_pf);

This at least need commenting. What prevents this code from getting
called on other vendors' CPUs? What about models of Zhaoxin CPUs that
don't have this MSR?

> +    /* chip_pf represents product version flag */
> +    chip_pf = (chip_pf >> 15) & 0x7;

Please use the GENMASK macros here.

> +    if (chip_pf == 0)
> +        fixed_ucode = 0x20e;
> +    if (chip_pf == 1)
> +        fixed_ucode = 0x208;
> +
> +    if (intel_get_microcode_revision() >= fixed_ucode)
> +        return;

It's probably worth commenting why this is calling an "intel"_ function.

> +    pr_warn_once("Broken FSGSBASE support, clearing feature\n");
> +    setup_clear_cpu_cap(X86_FEATURE_FSGSBASE);
> +}
> +
> +#endif