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

[Borah, Chaitanya Kumar]

Hello Rodrigo,

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Rodrigo
> Vivi
> Sent: Tuesday, October 8, 2024 11:04 PM
> To: Poosa, Karthik <karthik.poosa at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Gupta, Anshuman
> <anshuman.gupta at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>;
> Zhang, Rui <rui.zhang at intel.com>; Saarinen, Jani <jani.saarinen at intel.com>;
> Pandruvada, Srinivas <srinivas.pandruvada at intel.com>
> Subject: Re: [PATCH] [core-for-CI PATCH] x86/apic: Stop the TSC Deadline
> timer during lapic timer shutdown
> 
> 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?
> 
> 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
> 

Pitching in because I helped Karthik prepare the patch. The document says

"New commits always need an associated gitlab issue for tracking purposes. .... Reference the issue with
References: tag. "

That is why References: tag has the gitlab issue to track the patch's eventual removal. 

Ack to rest of the comments.

Regards

Chaitanya



> But was this really confirmed?
> 
> 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.
> 
> > 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
> >