diff mbox

[14/14] target-i386: Enable XCR0 features for user-mode

Message ID 1436429849-18052-15-git-send-email-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson July 9, 2015, 8:17 a.m. UTC 
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/cpu.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini July 9, 2015, 1:15 p.m. UTC | #1 
On 09/07/2015 10:17, Richard Henderson wrote:
> +
> +    /* ??? This variable is somewhat silly.  Methinks KVM should be
> +       using XCR0 to store into the XSTATE_BV field.  Either that or
> +       there's more missing information, e.g. the AVX bits.  */
> +    env->xstate_bv = XSTATE_FP;
> +    if (env->features[FEAT_1_EDX] & CPUID_SSE) {
> +        env->xstate_bv |= XSTATE_SSE;
> +    }
> +    if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
> +        env->xstate_bv |= XSTATE_BNDREGS | XSTATE_BNDCSR;
> +    }

xstate_bv != xcr0 if the kernel is using XSAVEOPT and some of the values
were in the initial state.  Legacy state is never optimized, hence the
value of env->xstate_bv after reset.  So I think this hunk is wrong,
while the other is correct.

Paolo
Richard Henderson July 10, 2015, 7:24 a.m. UTC | #2 
On 07/09/2015 02:15 PM, Paolo Bonzini wrote:
> On 09/07/2015 10:17, Richard Henderson wrote:
>> +
>> +    /* ??? This variable is somewhat silly.  Methinks KVM should be
>> +       using XCR0 to store into the XSTATE_BV field.  Either that or
>> +       there's more missing information, e.g. the AVX bits.  */
>> +    env->xstate_bv = XSTATE_FP;
>> +    if (env->features[FEAT_1_EDX] & CPUID_SSE) {
>> +        env->xstate_bv |= XSTATE_SSE;
>> +    }
>> +    if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
>> +        env->xstate_bv |= XSTATE_BNDREGS | XSTATE_BNDCSR;
>> +    }
>
> xstate_bv != xcr0 if the kernel is using XSAVEOPT and some of the values
> were in the initial state.  Legacy state is never optimized, hence the
> value of env->xstate_bv after reset.  So I think this hunk is wrong,
> while the other is correct.

Yes, it's a copy of the field of the same name from the xsave format.

Have we stopped using tcg entirely when kvm is enabled?  I guess so, since I 
seem to recall an effort to build qemu without tcg support.  So any fears about 
tcg corrupting kvm state would be unfounded, right?

If so, I can see how this variable aids initial xsave construction as well as 
copying the xsave block during migration.

It does beg the question of why xstate_bv isn't zero at reset.  Surely all of 
the xmm and fpu registers are in INIT state at this time, and that's what the 
XRSTOR that will consume this block is going to care about.


r~
Paolo Bonzini July 10, 2015, 9:36 a.m. UTC | #3 
On 10/07/2015 09:24, Richard Henderson wrote:
> On 07/09/2015 02:15 PM, Paolo Bonzini wrote:
>> On 09/07/2015 10:17, Richard Henderson wrote:
>>> +
>>> +    /* ??? This variable is somewhat silly.  Methinks KVM should be
>>> +       using XCR0 to store into the XSTATE_BV field.  Either that or
>>> +       there's more missing information, e.g. the AVX bits.  */
>>> +    env->xstate_bv = XSTATE_FP;
>>> +    if (env->features[FEAT_1_EDX] & CPUID_SSE) {
>>> +        env->xstate_bv |= XSTATE_SSE;
>>> +    }
>>> +    if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
>>> +        env->xstate_bv |= XSTATE_BNDREGS | XSTATE_BNDCSR;
>>> +    }
>>
>> xstate_bv != xcr0 if the kernel is using XSAVEOPT and some of the values
>> were in the initial state.  Legacy state is never optimized, hence the
>> value of env->xstate_bv after reset.  So I think this hunk is wrong,
>> while the other is correct.
> 
> Yes, it's a copy of the field of the same name from the xsave format.
> 
> Have we stopped using tcg entirely when kvm is enabled?

Yes, for about 8 years. :)

> I guess so,
> since I seem to recall an effort to build qemu without tcg support.  So
> any fears about tcg corrupting kvm state would be unfounded, right?
> 
> If so, I can see how this variable aids initial xsave construction as
> well as copying the xsave block during migration.
> 
> It does beg the question of why xstate_bv isn't zero at reset.  Surely
> all of the xmm and fpu registers are in INIT state at this time, and
> that's what the XRSTOR that will consume this block is going to care about.

That's a bug.  I was somehow convinced that XSAVEOPT never optimized the
FP and SSE state, but that's nonsense.

Paolo
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c19ff8a..2402c2a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2654,7 +2654,17 @@  static void x86_cpu_reset(CPUState *s)
     cpu_set_fpuc(env, 0x37f);
 
     env->mxcsr = 0x1f80;
-    env->xstate_bv = XSTATE_FP | XSTATE_SSE;
+
+    /* ??? This variable is somewhat silly.  Methinks KVM should be
+       using XCR0 to store into the XSTATE_BV field.  Either that or
+       there's more missing information, e.g. the AVX bits.  */
+    env->xstate_bv = XSTATE_FP;
+    if (env->features[FEAT_1_EDX] & CPUID_SSE) {
+        env->xstate_bv |= XSTATE_SSE;
+    }
+    if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
+        env->xstate_bv |= XSTATE_BNDREGS | XSTATE_BNDCSR;
+    }
 
     env->pat = 0x0007040600070406ULL;
     env->msr_ia32_misc_enable = MSR_IA32_MISC_ENABLE_DEFAULT;
@@ -2665,7 +2675,15 @@  static void x86_cpu_reset(CPUState *s)
     cpu_breakpoint_remove_all(s, BP_CPU);
     cpu_watchpoint_remove_all(s, BP_CPU);
 
-    env->xcr0 = 1;
+    env->xcr0 = XSTATE_FP;
+
+#ifdef CONFIG_USER_ONLY
+    /* Enable all the features for user-mode.  */
+    env->xcr0 = env->xstate_bv;
+    if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
+        cpu_x86_update_cr4(env, CR4_OSFXSR_MASK | CR4_OSXSAVE_MASK);
+    }
+#endif
 
     /*
      * SDM 11.11.5 requires: