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

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

On Thu Jul 18, 2024 at 12:49 PM BST, Jan Beulich wrote:
> 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.)

Sure.

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

Would you prefer "general purpose registers"? It's not quite that either, but
it's arguably closer. I can part with the comment altogether but I'd rather
leave a token amount of information to say "non-FPU register state" (but not
that, because that would be a terrible description). 

I'd rather update it to something that better reflects reality, as I found it
quite misleading when reading through. I initially thought it may have been
related to struct layout (as in C-style single-level inheritance), but as it
turns out it's merely establishing a vague relationship between arch_vcpu and
vcpu_guest_context. I can believe once upon a time the relationship was closer
than it it now, but with the guest context missing AVX state, MSR state and
other bits and pieces I thought it better to avoid such confusions for future
navigators down the line so limit its description to the line below.

>
> > --- 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 *.

Heh, took me a while looking at x86_fxsr to realise you mean the void pointer.

Ack.

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

I'm slightly confused.

XSTATE_AREA_MIN_SIZE is already 512 + 64 to account for the XSAVE header,
including its reserved fields. Did you mean something else?

    #define XSAVE_HDR_SIZE            64
    #define XSAVE_SSE_OFFSET          160
    #define XSTATE_YMM_SIZE           256
    #define FXSAVE_SIZE               512
    #define XSAVE_HDR_OFFSET          FXSAVE_SIZE
    #define XSTATE_AREA_MIN_SIZE      (FXSAVE_SIZE + XSAVE_HDR_SIZE)

Part of the rationale is to simplify other bits of code that are currently
conditionalized on v->xsave_header being NULL. And for that the full xsave
header must be present (even if unused because !cpu_xsave)

Do you mean something else?

Cheers,
Alejandro