linux-kernel - Re: [PATCH v2a 3/6] x86/microcode/intel: Establish staging control logic

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: <Z+OuPbNGC6mOCHZ1@intel.com>
Date: Wed, 26 Mar 2025 15:35:25 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
	<colinmitchell@...gle.com>
Subject: Re: [PATCH v2a 3/6] x86/microcode/intel: Establish staging control
 logic

>+static void stage_microcode(void)
>+{
>+	unsigned int pkg_id = UINT_MAX;
>+	enum ucode_state ret;
>+	u64 mmio_pa;
>+	int cpu;
>+
>+	if (!IS_ALIGNED(get_totalsize(&ucode_patch_late->hdr), sizeof(u32)))
>+		return;
>+
>+	lockdep_assert_cpus_held();
>+
>+	/*
>+	 * The MMIO address is unique per package, and all the SMT
>+	 * primary threads are online here. Find each MMIO space by
>+	 * their package ids to avoid duplicate staging.
>+	 */
>+	for_each_cpu(cpu, cpu_online_mask) {

for_each_online_cpu(cpu)?

>+		if (!topology_is_primary_thread(cpu) ||
>+		     topology_logical_package_id(cpu) == pkg_id)
>+			continue;

Documentation/arch/x86/topology.rst states:
  - topology_core_cpumask():

    The cpumask contains all online threads in the package to which a thread
    belongs.

    The number of online threads is also printed in /proc/cpuinfo "siblings."

So, how about:

		if (cpu != cpumask_first(topology_core_cpumask(cpu)))
			continue;

and dropping the pkg_id?

>+		pkg_id = topology_logical_package_id(cpu);
>+
>+		rdmsrl_on_cpu(cpu, MSR_IA32_MCU_STAGING_MBOX_ADDR, &mmio_pa);

Note rdmsrl_on_cpu() may return an error. please consider adding
error-handling. Is it possible that somehow one package doesn't support
this staging feature while others do?

>+
>+		ret = do_stage(mmio_pa);
>+		if (ret != UCODE_OK) {
>+			pr_err("Error: staging failed with %s for CPU%d at package %u.\n",
>+			       ret == UCODE_TIMEOUT ? "timeout" : "error state",
>+			       cpu, pkg_id);

Shall we print a message somewhere showing "Continuing updates without
staging"?

It could be confusing for users to see a success message following an error
message that states "Error: staging failed ..."

>+			return;
>+		}
>+	}
>+
>+	pr_info("Staging of patch revision 0x%x succeeded.\n",
>+		((struct microcode_header_intel *)ucode_patch_late)->rev);
>+}
>+
> static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
> 					  struct microcode_intel *mc,
> 					  u32 *cur_rev)
>@@ -648,6 +696,7 @@ static struct microcode_ops microcode_intel_ops = {
> 	.collect_cpu_info	= collect_cpu_info,
> 	.apply_microcode	= apply_microcode_late,
> 	.finalize_late_load	= finalize_late_load,
>+	.stage_microcode	= stage_microcode,
> 	.use_nmi		= IS_ENABLED(CONFIG_X86_64),
> };
> 
>-- 
>2.45.2
>