Re: [Xen-devel] [PATCH] x86/EFI: further correct FPU state handling around runtime calls

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/06/18 19:53, Jan Beulich wrote:
> We must not leave a vCPU with CR0.TS clear when it is not in fully eager
> mode and has not touched non-lazy state. Instead of adding a 3rd
> invocation of stts() to vcpu_restore_fpu_eager(), consolidate all of
> them into a single one done at the end of the function.
>
> The new function parameter is not really well named, but
> "need_stts_if_not_fully_eager" seemed excessive to me. Suggestions
> welcome.

I think "maybe_stts" is reasonable here.  At least it is accurate.

OTOH, as we're changing all callsites, can we please rename the function
to vcpu_restore_fpu_nonlazy() to match the rest of the terminology, and
avoid this function looking like it restores all state.

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -206,11 +206,11 @@ static inline void fpu_fxsave(struct vcp
>  /*       VCPU FPU Functions    */
>  /*******************************/
>  /* Restore FPU state whenever VCPU is schduled in. */
> -void vcpu_restore_fpu_eager(struct vcpu *v)
> +void vcpu_restore_fpu_eager(struct vcpu *v, bool need_stts)
>  {
>      /* Restore nonlazy extended state (i.e. parts not tracked by CR0.TS). */
>      if ( !v->arch.fully_eager_fpu && !v->arch.nonlazy_xstate_used )
> -        return;
> +        goto maybe_stts;

This surely needs to be is_pv_vcpu(v) && (v->arch.pv_vcpu.ctrlreg[0] &
X86_CR0_TS); ?

Otherwise, this patch reintroduces the path which unconditionally uses
stts() around an EFI RS call.

With that done, the parameter becomes dead code, AFAICT.

~Andrew

>  
>      ASSERT(!is_idle_vcpu(v));
>  
> @@ -233,14 +233,17 @@ void vcpu_restore_fpu_eager(struct vcpu
>          v->fpu_dirtied = 1;
>  
>          /* Xen doesn't need TS set, but the guest might. */
> -        if ( is_pv_vcpu(v) && (v->arch.pv_vcpu.ctrlreg[0] & X86_CR0_TS) )
> -            stts();
> +        need_stts = is_pv_vcpu(v) && (v->arch.pv_vcpu.ctrlreg[0] & 
> X86_CR0_TS);
>      }
>      else
>      {
>          fpu_xrstor(v, XSTATE_NONLAZY);
> -        stts();
> +        need_stts = true;
>      }
> +
> + maybe_stts:
> +    if ( need_stts )
> +        stts();
>  }
>  
>  /* 
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel