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

[Pandruvada, Srinivas]

This patch will be merged as several maintainers are OK with this
change.

This is now more about how to communicate correctly.

Thanks,
Srinivas
On Wed, 2024-10-09 at 23:53 +0530, Poosa, Karthik wrote:
> 
> 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
> > > >