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

From: Borislav Petkov
Date: Fri Nov 04 2016 - 06:52:46 EST


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.