Introduce support for Guest CET feature
v6 cover Introduce support for Guest CET feature
Control-flow Enforcement Technology (CET) provides protection against Return/Jump-Oriented Programming (ROP/JOP) attack. It includes two
sub-features: Shadow Stack (SHSTK) and Indirect Branch Tracking (IBT).
KVM modification is required to support Guest CET feature. This patch serial implemented CET related CPUID/XSAVES enumeration, MSRs and VMEntry configuration etc.so that Guest kernel can setup CET runtime infrastructure based on them. Some MSRs and related feature flags used in the patches reference the definitions in kernel patch.
CET kernel patches is here:
https://lkml.org/lkml/2019/6/6/1003
https://lkml.org/lkml/2019/6/6/1030
v5 -> v6:
- Rebase patch to kernel v5.2.
- Move CPUID(0xD, n>=1) helper to a seperate patch.
- Merge xsave size fix with other patch.
- Other minor fixes per community feedback.
v4 -> v5:
- Rebase patch to kernel v5.1.
- Wrap CPUID(0xD, n>=1) code to a helper function.
- Pass through MSR_IA32_PL1_SSP and MSR_IA32_PL2_SSP to Guest.
- Add Co-developed-by expression in patch description.
- Refine patch description.
v3 -> v4:
- Add Sean's patch for loading Guest fpu state before access XSAVES managed CET MSRs.
- Melt down CET bits setting into CPUID configuration patch.
- Add VMX interface to query Host XSS.
- Check Host and Guest XSS support bits before set Guest XSS.
- Make Guest SHSTK and IBT feature enabling independent.
- Do not report CET support to Guest when Host CET feature is Disabled.
v2 -> v3:
- Modified patches to make Guest CET independent to Host enabling.
- Added patch 8 to add user space access for Guest CET MSR access.
- Modified code comments and patch description to reflect changes.
v1 -> v2:
- Re-ordered patch sequence, combined one patch.
- Added more description for CET related VMCS fields.
- Added Host CET capability check while enabling Guest CET loading bit.
- Added Host CET capability check while reporting Guest CPUID(EAX=7, EXC=0).
- Modified code in reporting Guest CPUID(EAX=D,ECX>=1), make it clearer.
- Added Host and Guest XSS mask check while setting bits for Guest XSS.
PATCH 1 : Define CET VMCS fields and bits.
PATCH 2 : Add a helper function for CPUID(0xD, n>=1) enumeration.
PATCH 3 : Enumerate CET features/XSAVES in CPUID.
PATCH 4 : Pass through CET MSRs to Guest.
PATCH 5 : Load Guest CET states via VMCS.
PATCH 6 : Add CET bits setting in CR4 and IA32_XSS.
PATCH 7 : Load Guest FPU states for XSAVES managed MSRs.
PATCH 8 : Add user-space access interface for CET states.
Sean Christopherson (1):
KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
Yang Weijiang (7):
KVM: VMX: Define CET VMCS fields and control bits
KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration
KVM: x86: Implement CET CPUID enumeration for Guest
KVM: VMX: Pass through CET related MSRs to Guest
KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest
KVM: x86: Add CET bits setting in CR4 and XSS
KVM: x86: Add user-space access interface for CET MSRs
arch/x86/include/asm/kvm_host.h | 5 +- arch/x86/include/asm/vmx.h | 8 +++ arch/x86/kvm/cpuid.c | 107 +++++++++++++++++++++----------- arch/x86/kvm/vmx/vmx.c | 83 +++++++++++++++++++++++-- arch/x86/kvm/x86.c | 29 ++++++++- arch/x86/kvm/x86.h | 4 ++ 6 files changed, 193 insertions(+), 43 deletions(-)
Signature · 1 line
2.17.2
PATCH v6 1/8 KVM: VMX: Define CET VMCS fields and control bits
CET(Control-flow Enforcement Technology) is an upcoming Intel(R) processor feature that blocks Return/Jump-Oriented Programming(ROP) attacks. It provides the following capabilities to defend against ROP/JOP style control-flow subversion attacks:
Shadow Stack (SHSTK):
A second stack for program which is used exclusively for control transfer operations.
Indirect Branch Tracking (IBT):
Code branching protection to defend against jump/call oriented programming.
Several new CET MSRs are defined in kernel to support CET:
MSR_IA32_{U,S}_CET: Controls the CET settings for user
mode and suervisor mode respectively.
MSR_IA32_PL{0,1,2,3}_SSP: Stores shadow stack pointers for
CPL-0,1,2,3 level respectively.
MSR_IA32_INT_SSP_TAB: Stores base address of shadow stack
pointer table.
Two XSAVES state bits are introduced for CET:
IA32_XSS:[bit 11]: For saving/restoring user mode CET states IA32_XSS:[bit 12]: For saving/restoring supervisor mode CET states.
Six VMCS fields are introduced for CET:
{HOST,GUEST}_S_CET: Stores CET settings for supervisor mode. {HOST,GUEST}_SSP: Stores shadow stack pointer for supervisor mode. {HOST,GUEST}_INTR_SSP_TABLE: Stores base address of shadow stack pointer
table.
If VM_EXIT_LOAD_HOST_CET_STATE = 1, the host's CET MSRs are restored from below VMCS fields at VM-Exit:
HOST_S_CET
HOST_SSP
HOST_INTR_SSP_TABLE
If VM_ENTRY_LOAD_GUEST_CET_STATE = 1, the guest's CET MSRs are loaded from below VMCS fields at VM-Entry:
GUEST_S_CET
GUEST_SSP
GUEST_INTR_SSP_TABLE
--- arch/x86/include/asm/vmx.h | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/x86/include/asm/vmx.h +8 -0
@@ -90,6 +90,7 @@#define VM_EXIT_CLEAR_BNDCFGS 0x00800000#define VM_EXIT_PT_CONCEAL_PIP 0x01000000#define VM_EXIT_CLEAR_IA32_RTIT_CTL 0x02000000 +#define VM_EXIT_LOAD_HOST_CET_STATE 0x10000000#define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR 0x00036dff
@@ -103,6 +104,7 @@#define VM_ENTRY_LOAD_BNDCFGS 0x00010000#define VM_ENTRY_PT_CONCEAL_PIP 0x00020000#define VM_ENTRY_LOAD_IA32_RTIT_CTL 0x00040000 +#define VM_ENTRY_LOAD_GUEST_CET_STATE 0x00100000#define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR 0x000011ff
@@ -321,6 +323,9 @@ enum vmcs_field {GUEST_PENDING_DBG_EXCEPTIONS =0x00006822 ,GUEST_SYSENTER_ESP =0x00006824 ,GUEST_SYSENTER_EIP =0x00006826 , +GUEST_S_CET =0x00006828 , +GUEST_SSP =0x0000682a , +GUEST_INTR_SSP_TABLE =0x0000682c ,HOST_CR0 =0x00006c00 ,HOST_CR3 =0x00006c02 , HOST_CR4 = 0x00006c04,
@@ -333,6 +338,9 @@ enum vmcs_field {HOST_IA32_SYSENTER_EIP =0x00006c12 ,HOST_RSP =0x00006c14 ,HOST_RIP =0x00006c16 , +HOST_S_CET =0x00006c18 , +HOST_SSP =0x00006c1a , + HOST_INTR_SSP_TABLE = 0x00006c1c };/*
Signature · 1 line
2.17.2
PATCH v6 2/8 KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration
In reply to Yang WeijiangTo make the code look clean, wrap CPUID(0xD,n>=1) enumeration code in a helper function now.
--- arch/x86/kvm/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
arch/x86/kvm/cpuid.c +44 -0
@@ -313,6 +313,50 @@ static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,return 0 ; } +static inline int __do_cpuid_dx_leaf (struct kvm_cpuid_entry2 *entry,int *nent, +int maxnent,u64 xss_mask,u64 xcr0_mask, +u32 eax_mask) +{ +int idx, i; +u64 mask; +u64 supported; + +for (idx =1 , i =1 ; idx <64 ; ++idx) { + mask = ((u64 )1 << idx); +if (*nent >= maxnent) +return -EINVAL ; + +do_cpuid_1_ent (&entry[i],0xD , idx); +if (idx ==1 ) { + entry[i].eax &= eax_mask; +cpuid_mask (&entry[i].eax,CPUID_D_1_EAX ); + supported = xcr0_mask | xss_mask; + entry[i].ebx =0 ; + entry[i].edx =0 ; + entry[i].ecx &= xss_mask; +if (entry[i].eax & (F (XSAVES ) |F (XSAVEC ))) { + entry[i].ebx = +xstate_required_size (supported, + true); + } + }else { + supported = (entry[i].ecx &1 ) ? xss_mask : + xcr0_mask; +if (entry[i].eax ==0 || !(supported & mask)) +continue ; + entry[i].ecx &=1 ; + entry[i].edx =0 ; +if (entry[i].ecx) + entry[i].ebx =0 ; + } + entry[i].flags |= +KVM_CPUID_FLAG_SIGNIFCANT_INDEX ; + ++*nent; + ++i; + } +return 0 ; +} +static inline int __do_cpuid_ent (struct kvm_cpuid_entry2 *entry,u32 function,u32 index,int *nent,int maxnent) {
Signature · 1 line
2.17.2
Re: [PATCH v6 2/8] KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration
Quoted 13 lines · Yang Weijiang
To make the code look clean, wrap
CPUID(0xD,n>=1)enumeration code in a helper function now.Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)arch/x86/kvm/cpuid.c +0
-0
arch/x86/kvm/cpuid.c static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry, · line 316
Quoted 4 lines
arch/x86/kvm/cpuid.c +1
-0@@ -313,6 +313,50 @@ static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,return 0 ; } +static inline int __do_cpuid_dx_leaf(struct kvm_cpuid_entry2 *entry, int *nent,
'dx' makes me think of the generic reference to RDX vs EDX. Maybe __do_cpuid_0xd_leaf()?
arch/x86/kvm/cpuid.c static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry, · line 324
Quoted 8 lines
arch/x86/kvm/cpuid.c +8
-0+ int maxnent, u64 xss_mask, u64 xcr0_mask, + u32 eax_mask) +{ + int idx, i; +u64 mask; +u64 supported; + +for (idx =1 , i =1 ; idx <64 ; ++idx) {
Rather than loop here, I think it makes sense to loop in the caller to make the code consistent with do_cpuid_7_mask() added by commit
54d360d41211 ("KVM: cpuid: extract do_cpuid_7_mask and support multiple subleafs")
Quoted 25 lines
arch/x86/kvm/cpuid.c +25
-0+ mask = ((u64 )1 << idx); +if (*nent >= maxnent) +return -EINVAL ; + +do_cpuid_1_ent (&entry[i],0xD , idx); +if (idx ==1 ) { + entry[i].eax &= eax_mask; +cpuid_mask (&entry[i].eax,CPUID_D_1_EAX ); + supported = xcr0_mask | xss_mask; + entry[i].ebx =0 ; + entry[i].edx =0 ; + entry[i].ecx &= xss_mask; +if (entry[i].eax & (F (XSAVES ) |F (XSAVEC ))) { + entry[i].ebx = +xstate_required_size (supported, + true); + } + }else { + supported = (entry[i].ecx &1 ) ? xss_mask : + xcr0_mask; +if (entry[i].eax ==0 || !(supported & mask)) +continue ; + entry[i].ecx &=1 ; + entry[i].edx =0 ; +if (entry[i].ecx)
arch/x86/kvm/cpuid.c static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry, · line 359
Quoted 10 lines
arch/x86/kvm/cpuid.c +10
-0+ entry[i].ebx =0 ; + } + entry[i].flags |= +KVM_CPUID_FLAG_SIGNIFCANT_INDEX ; + ++*nent; + ++i; + } +return 0 ; +} +
Extracting code into a helper should be done in the same patch that the existing code is replaced with a call to the helper, i.e. the chunk of the next patch that invokes the helper should be squashed with this patch.
Quoted 6 lines
arch/x86/kvm/cpuid.c +0
-0static inline int __do_cpuid_ent (struct kvm_cpuid_entry2 *entry,u32 function,u32 index,int *nent,int maxnent) {Signature · 1 line
2.17.2
Re: [PATCH v6 2/8] KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration
Quoted 17 lines · Sean Christopherson
Quoted 16 lines · Yang Weijiang
To make the code look clean, wrap
CPUID(0xD,n>=1)enumeration code in a helper function now.Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)arch/x86/kvm/cpuid.c +0
-0@@ -313,6 +313,50 @@ static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,return 0 ; }
Quoted 5 lines
arch/x86/kvm/cpuid.c static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry, · line 316
Quoted 1 line
arch/x86/kvm/cpuid.c +1
-0+static inline int __do_cpuid_dx_leaf(struct kvm_cpuid_entry2 *entry, int *nent,'dx' makes me think of the generic reference to RDX vs EDX. Maybe
__do_cpuid_0xd_leaf()?
OK, will change it.
Quoted 12 lines
arch/x86/kvm/cpuid.c static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry, · line 324
Quoted 8 lines
arch/x86/kvm/cpuid.c +8
-0+ int maxnent, u64 xss_mask, u64 xcr0_mask, + u32 eax_mask) +{ + int idx, i; +u64 mask; +u64 supported; + +for (idx =1 , i =1 ; idx <64 ; ++idx) {Rather than loop here, I think it makes sense to loop in the caller to make the code consistent with
do_cpuid_7_mask()added by commit
Quoted 2 lines
54d360d41211("KVM: cpuid: extractdo_cpuid_7_maskand support multiple subleafs")
OK, will follow the patch to modify the helper.
Quoted 31 lines
Quoted 31 lines
arch/x86/kvm/cpuid.c +31
-0+ mask = ((u64 )1 << idx); +if (*nent >= maxnent) +return -EINVAL ; + +do_cpuid_1_ent (&entry[i],0xD , idx); +if (idx ==1 ) { + entry[i].eax &= eax_mask; +cpuid_mask (&entry[i].eax,CPUID_D_1_EAX ); + supported = xcr0_mask | xss_mask; + entry[i].ebx =0 ; + entry[i].edx =0 ; + entry[i].ecx &= xss_mask; +if (entry[i].eax & (F (XSAVES ) |F (XSAVEC ))) { + entry[i].ebx = +xstate_required_size (supported, + true); + } + }else { + supported = (entry[i].ecx &1 ) ? xss_mask : + xcr0_mask; +if (entry[i].eax ==0 || !(supported & mask)) +continue ; + entry[i].ecx &=1 ; + entry[i].edx =0 ; +if (entry[i].ecx) + entry[i].ebx =0 ; + } + entry[i].flags |= +KVM_CPUID_FLAG_SIGNIFCANT_INDEX ; + ++*nent; + ++i;
Quoted 9 lines
arch/x86/kvm/cpuid.c static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry, · line 359
Quoted 4 lines
arch/x86/kvm/cpuid.c +4
-0+ } +return 0 ; +} +Extracting code into a helper should be done in the same patch that the existing code is replaced with a call to the helper, i.e. the chunk of the next patch that invokes the helper should be squashed with this patch.
OK, will squash this patch in next release.
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/cpuid.c +0
-0static inline int __do_cpuid_ent (struct kvm_cpuid_entry2 *entry,u32 function,u32 index,int *nent,int maxnent) {Signature · 1 line
2.17.2
PATCH v6 3/8 KVM: x86: Implement CET CPUID enumeration for Guest
In reply to Yang WeijiangCPUID.(EAX=7, ECX=0):ECX[bit 7] and EDX[bit 20] correspond to CET SHSTK and IBT respectively.
CET xsave components for supervisor and user mode are reported via CPUID.(EAX=0xD, ECX=1):ECX[bit 11] and ECX[bit 12] respectively.
--- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/cpuid.c | 52 ++++++++++++--------------------- arch/x86/kvm/vmx/vmx.c | 6 ++++ arch/x86/kvm/x86.h | 4 +++ 4 files changed, 29 insertions(+), 34 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
@@ -1199,6 +1199,7 @@ struct kvm_x86_ops {uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu);bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu); +u64 (*supported_xss)(void); };struct kvm_arch_async_pf {
arch/x86/kvm/cpuid.c +18 -34
81 lines not on this page · raw message
arch/x86/kvm/vmx/vmx.c +6 -0
@@ -1637,6 +1637,11 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,return !(val & ~valid_bits); } +static inline u64 vmx_supported_xss (void ) +{ +return host_xss; +} +static int vmx_get_msr_feature (struct kvm_msr_entry *msr) {switch (msr->index) {
@@ -7724,6 +7729,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .get_vmcs12_pages = NULL, .nested_enable_evmcs = NULL, .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, + .supported_xss = vmx_supported_xss, };static void vmx_cleanup_l1d_flush (void )
arch/x86/kvm/x86.h +4 -0
@@ -288,6 +288,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, unsigned long cr2, | XFEATURE_MASK_YMM | XFEATURE_MASK_BNDREGS \ | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \ | XFEATURE_MASK_PKRU) + +#define KVM_SUPPORTED_XSS (XFEATURE_MASK_CET_USER \ + | XFEATURE_MASK_CET_KERNEL) +extern u64 host_xcr0;extern u64 kvm_supported_xcr0 (void );
Signature · 1 line
2.17.2
Re: [PATCH v6 3/8] KVM: x86: Implement CET CPUID enumeration for Guest
arch/x86/kvm/vmx/vmx.c static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, · line 1642
Quoted 11 lines · Yang Weijiang
arch/x86/kvm/vmx/vmx.c +3
-0@@ -1637,6 +1637,11 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,return !(val & ~valid_bits); } +static inline u64 vmx_supported_xss (void ) +{ +return host_xss;
Do you know if the kernel will ever enable CET_USER but not CET_KERNEL, and vice versa? I tried hunting down the logic in the main CET enabling series but couldn't find the relevant code.
If the kernel does enable USER vs. KERNEL independently, are we sure that KVM can correctly virtualize that state and that the guest OS won't die due to expecting all CET features or no CET features?
In other words, do we want to return host_xss as is, or do we want to make CET_USER and CET_KERNEL a bundle deal and avoid the headache, e.g.:
if (!(host_xss & XFEATURE_MASK_CET_USER) || !(host_xss & XFEATURE_MASK_CET_KERNEL)) return host_xss & ~(XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL); return host_xss;
Quoted 31 lines
arch/x86/kvm/vmx/vmx.c +3
-0+} +static int vmx_get_msr_feature (struct kvm_msr_entry *msr) {switch (msr->index) {@@ -7724,6 +7729,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .get_vmcs12_pages = NULL, .nested_enable_evmcs = NULL, .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, + .supported_xss = vmx_supported_xss, };static void vmx_cleanup_l1d_flush (void )arch/x86/kvm/x86.h +4
-0@@ -288,6 +288,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, unsigned long cr2, | XFEATURE_MASK_YMM | XFEATURE_MASK_BNDREGS \ | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \ | XFEATURE_MASK_PKRU) + +#define KVM_SUPPORTED_XSS (XFEATURE_MASK_CET_USER \ + | XFEATURE_MASK_CET_KERNEL) +extern u64 host_xcr0;extern u64 kvm_supported_xcr0 (void );Signature · 1 line
2.17.2
Re: [PATCH v6 3/8] KVM: x86: Implement CET CPUID enumeration for Guest
Quoted 21 lines · Sean Christopherson
arch/x86/kvm/vmx/vmx.c static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, · line 1642
Quoted 11 lines · Yang Weijiang
arch/x86/kvm/vmx/vmx.c +3
-0@@ -1637,6 +1637,11 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,return !(val & ~valid_bits); } +static inline u64 vmx_supported_xss (void ) +{ +return host_xss;Do you know if the kernel will ever enable
CET_USERbut notCET_KERNEL, and vice versa? I tried hunting down the logic in the main CET enabling series but couldn't find the relevant code.If the kernel does enable USER vs. KERNEL independently, are we sure that KVM can correctly virtualize that state and that the guest OS won't die due to expecting all CET features or no CET features?
Quoted 9 lines
In other words, do we want to return
host_xssas is, or do we want to makeCET_USERandCET_KERNELa bundle deal and avoid the headache, e.g.:if (!(host_xss & XFEATURE_MASK_CET_USER) || !(host_xss & XFEATURE_MASK_CET_KERNEL)) return host_xss & ~(XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL); return host_xss;
Hi, Sean,
Thanks for review! CET_USER and CET_KERNEL are two independent parts of CET, but CET_KERNEL part has not been fully implemented yet, the final target is to enable CET_USER + CET_KERNEL in kernel. In the VMM patch, it's supposed to enable both CET_USER and CET_KERNEL mode at one time, so the patches expose all the features of CET to guest OS as long as platform and host kernel support so.
Quoted 31 lines
Quoted 31 lines
arch/x86/kvm/vmx/vmx.c +3
-0+} +static int vmx_get_msr_feature (struct kvm_msr_entry *msr) {switch (msr->index) {@@ -7724,6 +7729,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .get_vmcs12_pages = NULL, .nested_enable_evmcs = NULL, .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, + .supported_xss = vmx_supported_xss, };static void vmx_cleanup_l1d_flush (void )arch/x86/kvm/x86.h +4
-0@@ -288,6 +288,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, unsigned long cr2, | XFEATURE_MASK_YMM | XFEATURE_MASK_BNDREGS \ | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \ | XFEATURE_MASK_PKRU) + +#define KVM_SUPPORTED_XSS (XFEATURE_MASK_CET_USER \ + | XFEATURE_MASK_CET_KERNEL) +extern u64 host_xcr0;extern u64 kvm_supported_xcr0 (void );Signature · 1 line
2.17.2
PATCH v6 4/8 KVM: VMX: Pass through CET related MSRs to Guest
In reply to Yang WeijiangCET MSRs pass through Guest directly to enhance performance. CET runtime control settings are stored in MSR_IA32_{U,S}_CET, Shadow Stack Pointer(SSP) are stored in MSR_IA32_PL{0,1,2,3}_SSP, SSP table base address is stored in MSR_IA32_INT_SSP_TAB, these MSRs are defined in kernel and re-used here.
MSR_IA32_U_CET and MSR_IA32_PL3_SSP are used for user mode protection, the contents could differ from process to process, therefore, kernel needs to save/restore them during context switch, it makes sense to pass through them so that the guest kernel can
use xsaves/xrstors to operate them efficiently. Other MSRs are used for non-user mode protection. See CET spec for detailed info.
The difference between CET VMCS state fields and xsave components is that, the former used for CET state storage during VMEnter/VMExit, whereas the latter used for state retention between Guest task/process switch.
--- arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
arch/x86/kvm/vmx/vmx.c +14 -0
@@ -6952,6 +6952,7 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)static void vmx_cpuid_update (struct kvm_vcpu *vcpu) {struct vcpu_vmx *vmx =to_vmx (vcpu); +unsigned long *msr_bitmap;if (cpu_has_secondary_exec_ctrls ()) {vmx_compute_secondary_exec_control (vmx);
@@ -6973,6 +6974,19 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)if (boot_cpu_has (X86_FEATURE_INTEL_PT ) &&guest_cpuid_has (vcpu,X86_FEATURE_INTEL_PT ))update_intel_pt_cfg (vcpu); + + msr_bitmap = vmx->vmcs01.msr_bitmap; + +if (guest_cpuid_has (vcpu,X86_FEATURE_SHSTK ) || +guest_cpuid_has (vcpu,X86_FEATURE_IBT )) { +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_PL1_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL2_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL3_SSP ,MSR_TYPE_RW ); + } }static void vmx_set_supported_cpuid (u32 func,struct kvm_cpuid_entry2 *entry)
Signature · 1 line
2.17.2
Re: [PATCH v6 4/8] KVM: VMX: Pass through CET related MSRs to Guest
Quoted 38 lines · Yang Weijiang
CET MSRs pass through Guest directly to enhance performance. CET runtime control settings are stored in MSR_IA32_{U,S}_CET, Shadow Stack Pointer(SSP) are stored in MSR_IA32_PL{0,1,2,3}_SSP, SSP table base address is stored in
MSR_IA32_INT_SSP_TAB, these MSRs are defined in kernel and re-used here.
MSR_IA32_U_CETandMSR_IA32_PL3_SSPare used for user mode protection, the contents could differ from process to process, therefore, kernel needs to save/restore them during context switch, it makes sense to pass through them so that the guest kernel can
use xsaves/xrstors to operate them efficiently. Other MSRs are used for non-user mode protection. See CET spec for detailed info.The difference between CET VMCS state fields and xsave components is that, the former used for CET state storage during VMEnter/VMExit, whereas the latter used for state retention between Guest task/process switch.
Co-developed-by Zhang Yi Z <[email protected]>Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)arch/x86/kvm/vmx/vmx.c +1
-0@@ -6952,6 +6952,7 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)static void vmx_cpuid_update (struct kvm_vcpu *vcpu) {struct vcpu_vmx *vmx =to_vmx (vcpu); +unsigned long *msr_bitmap;if (cpu_has_secondary_exec_ctrls ()) {vmx_compute_secondary_exec_control (vmx);
arch/x86/kvm/vmx/vmx.c static void vmx_cpuid_update(struct kvm_vcpu *vcpu) · line 6981
Quoted 8 lines
arch/x86/kvm/vmx/vmx.c +5
-0@@ -6973,6 +6974,19 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)if (boot_cpu_has (X86_FEATURE_INTEL_PT ) &&guest_cpuid_has (vcpu,X86_FEATURE_INTEL_PT ))update_intel_pt_cfg (vcpu); + + msr_bitmap = vmx->vmcs01.msr_bitmap; + +if (guest_cpuid_has (vcpu,X86_FEATURE_SHSTK ) || +guest_cpuid_has (vcpu,X86_FEATURE_IBT )) {
These should be exposed to the guest if and only if they're supported in the host and guest, i.e. kvm_supported_xss() needs to be checked. And, again assuming USER and KERNEL can be virtualized independently, the logic needs to account for exposting USER but KERNEL and vice versa.
arch/x86/kvm/vmx/vmx.c static void vmx_cpuid_update(struct kvm_vcpu *vcpu) · line 6988
Quoted 7 lines
arch/x86/kvm/vmx/vmx.c +7
-0+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_PL1_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL2_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL3_SSP ,MSR_TYPE_RW );
The SSP MSRs should only be passed through if the guest has SHSTK, e.g. KVM should intercept RDMSR and WRMSR to inject #GP in those cases.
Quoted 7 lines
arch/x86/kvm/vmx/vmx.c +1
-0+ } }static void vmx_set_supported_cpuid (u32 func,struct kvm_cpuid_entry2 *entry)Signature · 1 line
2.17.2
Re: [PATCH v6 4/8] KVM: VMX: Pass through CET related MSRs to Guest
Quoted 36 lines · Sean Christopherson
Quoted 35 lines · Yang Weijiang
CET MSRs pass through Guest directly to enhance performance. CET runtime control settings are stored in MSR_IA32_{U,S}_CET, Shadow Stack Pointer(SSP) are stored in MSR_IA32_PL{0,1,2,3}_SSP, SSP table base address is stored in
MSR_IA32_INT_SSP_TAB, these MSRs are defined in kernel and re-used here.
MSR_IA32_U_CETandMSR_IA32_PL3_SSPare used for user mode protection, the contents could differ from process to process, therefore, kernel needs to save/restore them during context switch, it makes sense to pass through them so that the guest kernel can
use xsaves/xrstors to operate them efficiently. Other MSRs are used for non-user mode protection. See CET spec for detailed info.The difference between CET VMCS state fields and xsave components is that, the former used for CET state storage during VMEnter/VMExit, whereas the latter used for state retention between Guest task/process switch.
Co-developed-by Zhang Yi Z <[email protected]>Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)arch/x86/kvm/vmx/vmx.c +1
-0@@ -6952,6 +6952,7 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)static void vmx_cpuid_update (struct kvm_vcpu *vcpu) {struct vcpu_vmx *vmx =to_vmx (vcpu); +unsigned long *msr_bitmap;
Quoted 17 lines
arch/x86/kvm/vmx/vmx.c static void vmx_cpuid_update(struct kvm_vcpu *vcpu) · line 6981
Quoted 11 lines
arch/x86/kvm/vmx/vmx.c +5
-0if (cpu_has_secondary_exec_ctrls ()) {vmx_compute_secondary_exec_control (vmx);@@ -6973,6 +6974,19 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)if (boot_cpu_has (X86_FEATURE_INTEL_PT ) &&guest_cpuid_has (vcpu,X86_FEATURE_INTEL_PT ))update_intel_pt_cfg (vcpu); + + msr_bitmap = vmx->vmcs01.msr_bitmap; + +if (guest_cpuid_has (vcpu,X86_FEATURE_SHSTK ) || +guest_cpuid_has (vcpu,X86_FEATURE_IBT )) {These should be exposed to the guest if and only if they're supported in the host and guest, i.e.
kvm_supported_xss()needs to be checked. And, again assuming USER and KERNEL can be virtualized independently, the logic needs to account for exposting USER but KERNEL and vice versa.
this patch serial is supposed to enable both USER and KERNEL mode CET as long as platform and host kernel support so. I'll add condition check before pass through correspond MSR to guest OS.
Quoted 18 lines
arch/x86/kvm/vmx/vmx.c static void vmx_cpuid_update(struct kvm_vcpu *vcpu) · line 6988
Quoted 7 lines
arch/x86/kvm/vmx/vmx.c +7
-0+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_PL1_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL2_SSP ,MSR_TYPE_RW ); +vmx_disable_intercept_for_msr (msr_bitmap,MSR_IA32_PL3_SSP ,MSR_TYPE_RW );The SSP MSRs should only be passed through if the guest has SHSTK, e.g. KVM should intercept RDMSR and WRMSR to inject #GP in those cases.
Quoted 7 lines
arch/x86/kvm/vmx/vmx.c +1
-0+ } }static void vmx_set_supported_cpuid (u32 func,struct kvm_cpuid_entry2 *entry)Signature · 1 line
2.17.2
PATCH v6 5/8 KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest
In reply to Yang Weijiang"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 enabled on host and 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.
--- arch/x86/kvm/vmx/vmx.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
arch/x86/kvm/vmx/vmx.c +13 -0
@@ -44,6 +44,7 @@#include <asm/spec-ctrl.h> #include <asm/virtext.h> #include <asm/vmx.h> +#include <asm/cet.h> #include "capabilities.h" #include "cpuid.h"
@@ -2923,6 +2924,18 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)if (!nested_vmx_allowed (vcpu) ||is_smm (vcpu))return 1 ; } +if (cpu_x86_cet_enabled () && + (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 ;
Signature · 1 line
2.17.2
Re: [PATCH v6 5/8] KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest
Quoted 32 lines · Yang Weijiang
"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 enabled on host and 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.Co-developed-by Zhang Yi Z <[email protected]>Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx/vmx.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)arch/x86/kvm/vmx/vmx.c +1
-0@@ -44,6 +44,7 @@#include <asm/spec-ctrl.h> #include <asm/virtext.h> #include <asm/vmx.h> +#include <asm/cet.h> #include "capabilities.h" #include "cpuid.h"
arch/x86/kvm/vmx/vmx.c int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) · line 2927
Quoted 4 lines
arch/x86/kvm/vmx/vmx.c +1
-0@@ -2923,6 +2924,18 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)if (!nested_vmx_allowed (vcpu) ||is_smm (vcpu))return 1 ; } + if (cpu_x86_cet_enabled() &&
It'd probably be better to check a KVM function here, e.g. a wrapper of kvm_supported_xss(). I don't think it will ever matter, but it'd be nice to have a single kill switch given the variety of different enable bits for CET.
Quoted 17 lines
arch/x86/kvm/vmx/vmx.c +11
-0+ (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 ;Signature · 1 line
2.17.2
Re: [PATCH v6 5/8] KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest
Quoted 30 lines · Sean Christopherson
Quoted 29 lines · Yang Weijiang
"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 enabled on host and 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.Co-developed-by Zhang Yi Z <[email protected]>Signed-off-by Zhang Yi Z <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx/vmx.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)arch/x86/kvm/vmx/vmx.c +1
-0@@ -44,6 +44,7 @@#include <asm/spec-ctrl.h> #include <asm/virtext.h> #include <asm/vmx.h> +#include <asm/cet.h>
Quoted 13 lines
arch/x86/kvm/vmx/vmx.c int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) · line 2927
Quoted 7 lines
arch/x86/kvm/vmx/vmx.c +1
-0#include "capabilities.h" #include "cpuid.h" @@ -2923,6 +2924,18 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)if (!nested_vmx_allowed (vcpu) ||is_smm (vcpu))return 1 ; } + if (cpu_x86_cet_enabled() &&It'd probably be better to check a KVM function here, e.g. a wrapper of
kvm_supported_xss(). I don't think it will ever matter, but it'd be nice to have a single kill switch given the variety of different enable bits for CET.
OK, will try to make it nicer in next version.
Quoted 17 lines
Quoted 17 lines
arch/x86/kvm/vmx/vmx.c +11
-0+ (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 ;Signature · 1 line
2.17.2
PATCH v6 6/8 KVM: x86: Add CET bits setting in CR4 and XSS
In reply to Yang WeijiangCR4.CET(bit 23) is master enable bit for CET feature. Previously, KVM did not support setting any bits in XSS so it's hardcoded to check and inject a #GP if Guest attempted to write a non-zero value to XSS, now it supports CET related bits setting.
--- arch/x86/include/asm/kvm_host.h | 4 +++- arch/x86/kvm/cpuid.c | 11 +++++++++-- arch/x86/kvm/vmx/vmx.c | 7 ++----- 3 files changed, 14 insertions(+), 8 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -1
@@ -88,7 +88,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)
@@ -620,6 +621,7 @@ struct kvm_vcpu_arch {u64 xcr0;u64 guest_supported_xcr0; +u64 guest_supported_xss;u32 guest_xstate_size;struct kvm_pio_request pio;
arch/x86/kvm/cpuid.c +9 -2
@@ -119,8 +119,15 @@ 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); +if (best && (best->eax & (F (XSAVES ) |F (XSAVEC )))) { + best->ebx =xstate_required_size (vcpu->arch.xcr0 | +kvm_supported_xss (), true); + + vcpu->arch.guest_supported_xss = best->ecx & +kvm_supported_xss (); + }else { + vcpu->arch.guest_supported_xss =0 ; + }/* * The existing code assumes virtual address is 48-bit or 57-bit in the
arch/x86/kvm/vmx/vmx.c +2 -5
@@ -1940,12 +1940,9 @@ 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. -*/ -if (data !=0 ) +if (data & ~vcpu->arch.guest_supported_xss)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.2
PATCH v6 7/8 KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
In reply to Yang WeijiangFrom: Sean Christopherson <[email protected]>
A handful of CET MSRs are not context switched through "traditional" methods, e.g. VMCS or manual switching, but rather are passed through to the guest and are saved and restored by XSAVES/XRSTORS, i.e. the guest's FPU state.
Load the guest's FPU state if userspace is accessing MSRs whose values are managed by XSAVES so that the MSR helper, e.g. vmx_{get,set}_msr(), can simply do {RD,WR}MSR to access the guest's value.
Note that guest_cpuid_has() is not queried as host userspace is allowed to access MSRs that have not been exposed to the guest, e.g. it might do KVM_SET_MSRS prior to KVM_SET_CPUID2.
--- arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)
arch/x86/kvm/x86.c +28 -1
@@ -102,6 +102,8 @@ static void enter_smm(struct kvm_vcpu *vcpu);static void __kvm_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags);static void store_regs (struct kvm_vcpu *vcpu);static int sync_regs (struct kvm_vcpu *vcpu); +static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu); +static void kvm_put_guest_fpu (struct kvm_vcpu *vcpu);struct kvm_x86_ops *kvm_x86_ops__read_mostly ;EXPORT_SYMBOL_GPL (kvm_x86_ops);
@@ -2959,6 +2961,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) }EXPORT_SYMBOL_GPL (kvm_get_msr_common); +static bool is_xsaves_msr (u32 index) +{ +return index ==MSR_IA32_U_CET || + (index >=MSR_IA32_PL0_SSP && index <=MSR_IA32_PL3_SSP ); +} +/* * Read or write a bunch of msrs. All parameters are kernel addresses. *
@@ -2969,11 +2977,30 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,int (*do_msr)(struct kvm_vcpu *vcpu, unsigned index, u64 *data)) { +bool fpu_loaded = false;int i; +u64 cet_bits =XFEATURE_MASK_CET_USER |XFEATURE_MASK_CET_KERNEL ; +u64 host_xss =0 ; + +for (i =0 ; i < msrs->nmsrs; ++i) { +if (!fpu_loaded &&is_xsaves_msr (entries[i].index)) { +if (!kvm_x86_ops->xsaves_supported () || + !kvm_x86_ops->supported_xss ()) +continue ; + + host_xss = kvm_x86_ops->supported_xss (); -for (i =0 ; i < msrs->nmsrs; ++i) +if ((host_xss & cet_bits) != cet_bits) +continue ; + +kvm_load_guest_fpu (vcpu); + fpu_loaded = true; + }if (do_msr (vcpu, entries[i].index, &entries[i].data))break ; + } +if (fpu_loaded) +kvm_put_guest_fpu (vcpu);return i; }
Signature · 1 line
2.17.2
Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
Quoted 49 lines · Yang Weijiang
From: Sean Christopherson <[email protected]>
A handful of CET MSRs are not context switched through "traditional" methods, e.g. VMCS or manual switching, but rather are passed through to the guest and are saved and restored by XSAVES/XRSTORS, i.e. the guest's FPU state.
Load the guest's FPU state if userspace is accessing MSRs whose values are managed by XSAVES so that the MSR helper, e.g. vmx_{get,set}_msr(), can simply do {RD,WR}MSR to access the guest's value.
Note that
guest_cpuid_has()is not queried as host userspace is allowed to access MSRs that have not been exposed to the guest, e.g. it might doKVM_SET_MSRSprior toKVM_SET_CPUID2.Signed-off-by Sean Christopherson <[email protected]>Co-developed-by Yang Weijiang <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +8
-0@@ -102,6 +102,8 @@ static void enter_smm(struct kvm_vcpu *vcpu);static void __kvm_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags);static void store_regs (struct kvm_vcpu *vcpu);static int sync_regs (struct kvm_vcpu *vcpu); +static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu); +static void kvm_put_guest_fpu (struct kvm_vcpu *vcpu);struct kvm_x86_ops *kvm_x86_ops__read_mostly ;EXPORT_SYMBOL_GPL (kvm_x86_ops);@@ -2959,6 +2961,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) }EXPORT_SYMBOL_GPL (kvm_get_msr_common); +static bool is_xsaves_msr (u32 index) +{ +return index ==MSR_IA32_U_CET || + (index >=MSR_IA32_PL0_SSP && index <=MSR_IA32_PL3_SSP ); +} +/* * Read or write a bunch of msrs. All parameters are kernel addresses. *
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2982
Quoted 6 lines
arch/x86/kvm/x86.c +2
-0@@ -2969,11 +2977,30 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,int (*do_msr)(struct kvm_vcpu *vcpu, unsigned index, u64 *data)) { +bool fpu_loaded = false;int i; +u64 cet_bits =XFEATURE_MASK_CET_USER |XFEATURE_MASK_CET_KERNEL ;
Dunno if the compiler will actually generate different code, but this can be a const.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2988
Quoted 6 lines
arch/x86/kvm/x86.c +6
-0+u64 host_xss =0 ; + +for (i =0 ; i < msrs->nmsrs; ++i) { +if (!fpu_loaded &&is_xsaves_msr (entries[i].index)) { +if (!kvm_x86_ops->xsaves_supported () || + !kvm_x86_ops->supported_xss ())
The "!kvm_x86_ops->supported_xss()" is redundant with the host_xss check below.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2989
Quoted 1 line
arch/x86/kvm/x86.c +1
-0+continue ;
Hmm, vmx_set_msr() should be checking host_xss, arguably we should call do_msr() and let it handle the bad MSR access. I don't have a strong opinion either way, practically speaking the end result will be the same.
If we do want to handle a misbehaving userspace here, this should be 'break' instead of 'continue'.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2993
Quoted 5 lines
arch/x86/kvm/x86.c +3
-1+ + host_xss = kvm_x86_ops->supported_xss (); -for (i =0 ; i < msrs->nmsrs; ++i) +if ((host_xss & cet_bits) != cet_bits)
I'm pretty sure this should check for either CET bit being set, not both, e.g. I assume it's possible to enable and expose XFEATURE_MASK_CET_USER but not XFEATURE_MASK_CET_KERNEL.
So something like
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL;
const bool cet_supported = kvm_x86_ops->xsaves_supported() &&
(kvm_x86_ops->supported_xss() & cet_bits);
for (i = 0; i < msrs->nmsrs; ++i) {
if (!fpu_loaded && cet_supported &&
is_xsaves_msr(entries[i].index)) {
kvm_load_guest_fpu(vcpu);
fpu_loaded = true;
}
if (do_msr(vcpu, entries[i].index, &entries[i].data))
break;
}or
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL;
for (i = 0; i < msrs->nmsrs; ++i) {
if (!fpu_loaded && is_xsaves_msr(entries[i].index)) {
if (!kvm_x86_ops->supported_xss() ||
!(kvm_x86_ops->supported_xss() & cet_bits))
break;
kvm_load_guest_fpu(vcpu);
fpu_loaded = true;
}
if (do_msr(vcpu, entries[i].index, &entries[i].data))
break;
}Quoted 16 lines
arch/x86/kvm/x86.c +8
-0+continue ; + +kvm_load_guest_fpu (vcpu); + fpu_loaded = true; + }if (do_msr (vcpu, entries[i].index, &entries[i].data))break ; + } +if (fpu_loaded) +kvm_put_guest_fpu (vcpu);return i; }Signature · 1 line
2.17.2
Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
Quoted 89 lines · Sean Christopherson
Quoted 49 lines · Yang Weijiang
From: Sean Christopherson <[email protected]>
A handful of CET MSRs are not context switched through "traditional" methods, e.g. VMCS or manual switching, but rather are passed through to the guest and are saved and restored by XSAVES/XRSTORS, i.e. the guest's FPU state.
Load the guest's FPU state if userspace is accessing MSRs whose values are managed by XSAVES so that the MSR helper, e.g. vmx_{get,set}_msr(), can simply do {RD,WR}MSR to access the guest's value.
Note that
guest_cpuid_has()is not queried as host userspace is allowed to access MSRs that have not been exposed to the guest, e.g. it might doKVM_SET_MSRSprior toKVM_SET_CPUID2.Signed-off-by Sean Christopherson <[email protected]>Co-developed-by Yang Weijiang <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +8
-0@@ -102,6 +102,8 @@ static void enter_smm(struct kvm_vcpu *vcpu);static void __kvm_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags);static void store_regs (struct kvm_vcpu *vcpu);static int sync_regs (struct kvm_vcpu *vcpu); +static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu); +static void kvm_put_guest_fpu (struct kvm_vcpu *vcpu);struct kvm_x86_ops *kvm_x86_ops__read_mostly ;EXPORT_SYMBOL_GPL (kvm_x86_ops);@@ -2959,6 +2961,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) }EXPORT_SYMBOL_GPL (kvm_get_msr_common); +static bool is_xsaves_msr (u32 index) +{ +return index ==MSR_IA32_U_CET || + (index >=MSR_IA32_PL0_SSP && index <=MSR_IA32_PL3_SSP ); +} +/* * Read or write a bunch of msrs. All parameters are kernel addresses. * arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2982
Quoted 6 lines
arch/x86/kvm/x86.c +2
-0@@ -2969,11 +2977,30 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,int (*do_msr)(struct kvm_vcpu *vcpu, unsigned index, u64 *data)) { +bool fpu_loaded = false;int i; +u64 cet_bits =XFEATURE_MASK_CET_USER |XFEATURE_MASK_CET_KERNEL ;Dunno if the compiler will actually generate different code, but this can be a const.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2988
Quoted 6 lines
arch/x86/kvm/x86.c +6
-0+u64 host_xss =0 ; + +for (i =0 ; i < msrs->nmsrs; ++i) { +if (!fpu_loaded &&is_xsaves_msr (entries[i].index)) { +if (!kvm_x86_ops->xsaves_supported () || + !kvm_x86_ops->supported_xss ())The "!kvm_x86_ops->supported_xss()" is redundant with the
host_xsscheck below.arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2989
Quoted 1 line
arch/x86/kvm/x86.c +1
-0+continue ;Hmm,
vmx_set_msr()should be checkinghost_xss, arguably we should calldo_msr()and let it handle the bad MSR access. I don't have a strong opinion either way, practically speaking the end result will be the same.If we do want to handle a misbehaving userspace here, this should be 'break' instead of 'continue'.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2993
Quoted 5 lines
arch/x86/kvm/x86.c +3
-1+ + host_xss = kvm_x86_ops->supported_xss (); -for (i =0 ; i < msrs->nmsrs; ++i) +if ((host_xss & cet_bits) != cet_bits)I'm pretty sure this should check for either CET bit being set, not both, e.g. I assume it's possible to enable and expose
XFEATURE_MASK_CET_USERbut notXFEATURE_MASK_CET_KERNEL.
Quoted 5 lines
So something like
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; const bool cet_supported = kvm_x86_ops->xsaves_supported() && (kvm_x86_ops->supported_xss() & cet_bits);
Oh, and this should use kvm_supported_xss(), which masks ->supported_xss() with KVM_SUPPORTED_XSS .
Quoted 44 lines
for (i = 0; i < msrs->nmsrs; ++i) { if (!fpu_loaded && cet_supported && is_xsaves_msr(entries[i].index)) { kvm_load_guest_fpu(vcpu); fpu_loaded = true; } if (do_msr(vcpu, entries[i].index, &entries[i].data)) break; }or
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; for (i = 0; i < msrs->nmsrs; ++i) { if (!fpu_loaded && is_xsaves_msr(entries[i].index)) { if (!kvm_x86_ops->supported_xss() || !(kvm_x86_ops->supported_xss() & cet_bits)) break; kvm_load_guest_fpu(vcpu); fpu_loaded = true; } if (do_msr(vcpu, entries[i].index, &entries[i].data)) break; }Quoted 16 lines
arch/x86/kvm/x86.c +8
-0+continue ; + +kvm_load_guest_fpu (vcpu); + fpu_loaded = true; + }if (do_msr (vcpu, entries[i].index, &entries[i].data))break ; + } +if (fpu_loaded) +kvm_put_guest_fpu (vcpu);return i; }Signature · 1 line
2.17.2
Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
In reply to Sean ChristophersonQuoted 91 lines · Sean Christopherson
Quoted 49 lines · Yang Weijiang
From: Sean Christopherson <[email protected]>
A handful of CET MSRs are not context switched through "traditional" methods, e.g. VMCS or manual switching, but rather are passed through to the guest and are saved and restored by XSAVES/XRSTORS, i.e. the guest's FPU state.
Load the guest's FPU state if userspace is accessing MSRs whose values are managed by XSAVES so that the MSR helper, e.g. vmx_{get,set}_msr(), can simply do {RD,WR}MSR to access the guest's value.
Note that
guest_cpuid_has()is not queried as host userspace is allowed to access MSRs that have not been exposed to the guest, e.g. it might doKVM_SET_MSRSprior toKVM_SET_CPUID2.Signed-off-by Sean Christopherson <[email protected]>Co-developed-by Yang Weijiang <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +8
-0@@ -102,6 +102,8 @@ static void enter_smm(struct kvm_vcpu *vcpu);static void __kvm_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags);static void store_regs (struct kvm_vcpu *vcpu);static int sync_regs (struct kvm_vcpu *vcpu); +static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu); +static void kvm_put_guest_fpu (struct kvm_vcpu *vcpu);struct kvm_x86_ops *kvm_x86_ops__read_mostly ;EXPORT_SYMBOL_GPL (kvm_x86_ops);@@ -2959,6 +2961,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) }EXPORT_SYMBOL_GPL (kvm_get_msr_common); +static bool is_xsaves_msr (u32 index) +{ +return index ==MSR_IA32_U_CET || + (index >=MSR_IA32_PL0_SSP && index <=MSR_IA32_PL3_SSP ); +} +/* * Read or write a bunch of msrs. All parameters are kernel addresses. * arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2982
Quoted 6 lines
arch/x86/kvm/x86.c +2
-0@@ -2969,11 +2977,30 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,int (*do_msr)(struct kvm_vcpu *vcpu, unsigned index, u64 *data)) { +bool fpu_loaded = false;int i; +u64 cet_bits =XFEATURE_MASK_CET_USER |XFEATURE_MASK_CET_KERNEL ;Dunno if the compiler will actually generate different code, but this can be a const.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2988
Quoted 6 lines
arch/x86/kvm/x86.c +6
-0+u64 host_xss =0 ; + +for (i =0 ; i < msrs->nmsrs; ++i) { +if (!fpu_loaded &&is_xsaves_msr (entries[i].index)) { +if (!kvm_x86_ops->xsaves_supported () || + !kvm_x86_ops->supported_xss ())The "!kvm_x86_ops->supported_xss()" is redundant with the
host_xsscheck below.arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2989
Quoted 1 line
arch/x86/kvm/x86.c +1
-0+continue ;Hmm,
vmx_set_msr()should be checkinghost_xss, arguably we should calldo_msr()and let it handle the bad MSR access. I don't have a strong opinion either way, practically speaking the end result will be the same.If we do want to handle a misbehaving userspace here, this should be 'break' instead of 'continue'.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2993
Quoted 5 lines
arch/x86/kvm/x86.c +3
-1+ + host_xss = kvm_x86_ops->supported_xss (); -for (i =0 ; i < msrs->nmsrs; ++i) +if ((host_xss & cet_bits) != cet_bits)I'm pretty sure this should check for either CET bit being set, not both, e.g. I assume it's possible to enable and expose
XFEATURE_MASK_CET_USERbut notXFEATURE_MASK_CET_KERNEL.So something like
Quoted 13 lines
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL;
const bool cet_supported = kvm_x86_ops->xsaves_supported() &&
(kvm_x86_ops->supported_xss() & cet_bits);
for (i = 0; i < msrs->nmsrs; ++i) {
if (!fpu_loaded && cet_supported &&
is_xsaves_msr(entries[i].index)) {
kvm_load_guest_fpu(vcpu);
fpu_loaded = true;
}
if (do_msr(vcpu, entries[i].index, &entries[i].data))
break;
}After looking at patch 8/8, and assuming KVM can actually virtualize USER and KERNEL independently, we should go with this version that defers to do_msr(), otherwise this code would also need to differentiate between USER and KERNEL MSRs. In other words, have __msr_io() load the guest fpu if CET is support and any CET MSRs is being accessed, and let vmx_set_msr() do the fine grained fault/error handling.
Quoted 33 lines
or
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; for (i = 0; i < msrs->nmsrs; ++i) { if (!fpu_loaded && is_xsaves_msr(entries[i].index)) { if (!kvm_x86_ops->supported_xss() || !(kvm_x86_ops->supported_xss() & cet_bits)) break; kvm_load_guest_fpu(vcpu); fpu_loaded = true; } if (do_msr(vcpu, entries[i].index, &entries[i].data)) break; }Quoted 16 lines
arch/x86/kvm/x86.c +8
-0+continue ; + +kvm_load_guest_fpu (vcpu); + fpu_loaded = true; + }if (do_msr (vcpu, entries[i].index, &entries[i].data))break ; + } +if (fpu_loaded) +kvm_put_guest_fpu (vcpu);return i; }Signature · 1 line
2.17.2
Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
Quoted 106 lines · Sean Christopherson
Quoted 91 lines · Sean Christopherson
Quoted 49 lines · Yang Weijiang
From: Sean Christopherson <[email protected]>
A handful of CET MSRs are not context switched through "traditional" methods, e.g. VMCS or manual switching, but rather are passed through to the guest and are saved and restored by XSAVES/XRSTORS, i.e. the guest's FPU state.
Load the guest's FPU state if userspace is accessing MSRs whose values are managed by XSAVES so that the MSR helper, e.g. vmx_{get,set}_msr(), can simply do {RD,WR}MSR to access the guest's value.
Note that
guest_cpuid_has()is not queried as host userspace is allowed to access MSRs that have not been exposed to the guest, e.g. it might doKVM_SET_MSRSprior toKVM_SET_CPUID2.Signed-off-by Sean Christopherson <[email protected]>Co-developed-by Yang Weijiang <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +8
-0@@ -102,6 +102,8 @@ static void enter_smm(struct kvm_vcpu *vcpu);static void __kvm_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags);static void store_regs (struct kvm_vcpu *vcpu);static int sync_regs (struct kvm_vcpu *vcpu); +static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu); +static void kvm_put_guest_fpu (struct kvm_vcpu *vcpu);struct kvm_x86_ops *kvm_x86_ops__read_mostly ;EXPORT_SYMBOL_GPL (kvm_x86_ops);@@ -2959,6 +2961,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) }EXPORT_SYMBOL_GPL (kvm_get_msr_common); +static bool is_xsaves_msr (u32 index) +{ +return index ==MSR_IA32_U_CET || + (index >=MSR_IA32_PL0_SSP && index <=MSR_IA32_PL3_SSP ); +} +/* * Read or write a bunch of msrs. All parameters are kernel addresses. * arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2982
Quoted 6 lines
arch/x86/kvm/x86.c +2
-0@@ -2969,11 +2977,30 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,int (*do_msr)(struct kvm_vcpu *vcpu, unsigned index, u64 *data)) { +bool fpu_loaded = false;int i; +u64 cet_bits =XFEATURE_MASK_CET_USER |XFEATURE_MASK_CET_KERNEL ;Dunno if the compiler will actually generate different code, but this can be a const.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2988
Quoted 6 lines
arch/x86/kvm/x86.c +6
-0+u64 host_xss =0 ; + +for (i =0 ; i < msrs->nmsrs; ++i) { +if (!fpu_loaded &&is_xsaves_msr (entries[i].index)) { +if (!kvm_x86_ops->xsaves_supported () || + !kvm_x86_ops->supported_xss ())The "!kvm_x86_ops->supported_xss()" is redundant with the
host_xsscheck below.arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2989
Quoted 1 line
arch/x86/kvm/x86.c +1
-0+continue ;Hmm,
vmx_set_msr()should be checkinghost_xss, arguably we should calldo_msr()and let it handle the bad MSR access. I don't have a strong opinion either way, practically speaking the end result will be the same.If we do want to handle a misbehaving userspace here, this should be 'break' instead of 'continue'.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2993
Quoted 5 lines
arch/x86/kvm/x86.c +3
-1+ + host_xss = kvm_x86_ops->supported_xss (); -for (i =0 ; i < msrs->nmsrs; ++i) +if ((host_xss & cet_bits) != cet_bits)I'm pretty sure this should check for either CET bit being set, not both, e.g. I assume it's possible to enable and expose
XFEATURE_MASK_CET_USERbut notXFEATURE_MASK_CET_KERNEL.So something like
Quoted 13 lines
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; const bool cet_supported = kvm_x86_ops->xsaves_supported() && (kvm_x86_ops->supported_xss() & cet_bits); for (i = 0; i < msrs->nmsrs; ++i) { if (!fpu_loaded && cet_supported && is_xsaves_msr(entries[i].index)) { kvm_load_guest_fpu(vcpu); fpu_loaded = true; } if (do_msr(vcpu, entries[i].index, &entries[i].data)) break; }
Quoted 7 lines
After looking at patch 8/8, and assuming KVM can actually virtualize USER and KERNEL independently, we should go with this version that defers to
do_msr(), otherwise this code would also need to differentiate between USER and KERNEL MSRs. In other words, have__msr_io()load the guest fpu if CET is support and any CET MSRs is being accessed, and letvmx_set_msr()do the fine grained fault/error handling.
Sure, will follow that.
Quoted 33 lines
Quoted 33 lines
or
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; for (i = 0; i < msrs->nmsrs; ++i) { if (!fpu_loaded && is_xsaves_msr(entries[i].index)) { if (!kvm_x86_ops->supported_xss() || !(kvm_x86_ops->supported_xss() & cet_bits)) break; kvm_load_guest_fpu(vcpu); fpu_loaded = true; } if (do_msr(vcpu, entries[i].index, &entries[i].data)) break; }Quoted 16 lines
arch/x86/kvm/x86.c +8
-0+continue ; + +kvm_load_guest_fpu (vcpu); + fpu_loaded = true; + }if (do_msr (vcpu, entries[i].index, &entries[i].data))break ; + } +if (fpu_loaded) +kvm_put_guest_fpu (vcpu);return i; }Signature · 1 line
2.17.2
Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES
In reply to Sean ChristophersonQuoted 51 lines · Sean Christopherson
Quoted 50 lines · Yang Weijiang
From: Sean Christopherson <[email protected]>
A handful of CET MSRs are not context switched through "traditional" methods, e.g. VMCS or manual switching, but rather are passed through to the guest and are saved and restored by XSAVES/XRSTORS, i.e. the guest's FPU state.
Load the guest's FPU state if userspace is accessing MSRs whose values are managed by XSAVES so that the MSR helper, e.g. vmx_{get,set}_msr(), can simply do {RD,WR}MSR to access the guest's value.
Note that
guest_cpuid_has()is not queried as host userspace is allowed to access MSRs that have not been exposed to the guest, e.g. it might doKVM_SET_MSRSprior toKVM_SET_CPUID2.Signed-off-by Sean Christopherson <[email protected]>Co-developed-by Yang Weijiang <[email protected]>Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +8
-0@@ -102,6 +102,8 @@ static void enter_smm(struct kvm_vcpu *vcpu);static void __kvm_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags);static void store_regs (struct kvm_vcpu *vcpu);static int sync_regs (struct kvm_vcpu *vcpu); +static void kvm_load_guest_fpu (struct kvm_vcpu *vcpu); +static void kvm_put_guest_fpu (struct kvm_vcpu *vcpu);struct kvm_x86_ops *kvm_x86_ops__read_mostly ;EXPORT_SYMBOL_GPL (kvm_x86_ops);@@ -2959,6 +2961,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) }EXPORT_SYMBOL_GPL (kvm_get_msr_common); +static bool is_xsaves_msr (u32 index) +{ +return index ==MSR_IA32_U_CET || + (index >=MSR_IA32_PL0_SSP && index <=MSR_IA32_PL3_SSP ); +} +/* * Read or write a bunch of msrs. All parameters are kernel addresses. * @@ -2969,11 +2977,30 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, int (*do_msr)(struct kvm_vcpu *vcpu,
Quoted 9 lines
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2982
Quoted 5 lines
arch/x86/kvm/x86.c +2
-0unsigned index, u64 *data)) { + bool fpu_loaded = false;int i; +u64 cet_bits =XFEATURE_MASK_CET_USER |XFEATURE_MASK_CET_KERNEL ;Dunno if the compiler will actually generate different code, but this can be a const.
OK, will add it.
Quoted 31 lines
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2988
Quoted 6 lines
arch/x86/kvm/x86.c +6
-0+u64 host_xss =0 ; + +for (i =0 ; i < msrs->nmsrs; ++i) { +if (!fpu_loaded &&is_xsaves_msr (entries[i].index)) { +if (!kvm_x86_ops->xsaves_supported () || + !kvm_x86_ops->supported_xss ())The "!kvm_x86_ops->supported_xss()" is redundant with the
host_xsscheck below.arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2989
Quoted 1 line
arch/x86/kvm/x86.c +1
-0+continue ;Hmm,
vmx_set_msr()should be checkinghost_xss, arguably we should calldo_msr()and let it handle the bad MSR access. I don't have a strong opinion either way, practically speaking the end result will be the same.If we do want to handle a misbehaving userspace here, this should be 'break' instead of 'continue'.
arch/x86/kvm/x86.c static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs, · line 2993
Quoted 5 lines
arch/x86/kvm/x86.c +3
-1+ + host_xss = kvm_x86_ops->supported_xss (); -for (i =0 ; i < msrs->nmsrs; ++i) +if ((host_xss & cet_bits) != cet_bits)I'm pretty sure this should check for either CET bit being set, not both, e.g. I assume it's possible to enable and expose
XFEATURE_MASK_CET_USERbut notXFEATURE_MASK_CET_KERNEL.So something like
Quoted 14 lines
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL;
const bool cet_supported = kvm_x86_ops->xsaves_supported() &&
(kvm_x86_ops->supported_xss() & cet_bits);
for (i = 0; i < msrs->nmsrs; ++i) {
if (!fpu_loaded && cet_supported &&
is_xsaves_msr(entries[i].index)) {
kvm_load_guest_fpu(vcpu);
fpu_loaded = true;
}
if (do_msr(vcpu, entries[i].index, &entries[i].data))
break;
}thanks, will modify the patch.
Quoted 33 lines
or
const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; for (i = 0; i < msrs->nmsrs; ++i) { if (!fpu_loaded && is_xsaves_msr(entries[i].index)) { if (!kvm_x86_ops->supported_xss() || !(kvm_x86_ops->supported_xss() & cet_bits)) break; kvm_load_guest_fpu(vcpu); fpu_loaded = true; } if (do_msr(vcpu, entries[i].index, &entries[i].data)) break; }Quoted 16 lines
arch/x86/kvm/x86.c +8
-0+continue ; + +kvm_load_guest_fpu (vcpu); + fpu_loaded = true; + }if (do_msr (vcpu, entries[i].index, &entries[i].data))break ; + } +if (fpu_loaded) +kvm_put_guest_fpu (vcpu);return i; }Signature · 1 line
2.17.2
PATCH v6 8/8 KVM: x86: Add user-space access interface for CET MSRs
In reply to Yang WeijiangThere're two different places storing Guest CET states, the states managed with XSAVES/XRSTORS, as restored/saved
in previous patch, can be read/write directly from/to the MSRs. For those stored in VMCS fields, they're access via vmcs_read/ vmcs_write.
--- arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
arch/x86/kvm/vmx/vmx.c +43 -0
@@ -1774,6 +1774,27 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)else msr_info->data = vmx->pt_desc.guest.addr_a[index /2 ];break ; +case MSR_IA32_S_CET : + msr_info->data =vmcs_readl (GUEST_S_CET ); +break ; +case MSR_IA32_U_CET : +rdmsrl (MSR_IA32_U_CET , msr_info->data); +break ; +case MSR_IA32_INT_SSP_TAB : + msr_info->data =vmcs_readl (GUEST_INTR_SSP_TABLE ); +break ; +case MSR_IA32_PL0_SSP : +rdmsrl (MSR_IA32_PL0_SSP , msr_info->data); +break ; +case MSR_IA32_PL1_SSP : +rdmsrl (MSR_IA32_PL1_SSP , msr_info->data); +break ; +case MSR_IA32_PL2_SSP : +rdmsrl (MSR_IA32_PL2_SSP , msr_info->data); +break ; +case MSR_IA32_PL3_SSP : +rdmsrl (MSR_IA32_PL3_SSP , msr_info->data); +break ;case MSR_TSC_AUX :if (!msr_info->host_initiated && !guest_cpuid_has (vcpu,X86_FEATURE_RDTSCP ))
@@ -2007,6 +2028,28 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)else vmx->pt_desc.guest.addr_a[index /2 ] = data;break ; +case MSR_IA32_S_CET : +vmcs_writel (GUEST_S_CET , data); +break ; +case MSR_IA32_U_CET : +wrmsrl (MSR_IA32_U_CET , data); +break ; +case MSR_IA32_INT_SSP_TAB : +vmcs_writel (GUEST_INTR_SSP_TABLE , data); +break ; +case MSR_IA32_PL0_SSP : +wrmsrl (MSR_IA32_PL0_SSP , data); +break ; +case MSR_IA32_PL1_SSP : +wrmsrl (MSR_IA32_PL1_SSP , data); +break ; +case MSR_IA32_PL2_SSP : +wrmsrl (MSR_IA32_PL2_SSP , data); +break ; +case MSR_IA32_PL3_SSP : +wrmsrl (MSR_IA32_PL3_SSP , data); +break ; +case MSR_TSC_AUX :if (!msr_info->host_initiated && !guest_cpuid_has (vcpu,X86_FEATURE_RDTSCP ))
Signature · 1 line
2.17.2
Re: [PATCH v6 8/8] KVM: x86: Add user-space access interface for CET MSRs
Quoted 30 lines · Yang Weijiang
There're two different places storing Guest CET states, the states managed with XSAVES/XRSTORS, as restored/saved
in previous patch, can be read/write directly from/to the MSRs. For those stored in VMCS fields, they're access via vmcs_read/vmcs_write.Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)arch/x86/kvm/vmx/vmx.c +11
-0@@ -1774,6 +1774,27 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)else msr_info->data = vmx->pt_desc.guest.addr_a[index /2 ];break ; +case MSR_IA32_S_CET : + msr_info->data =vmcs_readl (GUEST_S_CET ); +break ; +case MSR_IA32_U_CET : +rdmsrl (MSR_IA32_U_CET , msr_info->data); +break ; +case MSR_IA32_INT_SSP_TAB : + msr_info->data =vmcs_readl (GUEST_INTR_SSP_TABLE ); +break ; +case MSR_IA32_PL0_SSP : +rdmsrl (MSR_IA32_PL0_SSP , msr_info->data);
arch/x86/kvm/vmx/vmx.c static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 1797
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +10
-0+break ; +case MSR_IA32_PL1_SSP : +rdmsrl (MSR_IA32_PL1_SSP , msr_info->data); +break ; +case MSR_IA32_PL2_SSP : +rdmsrl (MSR_IA32_PL2_SSP , msr_info->data); +break ; +case MSR_IA32_PL3_SSP : +rdmsrl (MSR_IA32_PL3_SSP , msr_info->data); +break ;
These all need appropriate checks on guest and host support. The guest checks won't come into play very often, if ever, for the MSRs that exist if IBT *or* SHSTK is supported due to passing the MSRs through to the guest, but I don't think we want this code reliant on the interception logic. E.g.:
case MSR_IA32_S_CET:
if (!(host_xss & XFEATURE_MASK_CET_KERNEL)) return 1; if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) return 1;
MSR_IA32_U_CET is same as above, s/KERNEL/USER.
case MSR_IA32_INT_SSP_TAB:
if (!(host_xss & (XFEATURE_MASK_CET_KERNEL | XFEATURE_MASK_CET_USER))) return 1; if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) return 1;
MSR_IA32_PL[0-3]_SSP are same as above, but only check the appropriate KERNEL or USER bit.
Note, the PL[0-2]_SSP MSRs can be collapsed into a single case, e.g.:
case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP: <error handling code>; rdmsrl(msr_index, msr_info->data); break;
Rinse and repeat for vmx_set_msr().
Quoted 35 lines
arch/x86/kvm/vmx/vmx.c +22
-0case MSR_TSC_AUX :if (!msr_info->host_initiated && !guest_cpuid_has (vcpu,X86_FEATURE_RDTSCP ))@@ -2007,6 +2028,28 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)else vmx->pt_desc.guest.addr_a[index /2 ] = data;break ; +case MSR_IA32_S_CET : +vmcs_writel (GUEST_S_CET , data); +break ; +case MSR_IA32_U_CET : +wrmsrl (MSR_IA32_U_CET , data); +break ; +case MSR_IA32_INT_SSP_TAB : +vmcs_writel (GUEST_INTR_SSP_TABLE , data); +break ; +case MSR_IA32_PL0_SSP : +wrmsrl (MSR_IA32_PL0_SSP , data); +break ; +case MSR_IA32_PL1_SSP : +wrmsrl (MSR_IA32_PL1_SSP , data); +break ; +case MSR_IA32_PL2_SSP : +wrmsrl (MSR_IA32_PL2_SSP , data); +break ; +case MSR_IA32_PL3_SSP : +wrmsrl (MSR_IA32_PL3_SSP , data); +break ; +case MSR_TSC_AUX :if (!msr_info->host_initiated && !guest_cpuid_has (vcpu,X86_FEATURE_RDTSCP ))Signature · 1 line
2.17.2
Re: [PATCH v6 8/8] KVM: x86: Add user-space access interface for CET MSRs
Quoted 73 lines · Sean Christopherson
Quoted 30 lines · Yang Weijiang
There're two different places storing Guest CET states, the states managed with XSAVES/XRSTORS, as restored/saved
in previous patch, can be read/write directly from/to the MSRs. For those stored in VMCS fields, they're access via vmcs_read/vmcs_write.Signed-off-by Yang Weijiang <[email protected]>--- arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)arch/x86/kvm/vmx/vmx.c +11
-0@@ -1774,6 +1774,27 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)else msr_info->data = vmx->pt_desc.guest.addr_a[index /2 ];break ; +case MSR_IA32_S_CET : + msr_info->data =vmcs_readl (GUEST_S_CET ); +break ; +case MSR_IA32_U_CET : +rdmsrl (MSR_IA32_U_CET , msr_info->data); +break ; +case MSR_IA32_INT_SSP_TAB : + msr_info->data =vmcs_readl (GUEST_INTR_SSP_TABLE ); +break ; +case MSR_IA32_PL0_SSP : +rdmsrl (MSR_IA32_PL0_SSP , msr_info->data);arch/x86/kvm/vmx/vmx.c static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 1797
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +10
-0+break ; +case MSR_IA32_PL1_SSP : +rdmsrl (MSR_IA32_PL1_SSP , msr_info->data); +break ; +case MSR_IA32_PL2_SSP : +rdmsrl (MSR_IA32_PL2_SSP , msr_info->data); +break ; +case MSR_IA32_PL3_SSP : +rdmsrl (MSR_IA32_PL3_SSP , msr_info->data); +break ;These all need appropriate checks on guest and host support. The guest checks won't come into play very often, if ever, for the MSRs that exist if IBT *or* SHSTK is supported due to passing the MSRs through to the guest, but I don't think we want this code reliant on the interception logic. E.g.:
case
MSR_IA32_S_CET:if (!(host_xss & XFEATURE_MASK_CET_KERNEL)) return 1; if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) return 1;
MSR_IA32_U_CETis same as above, s/KERNEL/USER.case
MSR_IA32_INT_SSP_TAB:if (!(host_xss & (XFEATURE_MASK_CET_KERNEL | XFEATURE_MASK_CET_USER))) return 1; if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) return 1;MSR_IA32_PL[0-3]_SSP are same as above, but only check the appropriate KERNEL or USER bit.
Note, the PL[0-2]_SSP MSRs can be collapsed into a single case, e.g.:
Quoted 9 lines
case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP: <error handling code>; rdmsrl(msr_index, msr_info->data); break;Rinse and repeat for
vmx_set_msr().
Thanks you, will modify this part of code.
Quoted 35 lines
Quoted 35 lines
arch/x86/kvm/vmx/vmx.c +22
-0case MSR_TSC_AUX :if (!msr_info->host_initiated && !guest_cpuid_has (vcpu,X86_FEATURE_RDTSCP ))@@ -2007,6 +2028,28 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)else vmx->pt_desc.guest.addr_a[index /2 ] = data;break ; +case MSR_IA32_S_CET : +vmcs_writel (GUEST_S_CET , data); +break ; +case MSR_IA32_U_CET : +wrmsrl (MSR_IA32_U_CET , data); +break ; +case MSR_IA32_INT_SSP_TAB : +vmcs_writel (GUEST_INTR_SSP_TABLE , data); +break ; +case MSR_IA32_PL0_SSP : +wrmsrl (MSR_IA32_PL0_SSP , data); +break ; +case MSR_IA32_PL1_SSP : +wrmsrl (MSR_IA32_PL1_SSP , data); +break ; +case MSR_IA32_PL2_SSP : +wrmsrl (MSR_IA32_PL2_SSP , data); +break ; +case MSR_IA32_PL3_SSP : +wrmsrl (MSR_IA32_PL3_SSP , data); +break ; +case MSR_TSC_AUX :if (!msr_info->host_initiated && !guest_cpuid_has (vcpu,X86_FEATURE_RDTSCP ))Signature · 1 line
2.17.2