Re: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, 20 Dec 2016, Grzegorz Andrejczuk wrote:

So how am I supposed to know which version of these patches is the right
one? Both subject lines are identical. 

> Enable ring 3 MONITOR/MWAIT for Intel Xeon Phi x200
> codenamed Knights Landing.
> 
> The patch:

>From your cover letter:

     Removed "This patch" from commit messages

Why is 'The patch any better' ?

> - Sets CPU feature X86_FEATURE_RING3MWAIT.
> - Sets HWCAP2_RING3MWAIT bit in ELF_HWCAP2.
> - Adds the ring3mwait=disable command line parameter.
> - Sets bit 1 of the MSR MISC_FEATURE_ENABLES or clears it when
>   the ring3mwait=disable command line parameter is used.

Changelogs should not describe WHAT the patch is doing. We can see that
from the patch. Changelogs should describe the WHY and CONCEPTS not
implementation details.

> +static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
> +{
> +	/*
> +	 * Ring 3 MONITOR/MWAIT feature cannot be detected without
> +	 * cpu model and family comparison.
> +	 */
> +	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
> +		return;
> +
> +	if (ring3mwait_disabled) {
> +		msr_clear_bit(MSR_MISC_FEATURE_ENABLES,
> +			      MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
> +		return;
> +	}
> +
> +	msr_set_bit(MSR_MISC_FEATURE_ENABLES,
> +		    MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
> +	set_cpu_cap(c, X86_FEATURE_RING3MWAIT);
> +	set_bit(HWCAP2_RING3MWAIT, (unsigned long *)&ELF_HWCAP2);

>From your cover letter:

     "Removed warning from 32-bit build"

First of all, the warning

   arch/x86/include/asm/bitops.h:72:1: note: expected 'volatile long unsigned int *'
but argument is of type 'unsigned int *'
    set_bit(long nr, volatile unsigned long *addr)

is not at all 32bit specific.

Handing an unsigned int pointer to a function which expects a unsigned long
is even more wrong on 64bit.

So now for your 'removal fix': It's just as sloppy as anything else what
I've seen from you before.

Handing a typecasted unsigned int pointer to a function which expects an
unsigned long pointer is just broken and a clear sign of careless
tinkering.

The only reason why this 'works' is because x86 is a little endian
architecture and the bit number is a constant and set_bit gets translated
it into:

    orb 0x02, 0x0(%rip) 

Now if you look really close to that disassembly then you might notice,
that this sets bit 1 and not as you tell in patch 2/5:

   "Introduce ELF_HWCAP2 variable for x86 and reserve its bit 0 to expose
    the ring 3 MONITOR/MWAIT."

So why does it not set bit 0?

Simply because you hand in HWCAP2_RING3MWAIT as bit number, which is
defined as:

+#define HWCAP2_RING3MWAIT              (1 << 0)

Crap, crap, crap.

What's so !$@&*(? wrong with doing the simple, obvious and correct:

       ELF_HWCAP2 |= HWCAP2_RING3MWAIT;

C is really hard, right?

Yours grumpy

      tglx