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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 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.

> +          (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).

Jan