Re: [PATCH 3/6] x86/cpuidle: Move buggy mwait implementations away from CPUIDLE_FLAG_MWAIT

["Rafael J. Wysocki" <rafael@kernel.org>]

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
>
>