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

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

[Paolo Bonzini]

The first patch ensures that XSAVES is not exposed in the guest until
we emulate MSR_IA32_XSS.  The second exports XSAVE data in the correct
format.

I tested these on a non-XSAVES system so they should not be completely
broken, but I need some help.  I am not even sure which XSAVE states
are _not_ enabled, and thus compacted, in Linux.

Note that these patches do not add support for XSAVES in the guest yet,
since MSR_IA32_XSS is not emulated.

If they fix the bug Nadav reported, I'll add Reported-by and commit.

Thanks,

Paolo

v1->v2: also adjust KVM_SET_XSAVE

Paolo Bonzini (2):
  kvm: x86: mask out XSAVES
  KVM: x86: support XSAVES usage in the host

 arch/x86/kvm/cpuid.c | 11 ++++++-
 arch/x86/kvm/x86.c   | 87 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 90 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[CFT PATCH v2 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: 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 v2 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.

Likewise, userspace is passing non-compacted format for KVM_SET_XSAVE
but the kernel will pass it to XRSTORS, and we need to convert back.

Fixes: f31a9f7c71691569359fa7fb8b0acaa44bce0324
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Nadav Amit <namit@cs.technion.ac.il>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 80 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 08b5657e57ed..373b0ab9a32e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3132,15 +3132,89 @@ 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;
+	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 load_xsave(struct kvm_vcpu *vcpu, u8 *src)
+{
+	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
+	u64 xstate_bv = *(u64 *)(src + XSAVE_HDR_OFFSET);
+	u64 valid;
+
+	/*
+	 * Copy legacy XSAVE area, to avoid complications with CPUID
+	 * leaves 0 and 1 in the loop below.
+	 */
+	memcpy(xsave, src, XSAVE_HDR_OFFSET);
+
+	/* Set XSTATE_BV and possibly XCOMP_BV.  */
+	xsave->xsave_hdr.xstate_bv = xstate_bv;
+	if (cpu_has_xsaves)
+		xsave->xsave_hdr.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
+
+	/*
+	 * Copy each region from the non-compacted offset to the
+	 * possibly compacted offset.
+	 */
+	valid = xstate_bv & ~XSTATE_FPSSE;
+	while (valid) {
+		u64 feature = valid & -valid;
+		int index = fls64(feature) - 1;
+		void *dest = get_xsave_addr(xsave, feature);
+
+		if (dest) {
+			u32 size, offset, ecx, edx;
+			cpuid_count(XSTATE_CPUID, index,
+				    &size, &offset, &ecx, &edx);
+			memcpy(dest, src + offset, size);
+		} else
+			WARN_ON_ONCE(1);
+
+		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,
@@ -3164,8 +3238,7 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
 		 */
 		if (xstate_bv & ~kvm_supported_xcr0())
 			return -EINVAL;
-		memcpy(&vcpu->arch.guest_fpu.state->xsave,
-			guest_xsave->region, vcpu->arch.guest_xstate_size);
+		load_xsave(vcpu, (u8 *)guest_xsave->region);
 	} else {
 		if (xstate_bv & ~XSTATE_FPSSE)
 			return -EINVAL;
-- 
1.8.3.1

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

[Wanpeng Li]

Hi Paolo,
On Mon, Nov 24, 2014 at 05:43:32PM +0100, Paolo Bonzini wrote:
>The first patch ensures that XSAVES is not exposed in the guest until
>we emulate MSR_IA32_XSS.  The second exports XSAVE data in the correct
>format.
>
>I tested these on a non-XSAVES system so they should not be completely
>broken, but I need some help.  I am not even sure which XSAVE states
>are _not_ enabled, and thus compacted, in Linux.
>
>Note that these patches do not add support for XSAVES in the guest yet,
>since MSR_IA32_XSS is not emulated.
>
>If they fix the bug Nadav reported, I'll add Reported-by and commit.

I test this patchset w/ your "KVM: x86: export get_xsave_addr" patch on 
Skylake and guest hang during boot. The guest screen show "Probing EDD 
(edd=off to disable)... ok", and no more dump.

Regards,
Wanpeng Li 

>
>Thanks,
>
>Paolo
>
>v1->v2: also adjust KVM_SET_XSAVE
>
>Paolo Bonzini (2):
>  kvm: x86: mask out XSAVES
>  KVM: x86: support XSAVES usage in the host
>
> arch/x86/kvm/cpuid.c | 11 ++++++-
> arch/x86/kvm/x86.c   | 87 +++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 90 insertions(+), 8 deletions(-)
>
>-- 
>1.8.3.1

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

[Paolo Bonzini]

On 25/11/2014 11:13, Wanpeng Li wrote:
> Hi Paolo,
> On Mon, Nov 24, 2014 at 05:43:32PM +0100, Paolo Bonzini wrote:
>> The first patch ensures that XSAVES is not exposed in the guest until
>> we emulate MSR_IA32_XSS.  The second exports XSAVE data in the correct
>> format.
>>
>> I tested these on a non-XSAVES system so they should not be completely
>> broken, but I need some help.  I am not even sure which XSAVE states
>> are _not_ enabled, and thus compacted, in Linux.
>>
>> Note that these patches do not add support for XSAVES in the guest yet,
>> since MSR_IA32_XSS is not emulated.
>>
>> If they fix the bug Nadav reported, I'll add Reported-by and commit.
> 
> I test this patchset w/ your "KVM: x86: export get_xsave_addr" patch on 
> Skylake and guest hang during boot. The guest screen show "Probing EDD 
> (edd=off to disable)... ok", and no more dump.

Anything in dmesg?  Can you try this patch on top?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 373b0ab9a32e..ca26681455c2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
 		return err;

 	fpu_finit(&vcpu->arch.guest_fpu);
+	if (cpu_has_xsaves)
+		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
+			host_xcr0 | XSTATE_COMPACTION_ENABLED;

 	/*
 	 * Ensure guest xcr0 is valid for loading

Paolo

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

[Nadav Amit]

> On Nov 25, 2014, at 12:36, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 25/11/2014 11:13, Wanpeng Li wrote:
>> Hi Paolo,
>> On Mon, Nov 24, 2014 at 05:43:32PM +0100, Paolo Bonzini wrote:
>>> The first patch ensures that XSAVES is not exposed in the guest until
>>> we emulate MSR_IA32_XSS.  The second exports XSAVE data in the correct
>>> format.
>>> 
>>> I tested these on a non-XSAVES system so they should not be completely
>>> broken, but I need some help.  I am not even sure which XSAVE states
>>> are _not_ enabled, and thus compacted, in Linux.
>>> 
>>> Note that these patches do not add support for XSAVES in the guest yet,
>>> since MSR_IA32_XSS is not emulated.
>>> 
>>> If they fix the bug Nadav reported, I'll add Reported-by and commit.
>> 
>> I test this patchset w/ your "KVM: x86: export get_xsave_addr" patch on 
>> Skylake and guest hang during boot. The guest screen show "Probing EDD 
>> (edd=off to disable)... ok", and no more dump.
> 
> Anything in dmesg?  Can you try this patch on top?
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 373b0ab9a32e..ca26681455c2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
> 		return err;
> 
> 	fpu_finit(&vcpu->arch.guest_fpu);
> +	if (cpu_has_xsaves)
> +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
> +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
> 
> 	/*
> 	 * Ensure guest xcr0 is valid for loading
> 

The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I did not need to apply this patch on top. [although I am not sure whether relying on userspace to call KVM_SET_XSAVE early enough is a good practice].

One disclaimer: Since I got limited time with the machine, I executed a slightly modified kernel/qemu, and not the latest version.
Anyhow, I don’t think these differences can have any impact.

Regards,
Nadav

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

[Paolo Bonzini]

On 25/11/2014 15:05, Nadav Amit wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 373b0ab9a32e..ca26681455c2 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
> > 		return err;
> > 
> > 	fpu_finit(&vcpu->arch.guest_fpu);
> > +	if (cpu_has_xsaves)
> > +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
> > +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
> > 
> > 	/*
> > 	 * Ensure guest xcr0 is valid for loading
> 
> The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I
> did not need to apply this patch on top. [although I am not sure whether
> relying on userspace to call KVM_SET_XSAVE early enough is a good practice].

Did you actually try the patch? :)  If it works, I'm tempted to apply it
anyway.

> One disclaimer: Since I got limited time with the machine, I executed
> a slightly modified kernel/qemu, and not the latest version.
> Anyhow, I don’t think these differences can have any impact.

Yes, that is no problem.

Paolo

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

[Nadav Amit]

> On Nov 25, 2014, at 16:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 25/11/2014 15:05, Nadav Amit wrote:
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 373b0ab9a32e..ca26681455c2 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
>>> 		return err;
>>> 
>>> 	fpu_finit(&vcpu->arch.guest_fpu);
>>> +	if (cpu_has_xsaves)
>>> +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
>>> +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
>>> 
>>> 	/*
>>> 	 * Ensure guest xcr0 is valid for loading
>> 
>> The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I
>> did not need to apply this patch on top. [although I am not sure whether
>> relying on userspace to call KVM_SET_XSAVE early enough is a good practice].
> 
> Did you actually try the patch? :)  If it works, I'm tempted to apply it
> anyway.
Yes, I tried it both with and without this patch.
Due to time constraints I only tested minimal functionality (Linux boot).
I will run more tests in the near future. Anyhow, you can put the:

Tested-by: Nadav Amit <namit@cs.technion.ac.il>

> 
>> One disclaimer: Since I got limited time with the machine, I executed
>> a slightly modified kernel/qemu, and not the latest version.
>> Anyhow, I don’t think these differences can have any impact.
> 
> Yes, that is no problem.

I am just worried that Wanpeng reported it fails, while I report it works...

Nadav

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

[Wanpeng Li]

Hi all,
On Tue, Nov 25, 2014 at 04:50:06PM +0200, Nadav Amit wrote:
>
>> On Nov 25, 2014, at 16:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 
>> 
>> 
>> On 25/11/2014 15:05, Nadav Amit wrote:
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 373b0ab9a32e..ca26681455c2 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
>>>> 		return err;
>>>> 
>>>> 	fpu_finit(&vcpu->arch.guest_fpu);
>>>> +	if (cpu_has_xsaves)
>>>> +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
>>>> +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
>>>> 
>>>> 	/*
>>>> 	 * Ensure guest xcr0 is valid for loading
>>> 
>>> The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I

Could you try 3.17 guest which has xsaves enabled? Because I'm not sure if 
the below codes from Paolo is enough to mask XSAVES, should we also add 
F(XSAVES)?

+       const u32 kvm_supported_word10_x86_features =
+               F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1);
+

In addition, the 3.17 guest is still hang as I mentioned even if I add the 
F(XSAVES) to the kvm_supported_word10_x86_features.

>>> did not need to apply this patch on top. [although I am not sure whether
>>> relying on userspace to call KVM_SET_XSAVE early enough is a good practice].
>> 
>> Did you actually try the patch? :)  If it works, I'm tempted to apply it
>> anyway.
>Yes, I tried it both with and without this patch.
>Due to time constraints I only tested minimal functionality (Linux boot).
>I will run more tests in the near future. Anyhow, you can put the:
>
>Tested-by: Nadav Amit <namit@cs.technion.ac.il>
>
>> 
>>> One disclaimer: Since I got limited time with the machine, I executed
>>> a slightly modified kernel/qemu, and not the latest version.
>>> Anyhow, I don’t think these differences can have any impact.
>> 
>> Yes, that is no problem.
>
>I am just worried that Wanpeng reported it fails, while I report it works...
>

I have another patch which enable xsaves in KVM and the patch is still
under debug with Paolo's patch "KVM: x86: support XSAVES usage in the host",
so the 1/2 patch from Paolo can be dropped if my patch is ready. Anyway,
a quick fix is needed before enable xsaves in kvm. 

Regards,
Wanpeng Li 

>Nadav

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

[Wanpeng Li]

Hi Nadav,
On Wed, Nov 26, 2014 at 11:00:34AM +0200, Nadav Amit wrote:
>Wanpeng Li <wanpeng.li@linux.intel.com> wrote:
>
>> Hi all,
>> On Tue, Nov 25, 2014 at 04:50:06PM +0200, Nadav Amit wrote:
>>>> On Nov 25, 2014, at 16:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 25/11/2014 15:05, Nadav Amit wrote:
>>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>>> index 373b0ab9a32e..ca26681455c2 100644
>>>>>> --- a/arch/x86/kvm/x86.c
>>>>>> +++ b/arch/x86/kvm/x86.c
>>>>>> @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
>>>>>> 		return err;
>>>>>> 
>>>>>> 	fpu_finit(&vcpu->arch.guest_fpu);
>>>>>> +	if (cpu_has_xsaves)
>>>>>> +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
>>>>>> +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
>>>>>> 
>>>>>> 	/*
>>>>>> 	 * Ensure guest xcr0 is valid for loading
>>>>> 
>>>>> The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I
>> 
>> Could you try 3.17 guest which has xsaves enabled? Because I'm not sure if 
>> the below codes from Paolo is enough to mask XSAVES, should we also add 
>> F(XSAVES)?
>I guess this paragraph is intended for me. As I said before, I got limited
>access to the machine - I will not be able to run these experiments before
>Sunday, and I am not sure that I will have time to fully debug it. If you
>insist, the very least please run some experiments running a 3.16 (or older)
>guest kernel and running 3.17 with ‘noxsaves’ kernel parameter.
>
>> +       const u32 kvm_supported_word10_x86_features =
>> +               F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1);
>> +
>> 
>> In addition, the 3.17 guest is still hang as I mentioned even if I add the 
>> F(XSAVES) to the kvm_supported_word10_x86_features.
>> 
>>>>> did not need to apply this patch on top. [although I am not sure whether
>>>>> relying on userspace to call KVM_SET_XSAVE early enough is a good practice].
>>>> 
>>>> Did you actually try the patch? :)  If it works, I'm tempted to apply it
>>>> anyway.
>>> Yes, I tried it both with and without this patch.
>>> Due to time constraints I only tested minimal functionality (Linux boot).
>>> I will run more tests in the near future. Anyhow, you can put the:
>>> 
>>> Tested-by: Nadav Amit <namit@cs.technion.ac.il>
>>> 
>>>>> One disclaimer: Since I got limited time with the machine, I executed
>>>>> a slightly modified kernel/qemu, and not the latest version.
>>>>> Anyhow, I don’t think these differences can have any impact.
>>>> 
>>>> Yes, that is no problem.
>>> 
>>> I am just worried that Wanpeng reported it fails, while I report it works...
>> 
>> I have another patch which enable xsaves in KVM and the patch is still
>> under debug with Paolo's patch "KVM: x86: support XSAVES usage in the host",
>> so the 1/2 patch from Paolo can be dropped if my patch is ready. Anyway,
>> a quick fix is needed before enable xsaves in kvm. 
>xsaves is already enabled in KVM (the host) since 3.17. It just didn’t work…
>

Not yet, it is enabled in native currrently instead of vmx and my patch
is still under debug.

Regards,
Wanpeng Li 

>Nadav

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

[Nadav Amit]

Wanpeng Li <wanpeng.li@linux.intel.com> wrote:

> Hi all,
> On Tue, Nov 25, 2014 at 04:50:06PM +0200, Nadav Amit wrote:
>>> On Nov 25, 2014, at 16:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> 
>>> 
>>> 
>>> On 25/11/2014 15:05, Nadav Amit wrote:
>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>> index 373b0ab9a32e..ca26681455c2 100644
>>>>> --- a/arch/x86/kvm/x86.c
>>>>> +++ b/arch/x86/kvm/x86.c
>>>>> @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
>>>>> 		return err;
>>>>> 
>>>>> 	fpu_finit(&vcpu->arch.guest_fpu);
>>>>> +	if (cpu_has_xsaves)
>>>>> +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
>>>>> +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
>>>>> 
>>>>> 	/*
>>>>> 	 * Ensure guest xcr0 is valid for loading
>>>> 
>>>> The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I
> 
> Could you try 3.17 guest which has xsaves enabled? Because I'm not sure if 
> the below codes from Paolo is enough to mask XSAVES, should we also add 
> F(XSAVES)?
I guess this paragraph is intended for me. As I said before, I got limited
access to the machine - I will not be able to run these experiments before
Sunday, and I am not sure that I will have time to fully debug it. If you
insist, the very least please run some experiments running a 3.16 (or older)
guest kernel and running 3.17 with ‘noxsaves’ kernel parameter.

> +       const u32 kvm_supported_word10_x86_features =
> +               F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1);
> +
> 
> In addition, the 3.17 guest is still hang as I mentioned even if I add the 
> F(XSAVES) to the kvm_supported_word10_x86_features.
> 
>>>> did not need to apply this patch on top. [although I am not sure whether
>>>> relying on userspace to call KVM_SET_XSAVE early enough is a good practice].
>>> 
>>> Did you actually try the patch? :)  If it works, I'm tempted to apply it
>>> anyway.
>> Yes, I tried it both with and without this patch.
>> Due to time constraints I only tested minimal functionality (Linux boot).
>> I will run more tests in the near future. Anyhow, you can put the:
>> 
>> Tested-by: Nadav Amit <namit@cs.technion.ac.il>
>> 
>>>> One disclaimer: Since I got limited time with the machine, I executed
>>>> a slightly modified kernel/qemu, and not the latest version.
>>>> Anyhow, I don’t think these differences can have any impact.
>>> 
>>> Yes, that is no problem.
>> 
>> I am just worried that Wanpeng reported it fails, while I report it works...
> 
> I have another patch which enable xsaves in KVM and the patch is still
> under debug with Paolo's patch "KVM: x86: support XSAVES usage in the host",
> so the 1/2 patch from Paolo can be dropped if my patch is ready. Anyway,
> a quick fix is needed before enable xsaves in kvm. 
xsaves is already enabled in KVM (the host) since 3.17. It just didn’t work…

Nadav

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

[Radim Krčmář]

2014-11-24 17:43+0100, 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.
> 
> Likewise, userspace is passing non-compacted format for KVM_SET_XSAVE
> but the kernel will pass it to XRSTORS, and we need to convert back.

Future instructions might force us to calling xsave/xrstor directly, so
we could do that even now and save the explicit conversion ...

What I mean is:  we could be using the native xsave.*/xrstor.* while in
kernel and use xsave/xrstor for communication with userspace.
Hardware would take care of everything in the conversion.

get_xsave = native_xrstor(guest_xsave);  xsave(aligned_userspace_buffer)
set_xsave = xrstor(aligned_userspace_buffer);  native_xsave(guest_xsave)

Could that work?

> Fixes: f31a9f7c71691569359fa7fb8b0acaa44bce0324
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Nadav Amit <namit@cs.technion.ac.il>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 08b5657e57ed..373b0ab9a32e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3132,15 +3132,89 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +#define XSTATE_COMPACTION_ENABLED (1ULL << 63)

(arch/x86/include/asm/xsave.h)

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

(I don't think this is necessary.  We haven't modified it before and
 userspace worked, so we can save explicit copying of initialized data.)

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

(Yeah, there is an exception for SSE;  I don't see any effect it has on
 restore though, so we could probably ignore it as well.)

> +
> +	/* 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;

(We could read xstate_bv from xsave and & it with supported.)

> +	while (valid) {
> +		u64 feature = valid & -valid;
> +		int index = fls64(feature) - 1;
> +		void *src = get_xsave_addr(xsave, feature);

(xcomp_bv never changes, so it works for compacted xsave.)

> +
> +		if (src) {
> +			u32 size, offset, ecx, edx;
> +			cpuid_count(XSTATE_CPUID, index,
> +				    &size, &offset, &ecx, &edx);

(ok, setup_xstate_features() has the same code.)

> +			memcpy(dest + offset, src, size);
> +		}
> +
> +		valid -= feature;
> +	}
> +}
> +
> +static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
> +{
> +	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
> +	u64 xstate_bv = *(u64 *)(src + XSAVE_HDR_OFFSET);
> +	u64 valid;
> +
> +	/*
> +	 * Copy legacy XSAVE area, to avoid complications with CPUID
> +	 * leaves 0 and 1 in the loop below.
> +	 */
> +	memcpy(xsave, src, XSAVE_HDR_OFFSET);
> +
> +	/* Set XSTATE_BV and possibly XCOMP_BV.  */
> +	xsave->xsave_hdr.xstate_bv = xstate_bv;
> +	if (cpu_has_xsaves)
> +		xsave->xsave_hdr.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;

Userspace can trigger a #GP if it passes xstate_bv bit that isn't in
xcomp_bv, so we could & them back into xstate_bv as well.

(Linux probably won't start using IA32_XSS, so using just xcr0 is fine.)

> +
> +	/*
> +	 * Copy each region from the non-compacted offset to the
> +	 * possibly compacted offset.
> +	 */
> +	valid = xstate_bv & ~XSTATE_FPSSE;
> +	while (valid) {
> +		u64 feature = valid & -valid;
> +		int index = fls64(feature) - 1;
> +		void *dest = get_xsave_addr(xsave, feature);
> +
> +		if (dest) {
> +			u32 size, offset, ecx, edx;
> +			cpuid_count(XSTATE_CPUID, index,
> +				    &size, &offset, &ecx, &edx);
> +			memcpy(dest, src + offset, size);
> +		} else
> +			WARN_ON_ONCE(1);
> +
> +		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,
> @@ -3164,8 +3238,7 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>  		 */
>  		if (xstate_bv & ~kvm_supported_xcr0())
>  			return -EINVAL;
> -		memcpy(&vcpu->arch.guest_fpu.state->xsave,
> -			guest_xsave->region, vcpu->arch.guest_xstate_size);
> +		load_xsave(vcpu, (u8 *)guest_xsave->region);
>  	} else {
>  		if (xstate_bv & ~XSTATE_FPSSE)
>  			return -EINVAL;

Likely works,

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

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

[Paolo Bonzini]

On 26/11/2014 02:24, Wanpeng Li wrote:
> Hi all,
> On Tue, Nov 25, 2014 at 04:50:06PM +0200, Nadav Amit wrote:
>>
>>> On Nov 25, 2014, at 16:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>>
>>> On 25/11/2014 15:05, Nadav Amit wrote:
>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>> index 373b0ab9a32e..ca26681455c2 100644
>>>>> --- a/arch/x86/kvm/x86.c
>>>>> +++ b/arch/x86/kvm/x86.c
>>>>> @@ -6955,6 +6955,9 @@ int fx_init(struct kvm_vcpu *vcpu)
>>>>> 		return err;
>>>>>
>>>>> 	fpu_finit(&vcpu->arch.guest_fpu);
>>>>> +	if (cpu_has_xsaves)
>>>>> +		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
>>>>> +			host_xcr0 | XSTATE_COMPACTION_ENABLED;
>>>>>
>>>>> 	/*
>>>>> 	 * Ensure guest xcr0 is valid for loading
>>>>
>>>> The second version works for me (w/qemu v2.1.0; Linux 3.13 guest). I
> 
> Could you try 3.17 guest which has xsaves enabled? Because I'm not sure if 
> the below codes from Paolo is enough to mask XSAVES, should we also add 
> F(XSAVES)?
> 
> +       const u32 kvm_supported_word10_x86_features =
> +               F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1);
> +
> 
> In addition, the 3.17 guest is still hang as I mentioned even if I add the 
> F(XSAVES) to the kvm_supported_word10_x86_features.

Note that the point is _hiding_ XSAVES while leaving
XSAVEOPT/XSAVEC/XGETBV1 enabled.  This series only fixes the problems
with xsaves in the host, it doesn't try making XSAVES work in the guest.

> I have another patch which enable xsaves in KVM and the patch is still
> under debug with Paolo's patch "KVM: x86: support XSAVES usage in the host",
> so the 1/2 patch from Paolo can be dropped if my patch is ready. Anyway,
> a quick fix is needed before enable xsaves in kvm. 

Please add XSAVES to kvm_supported_word10_x86_features in your patches
instead.  We still need to mask any (not yet defined) feature in bit 4
and above of CPUID[EAX=0xd,ECX].EAX.

Paolo

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

[Paolo Bonzini]

On 26/11/2014 13:07, Radim Krčmář wrote:
> 2014-11-24 17:43+0100, 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.
>>
>> Likewise, userspace is passing non-compacted format for KVM_SET_XSAVE
>> but the kernel will pass it to XRSTORS, and we need to convert back.
> 
> Future instructions might force us to calling xsave/xrstor directly, so
> we could do that even now and save the explicit conversion ...
> 
> What I mean is:  we could be using the native xsave.*/xrstor.* while in
> kernel and use xsave/xrstor for communication with userspace.
> Hardware would take care of everything in the conversion.
> 
> get_xsave = native_xrstor(guest_xsave);  xsave(aligned_userspace_buffer)
> set_xsave = xrstor(aligned_userspace_buffer);  native_xsave(guest_xsave)
> 
> Could that work?

It could, though it is more like

   get_fpu()
   native_xrstor(guest_xsave)
   xsave(buffer)
   put_fpu()

and vice versa.  Also, the userspace buffer is mos likely not aligned,
so you need some kind of bounce buffer.  It can be done if the CPUID
turns out to be a bottleneck, apart from that it'd most likely be slower.

Paolo

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

[Radim Krčmář]

2014-11-26 14:13+0100, Paolo Bonzini:
> On 26/11/2014 13:07, Radim Krčmář wrote:
> > 2014-11-24 17:43+0100, 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.
> >>
> >> Likewise, userspace is passing non-compacted format for KVM_SET_XSAVE
> >> but the kernel will pass it to XRSTORS, and we need to convert back.
> > 
> > Future instructions might force us to calling xsave/xrstor directly, so
> > we could do that even now and save the explicit conversion ...
> > 
> > What I mean is:  we could be using the native xsave.*/xrstor.* while in
> > kernel and use xsave/xrstor for communication with userspace.
> > Hardware would take care of everything in the conversion.
> > 
> > get_xsave = native_xrstor(guest_xsave);  xsave(aligned_userspace_buffer)
> > set_xsave = xrstor(aligned_userspace_buffer);  native_xsave(guest_xsave)
> > 
> > Could that work?
> 
> It could, though it is more like
> 
>    get_fpu()
>    native_xrstor(guest_xsave)
>    xsave(buffer)
>    put_fpu()
> 
> and vice versa.  Also, the userspace buffer is mos likely not aligned,
> so you need some kind of bounce buffer.  It can be done if the CPUID
> turns out to be a bottleneck, apart from that it'd most likely be slower.

Yeah, it was mostly making this code more future-proof ... it is easier
to convince xsave.h to export its structures if CPUID is the problem.
(I still see some hope for Linux, so performance isn't my primary goal.)

I'm quite interested in CPUID now though, so I'll try to benchmark it,
someday.

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

[Paolo Bonzini]

On 26/11/2014 14:53, Radim Krčmář wrote:
>>> > > get_xsave = native_xrstor(guest_xsave);  xsave(aligned_userspace_buffer)
>>> > > set_xsave = xrstor(aligned_userspace_buffer);  native_xsave(guest_xsave)
>>> > > 
>>> > > Could that work?
>> > 
>> > It could, though it is more like
>> > 
>> >    get_fpu()
>> >    native_xrstor(guest_xsave)
>> >    xsave(buffer)
>> >    put_fpu()
>> > 
>> > and vice versa.  Also, the userspace buffer is mos likely not aligned,
>> > so you need some kind of bounce buffer.  It can be done if the CPUID
>> > turns out to be a bottleneck, apart from that it'd most likely be slower.
> Yeah, it was mostly making this code more future-proof ... it is easier
> to convince xsave.h to export its structures if CPUID is the problem.
> (I still see some hope for Linux, so performance isn't my primary goal.)
> 
> I'm quite interested in CPUID now though, so I'll try to benchmark it,
> someday.

I'm not sure what is more future proof. :)  I wonder if native_xrstor
could be a problem the day XRSTORS actually sets/restores MSRs as the
processor documentation promises.  We do not need that to pass them to
userspace via KVM_GET/SET_XSAVE because we have KVM_GET/SET_MSR for
that, but it may cause problems if get_xsave uses XRSTORS and thus sets
the MSRs to unanticipated values.  Difficult to say without more
information on Intel's plans.

Paolo

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

[Radim Krčmář]

2014-11-26 14:57+0100, Paolo Bonzini:
> 
> 
> On 26/11/2014 14:53, Radim Krčmář wrote:
> >>> > > get_xsave = native_xrstor(guest_xsave);  xsave(aligned_userspace_buffer)
> >>> > > set_xsave = xrstor(aligned_userspace_buffer);  native_xsave(guest_xsave)
> >>> > > 
> >>> > > Could that work?
> >> > 
> >> > It could, though it is more like
> >> > 
> >> >    get_fpu()
> >> >    native_xrstor(guest_xsave)
> >> >    xsave(buffer)
> >> >    put_fpu()
> >> > 
> >> > and vice versa.  Also, the userspace buffer is mos likely not aligned,
> >> > so you need some kind of bounce buffer.  It can be done if the CPUID
> >> > turns out to be a bottleneck, apart from that it'd most likely be slower.
> > Yeah, it was mostly making this code more future-proof ... it is easier
> > to convince xsave.h to export its structures if CPUID is the problem.
> > (I still see some hope for Linux, so performance isn't my primary goal.)
> > 
> > I'm quite interested in CPUID now though, so I'll try to benchmark it,
> > someday.

(Sorry, I don't fully understand your thoughts and I just talk more of
 the same in those scenarios.)

> I'm not sure what is more future proof. :)  I wonder if native_xrstor
> could be a problem the day XRSTORS actually sets/restores MSRs as the
> processor documentation promises.

Isn't that a problem only for emulation?

>                                    We do not need that to pass them to
> userspace via KVM_GET/SET_XSAVE because we have KVM_GET/SET_MSR for
> that, but it may cause problems if get_xsave uses XRSTORS and thus sets
> the MSRs to unanticipated values.

KVM_GET/SET_XSAVE is defined to use the format of XSAVE/XRSTOR.
(Userspace shouldn't know how we actually store guest's state;
 KVM_GET/SET_MSR doesn't read host's state.)

XRSTORS won't affect the guest in any way, we are just going to use it
to convert the xsave, so any side-effects are going to stay in the host.
(This could break the host though.)

>                                    Difficult to say without more
> information on Intel's plans.

My main presumption is that XSAVE*->XRSTOR*->XSAVE->XRSTOR has the same
result as XSAVE->XRSTOR, because we are only interested in the state,
not in any metadata.
(If it isn't possible to combine intructions, like XSAVE after XRSTORS,
 this solution won't work.)

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

[Paolo Bonzini]

On 26/11/2014 15:42, Radim Krčmář wrote:
> 2014-11-26 14:57+0100, Paolo Bonzini:
>>
>>
>> On 26/11/2014 14:53, Radim Krčmář wrote:
>>>>>>> get_xsave = native_xrstor(guest_xsave);  xsave(aligned_userspace_buffer)
>>>>>>> set_xsave = xrstor(aligned_userspace_buffer);  native_xsave(guest_xsave)
>>>>>>>
>>>>>>> Could that work?
>>>>>
>>>>> It could, though it is more like
>>>>>
>>>>>    get_fpu()
>>>>>    native_xrstor(guest_xsave)
>>>>>    xsave(buffer)
>>>>>    put_fpu()
>>>>>
>>>>> and vice versa.  Also, the userspace buffer is mos likely not aligned,
>>>>> so you need some kind of bounce buffer.  It can be done if the CPUID
>>>>> turns out to be a bottleneck, apart from that it'd most likely be slower.
>>> Yeah, it was mostly making this code more future-proof ... it is easier
>>> to convince xsave.h to export its structures if CPUID is the problem.
>>> (I still see some hope for Linux, so performance isn't my primary goal.)
>>>
>>> I'm quite interested in CPUID now though, so I'll try to benchmark it,
>>> someday.
> 
> (Sorry, I don't fully understand your thoughts and I just talk more of
>  the same in those scenarios.)
> 
>> I'm not sure what is more future proof. :)  I wonder if native_xrstor
>> could be a problem the day XRSTORS actually sets/restores MSRs as the
>> processor documentation promises.
> 
> XRSTORS won't affect the guest in any way, we are just going to use it
> to convert the xsave, so any side-effects are going to stay in the host.
> (This could break the host though.)

Yes, that's the problem. :)

> My main presumption is that XSAVE*->XRSTOR*->XSAVE->XRSTOR has the same
> result as XSAVE->XRSTOR, because we are only interested in the state,
> not in any metadata.
> (If it isn't possible to combine intructions, like XSAVE after XRSTORS,
>  this solution won't work.)

Yes, that should be right.  But actually what KVM would do it is
XRSTOR->XSAVE*->XRSTOR*->XSAVE.  The problem here is the side effects of
doing XRSTORS far from a guest entry... though that would likely be
handled by load_guest_fpu/put_guest_fpu.

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

[Radim Krčmář]

2014-11-26 17:26+0100, Paolo Bonzini:
> On 26/11/2014 15:42, Radim Krčmář wrote:
> >> I'm not sure what is more future proof. :)  I wonder if native_xrstor
> >> could be a problem the day XRSTORS actually sets/restores MSRs as the
> >> processor documentation promises.
> > 
> > XRSTORS won't affect the guest in any way, we are just going to use it
> > to convert the xsave, so any side-effects are going to stay in the host.
> > (This could break the host though.)
> 
> Yes, that's the problem. :)

(It would be a bug in Linux's xsave API, if we were using it correctly.)

> > My main presumption is that XSAVE*->XRSTOR*->XSAVE->XRSTOR has the same
> > result as XSAVE->XRSTOR, because we are only interested in the state,
> > not in any metadata.
> > (If it isn't possible to combine intructions, like XSAVE after XRSTORS,
> >  this solution won't work.)
> 
> Yes, that should be right.  But actually what KVM would do it is
> XRSTOR->XSAVE*->XRSTOR*->XSAVE.  The problem here is the side effects of
> doing XRSTORS far from a guest entry...

The entry can be aborted after doing XRSTORS, so we are going to know if
this doesn't work :)

>                                         though that would likely be
> handled by load_guest_fpu/put_guest_fpu.

Yes, I don't see a principal difference between manipulating xsave for
vmentry and this conversion, it can be wrapped in the same way.

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

[Wanpeng Li]

Hi Paolo,
On Tue, Nov 25, 2014 at 04:50:06PM +0200, Nadav Amit wrote:
[...]
>I am just worried that Wanpeng reported it fails, while I report it works...

I just get a real skylake-client machine on hand, both the two patches from 
you can run normally, it seems that the silly emulator which I used is buggy. 
Sorry for the false report. In addition, I send out the xsaves enable patch 
for kvm, it can work w/ your patch 2/2 correctly.

Regards,
Wanpeng Li 

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

[Nadav Amit]

Paolo Bonzini <pbonzini@redhat.com> 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.
> 
> Likewise, userspace is passing non-compacted format for KVM_SET_XSAVE
> but the kernel will pass it to XRSTORS, and we need to convert back.
> 
> Fixes: f31a9f7c71691569359fa7fb8b0acaa44bce0324
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Nadav Amit <namit@cs.technion.ac.il>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/x86.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 08b5657e57ed..373b0ab9a32e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3132,15 +3132,89 @@ 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;

I have a problem with this line. I ran some experiments and it has a
side-effect of causing XINUSE (an internal register which saves which state
components are not in the initial state) to be all set. As a results,
after load_xsave runs, when the guest runs xsave instruction, initialised
xsave state components are marked as not-initialised in the guest’s
xstate_bv.

This causes both transparency issues (the VM does not behave as bare-metal
machine). In addition it may cause performance overheads, since from this
point on, xsave and xrstor instructions would save and load state which is
in fact in the initial state.

I think it is better just to replace the last line with:

*(u64 *)(dest + XSAVE_HDR_OFFSET) = xsave->xsave_hdr.xstate_bv

Thanks,
Nadav

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

[Paolo Bonzini]

On 03/12/2014 15:23, Nadav Amit wrote:
> I think it is better just to replace the last line with:
> 
> *(u64 *)(dest + XSAVE_HDR_OFFSET) = xsave->xsave_hdr.xstate_bv

Right, this matches

        u64 xstate_bv = *(u64 *)(src + XSAVE_HDR_OFFSET);
	...
        xsave->xsave_hdr.xstate_bv = xstate_bv;

in load_xsave.

Paolo

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

[Radim Krčmář]

2014-12-03 15:26+0100, Paolo Bonzini:
> 
> 
> On 03/12/2014 15:23, Nadav Amit wrote:
> > I think it is better just to replace the last line with:
> > 
> > *(u64 *)(dest + XSAVE_HDR_OFFSET) = xsave->xsave_hdr.xstate_bv

Yeah, or we can use this value for xstate_bv to save some copying too,

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 19e5e8f..ba2b7bd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3137,7 +3137,7 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 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 xstate_bv = xsave->xsave_hdr.xstate_bv;
 	u64 valid;
 
 	/*

> Right, this matches
> 
>         u64 xstate_bv = *(u64 *)(src + XSAVE_HDR_OFFSET);
> 	...
>         xsave->xsave_hdr.xstate_bv = xstate_bv;
> 
> in load_xsave.

Btw, we don't care about crashers from userspace?

---8<---
KVM: x86: prevent #GP with malicious xsave

XRSTORS throws #GP when XSTATE_BV isn't a subset of XCOMP_BV.
Make it so.

SDM: XRSTORS Exceptions
 #GP If a bit in the XCOMP_BV field in the XSAVE header is 0 and the
     corresponding bit in the XSTATE_BV field is 1.
(Also in SDM: 13.11 OPERATION OF XRSTORS)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/x86.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ca26681..19e5e8f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3184,8 +3184,10 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
 
 	/* Set XSTATE_BV and possibly XCOMP_BV.  */
 	xsave->xsave_hdr.xstate_bv = xstate_bv;
-	if (cpu_has_xsaves)
+	if (cpu_has_xsaves) {
 		xsave->xsave_hdr.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
+		xsave->xsave_hdr.xstate_bv &= xsave->xsave_hdr.xcomp_bv;
+	}
 
 	/*
 	 * Copy each region from the non-compacted offset to the
-- 
2.2.0

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

[Paolo Bonzini]

On 03/12/2014 19:45, Radim Krčmář wrote:
> 2014-12-03 15:26+0100, Paolo Bonzini:
>>
>>
>> On 03/12/2014 15:23, Nadav Amit wrote:
>>> I think it is better just to replace the last line with:
>>>
>>> *(u64 *)(dest + XSAVE_HDR_OFFSET) = xsave->xsave_hdr.xstate_bv
> 
> Yeah, or we can use this value for xstate_bv to save some copying too,
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 19e5e8f..ba2b7bd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3137,7 +3137,7 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>  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 xstate_bv = xsave->xsave_hdr.xstate_bv;

Also good.

>  	u64 valid;
>  
>  	/*
> 
>> Right, this matches
>>
>>         u64 xstate_bv = *(u64 *)(src + XSAVE_HDR_OFFSET);
>> 	...
>>         xsave->xsave_hdr.xstate_bv = xstate_bv;
>>
>> in load_xsave.
> 
> Btw, we don't care about crashers from userspace?

We do, but they're taken care of by

                if (xstate_bv & ~kvm_supported_xcr0())
                        return -EINVAL;

in kvm_vcpu_ioctl_x86_set_xsave.  kvm_supported_xcr0 is always a subset
of host_xcr0, hence:

>  	/* Set XSTATE_BV and possibly XCOMP_BV.  */
>  	xsave->xsave_hdr.xstate_bv = xstate_bv;
> -	if (cpu_has_xsaves)
> +	if (cpu_has_xsaves) {
>  		xsave->xsave_hdr.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
> +		xsave->xsave_hdr.xstate_bv &= xsave->xsave_hdr.xcomp_bv;

this is not zeroing any bit.  Thanks for thinking about it, though. :)

Paolo

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

[Radim Krčmář]

2014-12-04 14:43+0100, Paolo Bonzini:
> On 03/12/2014 19:45, Radim Krčmář wrote:
> > Btw, we don't care about crashers from userspace?
> 
> We do, but they're taken care of by
> 
>                 if (xstate_bv & ~kvm_supported_xcr0())
>                         return -EINVAL;
> 
> in kvm_vcpu_ioctl_x86_set_xsave.  kvm_supported_xcr0 is always a subset
> of host_xcr0, hence:

Ah, thanks.  (If this trend continues, I won't be able to hold even
bodily fluids after few more reviews ...)