Re: [tip: x86/core] x86: Fix CPUIDLE_FLAG_IRQ_ENABLE leaking timer reprogram

Peter Zijlstra
2023-11-30T11:15:27Z

On Wed, Nov 29, 2023 at 02:55:55PM -0000, tip-bot2 for Peter Zijlstra wrote:
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index 341ee4f..920426d 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -124,8 +124,15 @@ static __always_inline void mwait_idle_with_hints(unsigned long eax, unsigned lo
> }
>
> __monitor((void *)&current_thread_info()->flags, 0, 0);
> - if (!need_resched())
> - __mwait(eax, ecx);
> +
> + if (!need_resched()) {
> + if (ecx & 1) {
> + __mwait(eax, ecx);
> + } else {
> + __sti_mwait(eax, ecx);
> + raw_local_irq_disable();
> + }
> + }

Andrew noted that this is only safe if it precludes #DB from happening
on mwait, because #DB can wreck the STI shadow thing.

> @@ -159,19 +160,13 @@ static __always_inline int __intel_idle(struct cpuidle_device *dev,
> static __cpuidle int intel_idle(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index)
> {
> + return __intel_idle(dev, drv, index, true);
> }
>
> static __cpuidle int intel_idle_irq(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index)
> {
> + return __intel_idle(dev, drv, index, false);
> }
>
> static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
> @@ -184,7 +179,7 @@ static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
> if (smt_active)
> __update_spec_ctrl(0);
>
> + ret = __intel_idle(dev, drv, index, true);
>
> if (smt_active)
> __update_spec_ctrl(spec_ctrl);
> @@ -196,7 +191,7 @@ static __cpuidle int intel_idle_xstate(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index)
> {
> fpu_idle_fpregs();
> + return __intel_idle(dev, drv, index, true);
> }

This is so, because all mwait users should be in __cpuidle section,
which itself is part of the noinstr section and as such
kprobes/hw-breakpoints etc.. are disallowed.

Notable vmlinux.lds.h has:

#define NOINSTR_TEXT \
ALIGN_FUNCTION(); \
__noinstr_text_start = .; \
*(.noinstr.text) \
__cpuidle_text_start = .; \
*(.cpuidle.text) \
__cpuidle_text_end = .; \
__noinstr_text_end = .;