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

[Paolo Bonzini <pbonzini@redhat.com>]

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 <pbonzini@redhat.com>
>> ---
>>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>