Skip to contentSkip to conversation

x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

PATCH In review He Chen 4 participants 12 messages Nov 4, 2016 – Nov 7, 2016

Subsystems (4)
x86 KVM perf events KVM x86
lore
Share & tools
lore

He Chen

author
#0 lore raw

PATCH v3 1/1 x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

The spec can be found in Intel Software Developer Manual or in Instruction Set Extensions Programming Reference.

Signed-off-by Luwei Kang <[email protected]>
Signed-off-by He Chen <[email protected]>

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

Changes in v2:
* add new macros for new AVX512 scattered features.
* add a cpuid_count_edx function to processor.h

---
 arch/x86/events/intel/pt.c       |  7 ------
 arch/x86/include/asm/processor.h |  9 +++++++
 arch/x86/kernel/cpu/scattered.c  | 52 +++++++++++++++++++++++++++-------------
 arch/x86/kvm/cpuid.c             | 14 ++++++++++-
 4 files changed, 57 insertions(+), 25 deletions(-)
arch/x86/events/intel/pt.c +0 -7
index c5047b8..5b4b972 100644
@@ -36,13 +36,6 @@ static DEFINE_PER_CPU(struct pt, pt_ctx);
 
 static struct pt_pmu pt_pmu;
 
-enum cpuid_regs {
-	CR_EAX = 0,
-	CR_ECX,
-	CR_EDX,
-	CR_EBX
-};
-
 /*
  * Capabilities of Intel PT hardware, such as number of address bits or
  * supported output schemes, are cached and exported to userspace as "caps"
arch/x86/include/asm/processor.h +9 -0
index 984a7bf..47978b7 100644
@@ -137,6 +137,13 @@ struct cpuinfo_x86 {
 	u32			microcode;
 };
 
+enum cpuid_regs_idx {
+	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);
 extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c);
 extern void init_amd_cacheinfo(struct cpuinfo_x86 *c);
arch/x86/kernel/cpu/scattered.c +35 -17

67 lines not on this page · raw message

arch/x86/kvm/cpuid.c +13 -1
index afa7bbb..1f63011 100644
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/vmalloc.h>
 #include <linux/uaccess.h>
+#include <asm/processor.h>
 #include <asm/fpu/internal.h> /* For use_eager_fpu.  Ugh! */
 #include <asm/user.h>
 #include <asm/fpu/xstate.h>

@@ -65,6 +66,11 @@ u64 kvm_supported_xcr0(void)
 
 #define F(x) bit(X86_FEATURE_##x)
 
+/* These are scattered features in cpufeatures.h. */
+#define KVM_CPUID_BIT_AVX512_4VNNIW     2
+#define KVM_CPUID_BIT_AVX512_4FMAPS     3
+#define KF(x) bit(KVM_CPUID_BIT_##x)
+
 int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;

@@ -376,6 +382,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	/* cpuid 7.0.ecx*/
 	const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 /*OSPKE*/;
 
+	/* cpuid 7.0.edx*/
+	const u32 kvm_cpuid_7_0_edx_x86_features =
+		KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS);
+
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 

@@ -458,12 +468,14 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 			/* PKU is not yet implemented for shadow paging. */
 			if (!tdp_enabled)
 				entry->ecx &= ~F(PKU);
+			entry->edx &= kvm_cpuid_7_0_edx_x86_features;
+			entry->edx &= get_scattered_cpuid_leaf(7, 0, CR_EDX);
 		} else {
 			entry->ebx = 0;
 			entry->ecx = 0;
+			entry->edx = 0;
 		}
 		entry->eax = 0;
-		entry->edx = 0;
 		break;
 	}
 	case 9:

--
2.7.4

maintainer
#1 lore raw

Please CC me on your future submissions, thanks.

Quoted 2 lines · He Chen

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.

Quoted 2 lines
Signed-off-by Luwei Kang <[email protected]>
Signed-off-by He Chen <[email protected]>

This SOB chain means what exactly?

Quoted 3 lines

---
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.

arch/x86/include/asm/processor.h struct cpuinfo_x86 { · line 140

Quoted 11 lines

* Capabilities of Intel PT hardware, such as number of address bits or
* supported output schemes, are cached and exported to userspace as "caps"

arch/x86/include/asm/processor.h +1 -0
index 984a7bf..47978b7 100644
@@ -137,6 +137,13 @@ struct cpuinfo_x86 {
 	u32			microcode;
 };
 
+enum cpuid_regs_idx {

cpuid_regs was just fine.

Quoted 10 lines
arch/x86/include/asm/processor.h +6 -0
+	CR_EAX = 0,
+	CR_ECX,
+	CR_EDX,
+	CR_EBX
+};
+
 #define X86_VENDOR_INTEL	0
 #define X86_VENDOR_CYRIX	1
 #define X86_VENDOR_AMD		2

arch/x86/include/asm/processor.h extern void identify_secondary_cpu(struct cpuinfo_x86 *); · line 189

Quoted 5 lines
arch/x86/include/asm/processor.h +2 -0
@@ -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);
Quoted 8 lines
arch/x86/include/asm/processor.h +0 -0
 extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c);
 extern void init_amd_cacheinfo(struct cpuinfo_x86 *c);
arch/x86/kernel/cpu/scattered.c +0 -0
index 1db8dc4..ca3c605 100644

arch/x86/kernel/cpu/scattered.c struct cpuid_bit { · line 20

Quoted 9 lines
arch/x86/kernel/cpu/scattered.c +1 -5
@@ -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. */

Quoted 11 lines
arch/x86/kernel/cpu/scattered.c +10 -0
+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 }
 };

...

arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 56

Quoted 7 lines
arch/x86/kernel/cpu/scattered.c +3 -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.

arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 59

Quoted 3 lines
arch/x86/kernel/cpu/scattered.c +3 -0
+{
+	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;
Quoted 17 lines
arch/x86/kernel/cpu/scattered.c +17 -0
+
+	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;
+}
Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
maintainer
#2 lore raw
Quoted 13 lines · Borislav Petkov

Please CC me on your future submissions, thanks.

Quoted 2 lines · He Chen

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.

Quoted 2 lines
Signed-off-by Luwei Kang <[email protected]>
Signed-off-by He Chen <[email protected]>
Quoted 8 lines

This SOB chain means what exactly?

Quoted 3 lines

---
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.

With no usage? Sounds like a good use of Acked-by. :)

Quoted 4 lines

arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 56

Quoted 2 lines
arch/x86/kernel/cpu/scattered.c +2 -0
+u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
+			     enum cpuid_regs_idx reg)

Align arguments on the opening brace.

They are aligned.

Paolo

Quoted 31 lines

arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 59

Quoted 3 lines
arch/x86/kernel/cpu/scattered.c +3 -0
+{
+	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;
Quoted 17 lines
arch/x86/kernel/cpu/scattered.c +17 -0
+
+	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;
+}
maintainer
#3 lore raw
Quoted 4 lines · Paolo Bonzini
Quoted 2 lines

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

With no usage? Sounds like a good use of Acked-by. :)

I don't understand what do you mean here?

Quoted 6 lines
Quoted 4 lines

arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 56

Quoted 2 lines
arch/x86/kernel/cpu/scattered.c +2 -0
+u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
+			     enum cpuid_regs_idx reg)

Align arguments on the opening brace.

They are aligned.

Right you are. I need to look into why those spaces get eaten in vim. Looks like something is replacing 4 spaces with a single one...

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
maintainer
#4 lore raw
Quoted 2 lines · Borislav Petkov
Quoted 1 line

With no usage? Sounds like a good use of Acked-by. :)

I don't understand what do you mean here?

I mean that the changes to scattered.c are small, so it makes no sense to split them. With an Acked-by I could simply take the patch into my tree.

Paolo

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 56

Quoted 2 lines
arch/x86/kernel/cpu/scattered.c +2 -0
+u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
+			     enum cpuid_regs_idx reg)

Align arguments on the opening brace.

They are aligned.

Right you are. I need to look into why those spaces get eaten in vim. Looks like something is replacing 4 spaces with a single one...

maintainer
#5 lore raw
Quoted 3 lines · Paolo Bonzini

I mean that the changes to scattered.c are small, so it makes no sense to split them. With an Acked-by I could simply take the patch into my tree.

On no, it is not about the size or which tree it goes thru - rather that having stuff separated conceptually makes the patches much more clear.

The patch order would be

* modify scattered.c to return leaf
* add user of new function

which also makes review almost trivial.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
maintainer
#6 lore raw
Quoted 8 lines · Borislav Petkov
Quoted 3 lines · Paolo Bonzini

I mean that the changes to scattered.c are small, so it makes no sense to split them. With an Acked-by I could simply take the patch into my tree.

On no, it is not about the size or which tree it goes thru - rather that having stuff separated conceptually makes the patches much more clear.

Quoted 6 lines

The patch order would be

* modify scattered.c to return leaf
* add user of new function

which also makes review almost trivial.

That's fine by me of course. I would probably take both patches anyway.

Paolo

#7 lore raw
In reply to Paolo Bonzini
Quoted 6 lines · Paolo Bonzini
Quoted 2 lines · Borislav Petkov
Quoted 1 line

With no usage? Sounds like a good use of Acked-by. :)

I don't understand what do you mean here?

I mean that the changes to scattered.c are small, so it makes no sense to split them. With an Acked-by I could simply take the patch into my tree.

Nope. We have other changes in that area coming up, so I rather take the cpuid bits into tip and provide you a branch with that commit to pull from.

Thanks,

tglx

author
#8 lore raw
In reply to Borislav Petkov
Quoted 2 lines · Borislav Petkov

Please CC me on your future submissions, thanks.

Sure.

Quoted 8 lines · He Chen
Quoted 2 lines

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.

My carelessness, will improve it in next patch. Thanks for kindly advices.

Quoted 6 lines
Quoted 2 lines

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.

Agreed.

Quoted 10 lines
Quoted 10 lines

* Capabilities of Intel PT hardware, such as number of address bits or
* supported output schemes, are cached and exported to userspace as "caps"

arch/x86/include/asm/processor.h +0 -0
index 984a7bf..47978b7 100644
@@ -137,6 +137,13 @@ struct cpuinfo_x86 {
 	u32			microcode;
 };
Quoted 4 lines

arch/x86/include/asm/processor.h struct cpuinfo_x86 { · line 140

Quoted 1 line
arch/x86/include/asm/processor.h +1 -0
+enum cpuid_regs_idx {

cpuid_regs was just fine.

It should be, but I found it conflcts with struct cpuid_regs in arch/x86/kernel/cpuid.c since it got exported.

Thanks,
-He

maintainer
#9 lore raw
Quoted 2 lines · He Chen

It should be, but I found it conflcts with struct cpuid_regs in arch/x86/kernel/cpuid.c since it got exported.

So make the cpuid.c-one static.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#10 lore raw
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · He Chen

It should be, but I found it conflcts with struct cpuid_regs in arch/x86/kernel/cpuid.c since it got exported.

So make the cpuid.c-one static.

That does not work.

processor.h

enum cpuid_regs {
     ....
};

cpuid.c

struct cpuid_regs {
      ....
};

How do you make that struct definition static? And even if that'd work, how would that help to avoid the name clash in cpuid.c?

Not at all.

Both the enum and the struct should be in processor.h obviously with different names so we won't trip over this once more. And the obvious naming is:

struct cpuid_regs {
      u32 eax, ebx, ecx, edx;
};

enum cpuid_regs_idx {
CPUID_EAX,
CPUID_EBX,
CPUID_ECX,
CPUID_EDX,
};

as CR_E*X is just not intuitive at all.

Thanks,

tglx

maintainer
#11 lore raw
Quoted 1 line · Thomas Gleixner

How do you make that struct definition static?

Not make it static - rename it. Sorry.

It is used only locally in that file anyway.

Quoted 8 lines

Both the enum and the struct should be in processor.h obviously with different names so we won't trip over this once more. And the obvious naming is:

struct cpuid_regs {
      u32 eax, ebx, ecx, edx;
};
Quoted 8 lines

enum cpuid_regs_idx {
CPUID_EAX,
CPUID_EBX,
CPUID_ECX,
CPUID_EDX,
};

as CR_E*X is just not intuitive at all.

Ok, that makes sense.

Also, grepping around the tree - we don't have one definitive enum containing all the architectural registers and maybe we should have one. We do have some PT_E*X ptrace definitions and others in entry*.S, and...

We probably should have something like:

enum regs {
        AX = 0,
        CX,
        DX,
        BX,
        SP,
        BP,
        SI,
        DI,
        R8,
        R9,
        R10,
        R11,
        R12,
        R13,
        R14,
        R15
};

in the exactly same order as they're encoded in the x86 opcodes.

Yeah, I don't see a pressing reason for that yet though but maybe we should think about it. My angle is, avoid confusion and ad-hoc definitions spreading around the code.

Thanks.

Signature · 4 lines
Regards/Gruss,
    Boris.

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