Re: [PATCH v2] x86/microcode/intel: Reject problematic loading on Granite Rapids systems

["Chang S. Bae" <chang.seok.bae@intel.com>]

On 9/8/2026 5:13 PM, Borislav Petkov wrote:
> On Tue, Sep 08, 2026 at 10:32:09PM +0000, Chang S. Bae wrote:
>> Microcode updates can usually jump revisions. However, there is an
>> erratum on Granite Rapids systems. If they "jump over" to revision
>> 0x1000405 or later, they result in #MC.
>>
>> Prevent loading 0x1000405 or later unless the running revision is already
>> at least 0x1000405. Apply this blocking to both early- and late-loading
>> paths.
> 
> People have got to stop explaining the patch in the commit message. That
> should be obvious from the diff. If you have to explain it then there is
> something very non-obvious here which I don't see it...

... If they "jump over" to revision 0x1000405 or later, they result in 
#MC. Avoid it.

>>   
>> +static bool revision_banned(struct cpu_signature *sig, u32 rev)
> 
> I like Andy's naming:
> 
> https://lore.kernel.org/xen-devel/20260908171525.3196765-1-andrew.cooper3@citrix.com/T/#u
> 
> ...is_safe is much better than banned.

s/revision_banned/revision_is_safe/ in V3.

> 
>> +{
>> +	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. This is
>> +	 * documented item GNR98, #835486 (Intel Xeon 6900/6700/6500-Series
>> +	 * Processors with P-Cores).
>> +	 */
> 
> What dhansen said - keep this short'n'sweet.

Will replace the above with Dave's.

> 
>> +	if (vfm == INTEL_GRANITERAPIDS_X &&
>> +	    x86_stepping(sig->sig) == 1 &&
>> +	    sig->pf & 0x95 &&
>> +	    sig->rev < 0x1000405 &&
>> +	    rev >= 0x1000405) {
>> +		if (rev == 0x1000405)
> 
> I also like Andy's testing of the patch revs:
> 
> +         ((cpu_sig->rev < 0x01000380 && mc->rev >= 0x01000405) ||
> +          (cpu_sig->rev < 0x01000405 && mc->rev >  0x01000405)) )

Let me double-check this with ucode folks, first. Then will take.

> 
>> +			pr_err_once("Erratum GNR98: revision 0x1000405 is not loadable.\n");
>> +		else
>> +			pr_err_once("Erratum GNR98: revision 0x1000405 is required before 0x%x.\n", rev);
 > > And you don't need those semi-identical strings here.

pr_err_once("Erratum GNR98: skipping revision 0x%x.\n", rev);

Yeah, overall I can hear "keep it simple & short" messages.

Thanks,
Chang