linux-kernel - Re: [PATCH][RFC] x86: override prefer_mwait_c1_over_halt to avoid loading cpuidle-haltpoll driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87fsdxprpr.ffs@tglx>
Date:   Fri, 02 Dec 2022 19:48:16 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     lirongqing@...du.com, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, peterz@...radead.org,
        tony.luck@...el.com, wyes.karny@....com,
        linux-kernel@...r.kernel.org, rafael.j.wysocki@...el.com
Subject: Re: [PATCH][RFC] x86: override prefer_mwait_c1_over_halt to avoid
 loading cpuidle-haltpoll driver

Li!

On Fri, Dec 02 2022 at 11:37, lirongqing@...du.com wrote:
> From: Li RongQing <lirongqing@...du.com>
>
> x86 KVM guests with MWAIT can load cpuidle-haltpoll driver, and will
> cause performance degradation, so override prefer_mwait_c1_over_halt
> to a new value, aviod loading cpuidle-haltpoll driver

Neither the subject line nor the above makes any sense to me.

prefer_mwait_c1_over_halt() is a function which is invoked and when it
returns true then the execution ends up in the code path you are
patching.

> @@ -889,6 +889,7 @@ void select_idle_routine(const struct cpuinfo_x86 *c)
>  	} else if (prefer_mwait_c1_over_halt(c)) {
>  		pr_info("using mwait in idle threads\n");
>  		x86_idle = mwait_idle;
> +		boot_option_idle_override = IDLE_PREF_MWAIT;

What you do is setting boot_option_idle_override to a new value, but
that has nothing to do with prefer_mwait_c1_over_halt() at all.

So how are you overriding that function to a new value?

But that's just a word smithing problem.

The real and way worse problem is that you pick a variable, which has
the purpose to capture the idle override on the kernel command line, and
modify it as you see fit, just to prevent that driver from loading.

select_idle_routine() reads it to check whether there was a command line
override or not. But it is not supposed to write it. Why?

Have you checked what else evaluates that variable? Obviously not,
because a simple grep would have told you:

drivers/cpuidle/cpuidle-haltpoll.c:     if (boot_option_idle_override != IDLE_NO_OVERRIDE)
drivers/idle/intel_idle.c:      if (boot_option_idle_override != IDLE_NO_OVERRIDE)

Congratulations!

Your patch breaks the default setup of every recent Intel system on the
planet because it not only prevents the cpuidle-haltpoll, but also the
intel_idle driver from loading.

Seriously. It's not too much asked to do at least a simple grep and look
at all _nine_ places which evaluate boot_option_idle_override.

Thanks,

      tglx