Re: [PATCH v2] x86/ucode: Work around Granite Rapids erraturm GNR98

[Jan Beulich <jbeulich@suse.com>]

On 08.09.2026 12:08, Andrew Cooper wrote:
> On 08/09/2026 7:26 am, Jan Beulich wrote:
>> On 07.09.2026 23:32, Andrew Cooper wrote:
>>> @@ -273,6 +274,44 @@ static bool microcode_fits_cpu(const struct microcode_patch *mc)
>>>      return false;
>>>  }
>>>  
>>> +static bool microcode_safe_to_load(const struct microcode_patch *mc)
>>> +{
>>> +    struct cpu_signature *cpu_sig = &this_cpu(cpu_sig);
>>> +
>>> +    /*
>>> +     * Treat pre-production as always safe - anyone using pre-production
>>> +     * microcode knows what they are doing, and can keep any resulting pieces.
>>> +     */
>>> +    if ( (int)cpu_sig->rev < 0 || mc->rev < 0 )
>>> +        return true;
>>> +
>>> +    /*
>>> +     * GNR98 states that Granite Rapids systems hang when loading new ucode on
>>> +     * sufficiently old firmware.  GNR101 retroactively declares that one
>>> +     * ucode had incorrect min_rev fields, in light of discovering GNR98.
>> We still have no min_rev field, so imo a reference to it wants some
>> clarification.
> 
> No, I don't think so.  The fact Xen has no min_rev field (yet) has no
> baring on the wording of GNR101.

The wording there is "Minimum Runtime Microcode Update Revision". As long
as we don't have a field of the name, how can such a comment be unambiguous?

>>> +     * Both are incomplete statements of the problem.
>>> +     *
>>> +     * At the time of writing (August 2026), the believed safe sequence is:
>>> +     *   0x01000370 -> [0x01000380...0x010003f3] -> 0x01000405 -> any later
>>> +     *
>>> +     * Disallow known-unsafe loads while permitting believed-safe loads.  For
>>> +     * GNR, this allows multi-hop loading to get up to the latest.
>>> +     */
>>> +    if ( boot_cpu_data.vfm == INTEL_GRANITERAPIDS_X &&
>>> +         boot_cpu_data.stepping == 1 && (cpu_sig->pf & 0x95) &&
>>> +         ((cpu_sig->rev < 0x01000380 && mc->rev >= 0x01000405) ||
>> This is odd: The lhs of && uses the lower bound of the inner permitted
>> range, while the rhs of the && doesn't use the upper one. If it's intended
>> that way, I think this also needs clarifying in the comment. Otherwise imo
>> lhs and rhs better would be consistent in this regard.
> 
> It is intentional.  Furthermore, it is the only coherent way of
> expressing the sequence as given.
> 
> I'm not writing a comment explaining why it's a good idea to use the
> same boundary numerals between the comment and the code.  It goes
> without saying.

But that's the problem - code and comment are not (obviously) in sync.

> I'm also not interested about pureness concerns about inner vs outer
> bounds.  I can't see a change here that won't make it worse.

So why are

         ((cpu_sig->rev <= 0x01000370 && mc->rev >= 0x01000405) ||

and

         ((cpu_sig->rev < 0x01000380 && mc->rev > 0x010003f3) ||

both worse? Both 0x01000370 ... 0x01000380 and 0x010003f3 ... 0x01000405
are discontiguous, which is bad enough. If then you pick apparently
randomly (and in any event inconsistently) from the possible boundaries,
how can that help the situation (and be "the only coherent way of
expressing the sequence as given")?

Even if it's merely a matter of us disagreeing on what "consistent" or
"coherent" would be here, doesn't me as the first reader not easily
spotting the "coherency" you claim already indicate there is a
(possible) issue? (For context: Whichever way the expressions are going
to end up, they'll make implicit statements on the gaps, i.e. on the
non-public 0x01000371 ... 0x0100037f and 0x010003f4 ... 0x01000404. You
may say that doesn't matter, because of their non-publicness, but that
won't make that implicit statement go away.)

Jan