linux-kernel - Re: [RFC PATCH v4 7/8] acpi_idle: Add FFH cstate handling

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: <CAJZ5v0gtUp9MK=0-g2kqAKccJDV_P01PwmuhPEJwv82EzvVECA@mail.gmail.com>
Date: Mon, 25 Nov 2024 15:00:50 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	rafael.j.wysocki@...el.com, len.brown@...el.com, 
	artem.bityutskiy@...ux.intel.com, dave.hansen@...ux.intel.com, 
	peterz@...radead.org, tglx@...utronix.de, gautham.shenoy@....com
Subject: Re: [RFC PATCH v4 7/8] acpi_idle: Add FFH cstate handling

On Mon, Nov 25, 2024 at 2:41 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Mon, Nov 25, 2024 at 2:21 PM Patryk Wlazlyn
> <patryk.wlazlyn@...ux.intel.com> wrote:
> >
> > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>
>
> The changes below look good to me, but the patch needs a changelog
> which should explain why it is needed or useful.
>
> In this particular case, you want to change the code ordering in
> native_play_dead() so that it calls cpuidle_play_dead() first and only
> fall back to anything else if that fails.
>
> In particular, this needs to work when intel_idle is not in use and
> the entry method for at least one idle state in the _CST return
> package for at least one CPU is ACPI_CSTATE_FFH, so this case needs to
> be added to acpi_idle_play_dead().
>
> You may also make a note in the changelog that had there been a
> non-Intel x86 platform with a _CST returning idle states where CPU is
> ACPI_CSTATE_FFH had been the entry method, it wouldn't have been
> handled correctly today (but this is academic because of the lack of
> such platforms).
>
> Also, this can be the first patch in your series if [1-3/7] are dropped.

Ah sorry, not quite.

You need to introduce mwait_play_dead_with_hint() first of course, so
it could be the second patch in your series and the intel_idle one
would be the third.

> > ---
> >  arch/x86/include/asm/cpufeatures.h | 1 +
> >  arch/x86/kernel/acpi/cstate.c      | 9 +++++++++
> >  drivers/acpi/processor_idle.c      | 2 ++
> >  include/acpi/processor.h           | 5 +++++
> >  4 files changed, 17 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > index ea33439a5d00..1da5e08de257 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -236,6 +236,7 @@
> >  #define X86_FEATURE_PVUNLOCK           ( 8*32+20) /* PV unlock function */
> >  #define X86_FEATURE_VCPUPREEMPT                ( 8*32+21) /* PV vcpu_is_preempted function */
> >  #define X86_FEATURE_TDX_GUEST          ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */
> > +#define X86_FEATURE_NO_MWAIT_OFFLINE    ( 8*32+23) /* Don't use MWAIT states for offlined CPUs */
> >
> >  /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
> >  #define X86_FEATURE_FSGSBASE           ( 9*32+ 0) /* "fsgsbase" RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
> > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> > index f3ffd0a3a012..c80a3e6dba5f 100644
> > --- a/arch/x86/kernel/acpi/cstate.c
> > +++ b/arch/x86/kernel/acpi/cstate.c
> > @@ -204,6 +204,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe);
> >
> > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> > +{
> > +       unsigned int cpu = smp_processor_id();
> > +       struct cstate_entry *percpu_entry;
> > +
> > +       percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> > +       mwait_play_dead_with_hint(percpu_entry->states[cx->index].eax);
> > +}
> > +
> >  void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
> >  {
> >         unsigned int cpu = smp_processor_id();
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index 698897b29de2..586cc7d1d8aa 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index)
> >                         raw_safe_halt();
> >                 else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
> >                         io_idle(cx->address);
> > +               } else if (cx->entry_method == ACPI_CSTATE_FFH) {
> > +                       acpi_processor_ffh_play_dead(cx);
> >                 } else
> >                         return;
> >         }
> > diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > index a17e97e634a6..63a37e72b721 100644
> > --- a/include/acpi/processor.h
> > +++ b/include/acpi/processor.h
> > @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
> >                                     struct acpi_processor_cx *cx,
> >                                     struct acpi_power_register *reg);
> >  void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
> > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx);
> >  #else
> >  static inline void acpi_processor_power_init_bm_check(struct
> >                                                       acpi_processor_flags
> > @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
> >  {
> >         return;
> >  }
> > +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> > +{
> > +       return;
> > +}
> >  #endif
> >
> >  static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg,
> > --
> > 2.47.0
> >
> >