On Mon, Nov 21, 2016 at 05:06:54PM +0100, Borislav Petkov wrote: > IOW, what's the worst thing that can happen if we did this below? > > We basically get rid of the detection and switch the timer to broadcast > mode immediately on the halting CPU. > > amd_e400_idle() is behind an "if (cpu_has_bug(c, X86_BUG_AMD_APIC_C1E))" > check so it will run on the affected CPUs only... > > Thoughts?
Actually, here's a better version. The E400 detection works only after ACPI has been enabled so we piggyback the end of acpi_init(). We don't need the MSR read now - we do if (static_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) on the idle path which is as fast as it gets. Any complaints about this before I go and test it everywhere? It builds and boots in my guest here ok, not that it means a whole lot. The good news is, I have collected a fleet of boxes which all have that erratum so testing should be pretty reliable. Something that doesn't happen everytime! :-) --- diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h index 5391b0ae7cc3..aa8c879e814f 100644 --- a/arch/x86/include/asm/acpi.h +++ b/arch/x86/include/asm/acpi.h @@ -94,7 +94,8 @@ static inline unsigned int acpi_processor_cstate_check(unsigned int max_cstate) boot_cpu_data.x86_model <= 0x05 && boot_cpu_data.x86_mask < 0x0A) return 1; - else if (amd_e400_c1e_detected) + + else if (boot_cpu_has(X86_BUG_AMD_APIC_C1E)) return 1; else return max_cstate; @@ -131,6 +132,7 @@ static inline bool acpi_has_cpu_in_madt(void) return !!acpi_lapic; } +extern void __init __weak arch_post_acpi_init(void); #else /* !CONFIG_ACPI */ #define acpi_lapic 0 diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h index 1d2b69fc0ceb..b3918609f6f1 100644 --- a/arch/x86/include/asm/cpufeature.h +++ b/arch/x86/include/asm/cpufeature.h @@ -204,6 +204,7 @@ static __always_inline __pure bool _static_cpu_has(u16 bit) #define static_cpu_has_bug(bit) static_cpu_has((bit)) #define boot_cpu_has_bug(bit) cpu_has_bug(&boot_cpu_data, (bit)) +#define setup_clear_cpu_bug(bit) setup_clear_cpu_cap((bit)) #define MAX_CPU_FEATURES (NCAPINTS * 32) #define cpu_have_feature boot_cpu_has diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 1f6a92903b09..d98c5c432209 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -636,7 +636,6 @@ extern void select_idle_routine(const struct cpuinfo_x86 *c); extern void init_amd_e400_c1e_mask(void); extern unsigned long boot_option_idle_override; -extern bool amd_e400_c1e_detected; enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT, IDLE_POLL}; diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index ee023919e476..56653b7322f9 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -289,9 +289,6 @@ void stop_this_cpu(void *dummy) halt(); } -bool amd_e400_c1e_detected; -EXPORT_SYMBOL(amd_e400_c1e_detected); - static cpumask_var_t amd_e400_c1e_mask; void amd_e400_remove_cpu(int cpu) @@ -301,26 +298,14 @@ void amd_e400_remove_cpu(int cpu) } /* - * AMD Erratum 400 aware idle routine. We check for C1E active in the interrupt - * pending message MSR. If we detect C1E, then we handle it the same - * way as C3 power states (local apic timer and TSC stop) + * AMD Erratum 400 aware idle routine. + * + * + * We handle it the same way as C3 power states (local apic timer and TSC stop) */ static void amd_e400_idle(void) { - if (!amd_e400_c1e_detected) { - u32 lo, hi; - - rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi); - - if (lo & K8_INTP_C1E_ACTIVE_MASK) { - amd_e400_c1e_detected = true; - if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC)) - mark_tsc_unstable("TSC halt in AMD C1E"); - pr_info("System has AMD C1E enabled\n"); - } - } - - if (amd_e400_c1e_detected) { + if (static_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) { int cpu = smp_processor_id(); if (!cpumask_test_cpu(cpu, amd_e400_c1e_mask)) { @@ -419,6 +404,30 @@ void __init init_amd_e400_c1e_mask(void) zalloc_cpumask_var(&amd_e400_c1e_mask, GFP_KERNEL); } +void __init arch_post_acpi_init(void) +{ + u32 lo, hi; + + if (!boot_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) + return; + + /* + * AMD E400 detection needs to happen *after* ACPI has been enabled. We + * check for C1E active in the interrupt pending message MSR. If we detect + * C1E enabled, machine is affected. + */ + rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi); + + if (!(lo & K8_INTP_C1E_ACTIVE_MASK)) { + setup_clear_cpu_bug(X86_BUG_AMD_APIC_C1E); + return; + } + + if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC)) + mark_tsc_unstable("TSC halt in AMD C1E"); + pr_info("System has AMD C1E enabled\n"); +} + static int __init idle_setup(char *str) { if (!str) diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 56190d00fd87..da42fe354ffc 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -34,6 +34,7 @@ #include <linux/reboot.h> #include <linux/delay.h> #ifdef CONFIG_X86 +#include <asm/acpi.h> #include <asm/mpspec.h> #endif #include <linux/acpi_iort.h> @@ -1166,6 +1167,13 @@ static int __init acpi_bus_init(void) struct kobject *acpi_kobj; EXPORT_SYMBOL_GPL(acpi_kobj); +/* + * Arch-specific one to override this. + */ +void __init __weak arch_post_acpi_init(void) +{ +} + static int __init acpi_init(void) { int result; @@ -1198,6 +1206,9 @@ static int __init acpi_init(void) acpi_debugger_init(); acpi_setup_sb_notify_handler(); acpi_set_processor_mapping(); + + arch_post_acpi_init(); + return 0; } diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 2237d3f24f0e..8eec3255d6f2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -141,7 +141,7 @@ static void lapic_timer_check_state(int state, struct acpi_processor *pr, if (cpu_has(&cpu_data(pr->id), X86_FEATURE_ARAT)) return; - if (amd_e400_c1e_detected) + if (boot_cpu_has(X86_BUG_AMD_APIC_C1E)) type = ACPI_STATE_C1; /* -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.