Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

2017-06-22 Thread Jonathan McDowell 
In article <20170621234106.16548-3-a...@firstfloor.org> you wrote:

> Some CPUID features depend on other features. Currently it's
> possible to to clear dependent features, but not clear the base features,
> which can cause various interesting problems.

> This patch implements a generic table to describe dependencies
> between CPUID features, to be used by all code that clears
> CPUID.

> Some subsystems (like XSAVE) had an own implementation of this,
> but it's better to do it all in a single place for everyone.

> Then clear_cpu_cap and setup_clear_cpu_cap always look up
> this table and clear all dependencies too.

> This is intended to be a practical table: only for features
> that make sense to clear. If someone for example clears FPU,
> or other features that are essentially part of the required
> base feature set, not much is going to work. Handling
> that is right now out of scope. We're only handling
> features which can be usefully cleared.

> v2: Add EXPORT_SYMBOL for clear_cpu_id for lguest
> Signed-off-by: Andi Kleen 
> ---
>  arch/x86/include/asm/cpufeature.h  |  8 +++-
>  arch/x86/include/asm/cpufeatures.h |  5 +++
>  arch/x86/kernel/cpu/Makefile   |  1 +
>  arch/x86/kernel/cpu/cpuid-deps.c   | 92 
> ++
>  4 files changed, 104 insertions(+), 2 deletions(-)
>  create mode 100644 arch/x86/kernel/cpu/cpuid-deps.c

> diff --git a/arch/x86/include/asm/cpufeature.h 
> b/arch/x86/include/asm/cpufeature.h
> index d59c15c3defd..e6145f383ff8 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -125,8 +125,12 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
>  #define boot_cpu_has(bit)  cpu_has(_cpu_data, bit)
>  
>  #define set_cpu_cap(c, bit)set_bit(bit, (unsigned long 
> *)((c)->x86_capability))
> -#define clear_cpu_cap(c, bit)  clear_bit(bit, (unsigned long 
> *)((c)->x86_capability))
> -#define setup_clear_cpu_cap(bit) do { \
> +#define __clear_cpu_cap(c, bit)clear_bit(bit, (unsigned long 
> *)((c)->x86_capability))
> +
> +extern void setup_clear_cpu_cap(int bit);
> +extern void clear_cpu_cap(struct cpuinfo_x86 *cpu, int bit);
> +
> +#define __setup_clear_cpu_cap(bit) do { \
> clear_cpu_cap(_cpu_data, bit); \
> set_bit(bit, (unsigned long *)cpu_caps_cleared); \
>  } while (0)
> diff --git a/arch/x86/include/asm/cpufeatures.h 
> b/arch/x86/include/asm/cpufeatures.h
> index 2701e5f8145b..8f371a5966e7 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -21,6 +21,11 @@
>   * this feature bit is not displayed in /proc/cpuinfo at all.
>   */
>  
> +/*
> + * When adding new features here that depend on other features,
> + * please update the table in kernel/cpu/cpuid-deps.c
> + */
> +
>  /* Intel-defined CPU features, CPUID level 0x0001 (edx), word 0 */
>  #define X86_FEATURE_FPU( 0*32+ 0) /* Onboard FPU */
>  #define X86_FEATURE_VME( 0*32+ 1) /* Virtual Mode Extensions 
> */
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 5210c62e..274fc0fee1e1 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -21,6 +21,7 @@ obj-y += common.o
>  obj-y  += rdrand.o
>  obj-y  += match.o
>  obj-y  += bugs.o
> +obj-y  += cpuid-deps.o
>  
>  obj-$(CONFIG_PROC_FS)  += proc.o
>  obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c 
> b/arch/x86/kernel/cpu/cpuid-deps.c
> new file mode 100644
> index ..08aff02cf2ff
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -0,0 +1,92 @@
> +/* Declare dependencies between CPUIDs */
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct cpuid_dep {
> +   int feature;
> +   int dep;
> +};

This seems a little confusing; I had to read through a couple of times
to understand that "dep" represents the feature that needs to be
disabled if "feature" is disabled, rather than dep being a dependency
of feature.

> +
> +/*
> + * Table of CPUID features that depend on others.
> + *
> + * This only includes dependencies that can be usefully disabled, not
> + * features part of the base set (like FPU).
> + */
> +const static struct cpuid_dep cpuid_deps[] = {
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XSAVEOPT },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XSAVEC },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XSAVES },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_AVX },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_AVX512F },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_PKU },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_MPX },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XGETBV1 },
> +   { X86_FEATURE_XMM, X86_FEATURE_XMM2 },
> +   { X86_FEATURE_XMM2,X86_FEATURE_XMM3 },
> +   { X86_FEATURE_XMM2,

Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

2017-06-22 Thread Jonathan McDowell 
In article <20170621234106.16548-3-a...@firstfloor.org> you wrote:

> Some CPUID features depend on other features. Currently it's
> possible to to clear dependent features, but not clear the base features,
> which can cause various interesting problems.

> This patch implements a generic table to describe dependencies
> between CPUID features, to be used by all code that clears
> CPUID.

> Some subsystems (like XSAVE) had an own implementation of this,
> but it's better to do it all in a single place for everyone.

> Then clear_cpu_cap and setup_clear_cpu_cap always look up
> this table and clear all dependencies too.

> This is intended to be a practical table: only for features
> that make sense to clear. If someone for example clears FPU,
> or other features that are essentially part of the required
> base feature set, not much is going to work. Handling
> that is right now out of scope. We're only handling
> features which can be usefully cleared.

> v2: Add EXPORT_SYMBOL for clear_cpu_id for lguest
> Signed-off-by: Andi Kleen 
> ---
>  arch/x86/include/asm/cpufeature.h  |  8 +++-
>  arch/x86/include/asm/cpufeatures.h |  5 +++
>  arch/x86/kernel/cpu/Makefile   |  1 +
>  arch/x86/kernel/cpu/cpuid-deps.c   | 92 
> ++
>  4 files changed, 104 insertions(+), 2 deletions(-)
>  create mode 100644 arch/x86/kernel/cpu/cpuid-deps.c

> diff --git a/arch/x86/include/asm/cpufeature.h 
> b/arch/x86/include/asm/cpufeature.h
> index d59c15c3defd..e6145f383ff8 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -125,8 +125,12 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
>  #define boot_cpu_has(bit)  cpu_has(_cpu_data, bit)
>  
>  #define set_cpu_cap(c, bit)set_bit(bit, (unsigned long 
> *)((c)->x86_capability))
> -#define clear_cpu_cap(c, bit)  clear_bit(bit, (unsigned long 
> *)((c)->x86_capability))
> -#define setup_clear_cpu_cap(bit) do { \
> +#define __clear_cpu_cap(c, bit)clear_bit(bit, (unsigned long 
> *)((c)->x86_capability))
> +
> +extern void setup_clear_cpu_cap(int bit);
> +extern void clear_cpu_cap(struct cpuinfo_x86 *cpu, int bit);
> +
> +#define __setup_clear_cpu_cap(bit) do { \
> clear_cpu_cap(_cpu_data, bit); \
> set_bit(bit, (unsigned long *)cpu_caps_cleared); \
>  } while (0)
> diff --git a/arch/x86/include/asm/cpufeatures.h 
> b/arch/x86/include/asm/cpufeatures.h
> index 2701e5f8145b..8f371a5966e7 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -21,6 +21,11 @@
>   * this feature bit is not displayed in /proc/cpuinfo at all.
>   */
>  
> +/*
> + * When adding new features here that depend on other features,
> + * please update the table in kernel/cpu/cpuid-deps.c
> + */
> +
>  /* Intel-defined CPU features, CPUID level 0x0001 (edx), word 0 */
>  #define X86_FEATURE_FPU( 0*32+ 0) /* Onboard FPU */
>  #define X86_FEATURE_VME( 0*32+ 1) /* Virtual Mode Extensions 
> */
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 5210c62e..274fc0fee1e1 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -21,6 +21,7 @@ obj-y += common.o
>  obj-y  += rdrand.o
>  obj-y  += match.o
>  obj-y  += bugs.o
> +obj-y  += cpuid-deps.o
>  
>  obj-$(CONFIG_PROC_FS)  += proc.o
>  obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c 
> b/arch/x86/kernel/cpu/cpuid-deps.c
> new file mode 100644
> index ..08aff02cf2ff
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -0,0 +1,92 @@
> +/* Declare dependencies between CPUIDs */
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct cpuid_dep {
> +   int feature;
> +   int dep;
> +};

This seems a little confusing; I had to read through a couple of times
to understand that "dep" represents the feature that needs to be
disabled if "feature" is disabled, rather than dep being a dependency
of feature.

> +
> +/*
> + * Table of CPUID features that depend on others.
> + *
> + * This only includes dependencies that can be usefully disabled, not
> + * features part of the base set (like FPU).
> + */
> +const static struct cpuid_dep cpuid_deps[] = {
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XSAVEOPT },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XSAVEC },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XSAVES },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_AVX },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_AVX512F },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_PKU },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_MPX },
> +   { X86_FEATURE_XSAVE,   X86_FEATURE_XGETBV1 },
> +   { X86_FEATURE_XMM, X86_FEATURE_XMM2 },
> +   { X86_FEATURE_XMM2,X86_FEATURE_XMM3 },
> +   { X86_FEATURE_XMM2,X86_FEATURE_XMM4_1 },
> +

Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

2017-06-05 Thread Andi Kleen 
> All errors (new ones prefixed by >>):
> 
> >> ERROR: "clear_cpu_cap" [drivers/lguest/lg.ko] undefined!

It looks very broken what lguest does here. But ok, we can add a EXPORT_SYMBOL.

-Andi

Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

2017-06-05 Thread Andi Kleen 
> All errors (new ones prefixed by >>):
> 
> >> ERROR: "clear_cpu_cap" [drivers/lguest/lg.ko] undefined!

It looks very broken what lguest does here. But ok, we can add a EXPORT_SYMBOL.

-Andi

Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

2017-06-03 Thread kbuild test robot 
Hi Andi,

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.12-rc3 next-20170602]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Andi-Kleen/x86-xsave-Move-xsave-initialization-to-after-parsing-early-parameters/20170601-085802
config: i386-randconfig-h0-06040930 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "clear_cpu_cap" [drivers/lguest/lg.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

2017-06-03 Thread kbuild test robot 
Hi Andi,

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.12-rc3 next-20170602]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Andi-Kleen/x86-xsave-Move-xsave-initialization-to-after-parsing-early-parameters/20170601-085802
config: i386-randconfig-h0-06040930 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "clear_cpu_cap" [drivers/lguest/lg.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip