| Message ID | 20241220213711.1892696-7-sohil.mehta@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Prepare for new Intel family models | expand |
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.
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.
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 */
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(-)