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

[Chao Gao]

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