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

On 12/21/2024 1:11 AM, Borislav Petkov wrote:
>>  
>> -	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.
> 

Sorry, this sneaked through. I was supposed to modify the comment before
sending it out. For this RFC, I was looking for suggestions since there
didn't seem a straight forward way to convert sig to VFM.

But as Dave mentioned in the other thread, maybe we don't need to redo
CPUID in the first place and this code can reuse cpu_data(). I'll
explore that path first.

Sohil