linux-kernel - Re: [PATCH v7 2/4] ACPI: processor

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: <CAJZ5v0jkWv3=XXDXOkrBiYowkdkH8NM29K=zjRz5fZETCb-GRQ@mail.gmail.com>
Date: Tue, 10 Dec 2024 21:15:37 +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, peterz@...radead.org, dave.hansen@...ux.intel.com, 
	gautham.shenoy@....com, tglx@...utronix.de, len.brown@...el.com, 
	artem.bityutskiy@...ux.intel.com
Subject: Re: [PATCH v7 2/4] ACPI: processor_idle: Add FFH state handling

On Fri, Nov 29, 2024 at 7:22 PM Patryk Wlazlyn
<patryk.wlazlyn@...ux.intel.com> wrote:
>
> Recent Intel platforms will depend on the idle driver to pass the
> correct hint for playing dead via mwait_play_dead_with_hint(). Expand
> the existing enter_dead interface with handling for FFH states and pass
> the MWAIT hint to the mwait_play_dead code.

First, the changelog doesn't match the code changes because you've
decided to use mwait_play_dead() without changing its name.

Second, "recent" is something that has happened already, so I would
say "new" or "upcoming" instead.

Overall, my changelog for this patch would be something like this:

"The ACPI processor_idle driver's implementation of the :enter_dead()
callback, acpi_idle_play_dead(), is missing the handling of FFH (Fixed
Functional Hardware) idle states that on x86 platforms are entered by
executing the MWAIT instruction.  If such idle states are listed by
_CST for the given CPU, the idle driver should be able to use the
MWAIT hints corresponding to them in order to put the CPU into an
appropriate idle state when it is going offline, so update
acpi_idle_play_dead() to take the FFH idle states into account.

Going forward this will be depended on by Intel platforms if the
(otherwise default) intel_idle driver is disabled."

The code changes look good to me.

> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>
> Suggested-by: Gautham R. Shenoy <gautham.shenoy@....com>
> ---
>  arch/x86/kernel/acpi/cstate.c | 10 ++++++++++
>  drivers/acpi/processor_idle.c |  2 ++
>  include/acpi/processor.h      |  5 +++++
>  3 files changed, 17 insertions(+)
>
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index f3ffd0a3a012..8d7b8b02ddb9 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -15,6 +15,7 @@
>  #include <acpi/processor.h>
>  #include <asm/mwait.h>
>  #include <asm/special_insns.h>
> +#include <asm/smp.h>
>
>  /*
>   * Initialize bm_flags based on the CPU cache properties
> @@ -204,6 +205,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(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 ce728cf7e301..83213fa47c1b 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,
> --