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. > >> 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? > > >> 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. > the CPUID bits so that a well-behaved guest will not use it. QEMU does > hide CPUID bits for non-supported XSAVE states, except for "-cpu host". > So this will not be a problem except with "-cpu host". > -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
