Re: [Qemu-devel] [PATCH uq/master 1/2] x86: fix migration from pre-version 12

[Paolo Bonzini]

Il 08/09/2013 13:40, Gleb Natapov ha scritto:
> On Thu, Sep 05, 2013 at 03:06:21PM +0200, Paolo Bonzini wrote:
>> On KVM, the KVM_SET_XSAVE would be executed with a 0 xstate_bv,
>> and not restore anything.
>>
> XRSTOR restores FP/SSE state to reset state if no bits are set in
> xstate_bv. This is what should happen on reset, no?

Yes. The problem happens on the migration destination when XSAVE data is
not transmitted.  FP/SSE data is transmitted and must be restored, but
xstate_bv is zero and KVM_SET_XSAVE restores FP/SSE state to reset
state.  The vcpu then loses the values that were set in the migration data.

>> Since FP and SSE data are always valid, set them in xstate_bv at reset
>> time.  In fact, that value is the same that KVM_GET_XSAVE returns on
>> pre-XSAVE hosts.
> It is needed for migration between non xsave host to xsave host.

Yes, and this patch does the same for migration between non-XSAVE QEMU
and XSAVE QEMU.

In fact, another bug is that kvm_vcpu_ioctl_x86_set_xsave ignores
xstate_bv when XSAVE is not available.  Instead, it should reset the
FXSAVE data to processor-reset values (except for MXCSR which always
comes from XRSTOR data), i.e. to all-zeros except for the x87 control
and tag words.  It should also check reserved bits of MXCSR.

>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  target-i386/cpu.c | 1 +
>>  target-i386/cpu.h | 5 +++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index c36345e..ac83106 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2386,6 +2386,7 @@ static void x86_cpu_reset(CPUState *s)
>>      env->fpuc = 0x37f;
>>  
>>      env->mxcsr = 0x1f80;
>> +    env->xstate_bv = XSTATE_FP | XSTATE_SSE;
>>  
>>      env->pat = 0x0007040600070406ULL;
>>      env->msr_ia32_misc_enable = MSR_IA32_MISC_ENABLE_DEFAULT;
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 5723eff..a153078 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -380,6 +380,11 @@
>>  
>>  #define MSR_VM_HSAVE_PA                 0xc0010117
>>  
>> +#define XSTATE_SUPPORTED            (XSTATE_FP|XSTATE_SSE|XSTATE_YMM)
> Supported by whom? By QEMU? We should filer unsupported bits from CPUID.0D 
> then too.

Yes.  QEMU unmarshals information from the XSAVE region and back, so it
cannot support MPX or AVX-512 yet (even if KVM were).  Separate bug, though.

Paolo

> 
>> +#define XSTATE_FP                   1
>> +#define XSTATE_SSE                  2
>> +#define XSTATE_YMM                  4
>> +
>>  /* CPUID feature words */
>>  typedef enum FeatureWord {
>>      FEAT_1_EDX,         /* CPUID[1].EDX */
>> -- 
>> 1.8.3.1
>>
> 
> --
>                       Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>