| Message ID | 1478243239-15586-1-git-send-email-he.chen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Signed-off-by: He Chen <he.chen@linux.intel.com> 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; > +}
On 04/11/2016 11:52, Borislav Petkov wrote: > 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@intel.com> >> Signed-off-by: He Chen <he.chen@linux.intel.com> > > 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. With no usage? Sounds like a good use of Acked-by. :) >> +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 >> +{ >> + 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; >> +} > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 04, 2016 at 03:53:02PM +0100, Paolo Bonzini wrote: > > 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? > >> +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...
On 04/11/2016 16:06, Borislav Petkov wrote: > > 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 >>>> > >> +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... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 04, 2016 at 04:13:24PM +0100, Paolo Bonzini wrote: > 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.
On 04/11/2016 16:57, Borislav Petkov wrote: > On Fri, Nov 04, 2016 at 04:13:24PM +0100, Paolo Bonzini wrote: >> 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. That's fine by me of course. I would probably take both patches anyway. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 04, 2016 at 11:52:35AM +0100, Borislav Petkov wrote: > Please CC me on your future submissions, thanks. > Sure. > 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. > My carelessness, will improve it in next patch. Thanks for kindly advices. > > 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. > > * 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. > It should be, but I found it conflcts with `struct cpuid_regs` in `arch/x86/kernel/cpuid.c` since it got exported. Thanks, -He -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 07, 2016 at 10:10:07AM +0800, He Chen wrote: > 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.
On Mon, 7 Nov 2016, Borislav Petkov wrote: > On Mon, Nov 07, 2016 at 10:10:07AM +0800, He Chen wrote: > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 4 Nov 2016, Paolo Bonzini wrote: > On 04/11/2016 16:06, Borislav Petkov wrote: > > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 07, 2016 at 05:47:21PM +0100, Thomas Gleixner wrote: > How do you make that struct definition static? Not make it static - rename it. Sorry. It is used only locally in that file anyway. > 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. 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.
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index c5047b8..5b4b972 100644 --- a/arch/x86/events/intel/pt.c +++ b/arch/x86/events/intel/pt.c @@ -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" 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 { + 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); 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. */ +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 } }; void init_scattered_cpuid_features(struct cpuinfo_x86 *c) @@ -30,18 +36,6 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c) u32 regs[4]; const struct cpuid_bit *cb; - static const struct cpuid_bit cpuid_bits[] = { - { 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_APERFMPERF, CR_ECX, 0, 0x00000006, 0 }, - { X86_FEATURE_EPB, CR_ECX, 3, 0x00000006, 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 } - }; - for (cb = cpuid_bits; cb->feature; cb++) { /* Verify that the level is valid */ @@ -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) +{ + u32 cpuid_val = 0; + const struct cpuid_bit *cb; + + 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; +} +EXPORT_SYMBOL_GPL(get_scattered_cpuid_leaf); diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index afa7bbb..1f63011 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -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: