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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Tue Oct 8, 2024 at 8:47 AM BST, Frediano Ziglio wrote:
> On Mon, Oct 7, 2024 at 4:52 PM Alejandro Vallejo
> <alejandro.vallejo@xxxxxxxxx> wrote:
> >
> > fpu_ctxt is either a pointer to the legacy x87/SSE save area (used by 
> > FXSAVE) or
> > a pointer aliased with xsave_area that points to its fpu_sse subfield. Such
> > subfield is at the base and is identical in size and layout to the legacy
> > buffer.
> >
> > This patch merges the 2 pointers in the arch_vcpu into a single XSAVE area. 
> > In
> > the very rare case in which the host doesn't support XSAVE all we're doing 
> > is
> > wasting a tiny amount of memory and trading those for a lot more simplicity 
> > in
> > the code.
> >
> > While at it, dedup the setup logic in vcpu_init_fpu() and integrate it
> > into xstate_alloc_save_area().
> >
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> > --
> > v4:
> >   * Amend commit message with extra note about deduping vcpu_init_fpu()
> >   * Remove comment on top of cpu_user_regs (though I really think there
> >     ought to be a credible one, in one form or another).
> >   * Remove cast from blk.c so FXSAVE_AREA is "void *"
> >   * Simplify comment in xstate_alloc_save_area() for the "host has no
> >     XSAVE" case.
> > ---
> >  xen/arch/x86/domctl.c             |  6 ++++-
> >  xen/arch/x86/hvm/emulate.c        |  4 +--
> >  xen/arch/x86/hvm/hvm.c            |  6 ++++-
> >  xen/arch/x86/i387.c               | 45 +++++--------------------------
> >  xen/arch/x86/include/asm/domain.h |  6 -----
> >  xen/arch/x86/x86_emulate/blk.c    |  2 +-
> >  xen/arch/x86/xstate.c             | 12 ++++++---
> >  7 files changed, 28 insertions(+), 53 deletions(-)
> >
> > diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
> > index e790f4f90056..08a05f8453f7 100644
> > --- a/xen/arch/x86/x86_emulate/blk.c
> > +++ b/xen/arch/x86/x86_emulate/blk.c
> > @@ -11,7 +11,7 @@
> >      !defined(X86EMUL_NO_SIMD)
> >  # ifdef __XEN__
> >  #  include <asm/xstate.h>
> > -#  define FXSAVE_AREA current->arch.fpu_ctxt
> > +#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
>
> Could you use "struct x86_fxsr *" instead of "void*" ?
> Maybe adding another "struct x86_fxsr fxsr" inside the anonymous
> fpu_sse union would help here.
>

I did in v3, and Andrew suggested to keep the (void *). See:

  
https://lore.kernel.org/xen-devel/2b42323a-961a-4dd8-8cde-f4b19eac0dc5@xxxxxxxxxx/

Cheers,
Alejandro