linux-kernel - Re: [PATCH 0/3] idle, Honor Hardware Disabled States

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]

Date:	Mon, 28 Mar 2016 10:48:25 -0400
From:	Prarit Bhargava <prarit@...hat.com>
To:	Len Brown <lenb@...nel.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	X86 ML <x86@...nel.org>,
	"Peter Zijlstra (Intel)" <peterz@...radead.org>,
	Borislav Petkov <bp@...e.de>, Andi Kleen <ak@...ux.intel.com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Dasaratharaman Chandramouli 
	<dasaratharaman.chandramouli@...el.com>,
	Linux PM list <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 0/3] idle, Honor Hardware Disabled States



On 03/24/2016 05:54 PM, Len Brown wrote:
> On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <prarit@...hat.com> wrote:
>>
>>
>> On 03/23/2016 04:05 PM, Len Brown wrote:
>>> This patch assumes that if a package state is disabled,
>>> the corresponding core state must be disabled.
>>> That assumption is false.
>>> Indeed, that is a very popular and useful configuration.
>>>
>>> But even if that were not the case, this software is not necessary,
>>> since the hardware handles demotion "c-state clipping" automatically.
>>>
>>> Yes, there is a case where a certain version of a certain processor
>>> has broken demotion, but this isn't the right fix for that.
>>> The right fix for that is here:
>>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>>
>> Len, should I rebase on top of this?  Would that work for you?
> 
> 
> I guess I wasn't clear.
> I don't see the benefit of your patch.
> Please explain it to me.
> 

Len,

Your patch does

+	skl_cstates[5].disabled = 1;	/* C8-SKL */
+	skl_cstates[6].disabled = 1;	/* C9-SKL */

and I don't think that is correct for SKY-H.

Your patch does not take into account that the states are explicitly disabled
in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
is simply hammered a disable into those states.

Additionally, your patch does not show the user the correct state information:

    [root@...p40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
    /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
    /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1

The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
for certain that ALL SKY-H are impacted (you are admittedly in better position
to say so or not).  I can say that on the 2 systems tested here the
MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.

/me could be missing some important info  -- again, perhaps there are some
SKY-H's out there that do not have states disabled in
MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
your change.

P.


> thanks,
> -Len
>