| 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
| ||
|
Message-ID: <CAJZ5v0j-U4ocmzVvgqEa-+8=aOjEFd8cU_j_9PXGNuRkr=qBzw@mail.gmail.com>
Date: Thu, 14 Nov 2024 18:58:55 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "Gautham R. Shenoy" <gautham.shenoy@....com>
Cc: Peter Zijlstra <peterz@...radead.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>, 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,
Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v3 2/3] x86/smp native_play_dead: Prefer
cpuidle_play_dead() over mwait_play_dead()
On Thu, Nov 14, 2024 at 6:36 PM Gautham R. Shenoy
<gautham.shenoy@....com> wrote:
>
> On Wed, Nov 13, 2024 at 05:22:22PM +0100, Peter Zijlstra wrote:
> > On Wed, Nov 13, 2024 at 05:11:38PM +0530, Gautham R. Shenoy wrote:
> >
> > > How about something like this (completely untested)
> > >
> > > ---------------------x8----------------------------------------------------
> > >
> > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> > > index f3ffd0a3a012..bd611771fa6c 100644
> > > --- a/arch/x86/kernel/acpi/cstate.c
> > > +++ b/arch/x86/kernel/acpi/cstate.c
> > > @@ -215,6 +215,24 @@ void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
> > > }
> > > EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter);
> > >
> > > +static int acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx)
> > > +{
> > > + unsigned int cpu = smp_processor_id();
> > > + struct cstate_entry *percpu_entry;
> > > +
> > > + /*
> > > + * This is ugly. But AMD processors don't prefer MWAIT based
> > > + * C-states when processors are offlined.
> > > + */
> > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> > > + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> > > + return -ENODEV;
> >
> > Are there AMD systems with FFh idle states at all?
>
> Yes. On AMD systems, the ACPI C1 state is an FFh idle state. An
> example from my server box.
>
>
> $ cpupower idle-info | grep "FFH" -B1 -A3
> C1:
> Flags/Description: ACPI FFH MWAIT 0x0
> Latency: 1
> Usage: 6591
> Duration: 1482606
>
> >
> > Also, I don't think this works right, the loop in cpuidle_play_dead()
> > will terminate on this, and not try a shallower state which might have
> > IO/HLT on.
>
> Ah yes. You are right. I didn't observe that cpuidle_play_dead() calls
> "return idle_state.enter_state". I suppose the solution would be to
> not populate .enter_dead callback for FFH based C-States on AMD/Hygon
> Platforms.
No, no, the solution is to change cpuidle_play_dead() to do the right thing.
Please see
https://lore.kernel.org/linux-pm/4992010.31r3eYUQgx@rjwysocki.net/
that I've just posted.
> How about something like the following? I have tested this on AMD
> servers by disabling the IO based C2 state, and I could observe that
> the offlined CPUs entered HLT bypassing the MWAIT based C1. When IO
> based C2 state is enabled, offlined CPUs enter the C2 state as before.
> If Global C-States are disabled, then, offlined CPUs enter HLT.
>
> This is on top of Patrick's series.
>
> I have defined X86_FEATURE_NO_MWAIT_OFFLINE as suggested by Dave to
> prevent MWAIT based C-states being used for CPU Offline on AMD and
> Hygon.
>
> ---------------------x8----------------------------------------------------
> From: "Gautham R. Shenoy" <gautham.shenoy@....com>
> Date: Thu, 14 Nov 2024 14:48:17 +0530
> Subject: [PATCH] acpi_idle: Teach acpi_idle_play_dead about FFH states
>
> Signed-off-by: Gautham R. Shenoy <gautham.shenoy@....com>
> ---
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/kernel/acpi/cstate.c | 14 ++++++++++++++
> arch/x86/kernel/smpboot.c | 6 +++---
> drivers/acpi/processor_idle.c | 24 ++++++++++++++++++++++--
> include/acpi/processor.h | 6 ++++++
> 5 files changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 05e985ce9880..ceb002406d0c 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..a5255a603745 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -215,6 +215,16 @@ void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
> }
> EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter);
>
> +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);
> +}
> +EXPORT_SYMBOL_GPL(acpi_processor_ffh_play_dead);
Why does it need to be exported to modules?
> +
> static int __init ffh_cstate_init(void)
> {
> struct cpuinfo_x86 *c = &boot_cpu_data;
> @@ -224,6 +234,10 @@ static int __init ffh_cstate_init(void)
> c->x86_vendor != X86_VENDOR_HYGON)
> return -1;
>
> + if (c->x86_vendor == X86_VENDOR_AMD ||
> + c->x86_vendor == X86_VENDOR_HYGON)
> + set_cpu_cap(c, X86_FEATURE_NO_MWAIT_OFFLINE);
> +
> cpu_cstate_entry = alloc_percpu(struct cstate_entry);
> return 0;
> }
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 106f26e976b8..b5939bf96be6 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1285,9 +1285,9 @@ static inline int mwait_play_dead(void)
> unsigned int highest_subcstate = 0;
> int i;
>
> - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> - boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> - return 1;
> + if (boot_cpu_has(X86_FEATURE_NO_MWAIT_OFFLINE))
> + return -ENODEV;
> +
> if (!this_cpu_has(X86_FEATURE_MWAIT))
> return 1;
> if (!this_cpu_has(X86_FEATURE_CLFLUSH))
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 831fa4a12159..1e2259b4395b 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -590,6 +590,8 @@ static int 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 -ENODEV;
> }
> @@ -772,6 +774,25 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> return 0;
> }
>
> +static inline bool valid_acpi_idle_type(struct acpi_processor_cx *cx)
> +{
> + if (cx->type == ACPI_STATE_C1 ||
> + cx->type == ACPI_STATE_C2 ||
> + cx->type == ACPI_STATE_C3)
> + return true;
> +
> + return false;
This could do
return cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 ||
cx->type == ACPI_STATE_C3;
but it may just not be necessary.
Please see
https://lore.kernel.org/linux-pm/2373563.ElGaqSPkdT@rjwysocki.net/
> +}
> +
> +static inline bool acpi_idle_can_play_dead(struct acpi_processor_cx *cx)
> +{
> + if (cx->entry_method == ACPI_CSTATE_FFH &&
> + boot_cpu_has(X86_FEATURE_NO_MWAIT_OFFLINE))
> + return false;
> +
> + return true;
> +}
> +
> static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> {
> int i, count;
> @@ -803,8 +824,7 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> state->enter = acpi_idle_enter;
>
> state->flags = 0;
> - if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 ||
> - cx->type == ACPI_STATE_C3) {
> + if (valid_acpi_idle_type(cx) && acpi_idle_can_play_dead(cx)) {
> state->enter_dead = acpi_idle_play_dead;
The check below is separate from the "enter_dead" check, so it should
not depend on acpi_idle_can_play_dead(cx).
> if (cx->type != ACPI_STATE_C3)
> drv->safe_state_index = count;
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index e6f6074eadbf..349fe47a579e 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 *cstate);
> #else
> static inline void acpi_processor_power_init_bm_check(struct
> acpi_processor_flags
> @@ -300,6 +301,11 @@ 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 *cstate)
> +{
> + return;
> +}
> #endif
>
> static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg,
>
>
> --
> Thanks and Regards
> gautham.
>
Powered by blists - more mailing lists