[PATCH] [core-for-CI PATCH] x86/apic: Stop the TSC Deadline timer during lapic timer shutdown

[Poosa, Karthik]

On 08-10-2024 23:07, Pandruvada, Srinivas wrote:
> On Tue, 2024-10-08 at 13:34 -0400, Rodrigo Vivi wrote:
>> On Mon, Oct 07, 2024 at 07:22:42PM +0530, Karthik Poosa wrote:
>>> From: Zhang Rui <rui.zhang at intel.com>
>>>
>>> This is a core-for-CI patch for
>>> https://lore.kernel.org/all/20240929063521.17284-1-rui.zhang@intel.com/
>>>
>>> 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."
>>>
>>> Current code in lapic_timer_shutdown() writes 0 to the initial-
>>> count
>>> register. This stops the local APIC timer for one-shot and periodic
>>> mode
>>> only. In TSC deadline mode, the timer is not properly stopped.
>>>
>>> Some CPUs are affected by this and they are woke up by the armed
>>> timer
>>> in s2idle in TSC deadline mode.
>>>
>>> Stop the TSC deadline timer in lapic_timer_shutdown() by writing 0
>>> to
>>> MSR_IA32_TSC_DEADLINE.
>>>
>>> Fixes: 279f1461432c ("x86: apic: Use tsc deadline for oneshot when
>>> available")
>>> Link:
>>> https://lore.kernel.org/all/20240929063521.17284-1-rui.zhang@intel.com/
>> The problem I see here is that this seems stalled. No review there.
>> Is that merged
>> in some branch and moving upstream and to stable?
> Not yet. arch/x86 patches are very slow to get merged.
> We can't ping unless no response for 2 weeks.
>
> Thanks,
> Srinivas

Hi Rodrigo,

Do you suggest that we wait for upstream patch to be both reviewed and 
merged ?

It seems we might need to wait at least another week before we can 
follow up.

>> Cc: Zhang Rui <rui.zhang at intel.com>
>>
>>> References:
>>> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12344
>> The right reference link we should have in this topic/core-for-CI
>> patch
>> is: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2766
>> showing the why we need the patch.
>>
>> https://drm.pages.freedesktop.org/maintainer-tools/committer/core-for-CI.html
Following Chaitanya’s input, I have created a GitLab issue for the 
core-for-CI patch and referenced it accordingly.
If a GitLab issue already exists, do we need a separate issue for the 
core-for-CI patch? If not, I will make the necessary corrections in the 
next revision.
>>
>> But was this really confirmed?
Based on the failure signature of continuous wakeup events, we have 
confirmed the issue.
Unfortunately, we were unable to reproduce the problem locally for 
further testing.
>>
>> Anyway, I'm hesitant here mostly because I don't believe we should
>> add this patch
>> if that is not getting propagated to Linus and/or stable trees.
Srinivas/Zhang,

Is there any possibility that this patch might not get propagated ?

>>> Signed-off-by: Zhang Rui <rui.zhang at intel.com>
>>> Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
>>>
>>> ---
>>>   arch/x86/kernel/apic/apic.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> 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;
>>>   }
>>>   
>>> -- 
>>> 2.25.1
>>>