linux-kernel - Re: [PATCH V2] x86/apic: Stop the TSC Deadline timer during lapic timer shutdown

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed43f0b5-0625-4a6b-b986-42583673d857@intel.com>
Date: Wed, 9 Oct 2024 09:49:54 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Zhang Rui <rui.zhang@...el.com>, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de, dave.hansen@...ux.intel.com, rafael.j.wysocki@...el.com,
 x86@...nel.org, linux-pm@...r.kernel.org
Cc: hpa@...or.com, peterz@...radead.org, thorsten.blum@...lux.com,
 yuntao.wang@...ux.dev, tony.luck@...el.com, len.brown@...el.com,
 srinivas.pandruvada@...el.com, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH V2] x86/apic: Stop the TSC Deadline timer during lapic
 timer shutdown

On 10/9/24 00:20, Zhang Rui wrote:
> This 12-year-old bug prevents some modern processors from achieving
> maximum power savings during suspend. For example, Lunar Lake systems
> gets 0% package C-states during suspend to idle and this causes energy
> star compliance tests to fail.

Why haven't we noticed or cared for the last 12 years?

Also, plain language really matters.  Is this as simple as: "you close
the lid on the laptop and the CPU doesn't power down at all"?

> According to Intel SDM, for the local APIC timer,
> 1. "The initial-count register is a read-write register. A write of 0 to
>    the initial-count register effectively stops the local APIC timer, in
>    both one-shot and periodic mode."
> 2. "In TSC deadline mode, writes to the initial-count register are
>    ignored; and current-count register always reads 0. Instead, timer
>    behavior is controlled using the IA32_TSC_DEADLINE MSR."
>    "In TSC-deadline mode, writing 0 to the IA32_TSC_DEADLINE MSR disarms
>    the local-APIC timer."

Is "stopping" and "disarming" the same thing?

Second, while quoting the SDM is great, it would be even better to
including the Linux naming for these things.  The Linux naming for the
APIC registers is completely missing from this changelog.  You could say:

	"In TSC deadline mode, writes to the initial-count register
	(APIC_TMICT) are ignored"

which makes it much easier to relate this code:

        apic_write(APIC_TMICT, 0);

back to the SDM language.  This is especially true because:

#define APIC_TMICT      0x380

doesn't make it obvious that "ICT" is the "Initial-Count Register".  I
had to go back to the SDM table to make 100% sure.

This also doesn't ever say which mode the kernel is running in.

> Stop the TSC Deadline timer in lapic_timer_shutdown() by writing 0 to
> MSR_IA32_TSC_DEADLINE.

This dances around the problem but never comes out and says it:

	The CPU package does not go into lower power modes (higher
	package C-states) unless all local-APIC timers are disabled.

Plus something to connect the old to the new:

	On older CPUs, setting APIC_TMICT=0 was sufficient for disabling
	the local-APIC timer, no matter the timer mode (deadline, one-
	shot or periodic).  But newer CPUs adhere to the strict letter
	of the law in the SDM and more fully ignore APIC_TMICT when in
	deadline mode.  Those CPUs also don't fully "disable" the timer
	when IA32_TSC_DEADLINE has passed.  They _require_ writing a 0.

Or am I missing something?

> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 6513c53c9459..d1006531729a 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -441,6 +441,10 @@ static int lapic_timer_shutdown(struct clock_event_device *evt)
>  	v |= (APIC_LVT_MASKED | LOCAL_TIMER_VECTOR);
>  	apic_write(APIC_LVTT, v);
>  	apic_write(APIC_TMICT, 0);
> +
> +	if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
> +		wrmsrl(MSR_IA32_TSC_DEADLINE, 0);
> +
>  	return 0;
>  }
>