linux-kernel - Re: [PATCH 1/6] x86/microcode: Introduce staging option to reduce late-loading latency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b88227ad-f5ed-4228-be08-29a4110a2478@intel.com>
Date: Mon, 17 Feb 2025 23:51:28 -0800
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Borislav Petkov <bp@...en8.de>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 1/6] x86/microcode: Introduce staging option to reduce
 late-loading latency

On 2/17/2025 5:33 AM, Borislav Petkov wrote:
> 
> Why are you even touching this function instead of doing the staging in the
> beginning of load_late_stop_cpus()?

I thought staging is logically distinguishable. While 
load_late_stop_cpus() currently performs loading when CPUs are 
_stopped_, staging occurs on a non-critical path and remains 
interruptible. So, the function name itself seems misaligned with the 
staging process.

It looks like commit 4b753955e915 ("x86/microcode: Add per CPU result 
state") renamed microcode_reload_late() to the current name:

   -/*
   - * Reload microcode late on all CPUs. Wait for a sec until they
   - * all gather together.
   - */
   -static int microcode_reload_late(void)
   +static int load_late_stop_cpus(void)

which primarily narrowed the function’s scope to microcode rendezvous 
for late loading.


Given them all, maybe another option is to introduce a wrapper, instead 
of modifying load_late_locked() directly, like:

@@ -536,11 +536,6 @@ static int load_late_stop_cpus(bool is_safe)
         int old_rev = boot_cpu_data.microcode;
         struct cpuinfo_x86 prev_info;

-       if (!is_safe) {
-               pr_err("Late microcode loading without minimal revision 
check.\n");
-               pr_err("You should switch to early loading, if 
possible.\n");
-       }
-
         atomic_set(&late_cpus_in, num_online_cpus());
         atomic_set(&offline_in_nmi, 0);
         loops_per_usec = loops_per_jiffy / (TICK_NSEC / 1000);
@@ -674,6 +669,20 @@ static bool setup_cpus(void)
         return true;
  }

+static int load_late_apply(bool is_safe)
+{
+       if (!is_safe) {
+               pr_err("Late microcode loading without minimal revision 
check.\n");
+               pr_err("You should switch to early loading, if 
possible.\n");
+       }
+
+       /* Stage microcode without stopping CPUs */
+       if (microcode_ops->use_staging)
+               microcode_ops->stage_microcode();
+
+       return load_late_stop_cpus(is_safe);
+}
+
  static int load_late_locked(void)
  {
         if (!setup_cpus())
@@ -681,9 +690,9 @@ static int load_late_locked(void)

         switch (microcode_ops->request_microcode_fw(0, 
&microcode_pdev->dev)) {
         case UCODE_NEW:
-               return load_late_stop_cpus(false);
+               return load_late_apply(false);
         case UCODE_NEW_SAFE:
-               return load_late_stop_cpus(true);
+               return load_late_apply(true);
         case UCODE_NFOUND:
                 return -ENOENT;
         default:

Thanks,
Chang