diff mbox series

[v1,6/8] kvm:cpuid Add CPUID support for CET xsaves component query.

Message ID 20181226081532.30698-7-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series This patch-set is to enable kvm Guest OS CET support. | expand

Commit Message

Yang, Weijiang Dec. 26, 2018, 8:15 a.m. UTC
CET xsaves component size is queried through CPUID.(EAX=0xD, ECX=11)
and CPUID.(EAX=0xD, ECX=12).

Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/cpuid.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Sean Christopherson Jan. 2, 2019, 6:49 p.m. UTC | #1
On Wed, Dec 26, 2018 at 04:15:30PM +0800, Yang Weijiang wrote:
> CET xsaves component size is queried through CPUID.(EAX=0xD, ECX=11)
> and CPUID.(EAX=0xD, ECX=12).
> 
> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 7bcfa61375c0..5bac31e58955 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -565,6 +565,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	case 0xd: {
>  		int idx, i;
>  		u64 supported = kvm_supported_xcr0();
> +		u64 sv_supported;

What about u_supported and s_supported?  sv_supported doesn't make me
think "supervisor"e.

>  		entry->eax &= supported;
>  		entry->ebx = xstate_required_size(supported, false);
> @@ -584,18 +585,23 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  				entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
>  				cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
>  				entry[i].ebx = 0;
> +				sv_supported = entry[i].ecx +
> +					((u64)entry[i].edx << 32);

Use '|' instead of '+' to smush ECX and EDX together, as is it looks
like the code is calculating a size or something.

>  				if (entry[i].eax & (F(XSAVES)|F(XSAVEC)))
>  					entry[i].ebx =
>  						xstate_required_size(supported,
>  								     true);
> -			} else {
> +			} else if (!(entry[i].ecx & 1)) {

Now that we're actually consuming bit 0, it'd be nice to formally define
it as referring to supervisor state.

What about styling the code like this?  Might make it more obvious that
the logic for user vs. supervisor is identical, i.e. ECX bit 0 only
determines which mask is applied.

			if (idx == 1) {
				...
			} else {
				supported = (entry[i].ecx & 1) ? s_supported :
								 u_supported;
				if (entry[i].eax == 0 || !(supported & mask))
					continue;
				entry[i].ecx &= 1;
				entry[i].edx = 0;
			}


>  				if (entry[i].eax == 0 || !(supported & mask))
>  					continue;
> -				if (WARN_ON_ONCE(entry[i].ecx & 1))
> +				entry[i].ecx = 0;
> +				entry[i].edx = 0;
> +			} else {
> +				if (entry[i].eax == 0 || !(sv_supported & mask))
>  					continue;
> +				entry[i].ecx = 1;
> +				entry[i].edx = 0;
>  			}
> -			entry[i].ecx = 0;
> -			entry[i].edx = 0;

Removing this entirely isn't correct for CPUID.0xD.0x1, KVM should still
zero out the bits it doesn't handle, e.g. KVM will signal a fault if the
guest attempts to set any bits in IA32_XSS other than the CET bits.

>  			entry[i].flags |=
>  			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>  			++*nent;
> -- 
> 2.17.1
>
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 7bcfa61375c0..5bac31e58955 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -565,6 +565,7 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0xd: {
 		int idx, i;
 		u64 supported = kvm_supported_xcr0();
+		u64 sv_supported;
 
 		entry->eax &= supported;
 		entry->ebx = xstate_required_size(supported, false);
@@ -584,18 +585,23 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 				entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
 				cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
 				entry[i].ebx = 0;
+				sv_supported = entry[i].ecx +
+					((u64)entry[i].edx << 32);
 				if (entry[i].eax & (F(XSAVES)|F(XSAVEC)))
 					entry[i].ebx =
 						xstate_required_size(supported,
 								     true);
-			} else {
+			} else if (!(entry[i].ecx & 1)) {
 				if (entry[i].eax == 0 || !(supported & mask))
 					continue;
-				if (WARN_ON_ONCE(entry[i].ecx & 1))
+				entry[i].ecx = 0;
+				entry[i].edx = 0;
+			} else {
+				if (entry[i].eax == 0 || !(sv_supported & mask))
 					continue;
+				entry[i].ecx = 1;
+				entry[i].edx = 0;
 			}
-			entry[i].ecx = 0;
-			entry[i].edx = 0;
 			entry[i].flags |=
 			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 			++*nent;