Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

From: Ashok Raj <ashok.raj@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting
Date: Thu, 30 Nov 2023 10:34:31 -0800	[thread overview]
Message-ID: <ZWjVt5dNRjbcvlzR@a4bf019067fa.jf.intel.com> (raw)
In-Reply-To: <20231115210212.9981-3-bp@alien8.de>

Hi Boris,

On Wed, Nov 15, 2023 at 10:02:12PM +0100, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> The AMD side of the loader issues the microcode revision for each
> logical thread on the system, which can become really noisy on huge
> machines. And doing that doesn't make a whole lot of sense - the
> microcode revision is already in /proc/cpuinfo.
> 
> So in case one is interested in the theoretical support of mixed silicon
> steppings on AMD, one can check there.
> 
> What is also missing on the AMD side - something which people have
> requested before - is showing the microcode revision the CPU had
> *before* the early update.
> 
> So abstract that up in the main code and have the BSP on each vendor
> provide those revision numbers.
> 
> Then, dump them only once on driver init.
> 
> On Intel, do not dump the patch date - it is not needed.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Link: https://lore.kernel.org/r/CAHk-=wg=%2B8rceshMkB4VnKxmRccVLtBLPBawnewZuuqyx5U=3A@mail.gmail.com
> ---
>  arch/x86/kernel/cpu/microcode/amd.c      | 39 +++++++-----------------
>  arch/x86/kernel/cpu/microcode/core.c     | 12 ++++++--
>  arch/x86/kernel/cpu/microcode/intel.c    | 17 +++++------
>  arch/x86/kernel/cpu/microcode/internal.h | 14 ++++++---
>  4 files changed, 38 insertions(+), 44 deletions(-)
> 

[snip]

>  void load_ucode_ap(void)
> @@ -826,6 +828,12 @@ static int __init microcode_init(void)
>  	if (!microcode_ops)
>  		return -ENODEV;
>  
> +	pr_info_once("Current revision: 0x%08x\n", (early_data.new_rev ?: early_data.old_rev));
> +
> +	if (early_data.new_rev)
> +		pr_info_once("Updated early from: 0x%08x\n",
> +			     early_data.old_rev);

See below, new_rev is always assigned. The above message appears even when
no new microcode was applied.

> +
>  	microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0);
>  	if (IS_ERR(microcode_pdev))
>  		return PTR_ERR(microcode_pdev);
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 6024feb98d29..070426b9895f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -339,16 +339,9 @@ static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
>  static enum ucode_state apply_microcode_early(struct ucode_cpu_info *uci)
>  {
>  	struct microcode_intel *mc = uci->mc;
> -	enum ucode_state ret;
> -	u32 cur_rev, date;
> +	u32 cur_rev;
>  
> -	ret = __apply_microcode(uci, mc, &cur_rev);
> -	if (ret == UCODE_UPDATED) {
> -		date = mc->hdr.date;
> -		pr_info_once("updated early: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
> -			     cur_rev, mc->hdr.rev, date & 0xffff, date >> 24, (date >> 16) & 0xff);
> -	}
> -	return ret;
> +	return __apply_microcode(uci, mc, &cur_rev);
>  }
>  
>  static __init bool load_builtin_intel_microcode(struct cpio_data *cp)
> @@ -413,13 +406,17 @@ static int __init save_builtin_microcode(void)
>  early_initcall(save_builtin_microcode);
>  
>  /* Load microcode on BSP from initrd or builtin blobs */
> -void __init load_ucode_intel_bsp(void)
> +void __init load_ucode_intel_bsp(struct early_load_data *ed)
>  {
>  	struct ucode_cpu_info uci;
>  
> +	ed->old_rev = intel_get_microcode_revision();
> +
>  	uci.mc = get_microcode_blob(&uci, false);
>  	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
>  		ucode_patch_va = UCODE_BSP_LOADED;
> +
> +	ed->new_rev = uci.cpu_sig.rev;

new_rev is always assigned even if there was no microcode to apply.

Feel free to squash this patch if it makes sense.

From: Ashok Raj <ashok.raj@intel.com>
Date: Tue, 28 Nov 2023 15:30:31 -0800
Subject: [PATCH] x86/microcode: Suppress early load message when the revision
 is unchanged

After early loading, early_data.old_rev is always updated, that results in
printing a message even if the revision is unchanged.

Currently, it's displayed as below:

[  113.395868] microcode: Current revision: 0x21000170
[  113.404244] microcode: Updated early from: 0x21000170

This should happen on both AMD and Intel. Although for different reasons.

- On AMD, the ucode is loaded even if the current revision matches what is
  being loaded.
- On Intel, load_ucode_intel_bsp() assigns new_rev unconditionally. So it's
  never 0.

Suppress the "Updated early" message when revision is unchanged.

Fixes: 080990aa3344 ("x86/microcode: Rework early revisions reporting")
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 232026a239a6..18e61ecab005 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -830,7 +830,7 @@ static int __init microcode_init(void)
 
 	pr_info_once("Current revision: 0x%08x\n", (early_data.new_rev ?: early_data.old_rev));
 
-	if (early_data.new_rev)
+	if (early_data.new_rev && early_data.new_rev != early_data.old_rev)
 		pr_info_once("Updated early from: 0x%08x\n", early_data.old_rev);
 
 	microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0);
-- 
2.39.2

