Re: [PATCH for-4.20 3/4] x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.07.2024 17:52, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1343,7 +1343,8 @@ void arch_get_info_guest(struct vcpu *v, 
> vcpu_guest_context_u c)
>  #define c(fld) (c.nat->fld)
>  #endif
>  
> -    memcpy(&c.nat->fpu_ctxt, v->arch.fpu_ctxt, sizeof(c.nat->fpu_ctxt));
> +    memcpy(&c.nat->fpu_ctxt, &v->arch.xsave_area->fpu_sse,
> +           sizeof(c.nat->fpu_ctxt));

Now that the middle argument has proper type, maybe take the opportunity
and add BUILD_BUG_ON(sizeof(...) == sizeof(...))? (Also in e.g.
hvm_save_cpu_ctxt() then.)

> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -591,12 +591,7 @@ struct pv_vcpu
>  
>  struct arch_vcpu
>  {
> -    /*
> -     * guest context (mirroring struct vcpu_guest_context) common
> -     * between pv and hvm guests
> -     */
> -
> -    void              *fpu_ctxt;
> +    /* Fixed point registers */
>      struct cpu_user_regs user_regs;

Not exactly, no. Selector registers are there as well for example, which
I wouldn't consider "fixed point" ones. I wonder why the existing comment
cannot simply be kept, perhaps extended to mention that fpu_ctxt now lives
elsewhere.

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

Nit: Blank missing after before *.

> --- 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.
> +         */
> +        size = XSTATE_AREA_MIN_SIZE;

What exactly would break if just (a little over) 512 bytes worth were allocated
when there's no XSAVE? If it was exactly 512, something like xstate_all() would
need to apply a little more care, I guess. Yet for that having just always-zero
xstate_bv and xcomp_bv there would already suffice (e.g. using
offsetof(..., xsave_hdr.reserved) here, to cover further fields gaining meaning
down the road). Remember that due to xmalloc() overhead and the 64-byte-aligned
requirement, you can only have 6 of them in a page the way you do it, when the
alternative way 7 would fit (if I got my math right).

Jan