| Message ID | 20210916131739.1260552-1-kuba@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | x86/intel: Disable HPET on another Intel Coffee Lake platform | expand |
On Thu, 16 Sep 2021 09:35:47 -0700 Paul E. McKenney wrote: > > > How did you pick v5.13? force_disable_hpet() was added by > > > 62187910b0fc ("x86/intel: Add quirk to disable HPET for the Baytrail > > > platform"), which appeared in v3.15. > > > > Erm, good question, it started happening for me (and others with the > > same laptop) with v5.13. I just sort of assumed it was 2e27e793e280 > > ("clocksource: Reduce clocksource-skew threshold"). > > > > It usually takes a day to repro (4 hours was the quickest repro I've > > seen) so bisection was kind of out of question. > > OK, so this is an intermittent condition where HPET is sometimes slow to > access for a short period of time? If that is the case, my thought is > to set the clocksource to be reinitialized (without a splat and without > marking the clocksource unstable), and to splat (and mark the clocksource > unstable) if it is not get a good read after 100 subsequent attempts. > > So as long as the period of slowness lasts for less than 50 seconds, > things would work fine. > > Seem reasonable? Could well be. Initially I thought it was suspend/resume related, then I looked closer and it did happen mostly after resume... but anywhere between 20 minutes to few hours after the resume. I'm here to test less crude patches but since that may take some time I'd hope we can get this merged and into stable ASAP. Hopefully it can make it to 5.13 while that branch is alive and into Fedora. It really makes Coffee Lake machines pretty much unusable.
On Thu, Sep 16, 2021 at 07:57:13PM -0700, Jakub Kicinski wrote: > On Thu, 16 Sep 2021 09:35:47 -0700 Paul E. McKenney wrote: > > > > How did you pick v5.13? force_disable_hpet() was added by > > > > 62187910b0fc ("x86/intel: Add quirk to disable HPET for the Baytrail > > > > platform"), which appeared in v3.15. > > > > > > Erm, good question, it started happening for me (and others with the > > > same laptop) with v5.13. I just sort of assumed it was 2e27e793e280 > > > ("clocksource: Reduce clocksource-skew threshold"). > > > > > > It usually takes a day to repro (4 hours was the quickest repro I've > > > seen) so bisection was kind of out of question. > > > > OK, so this is an intermittent condition where HPET is sometimes slow to > > access for a short period of time? If that is the case, my thought is > > to set the clocksource to be reinitialized (without a splat and without > > marking the clocksource unstable), and to splat (and mark the clocksource > > unstable) if it is not get a good read after 100 subsequent attempts. > > > > So as long as the period of slowness lasts for less than 50 seconds, > > things would work fine. > > > > Seem reasonable? > > Could well be. Initially I thought it was suspend/resume related, then > I looked closer and it did happen mostly after resume... but anywhere > between 20 minutes to few hours after the resume. > > I'm here to test less crude patches but since that may take some time > I'd hope we can get this merged and into stable ASAP. Hopefully it can > make it to 5.13 while that branch is alive and into Fedora. It really > makes Coffee Lake machines pretty much unusable. Indeed, I could see where your reproducer is at best rather annoying. But a good data point for me either way, so thank you! Thanx, Paul
On Thu, Sep 16, 2021 at 10:07:07AM -0500, Bjorn Helgaas wrote: > This seems to be an ongoing issue, not just a point defect in a single > product, and I really hate the onesy-twosy nature of this. Is there > really no way to detect this issue automatically or fix whatever Linux > bug makes us trip over this? I am no clock expert, so I have > absolutely no idea whether this is possible. X86 is gifted with the grant total of _0_ reliable clocks. Given no accurate time, it is impossible to tell which one of them is broken worst. Although I suppose we could attempt to synchronize against the PMU or MPERF.. We could possibly disable the tsc watchdog for X86_FEATURE_TSC_KNOWN_FREQ && X86_FEATURE_TSC_ADJUST I suppose. And then have people with 'creative' BIOS get to keep the pieces.
On Fri, Sep 17, 2021 at 11:11:49AM +0200, Peter Zijlstra wrote: > On Thu, Sep 16, 2021 at 10:07:07AM -0500, Bjorn Helgaas wrote: > > This seems to be an ongoing issue, not just a point defect in a single > > product, and I really hate the onesy-twosy nature of this. Is there > > really no way to detect this issue automatically or fix whatever Linux > > bug makes us trip over this? I am no clock expert, so I have > > absolutely no idea whether this is possible. > > X86 is gifted with the grant total of _0_ reliable clocks. Given no > accurate time, it is impossible to tell which one of them is broken > worst. Although I suppose we could attempt to synchronize against the > PMU or MPERF.. > > We could possibly disable the tsc watchdog for > X86_FEATURE_TSC_KNOWN_FREQ && X86_FEATURE_TSC_ADJUST I suppose. > > And then have people with 'creative' BIOS get to keep the pieces. Alternatively, we can change what the TSC watchdog does for X86_FEATURE_TSC_ADJUST machines. Instead of checking time against HPET it can check if TSC_ADJUST changes. That should make it more resillient vs HPET time itself being off.
Hi Jakub, [...] > The Intel documentation does not list my device [1], but > linuxhw [2] does, and it seems to list a few more bridges > we do not currently cover (3e31, 3ecc, 3e35, 3e0f). This might be out-of-scope for this particular patch, but it makes me wonder - if we know that these other bridges were identified as having the same issue, would it be prudent to add a quriks for them too? It might save some people a headache and such. Krzysztof
On Fri, 17 Sep 2021 16:00:03 +0200 Krzysztof Wilczyński wrote: > > The Intel documentation does not list my device [1], but > > linuxhw [2] does, and it seems to list a few more bridges > > we do not currently cover (3e31, 3ecc, 3e35, 3e0f). > > This might be out-of-scope for this particular patch, but it makes me > wonder - if we know that these other bridges were identified as having the > same issue, would it be prudent to add a quriks for them too? It might > save some people a headache and such. Indeed, I was about to do it but the disagreement between the Intel docs and the linuxhw device list discouraged me. It's confusing that the most popular SKU in the field (according to linuxhw, which is also the one I have and the one addressed by this patch) is not in the official Intel PDF. And what about the numbers that fall in between the known revisions? Perhaps we could use a mask instead of having 10+ entries? Then there's also a precedent of adding them one by one in the patches quoted my Bjorn. All in all I thought I'd just mention this and let the people familiar with x86 platforms guide me. Or maybe we can wait for patches from Paul or Peter to fix the less popular devices? Not sure.. I'm happy to produce a patch that adds more entries, or for someone else to do it..
On Fri, Sep 17 2021 at 11:34, Peter Zijlstra wrote: > On Fri, Sep 17, 2021 at 11:11:49AM +0200, Peter Zijlstra wrote: >> On Thu, Sep 16, 2021 at 10:07:07AM -0500, Bjorn Helgaas wrote: >> > This seems to be an ongoing issue, not just a point defect in a single >> > product, and I really hate the onesy-twosy nature of this. Is there >> > really no way to detect this issue automatically or fix whatever Linux >> > bug makes us trip over this? I am no clock expert, so I have >> > absolutely no idea whether this is possible. Right, we need to have all these quirks because we can't define a generation cutoff based on family/model because X86 model is simply a random number. There might be some scheme behind it, but it's neither obvious nor documented. But the HPET on the affected machines goes south when the system enters PC10. So the right thing to do is to check whether PC10 is supported and force disable HPET if that's the case. That disablement is required independent of the clocksource watchdog problem because HPET is exposed in other ways as well. Questions for Rafael: What's the proper way to figure out whether PC10 is supported? I got lost in the maze of intel_idle and ACPI muck and several other places which check that. Just grep for CPUID_MWAIT_LEAF and see how consistent all of that is. Why the heck can't we have _ONE_ authoritive implementation of that? Just because, right? Of course all of this is well documented as usual.... >> X86 is gifted with the grant total of _0_ reliable clocks. Given no >> accurate time, it is impossible to tell which one of them is broken >> worst. Although I suppose we could attempt to synchronize against the >> PMU or MPERF.. >> >> We could possibly disable the tsc watchdog for >> X86_FEATURE_TSC_KNOWN_FREQ && X86_FEATURE_TSC_ADJUST I suppose. >> >> And then have people with 'creative' BIOS get to keep the pieces. > > Alternatively, we can change what the TSC watchdog does for > X86_FEATURE_TSC_ADJUST machines. Instead of checking time against HPET > it can check if TSC_ADJUST changes. That should make it more resillient > vs HPET time itself being off. I tried that and I hated the mess it created. Abusing the clocksource watchdog machinery for that is a nightmare. Don't even think about it. When TSC_ADJUST is available then we check the MSR when a CPU goes idle, but not more often than once per second. My concern is that we can't check TSC_ADJUST for modifications on fully loaded CPUs, but why do I still care? The requirements for ditching the watchdog should be: X86_FEATURE_TSC_KNOWN_FREQ && X86_FEATURE_TSC_ADJUST && X86_FEATURE_NONSTOP_TSC && X86_FEATURE_ARAT But expecting X86_FEATURE_TSC_KNOWN_FREQ to be set on these HPET trainwreck equipped machines is wishful thinking: # cpuid -1 -l 0x15 CPU: Time Stamp Counter/Core Crystal Clock Information (0x15): TSC/clock ratio = 176/2 nominal core crystal clock = 0 Hz We calculate that missing frequency then from leaf 0x16: # cpuid -1 -l 0x16 CPU: Processor Frequency Information (0x16): Core Base Frequency (MHz) = 0x834 (2100) Core Maximum Frequency (MHz) = 0x1068 (4200) Bus (Reference) Frequency (MHz) = 0x64 (100) But we don't set the TSC_KNOWN_FREQ feature bit in the case that crystal clock is 0 and we need to use leaf 16. Which is entirely correct because the Core Base Frequency CPUID info is a joke: [ 3.045828] tsc: Refined TSC clocksource calibration: 2111.993 MHz The refined calibration is pretty accurate according to NTP and if you take the CPUID 15/16 numbers into account even obvious with pure math: TSC/clock ratio = 176/2 Core Base Frequency (MHz) = 0x834 (2100) 2100 / (176 / 2) = 23.8636 (MHz) which would be a very unusual crystal frequency. The refined calibration makes a lot more sense: 2112 / (176 / 2) = 24 (MHz) which is one of the very well known crystal frequencies of these machines. It's 2021 now and we are still not able to get reasonable information from hardware/firmware about this? Can the hardware and firmware people finaly get their act together? Here is the simple list of things we are asking for: - Reliable TSC which cannot be tinkered with even by "value add" BIOSes - Reliable information from hardware/firmware about the TSC frequency - Hardware enforced (or firmware assisted) guarantees of TSC being synchronized accross sockets We are asking for that for more than _twenty_ years now. All what we get are useless new features and as demonstrated in the case at hand new types of wreckage instead of a proper solution to the underlying problems. I'm personally dealing with the x86 timer hardware trainwrecks for more than 20 years now. TBH, I'm tired of this idiocy and very close to the point where I stop caring. Thanks, tglx
On 9/19/2021 2:14 AM, Thomas Gleixner wrote: > On Fri, Sep 17 2021 at 11:34, Peter Zijlstra wrote: >> On Fri, Sep 17, 2021 at 11:11:49AM +0200, Peter Zijlstra wrote: >>> On Thu, Sep 16, 2021 at 10:07:07AM -0500, Bjorn Helgaas wrote: >>>> This seems to be an ongoing issue, not just a point defect in a single >>>> product, and I really hate the onesy-twosy nature of this. Is there >>>> really no way to detect this issue automatically or fix whatever Linux >>>> bug makes us trip over this? I am no clock expert, so I have >>>> absolutely no idea whether this is possible. > Right, we need to have all these quirks because we can't define a > generation cutoff based on family/model because X86 model is simply a > random number. There might be some scheme behind it, but it's neither > obvious nor documented. > > But the HPET on the affected machines goes south when the system enters > PC10. So the right thing to do is to check whether PC10 is supported and > force disable HPET if that's the case. That disablement is required > independent of the clocksource watchdog problem because HPET is exposed > in other ways as well. > > Questions for Rafael: > > What's the proper way to figure out whether PC10 is supported? I can't say without research. I think it'd be sufficient to check if C10 is supported, because asking for it is the only way to get PC10. However, even if it is supported, the problem is not there until the kernel asks for C10. So instead, I'd disable the TSC watchdog on the first attempt to ask the processor for C10 from the cpuidle code and I'd do that from the relevant drivers (intel_idle and ACPI idle). There would be no TSC watchdog for the C10 users, but wouldn't that be a fair game? > I got lost in the maze of intel_idle and ACPI muck and several other places > which check that. Just grep for CPUID_MWAIT_LEAF and see how consistent > all of that is. > > Why the heck can't we have _ONE_ authoritive implementation of that? > Just because, right? > > Of course all of this is well documented as usual.... > >>> X86 is gifted with the grant total of _0_ reliable clocks. Given no >>> accurate time, it is impossible to tell which one of them is broken >>> worst. Although I suppose we could attempt to synchronize against the >>> PMU or MPERF.. >>> >>> We could possibly disable the tsc watchdog for >>> X86_FEATURE_TSC_KNOWN_FREQ && X86_FEATURE_TSC_ADJUST I suppose. >>> >>> And then have people with 'creative' BIOS get to keep the pieces. >> Alternatively, we can change what the TSC watchdog does for >> X86_FEATURE_TSC_ADJUST machines. Instead of checking time against HPET >> it can check if TSC_ADJUST changes. That should make it more resillient >> vs HPET time itself being off. > I tried that and I hated the mess it created. Abusing the clocksource > watchdog machinery for that is a nightmare. Don't even think about it. > > When TSC_ADJUST is available then we check the MSR when a CPU goes idle, > but not more often than once per second. My concern is that we can't > check TSC_ADJUST for modifications on fully loaded CPUs, but why do I > still care? > > The requirements for ditching the watchdog should be: > > X86_FEATURE_TSC_KNOWN_FREQ && > X86_FEATURE_TSC_ADJUST && > X86_FEATURE_NONSTOP_TSC && > X86_FEATURE_ARAT > > But expecting X86_FEATURE_TSC_KNOWN_FREQ to be set on these HPET > trainwreck equipped machines is wishful thinking: > > # cpuid -1 -l 0x15 > CPU: > Time Stamp Counter/Core Crystal Clock Information (0x15): > TSC/clock ratio = 176/2 > nominal core crystal clock = 0 Hz > > We calculate that missing frequency then from leaf 0x16: > > # cpuid -1 -l 0x16 > CPU: > Processor Frequency Information (0x16): > Core Base Frequency (MHz) = 0x834 (2100) > Core Maximum Frequency (MHz) = 0x1068 (4200) > Bus (Reference) Frequency (MHz) = 0x64 (100) > > But we don't set the TSC_KNOWN_FREQ feature bit in the case that crystal > clock is 0 and we need to use leaf 16. Which is entirely correct because > the Core Base Frequency CPUID info is a joke: > > [ 3.045828] tsc: Refined TSC clocksource calibration: 2111.993 MHz > > The refined calibration is pretty accurate according to NTP and if you > take the CPUID 15/16 numbers into account even obvious with pure math: > > TSC/clock ratio = 176/2 > Core Base Frequency (MHz) = 0x834 (2100) > > 2100 / (176 / 2) = 23.8636 (MHz) > > which would be a very unusual crystal frequency. The refined calibration > makes a lot more sense: > > 2112 / (176 / 2) = 24 (MHz) > > which is one of the very well known crystal frequencies of these > machines. > > It's 2021 now and we are still not able to get reasonable information > from hardware/firmware about this? > > Can the hardware and firmware people finaly get their act together? > > Here is the simple list of things we are asking for: > > - Reliable TSC which cannot be tinkered with even by "value add" BIOSes > > - Reliable information from hardware/firmware about the TSC frequency > > - Hardware enforced (or firmware assisted) guarantees of TSC being > synchronized accross sockets > > We are asking for that for more than _twenty_ years now. All what we get > are useless new features and as demonstrated in the case at hand new > types of wreckage instead of a proper solution to the underlying > problems. > > I'm personally dealing with the x86 timer hardware trainwrecks for more > than 20 years now. TBH, I'm tired of this idiocy and very close to the > point where I stop caring. > > Thanks, > > tglx
On Tue, Sep 21 2021 at 20:05, Rafael J. Wysocki wrote: > On 9/19/2021 2:14 AM, Thomas Gleixner wrote: >> What's the proper way to figure out whether PC10 is supported? > > I can't say without research. I think it'd be sufficient to check if > C10 is supported, because asking for it is the only way to get PC10. Do we have a common function for that or do I need to implement the gazillionst CPUID query for that? > However, even if it is supported, the problem is not there until the > kernel asks for C10. So instead, I'd disable the TSC watchdog on the > first attempt to ask the processor for C10 from the cpuidle code and I'd > do that from the relevant drivers (intel_idle and ACPI idle). > > There would be no TSC watchdog for the C10 users, but wouldn't that be a > fair game? Not really because that makes any other HPET usage broken as well and we can't pull the rug under that. So we are better off to disable it upfront. Thanks, tglx
On Tue, Sep 21, 2021 at 10:18 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Tue, Sep 21 2021 at 20:05, Rafael J. Wysocki wrote: > > On 9/19/2021 2:14 AM, Thomas Gleixner wrote: > >> What's the proper way to figure out whether PC10 is supported? > > > > I can't say without research. I think it'd be sufficient to check if > > C10 is supported, because asking for it is the only way to get PC10. > > Do we have a common function for that or do I need to implement the > gazillionst CPUID query for that? intel_idle has intel_idle_verify_cstate() that works on MWAIT substates from CPUID. It looks like this could be reused. > > However, even if it is supported, the problem is not there until the > > kernel asks for C10. So instead, I'd disable the TSC watchdog on the > > first attempt to ask the processor for C10 from the cpuidle code and I'd > > do that from the relevant drivers (intel_idle and ACPI idle). > > > > There would be no TSC watchdog for the C10 users, but wouldn't that be a > > fair game? > > Not really because that makes any other HPET usage broken as well and we > can't pull the rug under that. So we are better off to disable it > upfront. Allright.
On Wed, Sep 22 2021 at 22:27, Rafael J. Wysocki wrote: > On Tue, Sep 21, 2021 at 10:18 PM Thomas Gleixner <tglx@linutronix.de> wrote: >> >> On Tue, Sep 21 2021 at 20:05, Rafael J. Wysocki wrote: >> > On 9/19/2021 2:14 AM, Thomas Gleixner wrote: >> >> What's the proper way to figure out whether PC10 is supported? >> > >> > I can't say without research. I think it'd be sufficient to check if >> > C10 is supported, because asking for it is the only way to get PC10. >> >> Do we have a common function for that or do I need to implement the >> gazillionst CPUID query for that? > > intel_idle has intel_idle_verify_cstate() that works on MWAIT > substates from CPUID. It looks like this could be reused. Not to me. That's some cpuidle/intel_idle specific check which depends on cpuidle_state_table being set up which is not available during early boot. The question I was asking whether we have a central place where we can retrieve such information w/o invoking CPUID over and over again and applying voodoo checks on it. Obviously we don't, which sucks. Thanks, tglx
On Thu, Sep 23, 2021 at 12:21 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Wed, Sep 22 2021 at 22:27, Rafael J. Wysocki wrote: > > On Tue, Sep 21, 2021 at 10:18 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >> > >> On Tue, Sep 21 2021 at 20:05, Rafael J. Wysocki wrote: > >> > On 9/19/2021 2:14 AM, Thomas Gleixner wrote: > >> >> What's the proper way to figure out whether PC10 is supported? > >> > > >> > I can't say without research. I think it'd be sufficient to check if > >> > C10 is supported, because asking for it is the only way to get PC10. > >> > >> Do we have a common function for that or do I need to implement the > >> gazillionst CPUID query for that? > > > > intel_idle has intel_idle_verify_cstate() that works on MWAIT > > substates from CPUID. It looks like this could be reused. > > Not to me. That's some cpuidle/intel_idle specific check which depends > on cpuidle_state_table No, it doesn't. The only thing this depends on is mwait_substates which directly comes from the CPUID evaluation in intel_idle_init(). The argument is an MWAIT hint, but it doesn't have to be one, it could be a state number. Anyway, this is just part of what is needed. So the way to check the PC10 support is to get the mask of MWAIT substates from CPUID (like in intel_idle_init()) and check if there are any substates for C10 in that mask and check if PC10 is enabled in MSR_PKG_CST_CONFIG_CONTROL (like in sklh_idle_state_table_update()). The MWAIT substates checking part could then be used by intel_idle (and maybe by ACPI idle too). > being set up which is not available during early boot. > The question I was asking whether we have a central place where we can > retrieve such information w/o invoking CPUID over and over again and > applying voodoo checks on it. > > Obviously we don't, which sucks. Well, nobody except for intel_idle wanted it, so there was not much point doing it centrally for just one user.
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c index 38837dad46e6..7d2de04f8750 100644 --- a/arch/x86/kernel/early-quirks.c +++ b/arch/x86/kernel/early-quirks.c @@ -716,6 +716,8 @@ static struct chipset early_qrk[] __initdata = { PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet}, { PCI_VENDOR_ID_INTEL, 0x3e20, PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet}, + { PCI_VENDOR_ID_INTEL, 0x3e34, + PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet}, { PCI_VENDOR_ID_INTEL, 0x3ec4, PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet}, { PCI_VENDOR_ID_INTEL, 0x8a12,
My Lenovo T490s with i7-8665U had been marking TSC as unstable since v5.13, resulting in very sluggish desktop experience... I have a 8086:3e34 bridge, also known as "Host bridge: Intel Corporation Coffee Lake HOST and DRAM Controller (rev 0c)". Add it to the list. We should perhaps consider applying this quirk more widely. The Intel documentation does not list my device [1], but linuxhw [2] does, and it seems to list a few more bridges we do not currently cover (3e31, 3ecc, 3e35, 3e0f). [1] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/8th-gen-core-family-datasheet-vol-2.pdf [2] https://github.com/linuxhw/DevicePopulation/blob/master/README.md Cc: stable@vger.kernel.org # v5.13+ Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- --- arch/x86/kernel/early-quirks.c | 2 ++ 1 file changed, 2 insertions(+)