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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 25 June 2018 13:18
> To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; Juergen Gross <jgross@xxxxxxxx>
> Subject: [PATCH for-4.11 v2] x86/EFI: further correct FPU state handling
> around runtime calls
> 
> 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.
> 
> Rename the function at the same time to better reflect its purpose, as
> the patches touches all of its occurences anyway.
> 
> The new function parameter is not really well named, but
> "need_stts_if_not_fully_eager" seemed excessive to me.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Also rename the function.
> ---
> Suggestions on the parameter naming welcome; "maybe_stts", for example,
> is not a name I consider any better, as there's no "maybe" involved in
> the EFI runtime call case for a not-fully-eager vCPU.
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1636,7 +1636,7 @@ static void __context_switch(void)
>              if ( cpu_has_xsaves && is_hvm_vcpu(n) )
>                  set_msr_xss(n->arch.hvm_vcpu.msr_xss);
>          }
> -        vcpu_restore_fpu_eager(n);
> +        vcpu_restore_fpu_nonlazy(n, false);
>          nd->arch.ctxt_switch->to(n);
>      }
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2146,7 +2146,7 @@ static void hvmemul_put_fpu(
>           *   by hvmemul_get_fpu().
>           */
>          if ( curr->arch.fully_eager_fpu )
> -            vcpu_restore_fpu_eager(curr);
> +            vcpu_restore_fpu_nonlazy(curr, false);
>          else
>          {
>              curr->fpu_dirtied = false;
> --- 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_nonlazy(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;

It's really just an 'out' label (AFAICT, since need_stts needs to be true for 
there to be any other semantic) so how about just calling it that rather than 
'maybe_stts'?

  Paul

> 
>      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();
>  }
> 
>  /*
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -135,7 +135,7 @@ void efi_rs_leave(struct efi_rs_state *s
>      irq_exit();
>      efi_rs_on_cpu = NR_CPUS;
>      spin_unlock(&efi_rs_lock);
> -    vcpu_restore_fpu_eager(curr);
> +    vcpu_restore_fpu_nonlazy(curr, true);
>  }
> 
>  bool efi_rs_using_pgtables(void)
> --- a/xen/include/asm-x86/i387.h
> +++ b/xen/include/asm-x86/i387.h
> @@ -28,7 +28,7 @@ struct ix87_env {
>      uint16_t fds, _res6;
>  };
> 
> -void vcpu_restore_fpu_eager(struct vcpu *v);
> +void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts);
>  void vcpu_restore_fpu_lazy(struct vcpu *v);
>  void vcpu_save_fpu(struct vcpu *v);
>  void save_fpu_enable(void);
> 
> 
> 


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