* [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave @ 2016-02-10 11:02 Asia Slowinska 2016-02-10 14:17 ` Paolo Bonzini 2016-02-12 20:20 ` Eduardo Habkost 0 siblings, 2 replies; 4+ messages in thread From: Asia Slowinska @ 2016-02-10 11:02 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, ehabkost, A Slowinska, rth [-- Attachment #1: Type: text/plain, Size: 2148 bytes --] Stick to the expected order of the FPU registers in xsave (as specified in the Intel manual.) Otherwise, instructions loading the FPU state don't set it up correctly. To set up FPU, software needs to provide a buffer of 80 bytes storing 8 FPU registers. They are organized in a stack. FPU assumes that the first field of the buffer is ST0, then ST1, and so on. QEMU maintains a circular buffer. When preparing these 80 bytes for KVM, QEMU just uses memcpy instead of copying the elements in a proper order. Signed-off-by: Asia Slowinska <asia@lastline.com> --- target-i386/kvm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 94024bc..c77fe73 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -1325,8 +1325,10 @@ static int kvm_put_xsave(X86CPU *cpu) xsave->region[XSAVE_FTW_FOP] = (uint32_t)(env->fpop << 16) + twd; memcpy(&xsave->region[XSAVE_CWD_RIP], &env->fpip, sizeof(env->fpip)); memcpy(&xsave->region[XSAVE_CWD_RDP], &env->fpdp, sizeof(env->fpdp)); - memcpy(&xsave->region[XSAVE_ST_SPACE], env->fpregs, - sizeof env->fpregs); + for (i = 0; i < 8; i++) { + memcpy(&xsave_region[HXSAVE_ST_SPACE + i * 4], + &env->fpregs[(env->fpstt + i) & 7], 16); + } xsave->region[XSAVE_MXCSR] = env->mxcsr; *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] = env->xstate_bv; memcpy(&xsave->region[XSAVE_BNDREGS], env->bnd_regs, @@ -1745,8 +1747,10 @@ static int kvm_get_xsave(X86CPU *cpu) memcpy(&env->fpip, &xsave->region[XSAVE_CWD_RIP], sizeof(env->fpip)); memcpy(&env->fpdp, &xsave->region[XSAVE_CWD_RDP], sizeof(env->fpdp)); env->mxcsr = xsave->region[XSAVE_MXCSR]; - memcpy(env->fpregs, &xsave->region[XSAVE_ST_SPACE], - sizeof env->fpregs); + for (i = 0; i < 8; i++) { + memcpy(&env->fpregs[(env->fpstt + i) & 7], + &xsave_region[HXSAVE_ST_SPACE + i * 4], 16); + } env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV]; memcpy(env->bnd_regs, &xsave->region[XSAVE_BNDREGS], sizeof env->bnd_regs); -- 1.9.1 [-- Attachment #2: Type: text/html, Size: 2753 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave 2016-02-10 11:02 [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave Asia Slowinska @ 2016-02-10 14:17 ` Paolo Bonzini 2016-02-10 14:39 ` Asia Slowinska 2016-02-12 20:20 ` Eduardo Habkost 1 sibling, 1 reply; 4+ messages in thread From: Paolo Bonzini @ 2016-02-10 14:17 UTC (permalink / raw) To: asia, qemu-devel; +Cc: ehabkost, A Slowinska, rth On 10/02/2016 12:02, Asia Slowinska wrote: > Stick to the expected order of the FPU registers in xsave (as specified > in the > Intel manual.) Otherwise, instructions loading the FPU state don't set > it up > correctly. > > To set up FPU, software needs to provide a buffer of 80 bytes > storing 8 FPU registers. They are organized in a stack. FPU assumes that the > first field of the buffer is ST0, then ST1, and so on. QEMU maintains a > circular buffer. When preparing these 80 bytes for KVM, QEMU just uses > memcpy > instead of copying the elements in a proper order. > > Signed-off-by: Asia Slowinska <asia@lastline.com <mailto:asia@lastline.com>> > --- > target-i386/kvm.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 94024bc..c77fe73 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -1325,8 +1325,10 @@ static int kvm_put_xsave(X86CPU *cpu) > xsave->region[XSAVE_FTW_FOP] = (uint32_t)(env->fpop << 16) + twd; > memcpy(&xsave->region[XSAVE_CWD_RIP], &env->fpip, sizeof(env->fpip)); > memcpy(&xsave->region[XSAVE_CWD_RDP], &env->fpdp, sizeof(env->fpdp)); > - memcpy(&xsave->region[XSAVE_ST_SPACE], env->fpregs, > - sizeof env->fpregs); > + for (i = 0; i < 8; i++) { > + memcpy(&xsave_region[HXSAVE_ST_SPACE + i * 4], > + &env->fpregs[(env->fpstt + i) & 7], 16); > + } > xsave->region[XSAVE_MXCSR] = env->mxcsr; > *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] = env->xstate_bv; > memcpy(&xsave->region[XSAVE_BNDREGS], env->bnd_regs, > @@ -1745,8 +1747,10 @@ static int kvm_get_xsave(X86CPU *cpu) > memcpy(&env->fpip, &xsave->region[XSAVE_CWD_RIP], sizeof(env->fpip)); > memcpy(&env->fpdp, &xsave->region[XSAVE_CWD_RDP], sizeof(env->fpdp)); > env->mxcsr = xsave->region[XSAVE_MXCSR]; > - memcpy(env->fpregs, &xsave->region[XSAVE_ST_SPACE], > - sizeof env->fpregs); > + for (i = 0; i < 8; i++) { > + memcpy(&env->fpregs[(env->fpstt + i) & 7], > + &xsave_region[HXSAVE_ST_SPACE + i * 4], 16); Hi, the patch is missing a definition of HXSAVE_ST_SPACE. Thanks, Paolo > + } > env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV]; > memcpy(env->bnd_regs, &xsave->region[XSAVE_BNDREGS], > sizeof env->bnd_regs); > -- > 1.9.1 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave 2016-02-10 14:17 ` Paolo Bonzini @ 2016-02-10 14:39 ` Asia Slowinska 0 siblings, 0 replies; 4+ messages in thread From: Asia Slowinska @ 2016-02-10 14:39 UTC (permalink / raw) To: Paolo Bonzini; +Cc: A Slowinska, qemu-devel, ehabkost, rth [-- Attachment #1: Type: text/plain, Size: 4862 bytes --] Many thanks for the reply. I'm sorry for the typo in the previous patch. Below comes a new one. Best regards, asia Stick to the right order of the FPU registers in xsave (as specified in the Intel manual.) Otherwise, instructions loading the FPU state don't set it up correctly. To set up FPU, software needs to provide a buffer of 80 bytes storing 8 FPU registers. They are organized in a stack. FPU assumes that the first field of the buffer is ST0, then ST1, and so on. QEMU maintains a circular buffer. When preparing these 80 bytes for KVM, QEMU just uses memcpy instead of copying the elements in a proper order. Signed-off-by: Asia Slowinska <asia@lastline.com> --- target-i386/kvm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 94024bc..a9e6c75 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -1325,8 +1325,10 @@ static int kvm_put_xsave(X86CPU *cpu) xsave->region[XSAVE_FTW_FOP] = (uint32_t)(env->fpop << 16) + twd; memcpy(&xsave->region[XSAVE_CWD_RIP], &env->fpip, sizeof(env->fpip)); memcpy(&xsave->region[XSAVE_CWD_RDP], &env->fpdp, sizeof(env->fpdp)); - memcpy(&xsave->region[XSAVE_ST_SPACE], env->fpregs, - sizeof env->fpregs); + for (i = 0; i < 8; i++) { + memcpy(&xsave_region[XSAVE_ST_SPACE + i * 4], + &env->fpregs[(env->fpstt + i) & 7], 16); + } xsave->region[XSAVE_MXCSR] = env->mxcsr; *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] = env->xstate_bv; memcpy(&xsave->region[XSAVE_BNDREGS], env->bnd_regs, @@ -1745,8 +1747,10 @@ static int kvm_get_xsave(X86CPU *cpu) memcpy(&env->fpip, &xsave->region[XSAVE_CWD_RIP], sizeof(env->fpip)); memcpy(&env->fpdp, &xsave->region[XSAVE_CWD_RDP], sizeof(env->fpdp)); env->mxcsr = xsave->region[XSAVE_MXCSR]; - memcpy(env->fpregs, &xsave->region[XSAVE_ST_SPACE], - sizeof env->fpregs); + for (i = 0; i < 8; i++) { + memcpy(&env->fpregs[(env->fpstt + i) & 7], + &xsave_region[XSAVE_ST_SPACE + i * 4], 16); + } env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV]; memcpy(env->bnd_regs, &xsave->region[XSAVE_BNDREGS], sizeof env->bnd_regs); -- 1.9.1 On Wed, Feb 10, 2016 at 3:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 10/02/2016 12:02, Asia Slowinska wrote: > > Stick to the expected order of the FPU registers in xsave (as specified > > in the > > Intel manual.) Otherwise, instructions loading the FPU state don't set > > it up > > correctly. > > > > To set up FPU, software needs to provide a buffer of 80 bytes > > storing 8 FPU registers. They are organized in a stack. FPU assumes that > the > > first field of the buffer is ST0, then ST1, and so on. QEMU maintains a > > circular buffer. When preparing these 80 bytes for KVM, QEMU just uses > > memcpy > > instead of copying the elements in a proper order. > > > > Signed-off-by: Asia Slowinska <asia@lastline.com <mailto: > asia@lastline.com>> > > --- > > target-i386/kvm.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > > index 94024bc..c77fe73 100644 > > --- a/target-i386/kvm.c > > +++ b/target-i386/kvm.c > > @@ -1325,8 +1325,10 @@ static int kvm_put_xsave(X86CPU *cpu) > > xsave->region[XSAVE_FTW_FOP] = (uint32_t)(env->fpop << 16) + twd; > > memcpy(&xsave->region[XSAVE_CWD_RIP], &env->fpip, > sizeof(env->fpip)); > > memcpy(&xsave->region[XSAVE_CWD_RDP], &env->fpdp, > sizeof(env->fpdp)); > > - memcpy(&xsave->region[XSAVE_ST_SPACE], env->fpregs, > > - sizeof env->fpregs); > > + for (i = 0; i < 8; i++) { > > + memcpy(&xsave_region[HXSAVE_ST_SPACE + i * 4], > > + &env->fpregs[(env->fpstt + i) & 7], 16); > > + } > > xsave->region[XSAVE_MXCSR] = env->mxcsr; > > *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] = env->xstate_bv; > > memcpy(&xsave->region[XSAVE_BNDREGS], env->bnd_regs, > > @@ -1745,8 +1747,10 @@ static int kvm_get_xsave(X86CPU *cpu) > > memcpy(&env->fpip, &xsave->region[XSAVE_CWD_RIP], > sizeof(env->fpip)); > > memcpy(&env->fpdp, &xsave->region[XSAVE_CWD_RDP], > sizeof(env->fpdp)); > > env->mxcsr = xsave->region[XSAVE_MXCSR]; > > - memcpy(env->fpregs, &xsave->region[XSAVE_ST_SPACE], > > - sizeof env->fpregs); > > + for (i = 0; i < 8; i++) { > > + memcpy(&env->fpregs[(env->fpstt + i) & 7], > > + &xsave_region[HXSAVE_ST_SPACE + i * 4], 16); > > Hi, the patch is missing a definition of HXSAVE_ST_SPACE. > > Thanks, > > Paolo > > > + } > > env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV]; > > memcpy(env->bnd_regs, &xsave->region[XSAVE_BNDREGS], > > sizeof env->bnd_regs); > > -- > > 1.9.1 > > > > > [-- Attachment #2: Type: text/html, Size: 6555 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave 2016-02-10 11:02 [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave Asia Slowinska 2016-02-10 14:17 ` Paolo Bonzini @ 2016-02-12 20:20 ` Eduardo Habkost 1 sibling, 0 replies; 4+ messages in thread From: Eduardo Habkost @ 2016-02-12 20:20 UTC (permalink / raw) To: Asia Slowinska; +Cc: pbonzini, qemu-devel, A Slowinska, rth On Wed, Feb 10, 2016 at 12:02:47PM +0100, Asia Slowinska wrote: > Stick to the expected order of the FPU registers in xsave (as specified in > the > Intel manual.) Otherwise, instructions loading the FPU state don't set it > up > correctly. > > To set up FPU, software needs to provide a buffer of 80 bytes > storing 8 FPU registers. They are organized in a stack. FPU assumes that the > first field of the buffer is ST0, then ST1, and so on. QEMU maintains a > circular buffer. When preparing these 80 bytes for KVM, QEMU just uses > memcpy > instead of copying the elements in a proper order. > > Signed-off-by: Asia Slowinska <asia@lastline.com> Does it cause any guest-visible bugs? I believe you won't be able to trigger any bug, unless you can make QEMU write to fpregs when using KVM (can you?). If I understand this correctly, the format of CPUX86State.fpregs was always different in KVM mode (meaning ST0 is always fpregs[0], not fpregs[fpstt & 7]). It looks like this doesn't cause any problems because fpregs is always saved and restored in the same format, and (I believe) QEMU never touches fpregs when running KVM (except when loading/saving state). Your fix, on the other hand, seems to break migration from older QEMU versions, or between hosts with/without KVM_CAP_XSAVE. The data memcpy()ed from KVM_GET_FPU/KVM_GET_XSAVE in the origin will be "fixed" for KVM_SET_XSAVE in the destination, and the FPU state seen by the guest at the destination won't match the state seen at the origin. If we want to make the CPUX86State.fpregs format be the same in KVM and TCG mode (do we?), we need to fix this in a way that doesn't break migration. The cpu/fpregs migration data is already in a specific order when using KVM, and we need to keep it working when migrating from older QEMU. > --- > target-i386/kvm.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 94024bc..c77fe73 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -1325,8 +1325,10 @@ static int kvm_put_xsave(X86CPU *cpu) > xsave->region[XSAVE_FTW_FOP] = (uint32_t)(env->fpop << 16) + twd; > memcpy(&xsave->region[XSAVE_CWD_RIP], &env->fpip, sizeof(env->fpip)); > memcpy(&xsave->region[XSAVE_CWD_RDP], &env->fpdp, sizeof(env->fpdp)); > - memcpy(&xsave->region[XSAVE_ST_SPACE], env->fpregs, > - sizeof env->fpregs); > + for (i = 0; i < 8; i++) { > + memcpy(&xsave_region[HXSAVE_ST_SPACE + i * 4], > + &env->fpregs[(env->fpstt + i) & 7], 16); > + } > xsave->region[XSAVE_MXCSR] = env->mxcsr; > *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] = env->xstate_bv; > memcpy(&xsave->region[XSAVE_BNDREGS], env->bnd_regs, > @@ -1745,8 +1747,10 @@ static int kvm_get_xsave(X86CPU *cpu) > memcpy(&env->fpip, &xsave->region[XSAVE_CWD_RIP], sizeof(env->fpip)); > memcpy(&env->fpdp, &xsave->region[XSAVE_CWD_RDP], sizeof(env->fpdp)); > env->mxcsr = xsave->region[XSAVE_MXCSR]; > - memcpy(env->fpregs, &xsave->region[XSAVE_ST_SPACE], > - sizeof env->fpregs); > + for (i = 0; i < 8; i++) { > + memcpy(&env->fpregs[(env->fpstt + i) & 7], > + &xsave_region[HXSAVE_ST_SPACE + i * 4], 16); > + } > env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV]; > memcpy(env->bnd_regs, &xsave->region[XSAVE_BNDREGS], > sizeof env->bnd_regs); > -- > 1.9.1 -- Eduardo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-12 20:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-10 11:02 [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave Asia Slowinska 2016-02-10 14:17 ` Paolo Bonzini 2016-02-10 14:39 ` Asia Slowinska 2016-02-12 20:20 ` Eduardo Habkost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).