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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/09/2026 11:51 pm, Teddy Astie wrote:
> Le 03/09/2026 à 00:14, Andrew Cooper a écrit :
>> Block loads which are known to hang the system.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
>> CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
>>
>> A more complete solution is in the works, but it's taken 4 months to
>> get this
>> much published...
>> ---
>>   xen/arch/x86/cpu/microcode/intel.c | 31 ++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/intel.c
>> b/xen/arch/x86/cpu/microcode/intel.c
>> index c45b00c6b033..2160befd3197 100644
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -27,6 +27,7 @@
>>   #include <xen/string.h>
>>   #include <xen/xmalloc.h>
>>   +#include <asm/intel-family.h>
>>   #include <asm/msr.h>
>>   #include <asm/processor.h>
>>   #include <asm/system.h>
>> @@ -273,6 +274,35 @@ 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 ( cpu_sig->rev < 0 || mc->rev < 0 )
>
> cpu_sig->rev can't be negative as it's unsigned int.
> I guess that's something to fix in our struct cpu_signature and how we
> get it. Though the structure is shared with AMD, and I don't know if
> it also uses "negative" revisions for non-production ucode (PPR
> doesn't tell anything in that regard).

Oops, yes.  To answer Jan's question, GCC was perfectly happy with the
logic in this form.

The field needs casting locally.  It's officially unsigned according to
both Intel and AMD, despite Intel actually using it as a sign-magnitude
number indicating debugness.

>
>> +        return true;
>> +
>> +    /*
>> +     * GNR98.  Granite Rapids systems hang when loading new ucode on
>> +     * sufficiently old firmware.
>> +     */
>> +    if ( boot_cpu_data.vfm == INTEL_GRANITERAPIDS_X &&
>> +         boot_cpu_data.stepping == 1 && (cpu_sig->pf & 0x95) &&
>> +         cpu_sig->rev < 0x01000405 &&
>> +         mc->rev      > 0x01000405 )
>> +    {
>> +        printk_once(XENLOG_WARNING
>> +                    "microcode: Granite Rapids erratum GNR98
>> detected.  Skipping ucode 0x%08x\n"
>> +                    "microcode: Firmware update recommended\n",
>> mc->rev);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static int cf_check intel_compare(
>>       const struct microcode_patch *old, const struct microcode_patch
>> *new)
>>   {
>> @@ -365,6 +395,7 @@ static struct microcode_patch *cf_check
>> intel_ucode_parse(
>>            * one with higher revision.
>>            */
>>           if ( microcode_fits_cpu(mc) &&
>> +             microcode_safe_to_load(mc) &&
>>                (!saved || compare_revisions(saved->rev, mc->rev) ==
>> NEW_UCODE) )
>>               saved = mc;
>>   
>
> The rest looks good to me; as my concern is unrelated to change itself
> (and doesn't affect production microcode anyway).
>
> Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Thanks.

~Andrew