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
This feature is not supported inside KVM guests yet, because we do not emulate
MSR_IA32_XSS. Mask it out.
Cc: [email protected]
Cc: Nadav Amit <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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 <[email protected]>
Cc: [email protected]
Cc: Nadav Amit <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
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 <[email protected]>
> Cc: [email protected]
> Cc: Nadav Amit <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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
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
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 <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]> 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 [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> On Nov 23, 2014, at 10:24, Wanpeng Li <[email protected]> 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-
On 11/23/14, 4:31 PM, Nadav Amit wrote:
>> On Nov 23, 2014, at 10:24, Wanpeng Li <[email protected]> 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
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
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
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
> On Nov 24, 2014, at 13:39, Paolo Bonzini <[email protected]> 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
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
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
> On Nov 24, 2014, at 19:53, Paolo Bonzini <[email protected]> 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