| Message ID | 20241220213711.1892696-7-sohil.mehta@intel.com |
|---|---|
| State | New |
| 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.
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
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(-)