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

[Paolo Bonzini]

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
> 
>