| Message ID | 20190522070101.7636-6-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce support for Guest CET feature | expand |
On Wed, May 22, 2019 at 03:00:58PM +0800, Yang Weijiang wrote: > "Load Guest CET state" bit controls whether Guest CET states > will be loaded at Guest entry. Before doing that, KVM needs > to check if CPU CET feature is available to Guest. > > Note: SHSTK and IBT features share one control MSR: > MSR_IA32_{U,S}_CET, which means it's difficult to hide > one feature from another in the case of SHSTK != IBT, > after discussed in community, it's agreed to allow Guest > control two features independently as it won't introduce > security hole. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 9321da538f65..1c0d487a4037 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -47,6 +47,7 @@ > #include <asm/spec-ctrl.h> > #include <asm/virtext.h> > #include <asm/vmx.h> > +#include <asm/cet.h> Is this include actually needed? I haven't attempted to compile, but a glance everything should be in cpufeatures.h or vmx.h. > #include "capabilities.h" > #include "cpuid.h" > @@ -2929,6 +2930,17 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > if (!nested_vmx_allowed(vcpu) || is_smm(vcpu)) > return 1; > } > + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) { > + if (cr4 & X86_CR4_CET) > + vmcs_set_bits(VM_ENTRY_CONTROLS, > + VM_ENTRY_LOAD_GUEST_CET_STATE); > + else > + vmcs_clear_bits(VM_ENTRY_CONTROLS, > + VM_ENTRY_LOAD_GUEST_CET_STATE); > + } else if (cr4 & X86_CR4_CET) { > + return 1; > + } Don't we also need to check for host CET support prior to toggling VM_ENTRY_LOAD_GUEST_CET_STATE? > > if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) > return 1; > -- > 2.17.2 >
On Tue, Jun 04, 2019 at 01:03:36PM -0700, Sean Christopherson wrote: > On Wed, May 22, 2019 at 03:00:58PM +0800, Yang Weijiang wrote: > > "Load Guest CET state" bit controls whether Guest CET states > > will be loaded at Guest entry. Before doing that, KVM needs > > to check if CPU CET feature is available to Guest. > > > > Note: SHSTK and IBT features share one control MSR: > > MSR_IA32_{U,S}_CET, which means it's difficult to hide > > one feature from another in the case of SHSTK != IBT, > > after discussed in community, it's agreed to allow Guest > > control two features independently as it won't introduce > > security hole. > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 9321da538f65..1c0d487a4037 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -47,6 +47,7 @@ > > #include <asm/spec-ctrl.h> > > #include <asm/virtext.h> > > #include <asm/vmx.h> > > +#include <asm/cet.h> > > Is this include actually needed? I haven't attempted to compile, but a > glance everything should be in cpufeatures.h or vmx.h. > Thanks Sean! My original purpose is to re-use the macro cpu_x86_cet_enabled() to check host CET status, for somehow, the check is not there, but to resolve your below question, I need to use the macro to check it, so will keep this include and add the check in next version. > > #include "capabilities.h" > > #include "cpuid.h" > > @@ -2929,6 +2930,17 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > if (!nested_vmx_allowed(vcpu) || is_smm(vcpu)) > > return 1; > > } > > + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) { > > + if (cr4 & X86_CR4_CET) > > + vmcs_set_bits(VM_ENTRY_CONTROLS, > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > + else > > + vmcs_clear_bits(VM_ENTRY_CONTROLS, > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > + } else if (cr4 & X86_CR4_CET) { > > + return 1; > > + } > > Don't we also need to check for host CET support prior to toggling > VM_ENTRY_LOAD_GUEST_CET_STATE? Yes, need add back the check. v3 patch changed the CET CPUID enumeration to guest, and lost the check from then on. > > > > > if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) > > return 1; > > -- > > 2.17.2 > >
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 9321da538f65..1c0d487a4037 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -47,6 +47,7 @@ #include <asm/spec-ctrl.h> #include <asm/virtext.h> #include <asm/vmx.h> +#include <asm/cet.h> #include "capabilities.h" #include "cpuid.h" @@ -2929,6 +2930,17 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) if (!nested_vmx_allowed(vcpu) || is_smm(vcpu)) return 1; } + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) { + if (cr4 & X86_CR4_CET) + vmcs_set_bits(VM_ENTRY_CONTROLS, + VM_ENTRY_LOAD_GUEST_CET_STATE); + else + vmcs_clear_bits(VM_ENTRY_CONTROLS, + VM_ENTRY_LOAD_GUEST_CET_STATE); + } else if (cr4 & X86_CR4_CET) { + return 1; + } if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) return 1;
"Load Guest CET state" bit controls whether Guest CET states will be loaded at Guest entry. Before doing that, KVM needs to check if CPU CET feature is available to Guest. Note: SHSTK and IBT features share one control MSR: MSR_IA32_{U,S}_CET, which means it's difficult to hide one feature from another in the case of SHSTK != IBT, after discussed in community, it's agreed to allow Guest control two features independently as it won't introduce security hole. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> --- arch/x86/kvm/vmx/vmx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)