[CFT PATCH 1/2] kvm: x86: mask out XSAVES

[CFT PATCH 1/2] kvm: x86: mask out XSAVES

[Paolo Bonzini]

This feature is not supported inside KVM guests yet, because we do not emulate
MSR_IA32_XSS.  Mask it out.

Cc: stable@vger.kernel.org
Cc: Nadav Amit <namit@cs.technion.ac.il>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 20d83217fb1d..a4f5ac46226c 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -320,6 +320,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		F(ADX) | F(SMAP) | F(AVX512F) | F(AVX512PF) | F(AVX512ER) |
 		F(AVX512CD);
 
+	/* cpuid 0xD.1.eax */
+	const u32 kvm_supported_word10_x86_features =
+		F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1);
+
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 
@@ -456,13 +460,18 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->eax &= supported;
 		entry->edx &= supported >> 32;
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+		if (!supported)
+			break;
+
 		for (idx = 1, i = 1; idx < 64; ++idx) {
 			u64 mask = ((u64)1 << idx);
 			if (*nent >= maxnent)
 				goto out;
 
 			do_cpuid_1_ent(&entry[i], function, idx);
-			if (entry[i].eax == 0 || !(supported & mask))
+			if (idx == 1)
+				entry[i].eax &= kvm_supported_word10_x86_features;
+			else if (entry[i].eax == 0 || !(supported & mask))
 				continue;
 			entry[i].flags |=
 			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-- 
1.8.3.1

[CFT PATCH 2/2] KVM: x86: support XSAVES usage in the host

[Paolo Bonzini]

Userspace is expecting non-compacted format for KVM_GET_XSAVE, but
struct xsave_struct might be using the compacted format.  Convert
in order to preserve userspace ABI.

Fixes: f31a9f7c71691569359fa7fb8b0acaa44bce0324
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: stable@vger.kernel.org
Cc: Nadav Amit <namit@cs.technion.ac.il>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5337039427c8..7e8a20e5615a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3131,15 +3131,53 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+#define XSTATE_COMPACTION_ENABLED (1ULL << 63)
+
+static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
+{
+	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
+	u64 xstate_bv = vcpu->arch.guest_supported_xcr0 | XSTATE_FPSSE;
+	u64 valid;
+
+	/*
+	 * Copy legacy XSAVE area, to avoid complications with CPUID
+	 * leaves 0 and 1 in the loop below.
+	 */
+	memcpy(dest, xsave, XSAVE_HDR_OFFSET);
+
+	/* Set XSTATE_BV */
+	*(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv;
+
+	/*
+	 * Copy each region from the possibly compacted offset to the
+	 * non-compacted offset.
+	 */
+	valid = xstate_bv & ~XSTATE_FPSSE;
+	if (xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED)
+		valid &= xsave->xsave_hdr.xcomp_bv;
+
+	while (valid) {
+		u64 feature = valid & -valid;
+		int index = fls64(feature) - 1;
+		void *src = get_xsave_addr(xsave, feature);
+
+	        if (src) {
+			u32 size, offset, ecx, edx;
+			cpuid_count(XSTATE_CPUID, index,
+				    &size, &offset, &ecx, &edx);
+	                memcpy(dest + offset, src, size);
+		}
+
+		valid -= feature;
+	}
+}
+
 static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
 					 struct kvm_xsave *guest_xsave)
 {
 	if (cpu_has_xsave) {
-		memcpy(guest_xsave->region,
-			&vcpu->arch.guest_fpu.state->xsave,
-			vcpu->arch.guest_xstate_size);
-		*(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] &=
-			vcpu->arch.guest_supported_xcr0 | XSTATE_FPSSE;
+		memset(guest_xsave, 0, sizeof(struct kvm_xsave));
+		fill_xsave((u8 *) guest_xsave->region, vcpu);
 	} else {
 		memcpy(guest_xsave->region,
 			&vcpu->arch.guest_fpu.state->fxsave,
-- 
1.8.3.1

Re: [CFT PATCH 2/2] KVM: x86: support XSAVES usage in the host

[Andy Lutomirski]

On 11/21/2014 10:31 AM, Paolo Bonzini wrote:
> Userspace is expecting non-compacted format for KVM_GET_XSAVE, but
> struct xsave_struct might be using the compacted format.  Convert
> in order to preserve userspace ABI.
> 
> Fixes: f31a9f7c71691569359fa7fb8b0acaa44bce0324
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: stable@vger.kernel.org
> Cc: Nadav Amit <namit@cs.technion.ac.il>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5337039427c8..7e8a20e5615a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3131,15 +3131,53 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +#define XSTATE_COMPACTION_ENABLED (1ULL << 63)
> +
> +static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
> +{
> +	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
> +	u64 xstate_bv = vcpu->arch.guest_supported_xcr0 | XSTATE_FPSSE;
> +	u64 valid;
> +
> +	/*
> +	 * Copy legacy XSAVE area, to avoid complications with CPUID
> +	 * leaves 0 and 1 in the loop below.
> +	 */
> +	memcpy(dest, xsave, XSAVE_HDR_OFFSET);
> +
> +	/* Set XSTATE_BV */
> +	*(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv;
> +
> +	/*
> +	 * Copy each region from the possibly compacted offset to the
> +	 * non-compacted offset.
> +	 */
> +	valid = xstate_bv & ~XSTATE_FPSSE;
> +	if (xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED)
> +		valid &= xsave->xsave_hdr.xcomp_bv;
> +
> +	while (valid) {
> +		u64 feature = valid & -valid;
> +		int index = fls64(feature) - 1;
> +		void *src = get_xsave_addr(xsave, feature);
> +
> +	        if (src) {
> +			u32 size, offset, ecx, edx;
> +			cpuid_count(XSTATE_CPUID, index,
> +				    &size, &offset, &ecx, &edx);
> +	                memcpy(dest + offset, src, size);

Is this really the best way to do this?  cpuid is serializing, so this
is possibly *very* slow.

--Andy

Re: [CFT PATCH 2/2] KVM: x86: support XSAVES usage in the host

[Paolo Bonzini]

On 21/11/2014 21:06, Andy Lutomirski wrote:
>> > +			cpuid_count(XSTATE_CPUID, index,
>> > +				    &size, &offset, &ecx, &edx);
>> > +	                memcpy(dest + offset, src, size);
> Is this really the best way to do this?  cpuid is serializing, so this
> is possibly *very* slow.

The data is in arch/x86/kernel/xsave.c, but it is not exported.  But
this is absolutely not a hotspot.

Paolo

Re: [CFT PATCH 2/2] KVM: x86: support XSAVES usage in the host

[Wanpeng Li]

Hi Paolo,
On Fri, Nov 21, 2014 at 07:31:18PM +0100, Paolo Bonzini wrote:
[...]
>+		u64 feature = valid & -valid;
>+		int index = fls64(feature) - 1;
>+		void *src = get_xsave_addr(xsave, feature);
>+
>+	        if (src) {
>+			u32 size, offset, ecx, edx;
>+			cpuid_count(XSTATE_CPUID, index,
>+				    &size, &offset, &ecx, &edx);
>+	                memcpy(dest + offset, src, size);

The offset you get is still for compact format, so you almost convert compat 
format to compat format instead of convert compact format to standard format. 
In addition, I think convert standard format to compact format should be 
implemented in put path.

Regards,
Wanpeng Li 

Re: [CFT PATCH 2/2] KVM: x86: support XSAVES usage in the host

[Paolo Bonzini]

On 24/11/2014 03:10, Wanpeng Li wrote:
> Hi Paolo,
> On Fri, Nov 21, 2014 at 07:31:18PM +0100, Paolo Bonzini wrote:
> [...]
>> +		u64 feature = valid & -valid;
>> +		int index = fls64(feature) - 1;
>> +		void *src = get_xsave_addr(xsave, feature);
>> +
>> +	        if (src) {
>> +			u32 size, offset, ecx, edx;
>> +			cpuid_count(XSTATE_CPUID, index,
>> +				    &size, &offset, &ecx, &edx);
>> +	                memcpy(dest + offset, src, size);
> 
> The offset you get is still for compact format

No, it's not, or all old software using XSAVE/XRSTOR would be broken.

The code in arch/x86/kernel/xsave.c agrees with me; compacted offsets
(xsave_comp_offsets) are computed by summing sizes, while non-compacted
offsets (xsave_offsets) come for CPUID.

> , so you almost convert compat 
> format to compat format instead of convert compact format to standard format. 
> In addition, I think convert standard format to compact format should be 
> implemented in put path.

If I do that, userspace is broken because it expects standard format.
Hence, passing XSAVE data to userspace in compact format can be done,
but has to be guarded by an explicitly enabled capability (using
KVM_ENABLE_CAP).

I do not think that's useful, since no supervisor-specific states are
defined yet, and anyway they can be passed using KVM_GET/SET_MSR because
this is not a fast path.

Paolo