The check whether to read IA32_PLATFORM_ID MSR is misleading. It doesn't
seem to consider family while comparing the model number. This works
because init_intel_microcode() bails out if the processor family is less
than 6. It is better to update the current check to specifically include
family 6.

Ideally, a VFM check would make it more readable. But, there isn't a
macro to derive VFM from sig.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index f3d534807d91..734819a12d5f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -74,7 +74,8 @@ void intel_collect_cpu_info(struct cpu_signature *sig)
 	sig->pf = 0;
 	sig->rev = intel_get_microcode_revision();
 
-	if (x86_model(sig->sig) >= 5 || x86_family(sig->sig) > 6) {
+	/* TODO: Simplify this using a VFM check? */
+	if ((x86_family(sig->sig) == 6 && x86_model(sig->sig) >= 5) || x86_family(sig->sig) > 6) {
 		unsigned int val[2];
 
 		/* get processor flags from MSR 0x17 */
-- 
2.43.0

On 12/20/24 13:37, Sohil Mehta wrote:
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -74,7 +74,8 @@ void intel_collect_cpu_info(struct cpu_signature *sig)
>  	sig->pf = 0;
>  	sig->rev = intel_get_microcode_revision();
>  
> -	if (x86_model(sig->sig) >= 5 || x86_family(sig->sig) > 6) {
> +	/* TODO: Simplify this using a VFM check? */
> +	if ((x86_family(sig->sig) == 6 && x86_model(sig->sig) >= 5) || x86_family(sig->sig) > 6) {
>  		unsigned int val[2];
>  
>  		/* get processor flags from MSR 0x17 */

I suspect this code is kinda bogus in the first place. sig->sig is just
cpuid_eax(1) and:

	void cpu_detect(struct cpuinfo_x86 *c)
	{
		...
                cpuid(0x00000001, &tfms, &misc, &junk, &cap0);
                c->x86          = x86_family(tfms);

So I'm not quite sure why this code feels the need to redo CPUID and
re-parse it. Other bits of the microcode update may need 'sig' in its
unparsed form to conveniently compare with parts of the microcode image,
but that's no reason to re-parse it too.

I _think_ this code can just use good old cpu_data() and the existing
VFM mechanism.

On Fri, Dec 20, 2024 at 09:37:01PM +0000, Sohil Mehta wrote:
> The check whether to read IA32_PLATFORM_ID MSR is misleading. It doesn't
> seem to consider family while comparing the model number. This works
> because init_intel_microcode() bails out if the processor family is less
> than 6. It is better to update the current check to specifically include
> family 6.
> 
> Ideally, a VFM check would make it more readable. But, there isn't a
> macro to derive VFM from sig.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
>  arch/x86/kernel/cpu/microcode/intel.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index f3d534807d91..734819a12d5f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -74,7 +74,8 @@ void intel_collect_cpu_info(struct cpu_signature *sig)
>  	sig->pf = 0;
>  	sig->rev = intel_get_microcode_revision();
>  
> -	if (x86_model(sig->sig) >= 5 || x86_family(sig->sig) > 6) {
> +	/* TODO: Simplify this using a VFM check? */

No TODOs. Take your time and do it right from the get-go please.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette