Message ID | 1551494711-213533-16-git-send-email-fenghua.yu@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/split_lock: Enable #AC exception for split locked accesses | expand |
On 02/03/19 03:45, Fenghua Yu wrote: > From: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > In the latest Intel SDM, CPUID.(EAX=7H,ECX=0):EDX[30] will enumerate > the presence of the IA32_CORE_CAPABILITY MSR. > > Update GET_SUPPORTED_CPUID to expose this feature bit to user space, so > that user space know this bit can be enabled in CPUID. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > --- > arch/x86/kvm/cpuid.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index c07958b59f50..e0e17b9c65da 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -410,7 +410,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > /* cpuid 7.0.edx*/ > const u32 kvm_cpuid_7_0_edx_x86_features = > F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) | > - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP); > + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(CORE_CAPABILITY) | > + F(INTEL_STIBP); This should be enabled always if boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT), since the MSR is emulated. This way, guests can always rely on IA32_CORE_CAPABILITY_MSR and it won't have to rely on the FMS (which means nothing inside a guest). Paolo > /* all calls to cpuid_count() should be made on the same cpu */ > get_cpu(); >
On Mon, 2019-03-04 at 09:38 +0100, Paolo Bonzini wrote: > On 02/03/19 03:45, Fenghua Yu wrote: > > From: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > In the latest Intel SDM, CPUID.(EAX=7H,ECX=0):EDX[30] will enumerate > > the presence of the IA32_CORE_CAPABILITY MSR. > > > > Update GET_SUPPORTED_CPUID to expose this feature bit to user space, so > > that user space know this bit can be enabled in CPUID. > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > --- > > arch/x86/kvm/cpuid.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index c07958b59f50..e0e17b9c65da 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -410,7 +410,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 > > *entry, u32 function, > > /* cpuid 7.0.edx*/ > > const u32 kvm_cpuid_7_0_edx_x86_features = > > F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) | > > - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP); > > + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(CORE_CAPABILITY) | > > + F(INTEL_STIBP); > > This should be enabled always if boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT), > since the MSR is emulated. This way, guests can always rely on > IA32_CORE_CAPABILITY_MSR and it won't have to rely on the FMS > (which means nothing inside a guest). > > Paolo Hi, Paolo Do you mean that we don't need this here, but to add the handling below? static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 * entry, u32 function, ... switch (function) { ... case 7: { ... if (index ==0) { ... if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) entry->edx |= F(CORE_CAPABILITY); } ... ... } ... } > > /* all calls to cpuid_count() should be made on the same cpu */ > > get_cpu(); > > > >
On 04/03/19 11:47, Xiaoyao Li wrote: > On Mon, 2019-03-04 at 09:38 +0100, Paolo Bonzini wrote: >> On 02/03/19 03:45, Fenghua Yu wrote: >>> From: Xiaoyao Li <xiaoyao.li@linux.intel.com> >>> >>> In the latest Intel SDM, CPUID.(EAX=7H,ECX=0):EDX[30] will enumerate >>> the presence of the IA32_CORE_CAPABILITY MSR. >>> >>> Update GET_SUPPORTED_CPUID to expose this feature bit to user space, so >>> that user space know this bit can be enabled in CPUID. >>> >>> Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> >>> --- >>> arch/x86/kvm/cpuid.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index c07958b59f50..e0e17b9c65da 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -410,7 +410,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 >>> *entry, u32 function, >>> /* cpuid 7.0.edx*/ >>> const u32 kvm_cpuid_7_0_edx_x86_features = >>> F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) | >>> - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP); >>> + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(CORE_CAPABILITY) | >>> + F(INTEL_STIBP); >> >> This should be enabled always if boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT), >> since the MSR is emulated. This way, guests can always rely on >> IA32_CORE_CAPABILITY_MSR and it won't have to rely on the FMS >> (which means nothing inside a guest). >> >> Paolo > > Hi, Paolo > Do you mean that we don't need this here, but to add the handling below? > > static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 * entry, u32 function, > ... > switch (function) { > ... > case 7: { > ... > if (index ==0) { > ... > if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) > entry->edx |= F(CORE_CAPABILITY); > } > ... > ... > } > ... > } Yes, exactly. Paolo >>> /* all calls to cpuid_count() should be made on the same cpu */ >>> get_cpu(); >>> >> >> >
On Mon, 2019-03-04 at 11:49 +0100, Paolo Bonzini wrote: > On 04/03/19 11:47, Xiaoyao Li wrote: > > On Mon, 2019-03-04 at 09:38 +0100, Paolo Bonzini wrote: > > > On 02/03/19 03:45, Fenghua Yu wrote: > > > > From: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > > > > > In the latest Intel SDM, CPUID.(EAX=7H,ECX=0):EDX[30] will enumerate > > > > the presence of the IA32_CORE_CAPABILITY MSR. > > > > > > > > Update GET_SUPPORTED_CPUID to expose this feature bit to user space, so > > > > that user space know this bit can be enabled in CPUID. > > > > > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > --- > > > > arch/x86/kvm/cpuid.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > > index c07958b59f50..e0e17b9c65da 100644 > > > > --- a/arch/x86/kvm/cpuid.c > > > > +++ b/arch/x86/kvm/cpuid.c > > > > @@ -410,7 +410,8 @@ static inline int __do_cpuid_ent(struct > > > > kvm_cpuid_entry2 > > > > *entry, u32 function, > > > > /* cpuid 7.0.edx*/ > > > > const u32 kvm_cpuid_7_0_edx_x86_features = > > > > F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) | > > > > - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | > > > > F(INTEL_STIBP); > > > > + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | > > > > F(CORE_CAPABILITY) | > > > > + F(INTEL_STIBP); > > > > > > This should be enabled always if > > > boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT), > > > since the MSR is emulated. This way, guests can always rely on > > > IA32_CORE_CAPABILITY_MSR and it won't have to rely on the FMS > > > (which means nothing inside a guest). > > > > > > Paolo > > > > Hi, Paolo > > Do you mean that we don't need this here, but to add the handling below? > > > > static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 * entry, u32 > > function, > > ... > > switch (function) { > > ... > > case 7: { > > ... > > if (index ==0) { > > ... > > if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) > > entry->edx |= F(CORE_CAPABILITY); > > } > > ... > > ... > > } > > ... > > } > > Yes, exactly. > > Paolo Like you said before, I think we don't need the condition judgment "if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))", but to set F(CORE_CAPABILITY) always for guest since MSR_IA32_CORE_CAPABILITY is emulated. And we should set the right emulated value of MSR_IA32_CORE_CAPABILITY for guest in function kvm_get_core_capability() based on whether boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) just as you commented in the next patch. Xiaoyao > > > > /* all calls to cpuid_count() should be made on the same cpu */ > > > > get_cpu(); > > > > > > > > > > > >
On 04/03/19 12:10, Xiaoyao Li wrote: > Like you said before, I think we don't need the condition judgment > "if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))", but to set F(CORE_CAPABILITY) > always for guest since MSR_IA32_CORE_CAPABILITY is emulated. > > And we should set the right emulated value of MSR_IA32_CORE_CAPABILITY for guest > in function kvm_get_core_capability() based on whether > boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) just as you commented in the next > patch. Yes, that would certainly be better. However, you'd also have to move MSR_IA32_CORE_CAPABILITY handling to x86.c, because you'd have to enable X86_FEATURE_CORE_CAPABILITY for AMD. Paolo
On Mon, 2019-03-04 at 12:14 +0100, Paolo Bonzini wrote: > On 04/03/19 12:10, Xiaoyao Li wrote: > > Like you said before, I think we don't need the condition judgment > > "if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))", but to set > > F(CORE_CAPABILITY) > > always for guest since MSR_IA32_CORE_CAPABILITY is emulated. > > > > And we should set the right emulated value of MSR_IA32_CORE_CAPABILITY for > > guest > > in function kvm_get_core_capability() based on whether > > boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) just as you commented in the > > next > > patch. > > Yes, that would certainly be better. However, you'd also have to move > MSR_IA32_CORE_CAPABILITY handling to x86.c, because you'd have to enable > X86_FEATURE_CORE_CAPABILITY for AMD. > > Paolo Thanks for your comments and advises. I'll do it in next version. Xiaoyao
On Mon, 2019-03-04 at 12:14 +0100, Paolo Bonzini wrote: > On 04/03/19 12:10, Xiaoyao Li wrote: > > Like you said before, I think we don't need the condition judgment > > "if(boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))", but to set > > F(CORE_CAPABILITY) > > always for guest since MSR_IA32_CORE_CAPABILITY is emulated. > > > > And we should set the right emulated value of MSR_IA32_CORE_CAPABILITY for > > guest > > in function kvm_get_core_capability() based on whether > > boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) just as you commented in the > > next > > patch. > > Yes, that would certainly be better. However, you'd also have to move > MSR_IA32_CORE_CAPABILITY handling to x86.c, because you'd have to enable > X86_FEATURE_CORE_CAPABILITY for AMD. > > Paolo Hi, Paolo I just notice that F(ARCH_CAPABILITIES) is set unconditionally. However the handling of MSR_IA32_ARCH_CAPABILITIES only exists with vmx, and the emulation of this MSR is in vmx->arch_capabilities. These will cause #GP when guest kernel rdmsr(MSR_IA32_ARCH_CAPABILITES) with AMD CPU since there is handling for svm. Maybe what I think is not correct due to my limit knowledge of MSR_IA32_ARCH_CAPABILITIES and how kernel handles its related features. If what I said above is true and it's indeed an issue. So based on the fact that both MSR_IA32_ARCH_CAPABILITIES and MSR_IA32_CORE_CAPABILITY are feature- enumerating MSR and we emulate them in KVM, there are 2 choices for us to handle it: 1. we unconditionally set F(ARCH_CAPABILITIES) and F(CORE_CAPABILITY) for guest, move the emulation of these 2 MSRs to vcpu->arch.***, and move all the handling of these 2 MSRs to x86.c. 2. we conditionally set F(ARCH_CAPABILITIES) and F(CORE_CAPABILITY) only if it is intel CPU. So we just need to emulate these 2 MSRs in vmx->*** for intel CPU. I prefer option 2 personally for CORE_CAPABILITY since it makes no sense to expose MSR_IA32_CORE_CAPABILITY to other x86 vendors. About ARCH_CAPABILITIES, it seems that we emulate it for generic x86 cpus that !x86_match_cpu(cpu_no_speculation). So we should choose option 1, to move the emulation and handling to x86.c? Xiaoyao
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index c07958b59f50..e0e17b9c65da 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -410,7 +410,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, /* cpuid 7.0.edx*/ const u32 kvm_cpuid_7_0_edx_x86_features = F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) | - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP); + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(CORE_CAPABILITY) | + F(INTEL_STIBP); /* all calls to cpuid_count() should be made on the same cpu */ get_cpu();