Re: [PATCH v3] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

Please CC me on your future submissions, thanks.

On Fri, Nov 04, 2016 at 03:07:19PM +0800, He Chen wrote:
> The spec can be found in Intel Software Developer Manual or in
> Instruction Set Extensions Programming Reference.

This commit message is completely useless. Write commit messages in
the way as if you're explaining to another person *why* this change is
needed and that other person doesn't have an idea what you're doing.

> Signed-off-by: Luwei Kang <luwei.kang@xxxxxxxxx>
> Signed-off-by: He Chen <he.chen@xxxxxxxxxxxxxxx>

This SOB chain means what exactly?

> ---
> Changes in v3:
> * add a helper in scattered.c to get scattered leaf.

The modification to scattered et al without the kvm use should be a
separate patch.

>   * Capabilities of Intel PT hardware, such as number of address bits or
>   * supported output schemes, are cached and exported to userspace as "caps"
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 984a7bf..47978b7 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -137,6 +137,13 @@ struct cpuinfo_x86 {
>  	u32			microcode;
>  };
> 
> +enum cpuid_regs_idx {

cpuid_regs was just fine.

> +	CR_EAX = 0,
> +	CR_ECX,
> +	CR_EDX,
> +	CR_EBX
> +};
> +
>  #define X86_VENDOR_INTEL	0
>  #define X86_VENDOR_CYRIX	1
>  #define X86_VENDOR_AMD		2
> @@ -178,6 +185,8 @@ extern void identify_secondary_cpu(struct cpuinfo_x86 *);
>  extern void print_cpu_info(struct cpuinfo_x86 *);
>  void print_cpu_msr(struct cpuinfo_x86 *);
>  extern void init_scattered_cpuid_features(struct cpuinfo_x86 *c);
> +extern u32 get_scattered_cpuid_leaf(unsigned int level,
> +		unsigned int sub_leaf, enum cpuid_regs_idx reg);

Align arguments on the opening brace like this:

extern u32
get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
			 enum cpuid_regs_idx reg);


>  extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c);
>  extern void init_amd_cacheinfo(struct cpuinfo_x86 *c);
> 
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 1db8dc4..ca3c605 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -17,11 +17,17 @@ struct cpuid_bit {
>  	u32 sub_leaf;
>  };
> 
> -enum cpuid_regs {
> -	CR_EAX = 0,
> -	CR_ECX,
> -	CR_EDX,
> -	CR_EBX
> +/* Please keep the leaf sorted. */

				... by cpuid_bit.level for faster search. */

> +static const struct cpuid_bit cpuid_bits[] = {
> +	{ X86_FEATURE_APERFMPERF,	CR_ECX,  0, 0x00000006, 0 },
> +	{ X86_FEATURE_EPB,		CR_ECX,  3, 0x00000006, 0 },
> +	{ X86_FEATURE_INTEL_PT,		CR_EBX, 25, 0x00000007, 0 },
> +	{ X86_FEATURE_AVX512_4VNNIW,	CR_EDX,  2, 0x00000007, 0 },
> +	{ X86_FEATURE_AVX512_4FMAPS,	CR_EDX,  3, 0x00000007, 0 },
> +	{ X86_FEATURE_HW_PSTATE,	CR_EDX,  7, 0x80000007, 0 },
> +	{ X86_FEATURE_CPB,		CR_EDX,  9, 0x80000007, 0 },
> +	{ X86_FEATURE_PROC_FEEDBACK,	CR_EDX, 11, 0x80000007, 0 },
> +	{ 0, 0, 0, 0, 0 }
>  };

...

> @@ -57,3 +51,27 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  			set_cpu_cap(c, cb->feature);
>  	}
>  }
> +
> +u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
> +			     enum cpuid_regs_idx reg)

Align arguments on the opening brace.

> +{
> +	u32 cpuid_val = 0;
> +	const struct cpuid_bit *cb;


Please sort function local variables declaration in a reverse christmas
tree order:

	<type> longest_variable_name;
	<type> shorter_var_name;
	<type> even_shorter;
	<type> i;

> +
> +	for (cb = cpuid_bits; cb->feature; cb++) {
> +
> +		if (level > cb->level)
> +			continue;
> +
> +		if (level < cb->level)
> +			break;
> +
> +		if (reg == cb->reg && sub_leaf == cb->sub_leaf) {
> +			if (cpu_has(&boot_cpu_data, cb->feature))
> +				cpuid_val |= BIT(cb->bit);
> +		}
> +	}
> +
> +	return cpuid_val;
> +}

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.