This patch-set is to enable kvm Guest OS CET support.
cover This patch-set is to enable kvm Guest OS CET support.
Control-flow Enforcement Technology (CET) provides protection against return/jump-oriented programming (ROP) attacks. To make kvm Guest OS own the capability, this patch-set is required. It enables CET related CPUID report, xsaves/xrstors, vmx entry configuration etc for Guest OS.
PATCH 1/3/4: Enable CET support in VMCS.
PATCH 2 : Define CR4.CET master enable bit.
PATCH 5 : Enable xsave components for CET in XSS.
PATCH 6/7/8: Report CET feature support in CPUID.
Yang Weijiang (8):
kvm:vmx Introduce CET related VMCS field definitions.
kvm: Define CR4.CET[bit 23] (master enable bit) for guest OS. kvm:vmx Enable loading CET state bit while guest CR4.CET is being
set.
kvm:vmx Pass through host CET related MSRs to Guest. kvm:x86 Enable MSR_IA32_XSS bit 11 and 12 for CET xsaves/xrstors.
kvm:cpuid Add CPUID support for CET xsaves component query. kvm:cpuid Fix xsaves area size calculation for CPUID.(EAX=0xD,ECX=1). kvm:cpuid Report CET SHSTK and IBT support in CPUID.(EAX=0x7,ECX=0).
arch/x86/include/asm/kvm_host.h | 3 ++- arch/x86/include/asm/vmx.h | 8 +++++++ arch/x86/kvm/cpuid.c | 23 +++++++++++++------- arch/x86/kvm/vmx.c | 37 ++++++++++++++++++++++++++++++--- 4 files changed, 60 insertions(+), 11 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/8 kvm:vmx Introduce CET related VMCS field definitions.
VMX relies on these fields to save/restore CET states for guest and host. They are added here as VMCS placeholders for the function.
--- arch/x86/include/asm/vmx.h | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/x86/include/asm/vmx.h +8 -0
@@ -98,6 +98,7 @@#define VM_EXIT_LOAD_IA32_EFER 0x00200000#define VM_EXIT_SAVE_VMX_PREEMPTION_TIMER 0x00400000#define VM_EXIT_CLEAR_BNDCFGS 0x00800000 +#define VM_EXIT_LOAD_HOST_CET_STATE 0x10000000#define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR 0x00036dff
@@ -109,6 +110,7 @@#define VM_ENTRY_LOAD_IA32_PAT 0x00004000#define VM_ENTRY_LOAD_IA32_EFER 0x00008000#define VM_ENTRY_LOAD_BNDCFGS 0x00010000 +#define VM_ENTRY_LOAD_GUEST_CET_STATE 0x00100000#define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR 0x000011ff
@@ -325,6 +327,9 @@ enum vmcs_field {GUEST_PENDING_DBG_EXCEPTIONS =0x00006822 ,GUEST_SYSENTER_ESP =0x00006824 ,GUEST_SYSENTER_EIP =0x00006826 , +GUEST_IA32_S_CET =0x00006828 , +GUEST_SSP =0x0000682a , +GUEST_INT_SSP_TABL =0x0000682c ,HOST_CR0 =0x00006c00 ,HOST_CR3 =0x00006c02 , HOST_CR4 = 0x00006c04,
@@ -337,6 +342,9 @@ enum vmcs_field {HOST_IA32_SYSENTER_EIP =0x00006c12 ,HOST_RSP =0x00006c14 ,HOST_RIP =0x00006c16 , +HOST_IA32_S_CET =0x00006c18 , +HOST_SSP =0x00006c1a , + HOST_INT_SSP_TABL = 0x00006c1c };/*
Signature · 1 line
2.17.1
Re: [PATCH v1 1/8] kvm:vmx Introduce CET related VMCS field definitions.
Quoted 3 lines · Yang Weijiang
VMX relies on these fields to save/restore CET states for guest and host. They are added here as VMCS placeholders for the function.
The changelog needs a lot more detail on what is saved where and when, e.g. hardware unconditionally saves the guest MSRs to the VMCS fields on exit, hence there is no VM_EXIT_SAVE_GUEST_CET_STATE control bit.
It's also worth calling out that the other CET MSRs are saved/loaded via XSAVES/XRSTORS.
Quoted 28 lines
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/include/asm/vmx.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/x86/include/asm/vmx.h +2
-0@@ -98,6 +98,7 @@#define VM_EXIT_LOAD_IA32_EFER 0x00200000#define VM_EXIT_SAVE_VMX_PREEMPTION_TIMER 0x00400000#define VM_EXIT_CLEAR_BNDCFGS 0x00800000 +#define VM_EXIT_LOAD_HOST_CET_STATE 0x10000000#define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR 0x00036dff@@ -109,6 +110,7 @@#define VM_ENTRY_LOAD_IA32_PAT 0x00004000#define VM_ENTRY_LOAD_IA32_EFER 0x00008000#define VM_ENTRY_LOAD_BNDCFGS 0x00010000 +#define VM_ENTRY_LOAD_GUEST_CET_STATE 0x00100000#define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR 0x000011ff
arch/x86/include/asm/vmx.h enum vmcs_field { · line 332
Quoted 6 lines
arch/x86/include/asm/vmx.h +3
-0@@ -325,6 +327,9 @@ enum vmcs_field {GUEST_PENDING_DBG_EXCEPTIONS =0x00006822 ,GUEST_SYSENTER_ESP =0x00006824 ,GUEST_SYSENTER_EIP =0x00006826 , +GUEST_IA32_S_CET =0x00006828 , +GUEST_SSP =0x0000682a , + GUEST_INT_SSP_TABL = 0x0000682c,
Dropping a single letter from "TABLE" seems pointless, "INTERRUPT" is usually "INTR", and the docs I've seen list the name as IA32_INTERRUPT_SSP_TABLE_ADDR. So maybe?
{GUEST,HOST}_INTR_SSP_TABLE_ADDR
Also, the SSP and INTERRUPT_SSP_TABL_ADDR fields are 64-bit fields, we we should define the _HIGH variations.
Quoted 16 lines
arch/x86/include/asm/vmx.h +3
-0HOST_CR0 =0x00006c00 ,HOST_CR3 =0x00006c02 , HOST_CR4 = 0x00006c04,@@ -337,6 +342,9 @@ enum vmcs_field {HOST_IA32_SYSENTER_EIP =0x00006c12 ,HOST_RSP =0x00006c14 ,HOST_RIP =0x00006c16 , +HOST_IA32_S_CET =0x00006c18 , +HOST_SSP =0x00006c1a , + HOST_INT_SSP_TABL = 0x00006c1c };/* Signature · 1 line
2.17.1
PATCH 2/8 kvm: Define CR4.CET[bit 23] (master enable bit) for guest OS.
In reply to Yang WeijiangThis bit controls CET feature availability in guest OS.
--- arch/x86/include/asm/kvm_host.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/include/asm/kvm_host.h +2 -1
@@ -90,7 +90,8 @@ | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \ | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \ | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \ - | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP)) + | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \ + | X86_CR4_CET))#define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR)
Signature · 1 line
2.17.1
PATCH 3/8 kvm:vmx Enable loading CET state bit while guest CR4.CET is being set.
In reply to Yang WeijiangThis bit controls whether guest CET states will be loaded on guest entry.
--- arch/x86/kvm/vmx.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
arch/x86/kvm/vmx.c +19 -0
@@ -1045,6 +1045,8 @@ struct vcpu_vmx {bool req_immediate_exit; +bool vcpu_cet_on; +/* Support for PML */ #define PML_ENTITY_NUM 512struct page *pml_pg;
@@ -5409,6 +5411,23 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)return 1 ; } +/* +* When CET.CR4 is being set, it means we're enabling CET for +* the guest, then enable loading CET state bit in entry control. +* Otherwise, clear loading CET bit to disable guest CET. +*/ +if (cr4 &X86_CR4_CET ) { +if (!to_vmx (vcpu)->vcpu_cet_on) { +vmcs_set_bits (VM_ENTRY_CONTROLS , +VM_ENTRY_LOAD_GUEST_CET_STATE ); +to_vmx (vcpu)->vcpu_cet_on =1 ; + } + }else if (to_vmx (vcpu)->vcpu_cet_on) { +vmcs_clear_bits (VM_ENTRY_CONTROLS , +VM_ENTRY_LOAD_GUEST_CET_STATE ); +to_vmx (vcpu)->vcpu_cet_on =0 ; + } +if (to_vmx (vcpu)->nested.vmxon && !nested_cr4_valid (vcpu, cr4))return 1 ;
Signature · 1 line
2.17.1
Re: [PATCH v1 3/8] kvm:vmx Enable loading CET state bit while guest CR4.CET is being set.
Quoted 26 lines · Yang Weijiang
This bit controls whether guest CET states will be loaded on guest entry.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>---
arch/x86/kvm/vmx.c| 19 +++++++++++++++++++1 file changed, 19 insertions(+)arch/x86/kvm/vmx.c +2
-0@@ -1045,6 +1045,8 @@ struct vcpu_vmx {bool req_immediate_exit; +bool vcpu_cet_on; +/* Support for PML */ #define PML_ENTITY_NUM 512struct page *pml_pg;
arch/x86/kvm/vmx.c static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) · line 5415
Quoted 5 lines
arch/x86/kvm/vmx.c +2
-0@@ -5409,6 +5411,23 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)return 1 ; } +/* +* When CET.CR4 is being set, it means we're enabling CET for
You probably meant to write here CR4.CET.
Quoted 21 lines
arch/x86/kvm/vmx.c +15
-0+* the guest, then enable loading CET state bit in entry control. +* Otherwise, clear loading CET bit to disable guest CET. +*/ +if (cr4 &X86_CR4_CET ) { +if (!to_vmx (vcpu)->vcpu_cet_on) { +vmcs_set_bits (VM_ENTRY_CONTROLS , +VM_ENTRY_LOAD_GUEST_CET_STATE ); +to_vmx (vcpu)->vcpu_cet_on =1 ; + } + }else if (to_vmx (vcpu)->vcpu_cet_on) { +vmcs_clear_bits (VM_ENTRY_CONTROLS , +VM_ENTRY_LOAD_GUEST_CET_STATE ); +to_vmx (vcpu)->vcpu_cet_on =0 ; + } +if (to_vmx (vcpu)->nested.vmxon && !nested_cr4_valid (vcpu, cr4))return 1 ;Signature · 1 line
2.17.1
I haven’t seen a patch in the series that modifies kvm_set_cr4() to verify CR4.CET is not set when CET is not reported as supported by CPUID. I think that is missing from the series.
-Liran
Re: [PATCH v1 3/8] kvm:vmx Enable loading CET state bit while guest CR4.CET is being set.
On Wed, Dec 26, 2018 at 04:52:38PM +0800, Liran Alon wrote:
Thanks Liran for pointing out the issues!
I'll fix them in next version.
Quoted 62 lines
Quoted 26 lines · Yang Weijiang
This bit controls whether guest CET states will be loaded on guest entry.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>---
arch/x86/kvm/vmx.c| 19 +++++++++++++++++++1 file changed, 19 insertions(+)arch/x86/kvm/vmx.c +2
-0@@ -1045,6 +1045,8 @@ struct vcpu_vmx {bool req_immediate_exit; +bool vcpu_cet_on; +/* Support for PML */ #define PML_ENTITY_NUM 512struct page *pml_pg;arch/x86/kvm/vmx.c static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) · line 5415
Quoted 5 lines
arch/x86/kvm/vmx.c +2
-0@@ -5409,6 +5411,23 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)return 1 ; } +/* +* When CET.CR4 is being set, it means we're enabling CET for You probably meant to write here CR4.CET.
Quoted 21 lines
arch/x86/kvm/vmx.c +15
-0+* the guest, then enable loading CET state bit in entry control. +* Otherwise, clear loading CET bit to disable guest CET. +*/ +if (cr4 &X86_CR4_CET ) { +if (!to_vmx (vcpu)->vcpu_cet_on) { +vmcs_set_bits (VM_ENTRY_CONTROLS , +VM_ENTRY_LOAD_GUEST_CET_STATE ); +to_vmx (vcpu)->vcpu_cet_on =1 ; + } + }else if (to_vmx (vcpu)->vcpu_cet_on) { +vmcs_clear_bits (VM_ENTRY_CONTROLS , +VM_ENTRY_LOAD_GUEST_CET_STATE ); +to_vmx (vcpu)->vcpu_cet_on =0 ; + } +if (to_vmx (vcpu)->nested.vmxon && !nested_cr4_valid (vcpu, cr4))return 1 ;Signature · 1 line
2.17.1I haven’t seen a patch in the series that modifies
kvm_set_cr4()to verify CR4.CET is not set when CET is not reported as supported by CPUID. I think that is missing from the series.-Liran
PATCH 4/8 kvm:vmx Pass through host CET related MSRs to Guest.
In reply to Yang WeijiangDuring Guest OS execution, it accesses these MSRs to configure CET runtime settings.
--- arch/x86/kvm/vmx.c | 7 +++++++ 1 file changed, 7 insertions(+)
arch/x86/kvm/vmx.c +7 -0
@@ -11550,6 +11550,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_CS ,MSR_TYPE_RW );vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_ESP ,MSR_TYPE_RW );vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_EIP ,MSR_TYPE_RW ); + +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_U_CET ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_S_CET ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_INT_SSP_TAB ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL0_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL3_SSP ,MSR_TYPE_RW ); + vmx->msr_bitmap_mode =0 ; vmx->loaded_vmcs = &vmx->vmcs01;
Signature · 1 line
2.17.1
Re: [PATCH v1 4/8] kvm:vmx Pass through host CET related MSRs to Guest.
Quoted 2 lines · Yang Weijiang
During Guest OS execution, it accesses these MSRs to configure CET runtime settings.
The changelog needs to explain why it's ok to expose these MSRS to the guest, e.g. call out which are saved/loaded through the VMCS and which are saved/loaded via XSAVES/XRSTORS. And maybe expand "runtime settings" to elaborate on when most guest kernels will access the MSRs, i.e. to justify exposing them to the guest.
It should also clarify why IA32_PL1_SSP and IA32_PL2_SSP are NOT exposed to the guest.
Quoted 28 lines
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx.c | 7 +++++++ 1 file changed, 7 insertions(+)arch/x86/kvm/vmx.c +7
-0@@ -11550,6 +11550,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_CS ,MSR_TYPE_RW );vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_ESP ,MSR_TYPE_RW );vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_EIP ,MSR_TYPE_RW ); + +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_U_CET ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_S_CET ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_INT_SSP_TAB ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL0_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL3_SSP ,MSR_TYPE_RW ); + vmx->msr_bitmap_mode =0 ; vmx->loaded_vmcs = &vmx->vmcs01;Signature · 1 line
2.17.1
Re: [PATCH v1 4/8] kvm:vmx Pass through host CET related MSRs to Guest.
In reply to Yang WeijiangQuoted 15 lines · Yang Weijiang
During Guest OS execution, it accesses these MSRs to configure CET runtime settings.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx.c | 7 +++++++ 1 file changed, 7 insertions(+)arch/x86/kvm/vmx.c +0
-0
arch/x86/kvm/vmx.c static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) · line 11559
Quoted 10 lines
arch/x86/kvm/vmx.c +7
-0@@ -11550,6 +11550,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_CS ,MSR_TYPE_RW );vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_ESP ,MSR_TYPE_RW );vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_SYSENTER_EIP ,MSR_TYPE_RW ); + +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_U_CET ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_S_CET ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_INT_SSP_TAB ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL0_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL3_SSP ,MSR_TYPE_RW ); +
Shouldn't this be conditional on the guest having CET support enumerated in CPUID?
Quoted 6 lines
arch/x86/kvm/vmx.c +0
-1vmx->msr_bitmap_mode =0 ; vmx->loaded_vmcs = &vmx->vmcs01; --2.17.1
PATCH 5/8 kvm:x86 Enable MSR_IA32_XSS bit 11 and 12 for CET xsaves/xrstors.
In reply to Yang WeijiangFor kvm Guest OS, right now, only bit 11(user mode CET) and bit 12 (supervisor CET) are supported in XSS MSR, if other bits are being set, the write to XSS will be skipped.
--- arch/x86/kvm/vmx.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
arch/x86/kvm/vmx.c +8 -3
@@ -47,6 +47,7 @@#include <asm/virtext.h> #include <asm/mce.h> #include <asm/fpu/internal.h> +#include <asm/fpu/types.h> #include <asm/perf_event.h> #include <asm/debugreg.h> #include <asm/kexec.h>
@@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)case MSR_IA32_XSS :if (!vmx_xsaves_supported ())return 1 ; +/* -* The only supported bit as of Skylake is bit 8, but -* it is not supported on KVM. +* Right now, only support XSS_CET_U[bit 11] and +* XSS_CET_S[bit 12] in MSR_IA32_XSS. */ -if (data !=0 ) + +if (data & ~(XFEATURE_MASK_SHSTK_USER + |XFEATURE_MASK_SHSTK_KERNEL ))return 1 ; + vcpu->arch.ia32_xss = data;if (vcpu->arch.ia32_xss != host_xss) add_atomic_switch_msr(vmx, MSR_IA32_XSS,
Signature · 1 line
2.17.1
Re: [PATCH v1 5/8] kvm:x86 Enable MSR_IA32_XSS bit 11 and 12 for CET xsaves/xrstors.
Quoted 27 lines · Yang Weijiang
For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12 (supervisor CET) are supported in XSS MSR, if other bits are being set, the write to XSS will be skipped.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)arch/x86/kvm/vmx.c +2
-0@@ -47,6 +47,7 @@#include <asm/virtext.h> #include <asm/mce.h> #include <asm/fpu/internal.h> +#include <asm/fpu/types.h> #include <asm/perf_event.h> #include <asm/debugreg.h> #include <asm/kexec.h> @@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)case MSR_IA32_XSS :if (!vmx_xsaves_supported ())return 1 ; +
arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 4334
Quoted 10 lines
arch/x86/kvm/vmx.c +5
-3/* -* The only supported bit as of Skylake is bit 8, but -* it is not supported on KVM. +* Right now, only support XSS_CET_U[bit 11] and +* XSS_CET_S[bit 12] in MSR_IA32_XSS. */ -if (data !=0 ) + +if (data & ~(XFEATURE_MASK_SHSTK_USER + |XFEATURE_MASK_SHSTK_KERNEL ))
New lines are usually after the operator, e.g.:
if (data & ~(XFEATURE_MASK_SHSTK_USER |
XFEATURE_MASK_SHSTK_KERNEL))
And doesn't this flow need to check that the bits are actually supported?
Quoted 8 lines
arch/x86/kvm/vmx.c +1
-0return 1 ; + vcpu->arch.ia32_xss = data;if (vcpu->arch.ia32_xss != host_xss) add_atomic_switch_msr(vmx, MSR_IA32_XSS,Signature · 1 line
2.17.1
Re: [PATCH v1 5/8] kvm:x86 Enable MSR_IA32_XSS bit 11 and 12 for CET xsaves/xrstors.
Quoted 42 lines · Sean Christopherson
Quoted 27 lines · Yang Weijiang
For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12 (supervisor CET) are supported in XSS MSR, if other bits are being set, the write to XSS will be skipped.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)arch/x86/kvm/vmx.c +2
-0@@ -47,6 +47,7 @@#include <asm/virtext.h> #include <asm/mce.h> #include <asm/fpu/internal.h> +#include <asm/fpu/types.h> #include <asm/perf_event.h> #include <asm/debugreg.h> #include <asm/kexec.h> @@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)case MSR_IA32_XSS :if (!vmx_xsaves_supported ())return 1 ; +arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 4334
Quoted 10 lines
arch/x86/kvm/vmx.c +5
-3/* -* The only supported bit as of Skylake is bit 8, but -* it is not supported on KVM. +* Right now, only support XSS_CET_U[bit 11] and +* XSS_CET_S[bit 12] in MSR_IA32_XSS. */ -if (data !=0 ) + +if (data & ~(XFEATURE_MASK_SHSTK_USER + |XFEATURE_MASK_SHSTK_KERNEL ))New lines are usually after the operator, e.g.:
Quoted 4 lines
if (data & ~(XFEATURE_MASK_SHSTK_USER |
XFEATURE_MASK_SHSTK_KERNEL))And doesn't this flow need to check that the bits are actually supported?
Supported on the host and in the guest.
Re: [PATCH v1 5/8] kvm:x86 Enable MSR_IA32_XSS bit 11 and 12 for CET xsaves/xrstors.
On Wed, Jan 02, 2019 at 11:19:34AM -0800, Jim Mattson wrote:
Thanks Sean and Jim for your valuable comments, I'll consolidate them in next release!
Quoted 49 lines
Quoted 42 lines · Sean Christopherson
Quoted 27 lines · Yang Weijiang
For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12 (supervisor CET) are supported in XSS MSR, if other bits are being set, the write to XSS will be skipped.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)arch/x86/kvm/vmx.c +2
-0@@ -47,6 +47,7 @@#include <asm/virtext.h> #include <asm/mce.h> #include <asm/fpu/internal.h> +#include <asm/fpu/types.h> #include <asm/perf_event.h> #include <asm/debugreg.h> #include <asm/kexec.h> @@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)case MSR_IA32_XSS :if (!vmx_xsaves_supported ())return 1 ; +arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 4334
Quoted 10 lines
arch/x86/kvm/vmx.c +5
-3/* -* The only supported bit as of Skylake is bit 8, but -* it is not supported on KVM. +* Right now, only support XSS_CET_U[bit 11] and +* XSS_CET_S[bit 12] in MSR_IA32_XSS. */ -if (data !=0 ) + +if (data & ~(XFEATURE_MASK_SHSTK_USER + |XFEATURE_MASK_SHSTK_KERNEL ))New lines are usually after the operator, e.g.:
Quoted 4 lines
if (data & ~(XFEATURE_MASK_SHSTK_USER |
XFEATURE_MASK_SHSTK_KERNEL))And doesn't this flow need to check that the bits are actually supported?
Supported on the host and in the guest.
PATCH 6/8 kvm:cpuid Add CPUID support for CET xsaves component query.
In reply to Yang WeijiangCET xsaves component size is queried through CPUID.(EAX=0xD, ECX=11) and CPUID.(EAX=0xD, ECX=12).
--- arch/x86/kvm/cpuid.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
arch/x86/kvm/cpuid.c +10 -4
@@ -565,6 +565,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,case 0xd : {int idx, i;u64 supported =kvm_supported_xcr0 (); +u64 sv_supported; entry->eax &= supported; entry->ebx =xstate_required_size (supported, false);
@@ -584,18 +585,23 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;cpuid_mask (&entry[i].eax,CPUID_D_1_EAX ); entry[i].ebx =0 ; + sv_supported = entry[i].ecx + + ((u64 )entry[i].edx <<32 );if (entry[i].eax & (F (XSAVES )|F (XSAVEC ))) entry[i].ebx =xstate_required_size (supported, true); - }else { + }else if (!(entry[i].ecx &1 )) {if (entry[i].eax ==0 || !(supported & mask))continue ; -if (WARN_ON_ONCE (entry[i].ecx &1 )) + entry[i].ecx =0 ; + entry[i].edx =0 ; + }else { +if (entry[i].eax ==0 || !(sv_supported & mask))continue ; + entry[i].ecx =1 ; + entry[i].edx =0 ; } - entry[i].ecx =0 ; - entry[i].edx =0 ; entry[i].flags |=KVM_CPUID_FLAG_SIGNIFCANT_INDEX ; ++*nent;
Signature · 1 line
2.17.1
Re: [PATCH v1 6/8] kvm:cpuid Add CPUID support for CET xsaves component query.
Quoted 14 lines · Yang Weijiang
CET xsaves component size is queried through CPUID.(EAX=0xD, ECX=11) and CPUID.(EAX=0xD, ECX=12).
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/cpuid.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)arch/x86/kvm/cpuid.c +0
-0
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 568
Quoted 4 lines
arch/x86/kvm/cpuid.c +1
-0@@ -565,6 +565,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,case 0xd : {int idx, i;u64 supported =kvm_supported_xcr0 (); +u64 sv_supported;
What about u_supported and s_supported? sv_supported doesn't make me think "supervisor"e.
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 589
Quoted 8 lines
arch/x86/kvm/cpuid.c +2
-0entry->eax &= supported; entry->ebx =xstate_required_size (supported, false);@@ -584,18 +585,23 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;cpuid_mask (&entry[i].eax,CPUID_D_1_EAX ); entry[i].ebx =0 ; + sv_supported = entry[i].ecx + + ((u64 )entry[i].edx <<32 );
Use '|' instead of '+' to smush ECX and EDX together, as is it looks like the code is calculating a size or something.
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 594
Quoted 6 lines
arch/x86/kvm/cpuid.c +1
-1if (entry[i].eax & (F (XSAVES )|F (XSAVEC ))) entry[i].ebx =xstate_required_size (supported, true); - }else { + }else if (!(entry[i].ecx &1 )) {
Now that we're actually consuming bit 0, it'd be nice to formally define it as referring to supervisor state.
What about styling the code like this? Might make it more obvious that the logic for user vs. supervisor is identical, i.e. ECX bit 0 only determines which mask is applied.
if (idx == 1) {
...
} else {
supported = (entry[i].ecx & 1) ? s_supported :
u_supported;
if (entry[i].eax == 0 || !(supported & mask))
continue;
entry[i].ecx &= 1;
entry[i].edx = 0;
}arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 598
Quoted 13 lines
arch/x86/kvm/cpuid.c +6
-3if (entry[i].eax ==0 || !(supported & mask))continue ; -if (WARN_ON_ONCE (entry[i].ecx &1 )) + entry[i].ecx =0 ; + entry[i].edx =0 ; + }else { +if (entry[i].eax ==0 || !(sv_supported & mask))continue ; + entry[i].ecx =1 ; + entry[i].edx =0 ; } - entry[i].ecx =0 ; - entry[i].edx =0 ;
Removing this entirely isn't correct for CPUID.0xD.0x1, KVM should still zero out the bits it doesn't handle, e.g. KVM will signal a fault if the guest attempts to set any bits in IA32_XSS other than the CET bits.
Quoted 6 lines
arch/x86/kvm/cpuid.c +0
-0entry[i].flags |=KVM_CPUID_FLAG_SIGNIFCANT_INDEX ; ++*nent;Signature · 1 line
2.17.1
PATCH 7/8 kvm:cpuid Fix xsaves area size calculation for CPUID.(EAX=0xD,ECX=1).
In reply to Yang WeijiangAccording to latest Software Development Manual vol.2/3.2, for CPUID.(EAX=0xD,ECX=1), it should report xsaves area size containing all states enabled by XCR0|IA32_MSR_XSS.
--- arch/x86/kvm/cpuid.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +2 -1
@@ -121,7 +121,8 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) best =kvm_find_cpuid_entry (vcpu,0xD ,1 );if (best && (best->eax & (F (XSAVES ) |F (XSAVEC )))) - best->ebx =xstate_required_size (vcpu->arch.xcr0, true); + best->ebx =xstate_required_size (vcpu->arch.xcr0 | + best->ecx | ((u64 )best->edx <<32 ), true);/* * The existing code assumes virtual address is 48-bit or 57-bit in the
Signature · 1 line
2.17.1
Re: [PATCH v1 7/8] kvm:cpuid Fix xsaves area size calculation for CPUID.(EAX=0xD,ECX=1).
Quoted 15 lines · Yang Weijiang
According to latest Software Development Manual vol.2/3.2, for CPUID.(EAX=0xD,ECX=1), it should report xsaves area size containing all states enabled by XCR0|IA32_MSR_XSS.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/cpuid.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)arch/x86/kvm/cpuid.c +0
-0
arch/x86/kvm/cpuid.c int kvm_update_cpuid(struct kvm_vcpu *vcpu) · line 125
Quoted 6 lines
arch/x86/kvm/cpuid.c +2
-1@@ -121,7 +121,8 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) best =kvm_find_cpuid_entry (vcpu,0xD ,1 );if (best && (best->eax & (F (XSAVES ) |F (XSAVEC )))) - best->ebx =xstate_required_size (vcpu->arch.xcr0, true); + best->ebx =xstate_required_size (vcpu->arch.xcr0 | + best->ecx | ((u64 )best->edx <<32 ), true);
Passing the CPUID bits is wrong, it needs to be the guest's IA32_MSR_XSS. The CPUID bits enumerate what is legal to enable, not what features are actually enabled by software.
Quoted 6 lines
arch/x86/kvm/cpuid.c +0
-0/* * The existing code assumes virtual address is 48-bit or 57-bit in the Signature · 1 line
2.17.1
PATCH 8/8 kvm:cpuid Report CET SHSTK and IBT support in CPUID.(EAX=0x7,ECX=0).
In reply to Yang WeijiangGuest OS can query CET SHSTK and IBT support by CPUID.(EAX=0x7,ECX=0), in return, ECX[bit 7] corresponds to SHSTK feature, and EDX[bit 20] corresponds to IBT feature.
--- arch/x86/kvm/cpuid.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
arch/x86/kvm/cpuid.c +4 -2
@@ -67,6 +67,8 @@ u64 kvm_supported_xcr0(void)#define F (x) bit(X86_FEATURE_##x) +#define CET_IBT_BIT bit(20) +/* For scattered features from cpufeatures.h; we currently expose none */ #define KF (x) bit(KVM_CPUID_BIT_##x)
@@ -407,12 +409,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,F (AVX512VBMI ) |F (LA57 ) |F (PKU ) |0 /*OSPKE*/ |F (AVX512_VPOPCNTDQ ) |F (UMIP ) |F (AVX512_VBMI2 ) |F (GFNI ) |F (VAES ) |F (VPCLMULQDQ ) |F (AVX512_VNNI ) |F (AVX512_BITALG ) | -F (CLDEMOTE ); +F (CLDEMOTE ) |F (SHSTK );/* 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 (SPEC_CTRL_SSBD ) |F (ARCH_CAPABILITIES ) |CET_IBT_BIT ;/* all calls to cpuid_count() should be made on the same cpu */ get_cpu ();
Signature · 1 line
2.17.1
Re: [PATCH v1 8/8] kvm:cpuid Report CET SHSTK and IBT support in CPUID.(EAX=0x7,ECX=0).
Quoted 15 lines · Yang Weijiang
Guest OS can query CET SHSTK and IBT support by CPUID.(EAX=0x7,ECX=0), in return, ECX[bit 7] corresponds to SHSTK feature, and EDX[bit 20] corresponds to IBT feature.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/cpuid.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/x86/kvm/cpuid.c +0
-0
arch/x86/kvm/cpuid.c u64 kvm_supported_xcr0(void) · line 70
Quoted 4 lines
arch/x86/kvm/cpuid.c +1
-0@@ -67,6 +67,8 @@ u64 kvm_supported_xcr0(void)#define F (x) bit(X86_FEATURE_##x) +#define CET_IBT_BIT bit(20)
Properly define X86_FEATURE_CET_IBT if it's not being defined by some other series, even if KVM is the only immediate user of the bit. Linux reserves all of word 18 for CPUID_7_0_EDX, i.e. defining the bit won't bloat some random scattered word.
Quoted 22 lines
arch/x86/kvm/cpuid.c +3
-2+/* For scattered features from cpufeatures.h; we currently expose none */ #define KF (x) bit(KVM_CPUID_BIT_##x)@@ -407,12 +409,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,F (AVX512VBMI ) |F (LA57 ) |F (PKU ) |0 /*OSPKE*/ |F (AVX512_VPOPCNTDQ ) |F (UMIP ) |F (AVX512_VBMI2 ) |F (GFNI ) |F (VAES ) |F (VPCLMULQDQ ) |F (AVX512_VNNI ) |F (AVX512_BITALG ) | -F (CLDEMOTE ); +F (CLDEMOTE ) |F (SHSTK );/* 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 (SPEC_CTRL_SSBD ) |F (ARCH_CAPABILITIES ) |CET_IBT_BIT ;/* all calls to cpuid_count() should be made on the same cpu */ get_cpu ();Signature · 1 line
2.17.1
Paolo Bonzini
maintainerRe: [PATCH v1 8/8] kvm:cpuid Report CET SHSTK and IBT support in CPUID.(EAX=0x7,ECX=0).
In reply to Yang WeijiangQuoted 29 lines · Yang Weijiang
Guest OS can query CET SHSTK and IBT support by CPUID.(EAX=0x7,ECX=0), in return, ECX[bit 7] corresponds to SHSTK feature, and EDX[bit 20] corresponds to IBT feature.
Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/cpuid.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/x86/kvm/cpuid.c +2
-1@@ -67,6 +67,8 @@ u64 kvm_supported_xcr0(void)#define F (x) bit(X86_FEATURE_##x) +#define CET_IBT_BIT bit(20) +/* For scattered features from cpufeatures.h; we currently expose none */ #define KF (x) bit(KVM_CPUID_BIT_##x)@@ -407,12 +409,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,F (AVX512VBMI ) |F (LA57 ) |F (PKU ) |0 /*OSPKE*/ |F (AVX512_VPOPCNTDQ ) |F (UMIP ) |F (AVX512_VBMI2 ) |F (GFNI ) | F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | -F (CLDEMOTE );
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 420
Quoted 10 lines
arch/x86/kvm/cpuid.c +2
-1+F (CLDEMOTE ) |F (SHSTK );/* 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 (SPEC_CTRL_SSBD ) |F (ARCH_CAPABILITIES ) |CET_IBT_BIT ;/* all calls to cpuid_count() should be made on the same cpu */ get_cpu ();
This might change soon, but at least for now KVM's XSAVES/XRSTORS usage depends on Linux's. Therefore, this should only be enabled if Linux has set bits 11 and 12 in MSR_IA32_XSS.
Paolo