[PATCH v6 0/8] Introduce support for Guest CET feature

[PATCH v6 0/8] Introduce support for Guest CET feature

[Yang Weijiang]

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(-)

-- 
2.17.2

[PATCH v6 1/8] KVM: VMX: Define CET VMCS fields and control bits

[Yang Weijiang]

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

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/include/asm/vmx.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index a39136b0d509..68bca290a203 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -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
 };
 
 /*
-- 
2.17.2

[PATCH v6 2/8] KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration

[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 <weijiang.yang@intel.com>
---
 arch/x86/kvm/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 4992e7c99588..29cbde7538a3 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -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)
 {
-- 
2.17.2

Re: [PATCH v6 2/8] KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration

[Sean Christopherson]

On Thu, Jul 25, 2019 at 11:12:40AM +0800, Yang Weijiang wrote:
> To make the code look clean, wrap CPUID(0xD,n>=1) enumeration
> code in a helper function now.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 4992e7c99588..29cbde7538a3 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -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()?

> +				     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")

> +		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;
> +}
> +

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.

>  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  				 u32 index, int *nent, int maxnent)
>  {
> -- 
> 2.17.2
> 

Re: [PATCH v6 2/8] KVM: x86: Add a helper function for CPUID(0xD,n>=1) enumeration

[Yang Weijiang]

On Mon, Aug 12, 2019 at 03:18:11PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:40AM +0800, Yang Weijiang wrote:
> > To make the code look clean, wrap CPUID(0xD,n>=1) enumeration
> > code in a helper function now.
> > 
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 4992e7c99588..29cbde7538a3 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -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()?
>
OK, will change it.
> > +				     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")
> 
OK, will follow the patch to modify the helper.

> > +		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;
> > +}
> > +
> 
> 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.

> >  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >  				 u32 index, int *nent, int maxnent)
> >  {
> > -- 
> > 2.17.2
> > 

[PATCH v6 3/8] KVM: x86: Implement CET CPUID enumeration for Guest

[Yang Weijiang]

CPUID.(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.

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/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(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 26d1eb83f72a..3731ac37119a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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 {
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 29cbde7538a3..d96269a6bcf5 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -62,6 +62,11 @@ u64 kvm_supported_xcr0(void)
 	return xcr0;
 }
 
+u64 kvm_supported_xss(void)
+{
+	return KVM_SUPPORTED_XSS & kvm_x86_ops->supported_xss();
+}
+
 #define F(x) bit(X86_FEATURE_##x)
 
 int kvm_update_cpuid(struct kvm_vcpu *vcpu)
@@ -446,13 +451,13 @@ 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(MOVDIRI) | F(MOVDIR64B);
+		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 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(INTEL_STIBP) |
-		F(MD_CLEAR);
+		F(MD_CLEAR) | F(IBT);
 
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
@@ -608,44 +613,23 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		break;
 	}
 	case 0xd: {
-		int idx, i;
-		u64 supported = kvm_supported_xcr0();
+		u64 u_supported = kvm_supported_xcr0();
+		u64 s_supported = kvm_supported_xss();
+		u32 eax_mask = kvm_cpuid_D_1_eax_x86_features;
 
-		entry->eax &= supported;
-		entry->ebx = xstate_required_size(supported, false);
+		entry->eax &= u_supported;
+		entry->ebx = xstate_required_size(u_supported, false);
 		entry->ecx = entry->ebx;
-		entry->edx &= supported >> 32;
+		entry->edx &= u_supported >> 32;
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-		if (!supported)
+
+		if (!u_supported && !s_supported)
 			break;
 
-		for (idx = 1, i = 1; idx < 64; ++idx) {
-			u64 mask = ((u64)1 << idx);
-			if (*nent >= maxnent)
-				goto out;
+		if (__do_cpuid_dx_leaf(entry, nent, maxnent, s_supported,
+				       u_supported, eax_mask) < 0)
+			goto out;
 
-			do_cpuid_1_ent(&entry[i], function, idx);
-			if (idx == 1) {
-				entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
-				cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
-				entry[i].ebx = 0;
-				if (entry[i].eax & (F(XSAVES)|F(XSAVEC)))
-					entry[i].ebx =
-						xstate_required_size(supported,
-								     true);
-			} else {
-				if (entry[i].eax == 0 || !(supported & mask))
-					continue;
-				if (WARN_ON_ONCE(entry[i].ecx & 1))
-					continue;
-			}
-			entry[i].ecx = 0;
-			entry[i].edx = 0;
-			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-			++*nent;
-			++i;
-		}
 		break;
 	}
 	/* Intel PT */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 652b3876ea5c..ce1d6fe21780 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index a470ff0868c5..6a1870044752 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -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);
-- 
2.17.2

Re: [PATCH v6 3/8] KVM: x86: Implement CET CPUID enumeration for Guest

[Sean Christopherson]

On Thu, Jul 25, 2019 at 11:12:41AM +0800, Yang Weijiang wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 652b3876ea5c..ce1d6fe21780 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -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; 

> +}
> +
>  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)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index a470ff0868c5..6a1870044752 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -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);
> -- 
> 2.17.2
> 

Re: [PATCH v6 3/8] KVM: x86: Implement CET CPUID enumeration for Guest

[Yang Weijiang]

On Mon, Aug 12, 2019 at 05:06:04PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:41AM +0800, Yang Weijiang wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 652b3876ea5c..ce1d6fe21780 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -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; 
>
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.

> > +}
> > +
> >  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)
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index a470ff0868c5..6a1870044752 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -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);
> > -- 
> > 2.17.2
> > 

[PATCH v6 4/8] KVM: VMX: Pass through CET related MSRs to Guest

[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_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.

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index ce1d6fe21780..ce5d1e45b7a5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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)
-- 
2.17.2

Re: [PATCH v6 4/8] KVM: VMX: Pass through CET related MSRs to Guest

[Sean Christopherson]

On Thu, Jul 25, 2019 at 11:12:42AM +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 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 <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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index ce1d6fe21780..ce5d1e45b7a5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -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)) {

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.

> +		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.

> +	}
>  }
>  
>  static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> -- 
> 2.17.2
> 

Re: [PATCH v6 4/8] KVM: VMX: Pass through CET related MSRs to Guest

[Yang Weijiang]

On Mon, Aug 12, 2019 at 04:53:41PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:42AM +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 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 <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 | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index ce1d6fe21780..ce5d1e45b7a5 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -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)) {
> 
> 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.

> > +		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.
> 
> > +	}
> >  }
> >  
> >  static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> > -- 
> > 2.17.2
> > 

[PATCH v6 5/8] KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest

[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 <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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index ce5d1e45b7a5..fbf9c335cf7b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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;
-- 
2.17.2

Re: [PATCH v6 5/8] KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest

[Sean Christopherson]

On Thu, Jul 25, 2019 at 11:12:43AM +0800, Yang Weijiang wrote:
> "Load Guest CET state" bit controls whether Guest CET states
> will be loaded at Guest entry. Before doing that, KVM needs
> to check if CPU CET feature is 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 <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 | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index ce5d1e45b7a5..fbf9c335cf7b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -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() &&

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.

> +	    (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;
> -- 
> 2.17.2
> 

Re: [PATCH v6 5/8] KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest

[Yang Weijiang]

On Mon, Aug 12, 2019 at 04:56:32PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:43AM +0800, Yang Weijiang wrote:
> > "Load Guest CET state" bit controls whether Guest CET states
> > will be loaded at Guest entry. Before doing that, KVM needs
> > to check if CPU CET feature is 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 <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 | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index ce5d1e45b7a5..fbf9c335cf7b 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -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() &&
> 
> 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.

> > +	    (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;
> > -- 
> > 2.17.2
> > 

[PATCH v6 6/8] KVM: x86: Add CET bits setting in CR4 and XSS

[Yang Weijiang]

CR4.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.

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/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(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3731ac37119a..d6c9e44d0115 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index d96269a6bcf5..5d740d5a1148 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -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
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fbf9c335cf7b..123285177c6b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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,
-- 
2.17.2

[PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES

[Yang Weijiang]

From: Sean Christopherson <sean.j.christopherson@intel.com>

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.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Co-developed-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fafd81d2c9ea..c657e6a56527 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
 }
-- 
2.17.2

Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES

[Sean Christopherson]

On Thu, Jul 25, 2019 at 11:12:45AM +0800, Yang Weijiang wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> 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.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Yang Weijiang <weijiang.yang@intel.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fafd81d2c9ea..c657e6a56527 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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;

Dunno if the compiler will actually generate different code, but this can be a
const.

> +	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.

> +				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'.

> +
> +			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;	
	}


> +				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;
>  }
> -- 
> 2.17.2
> 

Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES

[Sean Christopherson]

On Mon, Aug 12, 2019 at 04:02:03PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:45AM +0800, Yang Weijiang wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > 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.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Yang Weijiang <weijiang.yang@intel.com>
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index fafd81d2c9ea..c657e6a56527 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -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;
> 
> Dunno if the compiler will actually generate different code, but this can be a
> const.
> 
> > +	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.
> 
> > +				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'.
> 
> > +
> > +			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);

Oh, and this should use kvm_supported_xss(), which masks ->supported_xss()
with KVM_SUPPORTED_XSS .

> 
> 	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;	
> 	}
> 
> 
> > +				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;
> >  }
> > -- 
> > 2.17.2
> > 

Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES

[Sean Christopherson]

On Mon, Aug 12, 2019 at 04:02:03PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:45AM +0800, Yang Weijiang wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > 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.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Yang Weijiang <weijiang.yang@intel.com>
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index fafd81d2c9ea..c657e6a56527 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -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;
> 
> Dunno if the compiler will actually generate different code, but this can be a
> const.
> 
> > +	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.
> 
> > +				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'.
> 
> > +
> > +			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;	
> 	}

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.

> 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;	
> 	}
> 
> 
> > +				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;
> >  }
> > -- 
> > 2.17.2
> > 

Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES

[Yang Weijiang]

On Mon, Aug 12, 2019 at 04:29:21PM -0700, Sean Christopherson wrote:
> On Mon, Aug 12, 2019 at 04:02:03PM -0700, Sean Christopherson wrote:
> > On Thu, Jul 25, 2019 at 11:12:45AM +0800, Yang Weijiang wrote:
> > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Co-developed-by: Yang Weijiang <weijiang.yang@intel.com>
> > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > > ---
> > >  arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++-
> > >  1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index fafd81d2c9ea..c657e6a56527 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -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;
> > 
> > Dunno if the compiler will actually generate different code, but this can be a
> > const.
> > 
> > > +	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.
> > 
> > > +				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'.
> > 
> > > +
> > > +			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;	
> > 	}
> 
> 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.
>
Sure, will follow that.
> > 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;	
> > 	}
> > 
> > 
> > > +				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;
> > >  }
> > > -- 
> > > 2.17.2
> > > 

Re: [PATCH v6 7/8] KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES

[Yang Weijiang]

On Mon, Aug 12, 2019 at 04:02:03PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:45AM +0800, Yang Weijiang wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > 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.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Yang Weijiang <weijiang.yang@intel.com>
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index fafd81d2c9ea..c657e6a56527 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -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;
> 
> Dunno if the compiler will actually generate different code, but this can be a
> const.
>
OK, will add it.
> > +	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.
> 
> > +				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'.
> 
> > +
> > +			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;	
> 	}
> 
thanks, will modify the patch.
> 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;	
> 	}
> 
> 
> > +				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;
> >  }
> > -- 
> > 2.17.2
> > 

[PATCH v6 8/8] KVM: x86: Add user-space access interface for CET MSRs

[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 <weijiang.yang@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 123285177c6b..e5eacd01e984 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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))
-- 
2.17.2

Re: [PATCH v6 8/8] KVM: x86: Add user-space access interface for CET MSRs

[Sean Christopherson]

On Thu, Jul 25, 2019 at 11:12:46AM +0800, Yang Weijiang wrote:
> 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 <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 123285177c6b..e5eacd01e984 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -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;

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().

>  	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))
> -- 
> 2.17.2
> 

Re: [PATCH v6 8/8] KVM: x86: Add user-space access interface for CET MSRs

[Yang Weijiang]

On Mon, Aug 12, 2019 at 04:43:36PM -0700, Sean Christopherson wrote:
> On Thu, Jul 25, 2019 at 11:12:46AM +0800, Yang Weijiang wrote:
> > 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 <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 123285177c6b..e5eacd01e984 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -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;
> 
> 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().
>
Thanks you, will modify this part of code.

> >  	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))
> > -- 
> > 2.17.2
> >