| Message ID | 20241220213711.1892696-1-sohil.mehta@intel.com |
|---|---|
| Headers | show |
| Series | Prepare for new Intel family models | expand |
On 12/20/24 13:36, Sohil Mehta wrote: > detect_init_APIC() limits the APIC detection to families 6 and 15. > Extend the check to family numbers beyond 15. Also, convert it to a VFM > check to make it simpler. This changelog doesn't _quite_ fit the code. I'd rather it was a bit more precise. It does not _just_ expand to "numbers beyond 15". Also, It doesn't _really_ help to have the changelog tell us the function name. That's why we have diff -p. How about: APIC detection is currently limited to a few specific families and will not match the upcoming families >=18. Extend the check to include all families 6 or greater. Also convert it to a VFM check to make it simpler. > arch/x86/kernel/apic/apic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index c5fb28e6451a..13dac8f78213 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -2014,8 +2014,8 @@ static bool __init detect_init_APIC(void) > case X86_VENDOR_HYGON: > break; > case X86_VENDOR_INTEL: > - if (boot_cpu_data.x86 == 6 || boot_cpu_data.x86 == 15 || > - (boot_cpu_data.x86 == 5 && boot_cpu_has(X86_FEATURE_APIC))) > + if ((boot_cpu_data.x86 == 5 && boot_cpu_has(X86_FEATURE_APIC)) || > + boot_cpu_data.x86_vfm >= INTEL_PENTIUM_PRO) > break; > goto no_apic; > default: With that changelog tweak: Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
On 12/20/24 13:36, Sohil Mehta wrote: > X86_FEATURE_REP_GOOD is only set for family 6 processors. Extend the > check to family numbers beyond 15. Could you explain why, please? > It is uncertain whether the Pentium 4s (family 15) should set the > feature flag as well. Commit 185f3b9da24c ("x86: make intel.c have > 64-bit support code") that originally set X86_FEATURE_REP_GOOD also set > the x86_cache_alignment preference for family 15 processors. The > omission of the family 15 seems intentional. > > Also, the 32-bit user copy alignment preference is only set for family 6 > and 15 processors. Extend the preference to family numbers beyond 15. Can you please provide some more context so it's clear which hunk this refers to? Alternatively, can you break this out into a separate patch?
> ---Noteworthy quirks--- > Pentium II Overdrive - A unique family number: > Wikipedia says[2], In Intel's "Family/Model/Stepping" scheme, the Pentium II > OverDrive CPU identifies itself as family 6, model 3, though this is > misleading, as it is not based on the family 6/model 3 Klamath core. As > mentioned in the Pentium II Processor update documentation from Intel, > "although this processor has a CPUID of 163xh, it uses a Pentium II processor > CPUID 065xh processor core." > > A dump of the microcode file 06-03-02 shows: > 001/001: sig 0x00001632, pf_mask 0x00, 1998-06-10, rev 0x0002, size 2048 > An archived CPUID dump [3] also says: > CPUID 00000001: 00001632-00000000-00000000-0183FBFF > > That would translate to a family number of 22 (0x16). This aberration is > not explicitly handled anywhere in the kernel so the platform might already > be broken. This series might make it worse for the platform if the latest > kernel works on it by chance. Are you sure? Bits 13:12 are the type field, and the 0x1 you've got is for an OverDrive processor. x86_family() will consider this to be family 6 as far as I can see. ~Andrew
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/20/24 13:37, Sohil Mehta wrote: > The current Intel family-model checks in the coretemp driver seem to > implicitly assume family 6. Extend the checks to include the extended > family numbers beyond 15 as well. > > Also, add explicit checks for family 6 in places where it is assumed > implicitly. > > x86_model checks seem inconsistent and scattered throughout the driver. > Consolidating and converting them to VFM ones would be a useful addition > in future. > That seems to be irrelevant for the patch description. > Signed-off-by: Sohil Mehta <sohil.mehta@intel.com> Acked-by: Guenter Roeck <linux@roeck-us.net> The patch is independent, but since it is submitted as part of a series and there is no comment suggesting otherwise, I assume that it is expected to be pushed together with that series, and I won't take it into the hwmon branch. Guenter
On 12/21/2024 9:27 AM, Guenter Roeck wrote: >> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com> > > Acked-by: Guenter Roeck <linux@roeck-us.net> > Thank you for the Ack. > The patch is independent, but since it is submitted as part of a series > and there is no comment suggesting otherwise, I assume that it is expected > to be pushed together with that series, and I won't take it into the hwmon > branch. > This first RFC version was mainly intended to be a conversation starter. I am not very familiar with hwmon and my testing has been fairly limited. I was hoping we can get at least one tested-by on this patch before merging it. I have tried to keep all the patches independent to make it easier to merge whenever they seem ready. Please feel free to merge the patch if it seems so. Sohil
On 12/20/2024 4:29 PM, Andrew Cooper wrote: >> >> A dump of the microcode file 06-03-02 shows: >> 001/001: sig 0x00001632, pf_mask 0x00, 1998-06-10, rev 0x0002, size 2048 >> An archived CPUID dump [3] also says: >> CPUID 00000001: 00001632-00000000-00000000-0183FBFF >> >> That would translate to a family number of 22 (0x16). This aberration is >> not explicitly handled anywhere in the kernel so the platform might already >> be broken. This series might make it worse for the platform if the latest >> kernel works on it by chance. > > Are you sure? Bits 13:12 are the type field, and the 0x1 you've got is > for an OverDrive processor. > You are right. This is indeed the processor type field. I should have spent more time reading the specification than Wikipedia :) > x86_family() will consider this to be family 6 as far as I can see. > On the bright side, we don't have to worry about breaking this CPU. Sohil
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
On 12/20/2024 3:13 PM, Dave Hansen wrote: > On 12/20/24 13:36, Sohil Mehta wrote: >> detect_init_APIC() limits the APIC detection to families 6 and 15. >> Extend the check to family numbers beyond 15. Also, convert it to a VFM >> check to make it simpler. > > This changelog doesn't _quite_ fit the code. I'd rather it was a bit > more precise. It does not _just_ expand to "numbers beyond 15". > > Also, It doesn't _really_ help to have the changelog tell us the > function name. That's why we have diff -p. How about: > > APIC detection is currently limited to a few specific families > and will not match the upcoming families >=18. > > Extend the check to include all families 6 or greater. Also > convert it to a VFM check to make it simpler. > >> arch/x86/kernel/apic/apic.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) ... > With that changelog tweak: > > Acked-by: Dave Hansen <dave.hansen@linux.intel.com> Thanks for the Ack! The updated changelog does make it more precise. I'll use that.
On 12/20/2024 3:27 PM, Dave Hansen wrote: > On 12/20/24 13:36, Sohil Mehta wrote: >> X86_FEATURE_REP_GOOD is only set for family 6 processors. Extend the >> check to family numbers beyond 15. > > Could you explain why, please? > To answer this I was trying to understand where Fast string (X86_FEATURE_REP_GOOD) is used. It looks like copy_page() in lib/copy_page_64.S is the only place it really matters. clear_page() in include/asm/page_64.h would likely use Enhanced fast strings (ERMS) if available. Would it be correct to say that copy_page() and potentially clear_page() would be slower on Family 18/19 CPUs without the fix? >> It is uncertain whether the Pentium 4s (family 15) should set the >> feature flag as well. Commit 185f3b9da24c ("x86: make intel.c have >> 64-bit support code") that originally set X86_FEATURE_REP_GOOD also set >> the x86_cache_alignment preference for family 15 processors. The >> omission of the family 15 seems intentional. >> Analyzing more, it seems the below check in early_init_intel() is not really effective. /* * If fast string is not enabled in IA32_MISC_ENABLE for any reason, * clear the fast string and enhanced fast string CPU capabilities. */ if (c->x86_vfm >= INTEL_PENTIUM_M_DOTHAN) { rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable); if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) { pr_info("Disabled fast string operations\n"); setup_clear_cpu_cap(X86_FEATURE_REP_GOOD); setup_clear_cpu_cap(X86_FEATURE_ERMS); } } It gets overridden later in intel_init() with the below code: if (c->x86 == 6) set_cpu_cap(c, X86_FEATURE_REP_GOOD); Shouldn't the order of this be the other way around? >> Also, the 32-bit user copy alignment preference is only set for family 6 >> and 15 processors. Extend the preference to family numbers beyond 15. > > Can you please provide some more context so it's clear which hunk this > refers to? Alternatively, can you break this out into a separate patch? > This is referring to the below chunk. Separating it seems like a better idea to avoid ambiguity. > - > #ifdef CONFIG_X86_INTEL_USERCOPY > /* > * Set up the preferred alignment for movsl bulk memory moves > + * Family 4 - 486: untested > + * Family 5 - Pentium: untested > + * Family 6 - PII/PIII only like movsl with 8-byte alignment > + * Family 15 - P4 is OK down to 8-byte alignment > */ > - switch (c->x86) { > - case 4: /* 486: untested */ > - break; > - case 5: /* Old Pentia: untested */ > - break; > - case 6: /* PII/PIII only like movsl with 8-byte alignment */ > - movsl_mask.mask = 7; > - break; > - case 15: /* P4 is OK down to 8-byte alignment */ > + if (c->x86_vfm >= INTEL_PENTIUM_PRO) > movsl_mask.mask = 7; > - break; > - } > #endif
---Tl;Dr--- Audit all the Intel family model checks to get ready for the upcoming family 18 and 19 models. Patches 1-5: Fixes in arch/x86 and drivers Patches 6-15: Cleanups in arch/x86 to convert x86_model checks to VFM ones. This series does not include cleanups in drivers/ ---Background--- Mainstream Intel processors have been using family number 6 for a couple of decades. The last deviation from this were the Netburst architecture based Pentium 4s that had a family number of 15. Intel has recently started to introduce extended family numbers greater than 15 [1]. Though newer CPUs can have any family number, the currently planned CPUs lie in families 18 and 19. Some kernel code assumes that the family number would always remain 6. There are checks that apply to recent family 6 models such as Lunar Lake and Clearwater Forest but don't automatically extend to family 19 models such as Diamond Rapids. This series aims to fix and cleanup all of such Intel specific checks (mainly in arch/x86) to avoid such issues in the future. It also converts almost all of the x86_model checks in arch/x86 to the new VFM ones. OTOH, x86_model usage in drivers/ is a huge mess. Some drivers might need to be completely rewritten to make them future proof. I have attempted a couple of fixes in cpufreq and hwmon but I am uncertain of their efficacy. A more thorough clean up of drivers is needed to replace all x86_model usage with the new VFM checks. ---Assumptions and Trade-offs--- Newer CPUs will have model numbers only in family 6 or after family 15. No new processors would be added between family 6 and 15. There are a couple of related quirks that have been described below. As a convention, Intel family numbers are referenced using decimals (family 15, 19, etc.) even though the AMD specific code might prefer hexadecimals in similar situations. It would be preferable to have simpler and more maintainable checks that might in a rare situation change the behavior on really old platforms. If someone pops up with an issue, the code would be fixed later. For example, the check, c->x86_vfm >= INTEL_PENTIUM_PRO is preferred over, c->x86 == 6 || c->x86 > 15 if the likelihood of adversely affecting family 15 is low. Also, the CPU defines in intel-family.h have been added as and when needed to make reviewing and applying patches out of order easier. Please let me know if it would be preferable to add all the defines in a single patch at the beginning. ---Noteworthy quirks--- Pentium II Overdrive - A unique family number: Wikipedia says[2], In Intel's "Family/Model/Stepping" scheme, the Pentium II OverDrive CPU identifies itself as family 6, model 3, though this is misleading, as it is not based on the family 6/model 3 Klamath core. As mentioned in the Pentium II Processor update documentation from Intel, "although this processor has a CPUID of 163xh, it uses a Pentium II processor CPUID 065xh processor core." A dump of the microcode file 06-03-02 shows: 001/001: sig 0x00001632, pf_mask 0x00, 1998-06-10, rev 0x0002, size 2048 An archived CPUID dump [3] also says: CPUID 00000001: 00001632-00000000-00000000-0183FBFF That would translate to a family number of 22 (0x16). This aberration is not explicitly handled anywhere in the kernel so the platform might already be broken. This series might make it worse for the platform if the latest kernel works on it by chance. Xeon Phi coprocessors: The first couple of Knights line of products (Knights Ferry and Knights Corner) have a family number of 11. But they were released purely as coprocessors and would not directly boot mainline Linux. The later versions Knights Landing and Knights Mill could act as a host processor but they used family number 6. So, the changes in this series should not affect any of these platforms. ---Links--- [1]: commit d1fb034b75a8 ("x86/cpu: Add two Intel CPU model numbers") [2]: https://en.wikipedia.org/wiki/Pentium_II#Pentium_II_OverDrive [3]: https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel0000632_P2OD_CPUID.txt The series has only been lightly tested on a couple of family 6 systems. Planning to test it on more systems in the future. Any testing on legacy platforms would be greatly helpful. Many thanks to Tony and Dave for guiding me through this historical maze. Sohil Mehta (15): x86/apic: Fix 32-bit APIC initialization for extended Intel families x86/apic: Fix smp init delay for extended Intel families x86/cpu/intel: Fix init_intel() checks for extended family numbers cpufreq: Fix the efficient idle check for Intel extended families hwmon: Fix Intel family checks to include extended family numbers x86/microcode: Update the Intel processor flag scan check x86/mtrr: Modify a x86_model check to an Intel VFM check x86/cpu/intel: Replace early family 6 checks with VFM ones x86/cpu/intel: Replace family 15 checks with VFM ones x86/cpu/intel: Replace family 5 model checks with VFM ones x86/pat: Replace Intel model checks with VFM ones x86/acpi/cstate: Improve Intel family model checks x86/cpu/intel: Bound the non-architectural constant_tsc model checks perf/x86: Simplify p6_pmu_init() perf/x86/p4: Replace Pentium 4 model checks with VFM ones arch/x86/events/intel/p4.c | 7 ++-- arch/x86/events/intel/p6.c | 28 ++++--------- arch/x86/include/asm/intel-family.h | 16 ++++++++ arch/x86/kernel/acpi/cstate.c | 8 ++-- arch/x86/kernel/apic/apic.c | 4 +- arch/x86/kernel/cpu/intel.c | 58 ++++++++++++--------------- arch/x86/kernel/cpu/microcode/intel.c | 3 +- arch/x86/kernel/cpu/mtrr/generic.c | 4 +- arch/x86/kernel/smpboot.c | 8 ++-- arch/x86/mm/pat/memtype.c | 7 ++-- drivers/cpufreq/cpufreq_ondemand.c | 13 +++--- drivers/hwmon/coretemp.c | 26 ++++++++---- 12 files changed, 96 insertions(+), 86 deletions(-) base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8