linux-kernel - Re: [PATCH v4 2/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 for Android: free password hash cracker in your pocket

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

Message-ID: <776e439c-60b6-474e-ac47-f33357c272de@intel.com>
Date: Wed, 13 Aug 2025 13:46:33 -0700
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Dave Hansen <dave.hansen@...el.com>, <x86@...nel.org>
CC: <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>, <colinmitchell@...gle.com>,
	<chao.gao@...el.com>, <abusse@...zon.de>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/6] x86/microcode/intel: Establish staging control
 logic

On 8/13/2025 11:21 AM, Dave Hansen wrote:
> On 8/13/25 10:26, 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.
> ...
>>   static inline unsigned int topology_amd_nodes_per_pkg(void) { return 1; }
>> +#define cpu_primary_thread_mask cpu_none_mask
>>   #endif /* !CONFIG_SMP */
> 
> Isn't 'cpu_none_mask' a mask containing no CPUs? How can that possible
> work here:
> 
> 	for_each_cpu(cpu, cpu_primary_thread_mask) {
> 
> ? Wouldn't it just not run through the for loop at all on CONFIG_SMP=n?
> Is that what we want for some reason? I would have thought that we'd
> still want to find the MMIO address for CPU 0, the one and only CPU.

Yeah, right.

Then, looking at it again, I see this:

config MICROCODE_LATE_LOADING
	bool "Late microcode loading (DANGEROUS)"
	default n
	depends on MICROCODE && SMP

This optimization only applies to the late-loading path. But, today I 
also had to clarify this dependency for myself. At least, my changelog 
could've made it clearer, sorry.

>> +
>> +	pr_info("Staging of patch revision 0x%x succeeded.\n",
>> +		((struct microcode_header_intel *)ucode_patch_late)->rev);
>> +}
> Hmmm. Consider:
> 
> 	static struct microcode_intel *ucode_patch_late __read_mostly;
> 
> and:
> 
> 	struct microcode_intel {
> 	        struct microcode_header_intel   hdr;
> 	        unsigned int                    bits[];
> 	};
> 
> So isn't this whole ugly cast thing equivalent to:
> 
> 	ucode_patch_late->hdr.rev
> 
> ?

Indeed. I must have been blind to that bit of ugliness. Thanks for 
spotting on it!

Chang