Re: [PATCH 1/8] x86/microcode/intel: Reject problematic loading on GNR systems

[Borislav Petkov]

On Tue, Sep 01, 2026 at 11:16:26PM +0000, Chang S. Bae wrote:
> +static bool is_loading_denied(struct cpu_signature *sig, u32 rev)

This naming is not better, sorry.

Is loading denied means the loading in general is denied because <raisin> or
are you trying to check whether this particular revision should not be loaded?

I think it is latter.

So you wanna say

	revision_blacklisted()

or so.

> +{
> +	u32 vfm = IFM(x86_family(sig->sig), x86_model(sig->sig));
> +
> +	/*
> +	 * Revision 0x1000405 contains prerequisite changes for subsequent
> +	 * microcode updates on Granite Rapids systems. Updates directly from
> +	 * an older revision to this or a newer one can result in #MC (GNR98).
> +	 *
> +	 * This dependency can be indicated from the minimum revision field.
> +	 * However, revision 0x1000423 has an incorrect minimum revision in its

So you lost me here: 0x1000423 is not tested anywhere - just mentioned here.

So what I understand is: usually, dependencies like that can be expressed with
minrev but *in addition* to the current issue, patch 0x1000423 has minrev
wrong so that dependency cannot be upheld there either.

But then why even mention it if you're not going to test it?

Why do we care about GNR101 at all?

> +	 * header (GNR101).
> +	 *
> +	 * Prevent loading 0x1000405 or later unless the CPU has already been
> +	 * updated to 0x1000405 or later.
> +	 */
> +	if (vfm == INTEL_GRANITERAPIDS_X &&
> +	    x86_stepping(sig->sig) == 1 &&
> +	    sig->pf & 0x95 &&
> +	    sig->rev < 0x1000405 &&
> +	    rev >= 0x1000405) {

You don't really need to test rev here - it is enough that sig->rev is
< 0x1000405 - that already makes you susceptible and then you can check rev
inside the { }.

> +		if (rev == 0x1000405)
> +			pr_err_once("Erratum GNR98: 0x1000405 is not loadable.\n");
> +		else
> +			pr_err_once("Erratum GNR98: 0x1000405 is required before 0x%x.\n", rev);
> +		pr_err_once("Please update the system BIOS or firmware.\n");

This is useless most of the time because client won't usually get BIOS
updates. You can tell people they should update their microcode packages
instead. That's where we can really help.

> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  /* Scan blob for microcode matching the boot CPUs family, model, stepping */
>  static __init struct microcode_intel *scan_microcode(void *data, size_t size,
>  						     struct ucode_cpu_info *uci,
> @@ -330,6 +362,9 @@ static __init struct microcode_intel *scan_microcode(void *data, size_t size,
>  		if (!intel_find_matching_signature(data, &uci->cpu_sig))
>  			continue;
>  
> +		if (is_loading_denied(&uci->cpu_sig, mc_header->rev))
> +			continue;
> +
>  		/*
>  		 * For saving the early microcode, find the matching revision which
>  		 * was loaded on the BSP.
> @@ -878,6 +913,9 @@ static enum ucode_state parse_microcode_blobs(int cpu, struct iov_iter *iter)
>  		if (!intel_find_matching_signature(mc, &uci->cpu_sig))
>  			continue;
>  
> +		if (is_loading_denied(&uci->cpu_sig, mc_header.rev))
> +			continue;
> +
>  		is_safe = ucode_validate_minrev(&mc_header);
>  		if (force_minrev && !is_safe)
>  			continue;
> @@ -905,7 +943,7 @@ static enum ucode_state parse_microcode_blobs(int cpu, struct iov_iter *iter)
>  	return UCODE_ERROR;
>  }
>  
> -static bool is_blacklisted(unsigned int cpu)
> +static bool is_late_loading_denied(unsigned int cpu)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  
> @@ -936,7 +974,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device)
>  	struct kvec kvec;
>  	char name[30];
>  
> -	if (is_blacklisted(cpu))
> +	if (is_late_loading_denied(cpu))

Aaaah, you wanna be politically correct and can't use "blacklisted" anymore.

Well, you're not introducing new usage so you don't have to touch old usage.
And "is denied" does not express the situation properly. Try a better one.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette