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

[CFT PATCH 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

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   | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[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 0/2] KVM: support XSAVES usage in the host

[Nadav Amit]

I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).

Thanks for the quick response.

Nadav

> On Nov 21, 2014, at 20:31, Paolo Bonzini <pbonzini@redhat.com> 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.
> 
> Thanks,
> 
> Paolo
> 
> 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   | 48 +++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 53 insertions(+), 6 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Wanpeng Li]

Hi Nadav,
On 11/23/14, 4:16 PM, Nadav Amit wrote:
> I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).

If the machine you mentioned support xsaves and what's machine you are 
using?

Regards,
Wanpeng Li

>
> Thanks for the quick response.
>
> Nadav
>
>> On Nov 21, 2014, at 20:31, Paolo Bonzini <pbonzini@redhat.com> 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.
>>
>> Thanks,
>>
>> Paolo
>>
>> 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   | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>> 2 files changed, 53 insertions(+), 6 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Nadav Amit]

> On Nov 23, 2014, at 10:24, Wanpeng Li <kernellwp@gmail.com> wrote:
> 
> Hi Nadav,
> On 11/23/14, 4:16 PM, Nadav Amit wrote:
>> I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).
> 
> If the machine you mentioned support xsaves and what's machine you are using?

It supports xsaves. I am using an experimental setup.

Nadav

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

[Nadav Amit]

> On Nov 23, 2014, at 10:24, Wanpeng Li <kernellwp@gmail.com> wrote:
> 
> Hi Nadav,
> On 11/23/14, 4:16 PM, Nadav Amit wrote:
>> I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).
> 
> If the machine you mentioned support xsaves and what's machine you are using?

It supports xsaves. I am using an experimental setup.

Nadav--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Wanpeng Li]

On 11/23/14, 4:31 PM, Nadav Amit wrote:
>> On Nov 23, 2014, at 10:24, Wanpeng Li <kernellwp@gmail.com> wrote:
>>
>> Hi Nadav,
>> On 11/23/14, 4:16 PM, Nadav Amit wrote:
>>> I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).
>> If the machine you mentioned support xsaves and what's machine you are using?
> It supports xsaves. I am using an experimental setup.

Interesting, I even can't get a machine which supports xsaves.

Regards,
Wanpeng Li

>
> Nadav

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

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

[Paolo Bonzini]

On 23/11/2014 09:16, Nadav Amit wrote:
> I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).

Thanks, you'll need to squash this in:

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 4c540c4719d8..0de1fae2bdf0 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -738,3 +738,4 @@ void *get_xsave_addr(struct xsave_struct *xsave, int xstate)
 
 	return (void *)xsave + xstate_comp_offsets[feature];
 }
+EXPORT_SYMBOL_GPL(get_xsave_addr);

Paolo

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

[Nadav Amit]

> On Nov 24, 2014, at 13:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 23/11/2014 09:16, Nadav Amit wrote:
>> I’ll try to check it tomorrow (I don’t have access to the failing machine at the moment).
> 
> Thanks, you'll need to squash this in:
> 
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index 4c540c4719d8..0de1fae2bdf0 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -738,3 +738,4 @@ void *get_xsave_addr(struct xsave_struct *xsave, int xstate)
> 
> 	return (void *)xsave + xstate_comp_offsets[feature];
> }
> +EXPORT_SYMBOL_GPL(get_xsave_addr);

I tested the patches but there are still problems.

Since kvm_load_guest_fpu is called before the guest_fpu is ever stored, there are 2 more problems that currently cause #GP:
1. XCOMP_BV[63] = 0
2. XSTATE_BV sets a bit (including bit 63) that is not set in XCOMP_BV (XCOMP_BV is initialised to zero).

[see SDM 13.11 "OPERATION OF XRSTORS”]

Once I initialise XCOMP_BV to (1ull << 63) | XSTATE_BV, the guest runs successfully.
I have not checked any other qemu functionality that might be affected by the patch.

Nadav

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

[Paolo Bonzini]

On 24/11/2014 16:28, Nadav Amit wrote:
> Since kvm_load_guest_fpu is called before the guest_fpu is ever stored, there are 2 more problems that currently cause #GP:
> 1. XCOMP_BV[63] = 0
> 2. XSTATE_BV sets a bit (including bit 63) that is not set in XCOMP_BV (XCOMP_BV is initialised to zero).
> 
> [see SDM 13.11 "OPERATION OF XRSTORS”]
> 
> Once I initialise XCOMP_BV to (1ull << 63) | XSTATE_BV, the guest runs successfully.
> I have not checked any other qemu functionality that might be affected by the patch.

Ah, so the problem is with KVM_SET_XSAVE.  Thanks!

Paolo

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

[Paolo Bonzini]

On 24/11/2014 16:28, Nadav Amit wrote:
> 
> Since kvm_load_guest_fpu is called before the guest_fpu is ever stored, there are 2 more problems that currently cause #GP:
> 1. XCOMP_BV[63] = 0
> 2. XSTATE_BV sets a bit (including bit 63) that is not set in XCOMP_BV (XCOMP_BV is initialised to zero).
> 
> [see SDM 13.11 "OPERATION OF XRSTORS”]
> 
> Once I initialise XCOMP_BV to (1ull << 63) | XSTATE_BV, the guest runs successfully.
> I have not checked any other qemu functionality that might be affected by the patch.

I posted patches that assume that QEMU calls KVM_SET_XSAVE early enough.
 If this is not the case, can you cook up and post a patch to
kvm_arch_vcpu_init that fixes the remaining problem?

Paolo

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

[Nadav Amit]

> On Nov 24, 2014, at 19:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 24/11/2014 16:28, Nadav Amit wrote:
>> 
>> Since kvm_load_guest_fpu is called before the guest_fpu is ever stored, there are 2 more problems that currently cause #GP:
>> 1. XCOMP_BV[63] = 0
>> 2. XSTATE_BV sets a bit (including bit 63) that is not set in XCOMP_BV (XCOMP_BV is initialised to zero).
>> 
>> [see SDM 13.11 "OPERATION OF XRSTORS”]
>> 
>> Once I initialise XCOMP_BV to (1ull << 63) | XSTATE_BV, the guest runs successfully.
>> I have not checked any other qemu functionality that might be affected by the patch.
> 
> I posted patches that assume that QEMU calls KVM_SET_XSAVE early enough.
> If this is not the case, can you cook up and post a patch to
> kvm_arch_vcpu_init that fixes the remaining problem?

Sure. I will try to do so tomorrow.

Nadav