linux-kernel - Re: [PATCH v5 3/7] 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: <641f321d-4266-46c0-9383-d06bcb909529@intel.com>
Date: Thu, 4 Sep 2025 17:04:24 -0700
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>, <chao.gao@...el.com>,
	<abusse@...zon.de>
Subject: Re: [PATCH v5 3/7] x86/microcode/intel: Establish staging control
 logic

On 9/4/2025 5:13 AM, Borislav Petkov wrote:
> On Sat, Aug 23, 2025 at 08:52:06AM -0700, Chang S. Bae wrote:
>>
>> Also, define cpu_primary_thread_mask for the CONFIG_SMP=n case, allowing
>> consistent use when narrowing down primary threads to locate the
>> per-package interface.
> 
> This paragraph is stale now and can go.

Ah, right. Sorry, I had to chop it out.

>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index b65c3ba5fa14..0356155f9264 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -913,6 +913,8 @@
>>   #define MSR_IA32_UCODE_WRITE		0x00000079
>>   #define MSR_IA32_UCODE_REV		0x0000008b
>>   
>> +#define MSR_IA32_MCU_STAGING_MBOX_ADDR	0x000007a5
> 
> Doesn't look sorted to me.

Okay, I think this 'MCU' looks a bit fuzzy. Intel folks already use this 
acronym elsewhere (e.g. MSR_IA32_MCU_OPT_CTRL). It might be clearer to 
consolidate them under one section, like:

/* Intel microcode update MSRs */
#define MSR_IA32_MCU_OPT_CTRL           0x00000123
#define MSR_IA32_MCU_ENUMERATION        0x0000007b  <- patch7 adds this
#define MSR_IA32_MCU_STAGING_MBOX_ADDR  0x000007a5
...

>> +	/*
>> +	 * 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_primary_thread_mask) {
>> +		if (topology_logical_package_id(cpu) == pkg_id)
>> +			continue;
> 
> <---- newline here.

Fixed. Thanks.

>> +		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",
> 
> What does "error state" mean?
> 
> Are we going to dump additional error state so that it is clear why it failed?

Yeah, right. The wording "error state" is vague.

The next two patches in this series introduce helpers that return an 
error code and *also* update this ucode_state. The latter could go away. 
Instead, the error code could be just down through here and decoded like:

static const char *staging_errstr(int code)
{
   switch (code) {
   case -ETIMEDOUT:	return "timeout";
   ...
   default:		return "unknown error";
   }
}

static void stage_microcode(void)
{
   ...

   err = do_stage(mmio_pa);
   if (err) {
     pr_err("Error: staging failed with %s for CPU%d at package %u.\n",
            staging_errstr(err), cpu, pkg_id);
     return;
   }
}

I've attached a diff on top of V5 to picture what it will look like.

View attachment "tmp.diff" of type "text/plain" (5703 bytes)