linux-kernel - Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

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:   Wed, 12 Oct 2016 15:34:08 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Grzegorz Andrejczuk <grzegorz.andrejczuk@...el.com>
cc:     mingo@...hat.com, hpa@...or.com, x86@...nel.org, bp@...e.de,
        dave.hansen@...ux.intel.com, linux-kernel@...r.kernel.org,
        lukasz.daniluk@...el.com, james.h.cownie@...el.com
Subject: Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for
 KNL

On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
>  
> +static int phir3mwait = 1;
> +static int __init phir3mwait_disable(char *value)

Can someone @Intel please tell everyone to stop this annoying habit of
glueing variable declarations without a newline to the function? And the
variable should be not in the middle of the code either. We have such stuff
on top of the file normaly if there is not a damned good reason to stick it
elsewhere.

That's just horrible and hard to read.

> +{
> +	phir3mwait = 0;
> +	return 1;
> +}
> +__setup("intel-phir3mwait=disable", phir3mwait_disable);
> +
>  static void early_init_intel(struct cpuinfo_x86 *c)
>  {
>  	u64 misc_enable;
> @@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	}
>  
>  	check_mpx_erratum(c);
> +
> +	/*
> +	* Setting ring 3 MONITOR/MWAIT for all threads
> +	* when CPU is Xeon Phi Family x200
> +	* This can be disabled with phir3mwait=disable cmdline switch.
> +	* We preserve the reserved values and set only 2nd bit.
> +	* Ref:
> +	* https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
> +	*/
> +	if (c->x86 == 6 &&
> +	    c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> +	    phir3mwait) {
> +		u64 prev;
> +
> +		rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> +		if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> +			wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> +			       prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);

The codingstyle here is just convoluted crap. What's wrong with writing it
proper?

	if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
		u64 msr;

		rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
		msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
		wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);

	}

No horrible to read line breaks, no redundant check for x->x86 == 6 because
model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
conditional is pointless as the feature is default disabled. And even if it
is enabled the extra msr write is not a problem at all. This is early init
code and not some hot path.

Thanks,

	tglx