Re: [PATCH v4 2/6] x86/microcode/intel: Establish staging control logic

["Chang S. Bae" <chang.seok.bae@intel.com>]

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