| Message ID | 20200326081847.5870-3-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce support for guest CET feature | expand |
On Thu, Mar 26, 2020 at 04:18:39PM +0800, Yang Weijiang wrote: > 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_CET and MSR_IA32_PL3_SSP are used for user-mode protection, > the MSR contents are switched between threads during scheduling, > 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 SDM for detailed info. > > The difference between CET VMCS fields and CET MSRs is that,the former > are used during VMEnter/VMExit, whereas the latter are used for CET > state storage between task/thread scheduling. > > Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 860e5f4a9f7b..1aca468d9a10 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -3033,6 +3033,13 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > vmcs_writel(GUEST_CR3, guest_cr3); > } > > +static bool is_cet_mode_allowed(struct kvm_vcpu *vcpu, u32 mode_mask) CET itself isn't a mode. And since this ends up being an inner helper for is_cet_supported(), I think __is_cet_supported() would be the way to go. Even @mode_mask is a bit confusing without the context of it being kernel vs. user. The callers are very readable, e.g. I'd much prefer passing the mask as opposed to doing 'bool kernel'. Maybe s/mode_mask/cet_mask? That doesn't exactly make things super clear, but at least the reader knows the mask is for CET features. > +{ > + return ((supported_xss & mode_mask) && > + (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > + guest_cpuid_has(vcpu, X86_FEATURE_IBT))); > +} > + > int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -7064,6 +7071,35 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) > vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4)); > } > > +static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap; > + bool flag; Maybe s/flag/incpt or something to make it more obvious that the bool is true if we want to intercept? vmx_set_intercept_for_msr()s's @value isn't any better :-/. > + > + flag = !is_cet_mode_allowed(vcpu, XFEATURE_MASK_CET_USER); > + /* > + * U_CET is required for USER CET, and U_CET, PL3_SPP are bound as > + * one component and controlled by IA32_XSS[bit 11]. > + */ > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_U_CET, MSR_TYPE_RW, flag); > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL3_SSP, MSR_TYPE_RW, flag); > + > + flag = !is_cet_mode_allowed(vcpu, XFEATURE_MASK_CET_KERNEL); > + /* > + * S_CET is required for KERNEL CET, and PL0_SSP ... PL2_SSP are > + * bound as one component and controlled by IA32_XSS[bit 12]. > + */ > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_S_CET, MSR_TYPE_RW, flag); > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL0_SSP, MSR_TYPE_RW, flag); > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL1_SSP, MSR_TYPE_RW, flag); > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL2_SSP, MSR_TYPE_RW, flag); > + > + flag |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK); > + /* SSP_TAB is only available for KERNEL SHSTK.*/ > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW, flag); > +} > + > static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -7102,6 +7138,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > vmx_set_guest_msr(vmx, msr, enabled ? 0 : TSX_CTRL_RTM_DISABLE); > } > } > + > + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > + vmx_update_intercept_for_cet_msr(vcpu); This is wrong, it will miss the case where userspace double configures CPUID and goes from CET=1 to CET=0. This should instead be: if (supported_xss & (XFEATURE_MASK_CET_KERNEL | XFEATURE_MASK_CET_USER)) vmx_update_intercept_for_cet_msr(vcpu); > } > > static __init void vmx_set_cpu_caps(void) > -- > 2.17.2 >
On Thu, Apr 23, 2020 at 09:27:49AM -0700, Sean Christopherson wrote: > On Thu, Mar 26, 2020 at 04:18:39PM +0800, Yang Weijiang wrote: > > @@ -3033,6 +3033,13 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > vmcs_writel(GUEST_CR3, guest_cr3); > > } > > > > +static bool is_cet_mode_allowed(struct kvm_vcpu *vcpu, u32 mode_mask) > > CET itself isn't a mode. And since this ends up being an inner helper for > is_cet_supported(), I think __is_cet_supported() would be the way to go. > > Even @mode_mask is a bit confusing without the context of it being kernel > vs. user. The callers are very readable, e.g. I'd much prefer passing the > mask as opposed to doing 'bool kernel'. Maybe s/mode_mask/cet_mask? That > doesn't exactly make things super clear, but at least the reader knows the > mask is for CET features. Make sense, will change it. > > > +{ > > + return ((supported_xss & mode_mask) && > > + (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > > + guest_cpuid_has(vcpu, X86_FEATURE_IBT))); > > +} > > + > > int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > @@ -7064,6 +7071,35 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) > > vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4)); > > } > > > > +static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu) > > +{ > > + struct vcpu_vmx *vmx = to_vmx(vcpu); > > + unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap; > > + bool flag; > > Maybe s/flag/incpt or something to make it more obvious that the bool is > true if we want to intercept? vmx_set_intercept_for_msr()s's @value isn't > any better :-/. I prefer using incpt now ;-) > > + > > + flag = !is_cet_mode_allowed(vcpu, XFEATURE_MASK_CET_USER); > > + /* > > + * U_CET is required for USER CET, and U_CET, PL3_SPP are bound as > > + * one component and controlled by IA32_XSS[bit 11]. > > + */ > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_U_CET, MSR_TYPE_RW, flag); > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL3_SSP, MSR_TYPE_RW, flag); > > + > > + flag = !is_cet_mode_allowed(vcpu, XFEATURE_MASK_CET_KERNEL); > > + /* > > + * S_CET is required for KERNEL CET, and PL0_SSP ... PL2_SSP are > > + * bound as one component and controlled by IA32_XSS[bit 12]. > > + */ > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_S_CET, MSR_TYPE_RW, flag); > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL0_SSP, MSR_TYPE_RW, flag); > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL1_SSP, MSR_TYPE_RW, flag); > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL2_SSP, MSR_TYPE_RW, flag); > > + > > + flag |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK); > > + /* SSP_TAB is only available for KERNEL SHSTK.*/ > > + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW, flag); > > +} > > + > > static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > @@ -7102,6 +7138,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > > vmx_set_guest_msr(vmx, msr, enabled ? 0 : TSX_CTRL_RTM_DISABLE); > > } > > } > > + > > + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > > + vmx_update_intercept_for_cet_msr(vcpu); > > This is wrong, it will miss the case where userspace double configures CPUID > and goes from CET=1 to CET=0. This should instead be: > > if (supported_xss & (XFEATURE_MASK_CET_KERNEL | XFEATURE_MASK_CET_USER)) > vmx_update_intercept_for_cet_msr(vcpu); > > > } Here CET=1/0, did you mean the CET bit in XSS or CR4.CET? If it's the former, then it's OK for me. > > > > static __init void vmx_set_cpu_caps(void) > > -- > > 2.17.2 > >
On Fri, Apr 24, 2020 at 10:07:51PM +0800, Yang Weijiang wrote: > On Thu, Apr 23, 2020 at 09:27:49AM -0700, Sean Christopherson wrote: > > On Thu, Mar 26, 2020 at 04:18:39PM +0800, Yang Weijiang wrote: > > > @@ -7102,6 +7138,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > > > vmx_set_guest_msr(vmx, msr, enabled ? 0 : TSX_CTRL_RTM_DISABLE); > > > } > > > } > > > + > > > + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > > > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > > > + vmx_update_intercept_for_cet_msr(vcpu); > > > > This is wrong, it will miss the case where userspace double configures CPUID > > and goes from CET=1 to CET=0. This should instead be: > > > > if (supported_xss & (XFEATURE_MASK_CET_KERNEL | XFEATURE_MASK_CET_USER)) > > vmx_update_intercept_for_cet_msr(vcpu); > > > > > } > Here CET=1/0, did you mean the CET bit in XSS or CR4.CET? If it's the > former, then it's OK for me. The former, i.e. update the CET MSRs if KVM supports CET virtualization and the guest's CPUID configuration is changing.
On Fri, Apr 24, 2020 at 07:55:29AM -0700, Sean Christopherson wrote: > On Fri, Apr 24, 2020 at 10:07:51PM +0800, Yang Weijiang wrote: > > On Thu, Apr 23, 2020 at 09:27:49AM -0700, Sean Christopherson wrote: > > > On Thu, Mar 26, 2020 at 04:18:39PM +0800, Yang Weijiang wrote: > > > > @@ -7102,6 +7138,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > > > > vmx_set_guest_msr(vmx, msr, enabled ? 0 : TSX_CTRL_RTM_DISABLE); > > > > } > > > > } > > > > + > > > > + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > > > > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > > > > + vmx_update_intercept_for_cet_msr(vcpu); > > > > > > This is wrong, it will miss the case where userspace double configures CPUID > > > and goes from CET=1 to CET=0. This should instead be: > > > > > > if (supported_xss & (XFEATURE_MASK_CET_KERNEL | XFEATURE_MASK_CET_USER)) > > > vmx_update_intercept_for_cet_msr(vcpu); > > > > > > > } > > Here CET=1/0, did you mean the CET bit in XSS or CR4.CET? If it's the > > former, then it's OK for me. > > The former, i.e. update the CET MSRs if KVM supports CET virtualization and > the guest's CPUID configuration is changing. Yep, this case should be taken into account, thank you for pointing it out!
On 23/04/20 18:27, Sean Christopherson wrote: >> >> +static bool is_cet_mode_allowed(struct kvm_vcpu *vcpu, u32 mode_mask) > CET itself isn't a mode. And since this ends up being an inner helper for > is_cet_supported(), I think __is_cet_supported() would be the way to go. > > Even @mode_mask is a bit confusing without the context of it being kernel > vs. user. The callers are very readable, e.g. I'd much prefer passing the > mask as opposed to doing 'bool kernel'. Maybe s/mode_mask/cet_mask? That > doesn't exactly make things super clear, but at least the reader knows the > mask is for CET features. What about is_cet_state_supported and xss_states? Paolo >> +{ >> + return ((supported_xss & mode_mask) && >> + (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || >> + guest_cpuid_has(vcpu, X86_FEATURE_IBT))); >> +}
On Sat, Apr 25, 2020 at 03:26:30PM +0200, Paolo Bonzini wrote: > On 23/04/20 18:27, Sean Christopherson wrote: > >> > >> +static bool is_cet_mode_allowed(struct kvm_vcpu *vcpu, u32 mode_mask) > > CET itself isn't a mode. And since this ends up being an inner helper for > > is_cet_supported(), I think __is_cet_supported() would be the way to go. > > > > Even @mode_mask is a bit confusing without the context of it being kernel > > vs. user. The callers are very readable, e.g. I'd much prefer passing the > > mask as opposed to doing 'bool kernel'. Maybe s/mode_mask/cet_mask? That > > doesn't exactly make things super clear, but at least the reader knows the > > mask is for CET features. > > What about is_cet_state_supported and xss_states? > It's good for me, I'll change them accordingly, thank you for review! > Paolo > > >> +{ > >> + return ((supported_xss & mode_mask) && > >> + (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > >> + guest_cpuid_has(vcpu, X86_FEATURE_IBT))); > >> +}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 860e5f4a9f7b..1aca468d9a10 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -3033,6 +3033,13 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) vmcs_writel(GUEST_CR3, guest_cr3); } +static bool is_cet_mode_allowed(struct kvm_vcpu *vcpu, u32 mode_mask) +{ + return ((supported_xss & mode_mask) && + (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || + guest_cpuid_has(vcpu, X86_FEATURE_IBT))); +} + int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) { struct vcpu_vmx *vmx = to_vmx(vcpu); @@ -7064,6 +7071,35 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4)); } +static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu) +{ + struct vcpu_vmx *vmx = to_vmx(vcpu); + unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap; + bool flag; + + flag = !is_cet_mode_allowed(vcpu, XFEATURE_MASK_CET_USER); + /* + * U_CET is required for USER CET, and U_CET, PL3_SPP are bound as + * one component and controlled by IA32_XSS[bit 11]. + */ + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_U_CET, MSR_TYPE_RW, flag); + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL3_SSP, MSR_TYPE_RW, flag); + + flag = !is_cet_mode_allowed(vcpu, XFEATURE_MASK_CET_KERNEL); + /* + * S_CET is required for KERNEL CET, and PL0_SSP ... PL2_SSP are + * bound as one component and controlled by IA32_XSS[bit 12]. + */ + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_S_CET, MSR_TYPE_RW, flag); + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL0_SSP, MSR_TYPE_RW, flag); + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL1_SSP, MSR_TYPE_RW, flag); + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_PL2_SSP, MSR_TYPE_RW, flag); + + flag |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK); + /* SSP_TAB is only available for KERNEL SHSTK.*/ + vmx_set_intercept_for_msr(msr_bitmap, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW, flag); +} + static void vmx_cpuid_update(struct kvm_vcpu *vcpu) { struct vcpu_vmx *vmx = to_vmx(vcpu); @@ -7102,6 +7138,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) vmx_set_guest_msr(vmx, msr, enabled ? 0 : TSX_CTRL_RTM_DISABLE); } } + + if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) + vmx_update_intercept_for_cet_msr(vcpu); } static __init void vmx_set_cpu_caps(void)
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_CET and MSR_IA32_PL3_SSP are used for user-mode protection, the MSR contents are switched between threads during scheduling, 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 SDM for detailed info. The difference between CET VMCS fields and CET MSRs is that,the former are used during VMEnter/VMExit, whereas the latter are used for CET state storage between task/thread scheduling. Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kvm/vmx/vmx.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)