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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/09/2026 11:55 am, Jan Beulich wrote:
> 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?

Fine, I'll say "minimum revision field", but the whole name is (and
always has been) silly.

>
>>>> +     * 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?

Because they are both buggy.  They fail to exclude some unsafe cases.

If it's not obvious, I do know more than I can say publicly.

~Andrew