* Re: [PATCH 3/6] x86/cpuidle: Move buggy mwait implementations away from CPUIDLE_FLAG_MWAIT
2025-01-02 15:01 ` [PATCH 3/6] x86/cpuidle: Move buggy mwait implementations away from CPUIDLE_FLAG_MWAIT Frederic Weisbecker
@ 2025-01-14 14:35 ` Rafael J. Wysocki
0 siblings, 0 replies; 2+ messages in thread
From: Rafael J. Wysocki @ 2025-01-14 14:35 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Peter Zijlstra, Daniel Lezcano, linux-pm, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, Russell King,
Catalin Marinas, Will Deacon, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Madhavan Srinivasan, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Len Brown, Arnd Bergmann, linux-arch
On Thu, Jan 2, 2025 at 4:02 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Buggy MWAIT implementations can't carry the CPUIDLE_FLAG_MWAIT flag
> because they require IPIs to wake up. Therefore they shouldn't be
> called with TIF_NR_POLLING.
>
> Reported-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
I would do a patch introducing arch_cpuidle_mwait_needs_ipi() or
equivalent before patch [2/6] and I would use it in patch [2/6] right
away.
> ---
> arch/arm/include/asm/cpuidle.h | 2 ++
> arch/arm64/include/asm/cpuidle.h | 3 +++
> arch/powerpc/include/asm/cpuidle.h | 4 ++++
> arch/riscv/include/asm/cpuidle.h | 2 ++
> arch/x86/include/asm/cpuidle.h | 12 ++++++++++++
> drivers/acpi/processor_idle.c | 4 +++-
> drivers/idle/intel_idle.c | 9 +++++++--
> include/asm-generic/Kbuild | 1 +
> include/asm-generic/cpuidle.h | 10 ++++++++++
> 9 files changed, 44 insertions(+), 3 deletions(-)
> create mode 100644 arch/x86/include/asm/cpuidle.h
> create mode 100644 include/asm-generic/cpuidle.h
>
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index 397be5ed30e7..0ea1d2ec837d 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -55,4 +55,6 @@ struct arm_cpuidle_irq_context { };
> #define arm_cpuidle_save_irq_context(c) (void)c
> #define arm_cpuidle_restore_irq_context(c) (void)c
>
> +#include <asm-generic/cpuidle.h>
> +
> #endif
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 2047713e097d..ef49124135a7 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -38,4 +38,7 @@ struct arm_cpuidle_irq_context { };
> #define arm_cpuidle_save_irq_context(c) (void)c
> #define arm_cpuidle_restore_irq_context(c) (void)c
> #endif
> +
> +#include <asm-generic/cpuidle.h>
> +
> #endif
> diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h
> index 0cce5dc7fb1c..788706bc04ec 100644
> --- a/arch/powerpc/include/asm/cpuidle.h
> +++ b/arch/powerpc/include/asm/cpuidle.h
> @@ -102,4 +102,8 @@ static inline void report_invalid_psscr_val(u64 psscr_val, int err)
>
> #endif
>
> +#ifndef __ASSEMBLY__
> +#include <asm-generic/cpuidle.h>
> +#endif
> +
> #endif
> diff --git a/arch/riscv/include/asm/cpuidle.h b/arch/riscv/include/asm/cpuidle.h
> index 71fdc607d4bc..1f1b24901d86 100644
> --- a/arch/riscv/include/asm/cpuidle.h
> +++ b/arch/riscv/include/asm/cpuidle.h
> @@ -21,4 +21,6 @@ static inline void cpu_do_idle(void)
> wait_for_interrupt();
> }
>
> +#include <asm-generic/cpuidle.h>
> +
> #endif
> diff --git a/arch/x86/include/asm/cpuidle.h b/arch/x86/include/asm/cpuidle.h
> new file mode 100644
> index 000000000000..a59db1a3314a
> --- /dev/null
> +++ b/arch/x86/include/asm/cpuidle.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_CPUIDLE_H
> +#define _ASM_X86_CPUIDLE_H
> +
> +#include <asm/cpufeature.h>
> +
> +static inline bool arch_cpuidle_mwait_needs_ipi(void)
> +{
> + return boot_cpu_has_bug(X86_BUG_MONITOR);
> +}
> +
> +#endif /* _ASM_X86_CPUIDLE_H */
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 66cb5536d91e..0f29dd92b346 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -23,6 +23,7 @@
> #include <linux/perf_event.h>
> #include <acpi/processor.h>
> #include <linux/context_tracking.h>
> +#include <asm/cpuidle.h>
>
> /*
> * Include the apic definitions for x86 to have the APIC timer related defines
> @@ -806,7 +807,8 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2)
> drv->safe_state_index = count;
>
> - if (cx->entry_method == ACPI_CSTATE_FFH)
> + if (cx->entry_method == ACPI_CSTATE_FFH &&
> + !arch_cpuidle_mwait_needs_ipi())
> state->flags |= CPUIDLE_FLAG_MWAIT;
>
> /*
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d52723fbeb04..b2f494effd4a 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,6 +56,7 @@
> #include <asm/mwait.h>
> #include <asm/spec-ctrl.h>
> #include <asm/fpu/api.h>
> +#include <asm/cpuidle.h>
>
> #define INTEL_IDLE_VERSION "0.5.1"
>
> @@ -1787,7 +1788,10 @@ static void __init intel_idle_init_cstates_acpi(struct cpuidle_driver *drv)
> if (cx->type > ACPI_STATE_C1)
> state->target_residency *= 3;
>
> - state->flags = MWAIT2flg(cx->address) | CPUIDLE_FLAG_MWAIT;
> + state->flags = MWAIT2flg(cx->address);
> +
> + if (!arch_cpuidle_mwait_needs_ipi())
> + state->flags |= CPUIDLE_FLAG_MWAIT;
>
> if (cx->type > ACPI_STATE_C2)
> state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
> @@ -2073,7 +2077,8 @@ static bool __init intel_idle_verify_cstate(unsigned int mwait_hint)
>
> static void state_update_enter_method(struct cpuidle_state *state, int cstate)
> {
> - state->flags |= CPUIDLE_FLAG_MWAIT;
> + if (!arch_cpuidle_mwait_needs_ipi())
> + state->flags |= CPUIDLE_FLAG_MWAIT;
Also, some code duplication could be avoided by having something like
arch_x86_mwait_state() returning the flag if the condition is met:
state->flags |= arch_x86_mwait_state();
>
> if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {
> /*
> diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
> index 1b43c3a77012..7754da499d16 100644
> --- a/include/asm-generic/Kbuild
> +++ b/include/asm-generic/Kbuild
> @@ -13,6 +13,7 @@ mandatory-y += cacheflush.h
> mandatory-y += cfi.h
> mandatory-y += checksum.h
> mandatory-y += compat.h
> +mandatory-y += cpuidle.h
> mandatory-y += current.h
> mandatory-y += delay.h
> mandatory-y += device.h
> diff --git a/include/asm-generic/cpuidle.h b/include/asm-generic/cpuidle.h
> new file mode 100644
> index 000000000000..748a2022ed2a
> --- /dev/null
> +++ b/include/asm-generic/cpuidle.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_CPUIDLE_H
> +#define __ASM_CPUIDLE_H
> +
> +static inline bool arch_cpuidle_mwait_needs_ipi(void)
> +{
> + return true;
> +}
> +
> +#endif /* __ASM_CPUIDLE_H */
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread