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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Oct 08, 2024 at 01:37:39PM -0400, 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@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.

okay, thanks for the confirmation.

I just pushed to our topic/core-for-CI branch.

Karthik, please check the CI results and let me know if this needs to be changed later.

> 
> Thanks,
> Srinivas
> 
> > 
> > Cc: Zhang Rui <rui.zhang@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
> > 
> > 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@intel.com>
> > > Signed-off-by: Karthik Poosa <karthik.poosa@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
> > > 
>