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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2026 19:10, Andrew Cooper wrote:
> 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.

Which I had in mind as a possible option. Problem being that with
incomplete information it's of questionable value to offer an ack
on such changes. Here you go:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan