Re: [PATCH 2/2] x86/fpu: Support disabling AVX and AVX512

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, 10 Mar 2017, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> For performance testing it is useful to be able to disable AVX
> and AVX512. User programs check in XGETBV if AVX is supported
> by the OS. If we don't initialize the XSAVE state for AVX it will
> appear as if the OS is not supporting AVX. For kernel users we
> can also clear the internal cpu feature bits.
> 
> This patch implements disable options for AVX and AVX512 for
> the XSAVE code.
>
> I originally considered a generic argument that would disable
> any XSAVE feature, but it turns out you need special code
> to also disable all the CPUID bits, because otherwise
> kernel code may assume it exists, when it doesn't. MPX
> already has an own disable flag. Not clear it is useful
> for the others. So we only do it for AVX/AVX512 for now.

Please read and follow Documentation/process/submitting-patches.rst.

Especially this paragraph:

   Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
   instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
   to do frotz", as if you are giving orders to the codebase to change
   its behaviour.


> +	disable_avx	[X86] Disable support for AVX on Intel CPUs.

So that command line option fails on AMD CPUs, right?

> +	disable_avx512	[X86] Disable support for AVX512 on Intel CPUs.

Please drop the Intel stuff here as well. It's just a question of time
until AMD gets that as well.

>  	disable_1tb_segments [PPC]
>  			Disables the use of 1TB hash page table segments. This
>  			causes the kernel to fall back to 256MB segments which
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c24ac1efb12d..cf75638ec657 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -16,6 +16,20 @@
>  
>  #include <asm/tlbflush.h>
>  
> +enum xsave_features {
> +	XSAVE_X87,
> +	XSAVE_SSE,
> +	XSAVE_AVX,
> +	XSAVE_MPX_BOUNDS,
> +	XSAVE_MPX_CSR,
> +	XSAVE_AVX512_OPMASK,
> +	XSAVE_AVX512_HI256,
> +	XSAVE_AVX512_ZMM_HI256,
> +	XSAVE_PT,
> +	XSAVE_PKU,
> +	XSAVE_UNKNOWN
> +};

What's that enum for? It's unused ....

>  /*
>   * Although we spell it out in here, the Processor Trace
>   * xfeature is completely unused.  We use other mechanisms
> @@ -41,6 +55,8 @@ static const char *xfeature_names[] =
>   */
>  u64 xfeatures_mask __read_mostly;
>  
> +u64 xfeatures_disabled __initdata;

Why is this global?

> +	xfeatures_mask &= ~xfeatures_disabled;
>  	xfeatures_mask &= fpu__get_supported_xfeatures_mask();

Thanks,

	tglx