qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registe


From: Asia Slowinska
Subject: Re: [Qemu-devel] [PATCH] target-i386/kvm.c: Fix the order of FPU registers in xsave
Date: Wed, 10 Feb 2016 15:39:10 +0100

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 <address@hidden>
---
 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 <address@hidden> 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 <address@hidden <mailto:address@hidden>>
> ---
>  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
>
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]