Re: [PATCH v5 1/3] xsave: fix initialization of FPU memory area

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/12/15 12:47, Roger Pau Monne wrote:
> When the CPU supports XSAVE, the initialization of the FPU memory area is
> done during allocation of the XSAVE area. This is different from the way
> it's done in non-xsave capable CPUs, so unify both paths and always
> initialise the FPU state in vcpu_restore_fpu_lazy.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/i387.c   | 2 +-
>  xen/arch/x86/xstate.c | 8 --------
>  2 files changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
> index b661d39..fcd87a6 100644
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -226,7 +226,7 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
>      if ( v->fpu_dirtied )
>          return;
>  
> -    if ( cpu_has_xsave )
> +    if ( cpu_has_xsave && v->fpu_initialised )
>          fpu_xrstor(v, XSTATE_LAZY);
>      else if ( v->fpu_initialised )
>          fpu_fxrstor(v);

I feel this would be clearer if it were rearranged as

if ( v->fpu_initialised )
{
    if ( cpu_has_xsave )
        fpu_xrstor(v, XSTATE_LAZY);
    else
    {
        ...
    }
}
else
    fpu_init();


Functionally however, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index b65da38..8007342 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -415,14 +415,6 @@ int xstate_alloc_save_area(struct vcpu *v)
>      if ( save_area == NULL )
>          return -ENOMEM;
>  
> -    /*
> -     * Set the memory image to default values, but don't force the context
> -     * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv
> -     * clear).
> -     */
> -    save_area->fpu_sse.fcw = FCW_DEFAULT;
> -    save_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
> -
>      v->arch.xsave_area = save_area;
>      v->arch.xcr0 = 0;
>      v->arch.xcr0_accum = 0;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel