On Thu, Jan 2, 2025 at 4:02 PM Frederic Weisbecker <frederic@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
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
>
>
