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: <alpine.DEB.2.20.1610300906390.6965@nanos>
Date:   Sun, 30 Oct 2016 19:15:18 -0600 (MDT)
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, lukasz.daniluk@...el.com,
        james.h.cownie@...el.com, jacob.jun.pan@...el.com,
        Piotr.Luc@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 4/4] x86: Add enabling of the R3MWAIT during boot

On Fri, 28 Oct 2016, Grzegorz Andrejczuk wrote:

> Subject: x86: Add enabling of the R3MWAIT during boot

This is not a proper sentence and aside of that it's wrong, because the
feature handling (enable or disable) is not only done during boot. It's
also done on each cpu hotplug operation.

  x86/cpufeatures: Handle RING3MWAIT on Xeon Phi models

would be a proper changelog.

> If processor is Intel Xeon Phi x200 we enable user-level mwait feature.
> Enabling this feature suppresses invalid-opcode error, when MONITOR/MWAIT
> is called from ring 3.

The changelog is missing an explanation that this feature is not
discoverable and is hardcoded enabled depending on cpu vendor/family/model/

> +	phir3mwait=	[X86] Disable Intel Xeon Phi x200 ring 3 MONITOR/MWAIT
> +			feature for all cpus.
> +			Format: { disable }
> +			See arch/x86/kernel/cpu/intel.c

The feature detection/enabling might move to some other file and then this
becomes stale. Add a proper documentation file.

> +#ifdef CONFIG_X86_64
> +static int phi_r3mwait_disabled __read_mostly;
> +
> +static int __init phir3mwait_disable(char *__unused)
> +{
> +	phi_r3mwait_disabled = 1;
> +	pr_warn("x86/phir3mwait: Disabled ring 3 MWAIT for Xeon Phi");

Are you actually reading my reviews at all? If you disagree, then please do
so in a reply to my review, but silently ignoring it is not working.

> +	return 1;
> +}
> +__setup("phir3mwait=disable", phir3mwait_disable);
> +
> +static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
> +{
> +	u64 msr;
> +
> +	/*
> +	* Setting ring 3 MONITOR/MWAIT for each logical CPU
> +	* return when CPU is not Xeon Phi Family x200 (KnightsLanding).

Comments which tell what the code does are pointless, because that can be
seen from the code. The information which needs to be in a comment is WHY
this is done, i.e. non discoverability wants to be documented.

> +	*/
> +	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
> +		return;
> +
> +	rdmsrl(MSR_MISC_FEATURE_ENABLES, msr);

We can avoid that whole rdmsrl/wrmsrl dance and simply use
msr_set/clear_bit() below.

> +
> +	if (phi_r3mwait_disabled) {
> +		msr &= ~MSR_MISC_FEATURE_ENABLES_PHIR3MWAIT;
> +		wrmsrl(MSR_MISC_FEATURE_ENABLES, msr);
> +	} else {
> +		msr |= MSR_MISC_FEATURE_ENABLES_PHIR3MWAIT;
> +		wrmsrl(MSR_MISC_FEATURE_ENABLES, msr);
> +		set_cpu_cap(c, X86_FEATURE_PHIR3MWAIT);
> +		ELF_HWCAP2 |= HWCAP2_PHIR3MWAIT;
> +	}
> +}

Thanks,

	tglx

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ