Re: [PATCH v3 1/2] x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/08/2024 3:21 pm, Alejandro Vallejo wrote:
> @@ -299,44 +299,14 @@ void save_fpu_enable(void)
>  /* Initialize FPU's context save area */
>  int vcpu_init_fpu(struct vcpu *v)
>  {
> -    int rc;
> -
>      v->arch.fully_eager_fpu = opt_eager_fpu;
> -
> -    if ( (rc = xstate_alloc_save_area(v)) != 0 )
> -        return rc;
> -
> -    if ( v->arch.xsave_area )
> -        v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse;
> -    else
> -    {
> -        BUILD_BUG_ON(__alignof(v->arch.xsave_area->fpu_sse) < 16);
> -        v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse),
> -                                    __alignof(v->arch.xsave_area->fpu_sse));
> -        if ( v->arch.fpu_ctxt )
> -        {
> -            fpusse_t *fpu_sse = v->arch.fpu_ctxt;
> -
> -            fpu_sse->fcw = FCW_DEFAULT;
> -            fpu_sse->mxcsr = MXCSR_DEFAULT;
> -        }
> -        else
> -            rc = -ENOMEM;

This looks wonky.  It's not, because xstate_alloc_save_area() contains
the same logic for setting up FCW/MXCSR.

It would be helpful to note this in the commit message.  Something about
deduplicating the setup alongside deduplicating the pointer.

> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index bca3258d69ac..3da60af2a44a 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -592,11 +592,11 @@ struct pv_vcpu
>  struct arch_vcpu
>  {
>      /*
> -     * guest context (mirroring struct vcpu_guest_context) common
> -     * between pv and hvm guests
> +     * Guest context common between PV and HVM guests. Includes general 
> purpose
> +     * registers, segment registers and other parts of the exception frame.
> +     *
> +     * It doesn't contain FPU state, as that lives in xsave_area instead.
>       */

This new comment isn't really correct either.  arch_vcpu contains the
PV/HVM union, so it not only things which are common between the two.

I'd either leave it alone, or delete it entirely.  It doesn't serve much
purpose IMO, and it is going to bitrot very quickly (FRED alone will
change two of the state groups you mention).

> -
> -    void              *fpu_ctxt;
>      struct cpu_user_regs user_regs;
>  
>      /* Debug registers. */
> diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
> index e790f4f90056..28b54f26fe29 100644
> --- a/xen/arch/x86/x86_emulate/blk.c
> +++ b/xen/arch/x86/x86_emulate/blk.c
> @@ -11,7 +11,8 @@
>      !defined(X86EMUL_NO_SIMD)
>  # ifdef __XEN__
>  #  include <asm/xstate.h>
> -#  define FXSAVE_AREA current->arch.fpu_ctxt
> +#  define FXSAVE_AREA ((struct x86_fxsr *) \
> +                           (void *)&current->arch.xsave_area->fpu_sse)

This isn't a like-for-like replacement.

Previously FXSAVE_AREA's type was void *.  I'd leave the expression as just

    (void *)&current->arch.xsave_area->fpu_sse

because struct x86_fxsr is not the only type needing to be used here in
due course.   (There are 8 variations of data layout for older
instructions.)

>  # else
>  #  define FXSAVE_AREA get_fpu_save_area()
>  # endif
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index 5c4144d55e89..850ee31bd18c 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -507,9 +507,16 @@ int xstate_alloc_save_area(struct vcpu *v)
>      unsigned int size;
>  
>      if ( !cpu_has_xsave )
> -        return 0;
> -
> -    if ( !is_idle_vcpu(v) || !cpu_has_xsavec )
> +    {
> +        /*
> +         * This is bigger than FXSAVE_SIZE by 64 bytes, but it helps treating
> +         * the FPU state uniformly as an XSAVE buffer even if XSAVE is not
> +         * available in the host. Note the alignment restriction of the XSAVE
> +         * area are stricter than those of the FXSAVE area.
> +         */

Can I suggest the following?

"On non-XSAVE systems, we allocate an XSTATE buffer for simplicity. 
XSTATE is backwards compatible to FXSAVE, and only one cacheline larger."

It's rather more concise.

~Andrew