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

[Dave Hansen <dave.hansen@intel.com>]

On 3/11/26 19:14, Tony W Wang-oc wrote:
> 
> 
> +       if (c->x86 == 6 && c->x86_model == 15 && c->x86_stepping >= 14) {
> +               native_rdmsr(0x1232, dummy, chip_pf);
> +               chip_pf = (chip_pf >> 15) & 0x7;
> +               c->microcode = intel_get_microcode_revision();
> +
> +               if ((chip_pf == 0 && c->microcode < 0x20e) ||
> +                       (chip_pf == 1 && c->microcode < 0x208)) {
> +                       pr_warn_once("CPU has broken FSGSBASE support;
> clear FSGSBASE feature\n");
> +                       setup_clear_cpu_cap(X86_FEATURE_FSGSBASE);
> +               }
> +       }

So, I'm sorry but that's not really consistent how we're doing things
these days.

The model needs a symbolic name.

The MSR you're reading is completely undocumented and unnamed.

"chip_pf" is nonsensical and unexplained.

Code is duplicated across the centaur and zhaoxin files.

Once you have all of that fixed, you should have a simple:

#define CENTAUR_MODEL_FOO VFM_MAKE(X86_VENDOR_CENTAUR, 6, 15)
#define ZHAOXIN_MODEL_BAR VFM_MAKE(X86_VENDOR_ZHAOXIN, 6, 25)

in a central header, plus:

struct x86_cpu_id *naughty_list[] = {
	X86_MATCH_VFM_STEPS(CENTAUR_MODEL_FOO,       14, MAX_STEP, 0),
	X86_MATCH_VFM_STEPS(ZHAOXIN_MODEL_BAR, MIN_STEP,        3, 0),
	{}
};

void check_fsgsbase_bugs()
{
	u32 fixed_ucode;

	if (!cpu_feature_enabled(X86_FEATURE_FSGSBASE))
		return;

	c = x86_match_cpu(naughty_list);
	if (!c)
		return;

	chip_pf = ...
	if (chip_pf == 0)
		fixed_ucode = 0x20e;
	if (chip_pf == 1)
		fixed_ucode = 0x208;

	if (intel_get_microcode_revision() >= fixed_ucode)
		return;

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

Then check_fsgsbase_bugs() can pretty much be called anywhere. It can
even be in generic code.

We are also getting some new matching fields in 'x86_cpu_id'. I suspect
'chip_pf' can be stored in there where Intel has the platform_id right
now. But you don't have to do that now.

Could you please go this route rather than copy-and-pasted chunks of
code sprinkled with a healthy dose of magic numbers?