Re: [PATCH v3] x86/microcode/intel: Reject problematic loading on Granite Rapids systems

[Ingo Molnar <mingo@kernel.org>]

* Chang S. Bae <chang.seok.bae@intel.com> wrote:

> Microcode updates can usually jump revisions. However, there is an
> erratum on Granite Rapids systems. If they "jump over" revision
> 0x1000405, they result in #MC. Avoid it.
> 
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> ---
> V2 -> V3:
> * Shorten the changelog and rename the function (Boris)
> * Reduce the code comment (Dave)
> * Allow 0x1000405 loading. Thanks to Andrew, this fix got attention.
> * Collect Dave review tag. Thanks, Dave!
> 
> Note:
> * GNR98 currently describes loading 0x1000405 itself is unsafe, but it
>   will be updated to say okay with that. I will watch out the GNR98
>   changes.
> * Jumping from < 0x1000380 to 0x1000405 was identified as an issue, but
>   0x1000380 is the first revision as GNR products. So loading 0x1000405
>   in production systems should be okay.
> ---
>  arch/x86/kernel/cpu/microcode/intel.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 1142183c950c..30a22388d4b1 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -309,6 +309,26 @@ static void save_microcode_patch(struct microcode_intel *patch)
>  		pr_err("Unable to allocate microcode memory size: %u\n", size);
>  }
>  
> +static bool revision_is_safe(struct cpu_signature *sig, u32 rev)
> +{
> +	u32 vfm = IFM(x86_family(sig->sig), x86_model(sig->sig));
> +
> +	/*
> +	 * Erratum GNR98 can cause #MC's if "jumping over" revision 0x1000405.
> +	 * Avoid the jumps.
> +	 */
> +	if (vfm == INTEL_GRANITERAPIDS_X &&
> +	    x86_stepping(sig->sig) == 1 &&
> +	    sig->pf & 0x95 &&
> +	    sig->rev < 0x1000405 &&
> +	    rev > 0x1000405) {
> +		pr_err_once("Erratum GNR98: skipping revision 0x%x.\n", rev);

So this was explained in a really confusing way, I had to read the
changelog and comments trice and then the code to figure out what's
going on:

 - There's a microcode bug that makes it unsafe to apply current
   post-0x1000405 revisions on Granite Rapid CPUs if the current
   microcode version is below 0x1000405. The interim 0x1000405
   version *must* be applied first for it to be safe to upgrade
   GNR CPUs. Will this be a problem perpetually? Will it be unsafe
   to have an older GNR CPU and simply apply fresh microcode to it,
   without first loading the interim 0x1000405 version? Will GNR
   microcode upgrades on pre-0x1000405 CPUs will always be a
   two-step process?

 - The new code doesn't declare it, but this patch creates a hidden,
   permanent microcode version upgrade barrier if user-space
   firmware/microcode tools do not provide the 0x1000405
   microcode version reliably and implement the two-step upgrade
   workaround.

 - The message the kernel prints is rather passive-aggressive as well:

		Erratum GNR98: skipping revision 0x%x

   It does not explain *why* the fresh microcode upgrade is skipped,
   and if firmware tooling does not apply 0x1000405 then the kernel
   stays in this state indefinitely.

   It should at minimum say something like:

      Erratum GNR98: new revision %x is unsafe to apply until 0x1000405 is applied first, skipping it. Please upgrade firmware tools.

   Because, presumably, this scenario should not be possible with
   new user-space tooling, right?

   The comment and changelog should be updated accordingly as well.

So this patch is still problematic IMO.

Thanks,

	Ingo