| Message ID | 20221019084734.3590760-2-jiaxi.chen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: KVM: Expose CPUID to guest for new Intel platform instructions | expand |
For all the shortlogs, "expose it to guest" is technically wrong. Adding recognition in kvm/cpuid.c advertises KVM support to host userspace. Whether or not a feature is exposed to the guest is up to the userspace VMM. On Wed, Oct 19, 2022, Jiaxi Chen wrote: > CMPccXADD is a new set of instructions in the latest Intel platform Sierra > Forest. It includes a semaphore operation that can compare and add the In general, avoid pronouns in changelogs, it's not clear what "it" refers to here. And for all of these changelogs, please explicitly state that there are no VMX controls for these instructions, assuming that's actually true. From a KVM perspective, that's far more interesting than the details of the instruction(s). > operands if condition is met, which can improve database performance. > > The bit definition: > CPUID.(EAX=7,ECX=1):EAX[bit 7] > > This patch enables this CPUID in the kernel feature bits and expose it to > guest OS. Same thing here, KVM doesn't decide whether or not to expose the feature to the guest. > Signed-off-by: Jiaxi Chen <jiaxi.chen@linux.intel.com> > --- > arch/x86/include/asm/cpufeatures.h | 1 + > arch/x86/kvm/cpuid.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > index ef4775c6db01..445626cb5779 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -308,6 +308,7 @@ > /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ > #define X86_FEATURE_AVX_VNNI (12*32+ 4) /* AVX VNNI instructions */ > #define X86_FEATURE_AVX512_BF16 (12*32+ 5) /* AVX512 BFLOAT16 instructions */ > +#define X86_FEATURE_CMPCCXADD (12*32+ 7) /* CMPccXADD instructions */ Boris, What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, KVM passthrough is the only reason the existing features are defined.
在 2022/10/19 23:15, Sean Christopherson 写道: > For all the shortlogs, "expose it to guest" is technically wrong. Adding > recognition in kvm/cpuid.c advertises KVM support to host userspace. Whether or > not a feature is exposed to the guest is up to the userspace VMM. Thanks for reminding. How about to change the subject to this: x86: KVM: Advertise CMPccXADD CPUID to userspace > > On Wed, Oct 19, 2022, Jiaxi Chen wrote: >> CMPccXADD is a new set of instructions in the latest Intel platform Sierra >> Forest. It includes a semaphore operation that can compare and add the > > In general, avoid pronouns in changelogs, it's not clear what "it" refers to here. > Will change it to: 'This new instruction set' here and avoid use pronouns in the future commit message. > And for all of these changelogs, please explicitly state that there are no VMX > controls for these instructions, assuming that's actually true. From a KVM > perspective, that's far more interesting than the details of the instruction(s). > Yes, thanks for comments. Will change this patch series to: This instruction has no other VMX control except for exposed to userspace. >> operands if condition is met, which can improve database performance. >> >> The bit definition: >> CPUID.(EAX=7,ECX=1):EAX[bit 7] >> >> This patch enables this CPUID in the kernel feature bits and expose it to >> guest OS. > > Same thing here, KVM doesn't decide whether or not to expose the feature to the > guest. > Applied.Thanks. >> Signed-off-by: Jiaxi Chen <jiaxi.chen@linux.intel.com> >> --- >> arch/x86/include/asm/cpufeatures.h | 1 + >> arch/x86/kvm/cpuid.c | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h >> index ef4775c6db01..445626cb5779 100644 >> --- a/arch/x86/include/asm/cpufeatures.h >> +++ b/arch/x86/include/asm/cpufeatures.h >> @@ -308,6 +308,7 @@ >> /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ >> #define X86_FEATURE_AVX_VNNI (12*32+ 4) /* AVX VNNI instructions */ >> #define X86_FEATURE_AVX512_BF16 (12*32+ 5) /* AVX512 BFLOAT16 instructions */ >> +#define X86_FEATURE_CMPCCXADD (12*32+ 7) /* CMPccXADD instructions */ > > Boris, > > What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, > KVM passthrough is the only reason the existing features are defined.
On 10/19/2022 11:15 PM, Sean Christopherson wrote: > For all the shortlogs, "expose it to guest" is technically wrong. Adding > recognition in kvm/cpuid.c advertises KVM support to host userspace. Whether or > not a feature is exposed to the guest is up to the userspace VMM. > > On Wed, Oct 19, 2022, Jiaxi Chen wrote: >> CMPccXADD is a new set of instructions in the latest Intel platform Sierra >> Forest. It includes a semaphore operation that can compare and add the > > In general, avoid pronouns in changelogs, it's not clear what "it" refers to here. > > And for all of these changelogs, please explicitly state that there are no VMX > controls for these instructions, assuming that's actually true. From a KVM > perspective, that's far more interesting than the details of the instruction(s). > >> operands if condition is met, which can improve database performance. >> >> The bit definition: >> CPUID.(EAX=7,ECX=1):EAX[bit 7] >> >> This patch enables this CPUID in the kernel feature bits and expose it to >> guest OS. > > Same thing here, KVM doesn't decide whether or not to expose the feature to the > guest. > >> Signed-off-by: Jiaxi Chen <jiaxi.chen@linux.intel.com> >> --- >> arch/x86/include/asm/cpufeatures.h | 1 + >> arch/x86/kvm/cpuid.c | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h >> index ef4775c6db01..445626cb5779 100644 >> --- a/arch/x86/include/asm/cpufeatures.h >> +++ b/arch/x86/include/asm/cpufeatures.h >> @@ -308,6 +308,7 @@ >> /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ >> #define X86_FEATURE_AVX_VNNI (12*32+ 4) /* AVX VNNI instructions */ >> #define X86_FEATURE_AVX512_BF16 (12*32+ 5) /* AVX512 BFLOAT16 instructions */ >> +#define X86_FEATURE_CMPCCXADD (12*32+ 7) /* CMPccXADD instructions */ > > Boris, > > What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, > KVM passthrough is the only reason the existing features are defined. Boris, Since CPUID_7_1_EAX has only 5 features now, it is a big waste, should we move it to KVM-only leaf as Sean suggested. What's your opinion about this?
On Wed, Oct 26, 2022 at 11:40:31AM +0800, Jiaxi Chen wrote: > > What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, > > KVM passthrough is the only reason the existing features are defined. Yap, looking at the patches which added those 2 feature flags upstream, they don't look like some particular use was the goal but rather to expose it to guests. Besides, AVX512 apps do their own CPUID detection. > Since CPUID_7_1_EAX has only 5 features now, it is a big waste, > should we move it to KVM-only leaf as Sean suggested. What's your > opinion about this? Yes, pls do. And when you do, make sure to undo what b302e4b176d0 ("x86/cpufeatures: Enumerate the new AVX512 BFLOAT16 instructions") added. Thx.
On 10/27/2022 1:15 AM, Borislav Petkov wrote: > On Wed, Oct 26, 2022 at 11:40:31AM +0800, Jiaxi Chen wrote: >>> What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, >>> KVM passthrough is the only reason the existing features are defined. > > Yap, looking at the patches which added those 2 feature flags upstream, > they don't look like some particular use was the goal but rather to > expose it to guests. Besides, AVX512 apps do their own CPUID detection. > >> Since CPUID_7_1_EAX has only 5 features now, it is a big waste, >> should we move it to KVM-only leaf as Sean suggested. What's your >> opinion about this? > > Yes, pls do. > > And when you do, make sure to undo what > > b302e4b176d0 ("x86/cpufeatures: Enumerate the new AVX512 BFLOAT16 instructions") > > added. > > Thx. > Yes, will do this in v2. Thanks for reminding~
On 10/27/2022 1:15 AM, Borislav Petkov wrote: > On Wed, Oct 26, 2022 at 11:40:31AM +0800, Jiaxi Chen wrote: >>> What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, >>> KVM passthrough is the only reason the existing features are defined. > > Yap, looking at the patches which added those 2 feature flags upstream, > they don't look like some particular use was the goal but rather to > expose it to guests. Besides, AVX512 apps do their own CPUID detection. > >> Since CPUID_7_1_EAX has only 5 features now, it is a big waste, >> should we move it to KVM-only leaf as Sean suggested. What's your >> opinion about this? > > Yes, pls do. > > And when you do, make sure to undo what > > b302e4b176d0 ("x86/cpufeatures: Enumerate the new AVX512 BFLOAT16 instructions") > > added. > > Thx. > Hi Sean and Boris, Just realized moving CPUID_7_1_EAX to kvm-only leaf will not save space in enum cpuid_leafs[]. CPUID_7_1_EAX is indeed removed, but someone else, ie. CPUID_DUMMY needs to take the place, otherwise the cpuid_leafs array would be deranged. Therefore, the length of x86 cpuid leaves is not decreased. Wonder if the intention of moving this leaf to kvm-only is for saving space in x86_capability[], or just because there's no other use case in the host kernel side and the cpuflags of this features can be removed. Hope for your suggestions.
On Tue, Nov 01, 2022, Jiaxi Chen wrote: > > > On 10/27/2022 1:15 AM, Borislav Petkov wrote: > > On Wed, Oct 26, 2022 at 11:40:31AM +0800, Jiaxi Chen wrote: > >>> What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, > >>> KVM passthrough is the only reason the existing features are defined. > > > > Yap, looking at the patches which added those 2 feature flags upstream, > > they don't look like some particular use was the goal but rather to > > expose it to guests. Besides, AVX512 apps do their own CPUID detection. > > > >> Since CPUID_7_1_EAX has only 5 features now, it is a big waste, > >> should we move it to KVM-only leaf as Sean suggested. What's your > >> opinion about this? > > > > Yes, pls do. > > > > And when you do, make sure to undo what > > > > b302e4b176d0 ("x86/cpufeatures: Enumerate the new AVX512 BFLOAT16 instructions") > > > > added. > > > > Thx. > > > Hi Sean and Boris, > > Just realized moving CPUID_7_1_EAX to kvm-only leaf will not save space > in enum cpuid_leafs[]. CPUID_7_1_EAX is indeed removed, but someone > else, ie. CPUID_DUMMY needs to take the place, otherwise the cpuid_leafs > array would be deranged. Therefore, the length of x86 cpuid leaves is > not decreased. The order of "enum cpuid_leafs" is completely arbitrary. After replacing CPUID_7_1_EAX with CPUID_DUMMY, replace CPUID_DUMMY with the last leaf, which is currently CPUID_8000_001F_EAX, and update the #defines accordingly. Alternatively, Boris may prefer skipping the intermediate CPUID_DUMMY step and just replace CPUID_7_1_EAX with CPUID_8000_001F_EAX straightaway.
On 11/1/2022 11:07 PM, Sean Christopherson wrote: > On Tue, Nov 01, 2022, Jiaxi Chen wrote: >> >> >> On 10/27/2022 1:15 AM, Borislav Petkov wrote: >>> On Wed, Oct 26, 2022 at 11:40:31AM +0800, Jiaxi Chen wrote: >>>>> What do you think about moving CPUID_7_1_EAX to be a KVM-only leaf too? AFAICT, >>>>> KVM passthrough is the only reason the existing features are defined. >>> >>> Yap, looking at the patches which added those 2 feature flags upstream, >>> they don't look like some particular use was the goal but rather to >>> expose it to guests. Besides, AVX512 apps do their own CPUID detection. >>> >>>> Since CPUID_7_1_EAX has only 5 features now, it is a big waste, >>>> should we move it to KVM-only leaf as Sean suggested. What's your >>>> opinion about this? >>> >>> Yes, pls do. >>> >>> And when you do, make sure to undo what >>> >>> b302e4b176d0 ("x86/cpufeatures: Enumerate the new AVX512 BFLOAT16 instructions") >>> >>> added. >>> >>> Thx. >>> >> Hi Sean and Boris, >> >> Just realized moving CPUID_7_1_EAX to kvm-only leaf will not save space >> in enum cpuid_leafs[]. CPUID_7_1_EAX is indeed removed, but someone >> else, ie. CPUID_DUMMY needs to take the place, otherwise the cpuid_leafs >> array would be deranged. Therefore, the length of x86 cpuid leaves is >> not decreased. > > The order of "enum cpuid_leafs" is completely arbitrary. > > After replacing CPUID_7_1_EAX with CPUID_DUMMY, replace CPUID_DUMMY with the last > leaf, which is currently CPUID_8000_001F_EAX, and update the #defines accordingly. > Alternatively, Boris may prefer skipping the intermediate CPUID_DUMMY step and > just replace CPUID_7_1_EAX with CPUID_8000_001F_EAX straightaway. Yes, thanks for Sean's kind suggestion. I think use CPUID_DUMMY as the transition leaf will make the code logic and commit message clearer. Will change it in v2.
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index ef4775c6db01..445626cb5779 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -308,6 +308,7 @@ /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ #define X86_FEATURE_AVX_VNNI (12*32+ 4) /* AVX VNNI instructions */ #define X86_FEATURE_AVX512_BF16 (12*32+ 5) /* AVX512 BFLOAT16 instructions */ +#define X86_FEATURE_CMPCCXADD (12*32+ 7) /* CMPccXADD instructions */ /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */ #define X86_FEATURE_CLZERO (13*32+ 0) /* CLZERO instruction */ diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 7065462378e2..3f745f6fdc43 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -657,7 +657,7 @@ void kvm_set_cpu_caps(void) kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL_SSBD); kvm_cpu_cap_mask(CPUID_7_1_EAX, - F(AVX_VNNI) | F(AVX512_BF16) + F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) ); kvm_cpu_cap_mask(CPUID_D_1_EAX,
CMPccXADD is a new set of instructions in the latest Intel platform Sierra Forest. It includes a semaphore operation that can compare and add the operands if condition is met, which can improve database performance. The bit definition: CPUID.(EAX=7,ECX=1):EAX[bit 7] This patch enables this CPUID in the kernel feature bits and expose it to guest OS. Signed-off-by: Jiaxi Chen <jiaxi.chen@linux.intel.com> --- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/kvm/cpuid.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)