| 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: <ZzY1JJequFwJr/of@BLRRASHENOY1.amd.com>
Date: Thu, 14 Nov 2024 23:06:36 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: "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 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.
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);
+
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;
+}
+
+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;
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