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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

This patch stops Xen hanging with the real ucode which has existed in
the world for 6 months.

I am still waiting on Intel to publish new blobs (including corrected
min_rev fields) before the multi-hop update can be made to work.  I have
no ETA on this.

>  Really I first meant to ask why there's no use of that field,
> to merely make that one exception.
>
>> +     * 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.

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.

>
>> +          (cpu_sig->rev < 0x01000405 && mc->rev >  0x01000405)) )
> This one, otoh, fully matches the comment.
>
>> +    {
>> +        printk_once(XENLOG_WARNING
>> +                    "microcode: Granite Rapids erratum GNR98 detected.  
>> Skipping ucode 0x%08x\n"
>> +                    "microcode: Firmware update recommended\n", mc->rev);
> I think a 2nd XENLOG_WARNING is wanted after the inner \n (or none at all,
> to use the default for both).

Fine, fixed up locally.

~Andrew