Re: [PATCH v7 4/4] x86: Add enabling of the R3MWAIT during boot

[Thomas Gleixner]

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