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

[Paolo Bonzini]

Il 09/09/2013 12:54, Gleb Natapov ha scritto:
> On Mon, Sep 09, 2013 at 11:53:45AM +0200, Paolo Bonzini wrote:
>> Il 09/09/2013 11:03, Gleb Natapov ha scritto:
>>> On Mon, Sep 09, 2013 at 10:31:15AM +0200, Paolo Bonzini wrote:
>>>> 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.
>>>>
>>> Can such migration happen? The commit that added xsave support
>>> (f1665b21f16c5dc0ac37de60233a4975aff31193) changed vmstate version id.
>>
>> Yes, old->new migration can happen.  New->old of course cannot.
>>
> I see. I am fine with the patch, but please drop defines that are not
> used in the patch itself.

Ok.

(For the "BTW" question, xstate_bv will not be zeroed, it will remain to
the default value).

>>>> 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.
>>>
>>> I do not see why.
>>
>> Because otherwise it behaves in a subtly different manner for XSAVE and
>> non-XSAVE hosts.
> 
> I do not see how. Can you elaborate?

Suppose userspace calls KVM_SET_XSAVE with XSTATE_BV=0.

On an XSAVE host, when the guest FPU state is loaded KVM will do an
XRSTOR.  The XRSTOR will restore the FPU state to default values.

On a non-XSAVE host, when the guest FPU state is loaded KVM will do an
FXRSTR.  The FXRSTR will load the FPU state from the first 512 bytes of
the block that was passed to KVM_SET_XSAVE.

This is not a problem because userspace will usually pass to
KVM_SET_XSAVE only something that it got from KVM_GET_XSAVE, and
KVM_GET_XSAVE will never set XSTATE_BV=0.  However, KVM_SET_XSAVE is
supposed to emulate XSAVE/XRSTOR if it is not available, and it is
failing to emulate this detail.

>>>> 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.
>>>>
>>> IMO this is the main issue here, not separate bug. If we gonna let guest
>>> use CPU state QEMU does not support we gonna have a bad time.
>>
>> We cannot force the guest not to use a feature; all we can do is hide
> 
> Of course we can't, this is correct for other features too, but this is
> guest's problem.

Ok, then we agree that QEMU doesn't have a problem?  The XSAVE data will
always be "fresh" as long as the guest obeys CPUID bits it receives, and
the CPUID bits that QEMU passes will never enable XSAVE data that QEMU
does not support.

Paolo