[PATCH for-4.20 0/4] x86: FPU handling cleanup

[PATCH for-4.20 0/4] x86: FPU handling cleanup

[Alejandro Vallejo]

I want to eventually reach a position in which the FPU state can be allocated
from the domheap and hidden via the same core mechanism proposed in Elias'
directmap removal series. Doing so is complicated by the presence of 2 aliased
pointers (v->arch.fpu_ctxt and v->arch.xsave_area) and the rather complicated
semantics of vcpu_setup_fpu(). This series tries to simplify the code so moving
to a "map/modify/unmap" model is more tractable.

Patches 1 and 2 are trivial refactors.

Patch 3 unifies FPU state so an XSAVE area is allocated per vCPU regardless of
the host supporting it or not. The rationale is that the memory savings are
negligible and not worth the extra complexity.

Patch 4 is a non-trivial split of the vcpu_setup_fpu() into 2 separate
functions. One to override x87/SSE state, and another to set a reset state.

Alejandro Vallejo (4):
  x86/xstate: Use compression check helper in xstate_all()
  x86/fpu: Create a typedef for the x87/SSE area inside "struct
    xsave_struct"
  x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu
  x86/fpu: Split fpu_setup_fpu() in two

 xen/arch/x86/domain.c             |  7 ++-
 xen/arch/x86/domctl.c             |  3 +-
 xen/arch/x86/hvm/emulate.c        |  5 +-
 xen/arch/x86/hvm/hvm.c            | 22 +++++---
 xen/arch/x86/i387.c               | 93 ++++++++-----------------------
 xen/arch/x86/include/asm/domain.h |  7 +--
 xen/arch/x86/include/asm/i387.h   | 27 +++++++--
 xen/arch/x86/include/asm/xstate.h | 17 +++---
 xen/arch/x86/x86_emulate/blk.c    |  3 +-
 xen/arch/x86/xstate.c             | 15 +++--
 10 files changed, 90 insertions(+), 109 deletions(-)

-- 
2.34.1

[PATCH for-4.20 1/4] x86/xstate: Use compression check helper in xstate_all()

[Alejandro Vallejo]

Minor refactor to make xstate_all() use a helper rather than poking directly
into the XSAVE header.

No functional change

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/include/asm/xstate.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index f4a8e5f814a0..f0eeb13b87a4 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -122,6 +122,12 @@ static inline uint64_t xgetbv(unsigned int index)
     return lo | ((uint64_t)hi << 32);
 }
 
+static inline bool __nonnull(1)
+xsave_area_compressed(const struct xsave_struct *xsave_area)
+{
+    return xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED;
+}
+
 static inline bool xstate_all(const struct vcpu *v)
 {
     /*
@@ -129,15 +135,8 @@ static inline bool xstate_all(const struct vcpu *v)
      * (in the legacy region of xsave area) are fixed, so saving
      * XSTATE_FP_SSE will not cause overwriting problem with XSAVES/XSAVEC.
      */
-    return (v->arch.xsave_area->xsave_hdr.xcomp_bv &
-            XSTATE_COMPACTION_ENABLED) &&
+    return xsave_area_compressed(v->arch.xsave_area) &&
            (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
 }
 
-static inline bool __nonnull(1)
-xsave_area_compressed(const struct xsave_struct *xsave_area)
-{
-    return xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED;
-}
-
 #endif /* __ASM_XSTATE_H */
-- 
2.34.1

Re: [PATCH for-4.20 1/4] x86/xstate: Use compression check helper in xstate_all()

[Jan Beulich]

On 09.07.2024 17:52, Alejandro Vallejo wrote:
> Minor refactor to make xstate_all() use a helper rather than poking directly
> into the XSAVE header.
> 
> No functional change
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH for-4.20 2/4] x86/fpu: Create a typedef for the x87/SSE area inside "struct xsave_struct"

[Alejandro Vallejo]

Making the union non-anonymous causes a lot of headaches, because a lot of code
relies on it being so, but it's possible to make a typedef of the anonymous
union so all callsites currently relying on typeof() can stop doing so directly.

This commit creates a `fpusse_t` typedef to the anonymous union at the head of
the XSAVE area and uses it instead of typeof().

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/hvm/emulate.c        | 5 ++---
 xen/arch/x86/i387.c               | 8 ++++----
 xen/arch/x86/include/asm/xstate.h | 2 ++
 xen/arch/x86/xstate.c             | 2 +-
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 02e378365b40..65ee70ce67db 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2364,8 +2364,7 @@ static int cf_check hvmemul_get_fpu(
         alternative_vcall(hvm_funcs.fpu_dirty_intercept);
     else if ( type == X86EMUL_FPU_fpu )
     {
-        const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
-            curr->arch.fpu_ctxt;
+        const fpusse_t *fpu_ctxt = curr->arch.fpu_ctxt;
 
         /*
          * Latch current register state so that we can back out changes
@@ -2405,7 +2404,7 @@ static void cf_check hvmemul_put_fpu(
 
     if ( aux )
     {
-        typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt = curr->arch.fpu_ctxt;
+        fpusse_t *fpu_ctxt = curr->arch.fpu_ctxt;
         bool dval = aux->dval;
         int mode = hvm_guest_x86_mode(curr);
 
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index fcdee10a6e69..89804435b659 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -39,7 +39,7 @@ static inline void fpu_xrstor(struct vcpu *v, uint64_t mask)
 /* Restore x87 FPU, MMX, SSE and SSE2 state */
 static inline void fpu_fxrstor(struct vcpu *v)
 {
-    const typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
+    const fpusse_t *fpu_ctxt = v->arch.fpu_ctxt;
 
     /*
      * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
@@ -152,7 +152,7 @@ static inline void fpu_xsave(struct vcpu *v)
 /* Save x87 FPU, MMX, SSE and SSE2 state */
 static inline void fpu_fxsave(struct vcpu *v)
 {
-    typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
+    fpusse_t *fpu_ctxt = v->arch.fpu_ctxt;
     unsigned int fip_width = v->domain->arch.x87_fip_width;
 
     if ( fip_width != 4 )
@@ -322,7 +322,7 @@ int vcpu_init_fpu(struct vcpu *v)
                                     __alignof(v->arch.xsave_area->fpu_sse));
         if ( v->arch.fpu_ctxt )
         {
-            typeof(v->arch.xsave_area->fpu_sse) *fpu_sse = v->arch.fpu_ctxt;
+            fpusse_t *fpu_sse = v->arch.fpu_ctxt;
 
             fpu_sse->fcw = FCW_DEFAULT;
             fpu_sse->mxcsr = MXCSR_DEFAULT;
@@ -343,7 +343,7 @@ void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
      * accesses through both pointers alias one another, and the shorter form
      * is used here.
      */
-    typeof(xsave_area->fpu_sse) *fpu_sse = v->arch.fpu_ctxt;
+    fpusse_t *fpu_sse = v->arch.fpu_ctxt;
 
     ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
 
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index f0eeb13b87a4..ebeb2a3dcaf9 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -82,6 +82,8 @@ struct __attribute__((aligned (64))) xsave_struct
     char data[];                             /* Variable layout states */
 };
 
+typedef typeof(((struct xsave_struct){}).fpu_sse) fpusse_t;
+
 struct xstate_bndcsr {
     uint64_t bndcfgu;
     uint64_t bndstatus;
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 68cdd8fcf021..5c4144d55e89 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -846,7 +846,7 @@ void xstate_init(struct cpuinfo_x86 *c)
 
     if ( bsp )
     {
-        static typeof(current->arch.xsave_area->fpu_sse) __initdata ctxt;
+        static fpusse_t __initdata ctxt;
 
         asm ( "fxsave %0" : "=m" (ctxt) );
         if ( ctxt.mxcsr_mask )
-- 
2.34.1

Re: [PATCH for-4.20 2/4] x86/fpu: Create a typedef for the x87/SSE area inside "struct xsave_struct"

[Jan Beulich]

On 09.07.2024 17:52, Alejandro Vallejo wrote:
> Making the union non-anonymous causes a lot of headaches,

Maybe better "would cause", as that's not what you're doing here?

> because a lot of code
> relies on it being so, but it's possible to make a typedef of the anonymous
> union so all callsites currently relying on typeof() can stop doing so directly.
> 
> This commit creates a `fpusse_t` typedef to the anonymous union at the head of
> the XSAVE area and uses it instead of typeof().
> 
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH for-4.20 2/4] x86/fpu: Create a typedef for the x87/SSE area inside "struct xsave_struct"

[Alejandro Vallejo]

On Thu Jul 18, 2024 at 12:23 PM BST, Jan Beulich wrote:
> On 09.07.2024 17:52, Alejandro Vallejo wrote:
> > Making the union non-anonymous causes a lot of headaches,
>
> Maybe better "would cause", as that's not what you're doing here?

Yes, sounds better.

>
> > because a lot of code
> > relies on it being so, but it's possible to make a typedef of the anonymous
> > union so all callsites currently relying on typeof() can stop doing so directly.
> > 
> > This commit creates a `fpusse_t` typedef to the anonymous union at the head of
> > the XSAVE area and uses it instead of typeof().
> > 
> > No functional change.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks

Alejandro

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

[Alejandro Vallejo]

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.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/domctl.c             |  3 ++-
 xen/arch/x86/hvm/emulate.c        |  4 +--
 xen/arch/x86/hvm/hvm.c            |  3 ++-
 xen/arch/x86/i387.c               | 45 +++++--------------------------
 xen/arch/x86/include/asm/domain.h |  7 +----
 xen/arch/x86/x86_emulate/blk.c    |  3 ++-
 xen/arch/x86/xstate.c             | 13 ++++++---
 7 files changed, 25 insertions(+), 53 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9190e11faaa3..7b04b584c540 100644
--- 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));
     if ( is_pv_domain(d) )
         c(flags = v->arch.pv.vgc_flags & ~(VGCF_i387_valid|VGCF_in_kernel));
     else
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 65ee70ce67db..72a8136a9bbf 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2364,7 +2364,7 @@ static int cf_check hvmemul_get_fpu(
         alternative_vcall(hvm_funcs.fpu_dirty_intercept);
     else if ( type == X86EMUL_FPU_fpu )
     {
-        const fpusse_t *fpu_ctxt = curr->arch.fpu_ctxt;
+        const fpusse_t *fpu_ctxt = &curr->arch.xsave_area->fpu_sse;
 
         /*
          * Latch current register state so that we can back out changes
@@ -2404,7 +2404,7 @@ static void cf_check hvmemul_put_fpu(
 
     if ( aux )
     {
-        fpusse_t *fpu_ctxt = curr->arch.fpu_ctxt;
+        fpusse_t *fpu_ctxt = &curr->arch.xsave_area->fpu_sse;
         bool dval = aux->dval;
         int mode = hvm_guest_x86_mode(curr);
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7f4b627b1f5f..09b1426ee314 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -916,7 +916,8 @@ static int cf_check hvm_save_cpu_ctxt(struct vcpu *v, hvm_domain_context_t *h)
 
     if ( v->fpu_initialised )
     {
-        memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt, sizeof(ctxt.fpu_regs));
+        memcpy(ctxt.fpu_regs, &v->arch.xsave_area->fpu_sse,
+               sizeof(ctxt.fpu_regs));
         ctxt.flags = XEN_X86_FPU_INITIALISED;
     }
 
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 89804435b659..a964b84757ec 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -39,7 +39,7 @@ static inline void fpu_xrstor(struct vcpu *v, uint64_t mask)
 /* Restore x87 FPU, MMX, SSE and SSE2 state */
 static inline void fpu_fxrstor(struct vcpu *v)
 {
-    const fpusse_t *fpu_ctxt = v->arch.fpu_ctxt;
+    const fpusse_t *fpu_ctxt = &v->arch.xsave_area->fpu_sse;
 
     /*
      * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
@@ -152,7 +152,7 @@ static inline void fpu_xsave(struct vcpu *v)
 /* Save x87 FPU, MMX, SSE and SSE2 state */
 static inline void fpu_fxsave(struct vcpu *v)
 {
-    fpusse_t *fpu_ctxt = v->arch.fpu_ctxt;
+    fpusse_t *fpu_ctxt = &v->arch.xsave_area->fpu_sse;
     unsigned int fip_width = v->domain->arch.x87_fip_width;
 
     if ( fip_width != 4 )
@@ -219,7 +219,7 @@ void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts)
      * above) we also need to restore full state, to prevent subsequently
      * saving state belonging to another vCPU.
      */
-    if ( v->arch.fully_eager_fpu || (v->arch.xsave_area && xstate_all(v)) )
+    if ( v->arch.fully_eager_fpu || xstate_all(v) )
     {
         if ( cpu_has_xsave )
             fpu_xrstor(v, XSTATE_ALL);
@@ -306,44 +306,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;
-    }
-
-    return rc;
+    return xstate_alloc_save_area(v);
 }
 
 void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
                     const void *data, unsigned int fcw_default)
 {
-    /*
-     * For the entire function please note that vcpu_init_fpu() (above) points
-     * v->arch.fpu_ctxt into v->arch.xsave_area when XSAVE is available. Hence
-     * accesses through both pointers alias one another, and the shorter form
-     * is used here.
-     */
-    fpusse_t *fpu_sse = v->arch.fpu_ctxt;
+    fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse;
 
     ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
 
@@ -380,10 +350,7 @@ void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
 /* Free FPU's context save area */
 void vcpu_destroy_fpu(struct vcpu *v)
 {
-    if ( v->arch.xsave_area )
-        xstate_free_save_area(v);
-    else
-        xfree(v->arch.fpu_ctxt);
+    xstate_free_save_area(v);
 }
 
 /*
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index f5daeb182baa..4740a1244fbb 100644
--- 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;
 
     /* Debug registers. */
diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
index e790f4f90056..8160da07295b 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)
 # 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.
+         */
+        size = XSTATE_AREA_MIN_SIZE;
+    }
+    else if ( !is_idle_vcpu(v) || !cpu_has_xsavec )
     {
         size = xsave_cntxt_size;
         BUG_ON(size < XSTATE_AREA_MIN_SIZE);
-- 
2.34.1

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

[Jan Beulich]

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

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

[Alejandro Vallejo]

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

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

[Jan Beulich]

On 18.07.2024 18:54, Alejandro Vallejo wrote:
> 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/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.

As said, I'd prefer if you amended the existing comment. Properly describing
what's in cpu_user_regs isn't quite as easy in only very few words. Neither
"fixed point register" nor "general purpose registers" really covers it. And
I'd really like to avoid having potentially confusing comments.

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

No, I didn't. I've in fact commented on it precisely because it is the value
you name. That's larger than necessary, and when suitably shrunk - as said -
one more of these structures could fit in a page (assuming they were all
allocated back-to-back, which isn't quite true right now, but other
intervening allocations may or may not take space from the same page, so
chances are still that the ones here all might come from one page as long as
there's space left).

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

But that's my point: The reserved[] part doesn't need to be there; it's
not being accessed anywhere, I don't think.

Jan

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

[Alejandro Vallejo]

Hi. Sorry, I've been busy and couldn't get back to this sooner.

On Fri Jul 19, 2024 at 10:14 AM BST, Jan Beulich wrote:
> On 18.07.2024 18:54, Alejandro Vallejo wrote:
> > 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/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.
>
> As said, I'd prefer if you amended the existing comment. Properly describing
> what's in cpu_user_regs isn't quite as easy in only very few words. Neither
> "fixed point register" nor "general purpose registers" really covers it. And
> I'd really like to avoid having potentially confusing comments.

Sure.

>
> >>> --- 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).
> > 
> > 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?
>
> No, I didn't. I've in fact commented on it precisely because it is the value
> you name. That's larger than necessary, and when suitably shrunk - as said -
> one more of these structures could fit in a page (assuming they were all
> allocated back-to-back, which isn't quite true right now, but other
> intervening allocations may or may not take space from the same page, so
> chances are still that the ones here all might come from one page as long as
> there's space left).
>
> >     #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)
>
> But that's my point: The reserved[] part doesn't need to be there; it's
> not being accessed anywhere, I don't think.
>
> Jan

The reserved octets want to be in the structure so we can zero them out on
machines with XSAVE. Once they are it's quite dangerous to not allocate space
for them, as doing something like "*xsave_area = *foo;" would overflow the
allocation silently (that's in fact what the next patch does).

Combined with the fact that (a) not having XSAVE is fairly rare and (b) all of
this is bound to end up round to the page anyway once we get Address Space
Isolation going... it really doesn't motivate me to save a handful of octets
per vCPU. The cost in memory would be negligible even if we had tens of
thousands of them running concurrently.

Cheers,
Alejandro

[PATCH for-4.20 4/4] x86/fpu: Split fpu_setup_fpu() in two

[Alejandro Vallejo]

It's doing too many things at once and there's no clear way of defining what
it's meant to do. This patch splits the function in two.

  1. A reset function, parameterized by the FCW value. FCW_RESET means to reset
     the state to power-on reset values, while FCW_DEFAULT means to reset to the
     default values present during vCPU creation.
  2. A x87/SSE state loader (equivalent to the old function when it took a data
     pointer).

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
I'm still not sure what the old function tries to do. The state we start vCPUs
in is _similar_ to the after-finit, but it's not quite (`ftw` is not -1). I went
for the "let's not deviate too much from previous behaviour", but maybe we did
intend for vCPUs to start as if `finit` had just been executed?
---
 xen/arch/x86/domain.c           |  7 +++--
 xen/arch/x86/hvm/hvm.c          | 19 ++++++++-----
 xen/arch/x86/i387.c             | 50 +++++++++++----------------------
 xen/arch/x86/include/asm/i387.h | 27 +++++++++++++++---
 4 files changed, 56 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ccadfe0c9e70..245899cc792f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1198,9 +1198,10 @@ int arch_set_info_guest(
          is_pv_64bit_domain(d) )
         v->arch.flags &= ~TF_kernel_mode;
 
-    vcpu_setup_fpu(v, v->arch.xsave_area,
-                   flags & VGCF_I387_VALID ? &c.nat->fpu_ctxt : NULL,
-                   FCW_DEFAULT);
+    if ( flags & VGCF_I387_VALID )
+        vcpu_setup_fpu(v, &c.nat->fpu_ctxt);
+    else
+        vcpu_reset_fpu(v, FCW_DEFAULT);
 
     if ( !compat )
     {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 09b1426ee314..bedbd2a0b888 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1162,10 +1162,17 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     seg.attr = ctxt.ldtr_arbytes;
     hvm_set_segment_register(v, x86_seg_ldtr, &seg);
 
-    /* Cover xsave-absent save file restoration on xsave-capable host. */
-    vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area,
-                   ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL,
-                   FCW_RESET);
+    /*
+     * On Xen 4.1 and later the FPU state is restored on a later HVM context, so
+     * what we're doing here is initialising the FPU state for guests from even
+     * older versions of Xen. In general such guests only use legacy x87/SSE
+     * state, and if they did use XSAVE then our best-effort strategy is to make
+     * an XSAVE header for x87 and SSE hoping that's good enough.
+     */
+    if ( ctxt.flags & XEN_X86_FPU_INITIALISED )
+        vcpu_setup_fpu(v, &ctxt.fpu_regs);
+    else
+        vcpu_reset_fpu(v, FCW_RESET);
 
     v->arch.user_regs.rax = ctxt.rax;
     v->arch.user_regs.rbx = ctxt.rbx;
@@ -4005,9 +4012,7 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
         v->arch.guest_table = pagetable_null();
     }
 
-    if ( v->arch.xsave_area )
-        v->arch.xsave_area->xsave_hdr.xstate_bv = 0;
-    vcpu_setup_fpu(v, v->arch.xsave_area, NULL, FCW_RESET);
+    vcpu_reset_fpu(v, FCW_RESET);
 
     arch_vcpu_regs_init(v);
     v->arch.user_regs.rip = ip;
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index a964b84757ec..7851f1b3f6e4 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -310,41 +310,25 @@ int vcpu_init_fpu(struct vcpu *v)
     return xstate_alloc_save_area(v);
 }
 
-void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
-                    const void *data, unsigned int fcw_default)
+void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw)
 {
-    fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse;
-
-    ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
-
-    v->fpu_initialised = !!data;
-
-    if ( data )
-    {
-        memcpy(fpu_sse, data, sizeof(*fpu_sse));
-        if ( xsave_area )
-            xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
-    }
-    else if ( xsave_area && fcw_default == FCW_DEFAULT )
-    {
-        xsave_area->xsave_hdr.xstate_bv = 0;
-        fpu_sse->mxcsr = MXCSR_DEFAULT;
-    }
-    else
-    {
-        memset(fpu_sse, 0, sizeof(*fpu_sse));
-        fpu_sse->fcw = fcw_default;
-        fpu_sse->mxcsr = MXCSR_DEFAULT;
-        if ( v->arch.xsave_area )
-        {
-            v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE;
-            if ( fcw_default != FCW_DEFAULT )
-                v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87;
-        }
-    }
+    v->fpu_initialised = false;
+    *v->arch.xsave_area = (struct xsave_struct) {
+        .fpu_sse = {
+            .mxcsr = MXCSR_DEFAULT,
+            .fcw = fcw,
+        },
+        .xsave_hdr.xstate_bv = fcw == FCW_RESET ? X86_XCR0_X87 : 0,
+    };
+}
 
-    if ( xsave_area )
-        xsave_area->xsave_hdr.xcomp_bv = 0;
+void vcpu_setup_fpu(struct vcpu *v, const void *data)
+{
+    v->fpu_initialised = true;
+    *v->arch.xsave_area = (struct xsave_struct) {
+        .fpu_sse = *(fpusse_t*)data,
+        .xsave_hdr.xstate_bv = XSTATE_FP_SSE,
+    };
 }
 
 /* Free FPU's context save area */
diff --git a/xen/arch/x86/include/asm/i387.h b/xen/arch/x86/include/asm/i387.h
index a783549db991..ce699fc66663 100644
--- a/xen/arch/x86/include/asm/i387.h
+++ b/xen/arch/x86/include/asm/i387.h
@@ -31,10 +31,29 @@ 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);
-
 int vcpu_init_fpu(struct vcpu *v);
-struct xsave_struct;
-void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
-                    const void *data, unsigned int fcw_default);
 void vcpu_destroy_fpu(struct vcpu *v);
+
+/*
+ * Restore `v`'s FPU to known values
+ *
+ * If fcw == FCW_RESET, then the reset state is power-on RESET.
+ *
+ * Otherwise `mxcsr` is set to `MXCSR_DEFAULT`, `fcw` is overriden with the
+ * `fcw` argument and everything else is zeroed out.
+ *
+ * @param v   vCPU containing the FPU
+ * @param fcw Intended FPU Control Word
+ */
+void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw);
+
+/*
+ * Load x87/SSE state into `v`'s FPU
+ *
+ * Overrides the XSAVE header to set the state components to be x87 and SSE.
+ *
+ * @param v    vCPU containing the FPU
+ * @param data 512-octet blob for x87/SSE state
+ */
+void vcpu_setup_fpu(struct vcpu *v, const void *data);
 #endif /* __ASM_I386_I387_H */
-- 
2.34.1

Re: [PATCH for-4.20 4/4] x86/fpu: Split fpu_setup_fpu() in two

[Jan Beulich]

On 09.07.2024 17:52, Alejandro Vallejo wrote:
> It's doing too many things at once and there's no clear way of defining what
> it's meant to do. This patch splits the function in two.
> 
>   1. A reset function, parameterized by the FCW value. FCW_RESET means to reset
>      the state to power-on reset values, while FCW_DEFAULT means to reset to the
>      default values present during vCPU creation.
>   2. A x87/SSE state loader (equivalent to the old function when it took a data
>      pointer).
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> I'm still not sure what the old function tries to do. The state we start vCPUs
> in is _similar_ to the after-finit, but it's not quite (`ftw` is not -1). I went
> for the "let's not deviate too much from previous behaviour", but maybe we did
> intend for vCPUs to start as if `finit` had just been executed?

A relevant aspect here may be that what FSXR and XSAVE area have is only an
abridged form of the tag word, being only 8 bits in size. 0x00 there is
equivalent to FTW=0xffff (all st(<N>) empty). That's not quite correct for
the reset case indeed, where FTW=0x5555 (i.e. all st(<N>) zero, requiring
the abridged form to hold 0xff instead). While no-one has reported issues
there so far, I think it wouldn't be inappropriate to correct this.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1162,10 +1162,17 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>      seg.attr = ctxt.ldtr_arbytes;
>      hvm_set_segment_register(v, x86_seg_ldtr, &seg);
>  
> -    /* Cover xsave-absent save file restoration on xsave-capable host. */
> -    vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area,
> -                   ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL,
> -                   FCW_RESET);
> +    /*
> +     * On Xen 4.1 and later the FPU state is restored on a later HVM context, so
> +     * what we're doing here is initialising the FPU state for guests from even
> +     * older versions of Xen. In general such guests only use legacy x87/SSE
> +     * state, and if they did use XSAVE then our best-effort strategy is to make
> +     * an XSAVE header for x87 and SSE hoping that's good enough.
> +     */
> +    if ( ctxt.flags & XEN_X86_FPU_INITIALISED )
> +        vcpu_setup_fpu(v, &ctxt.fpu_regs);
> +    else
> +        vcpu_reset_fpu(v, FCW_RESET);

I'm struggling with the use of "later" in the comment. What exactly is that
meant to express? Fundamentally the XSAVE data is fully backwards compatible
with the FXSR one, I think, so the mentioning of "best-effort" isn't quite
clear to me either.

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -310,41 +310,25 @@ int vcpu_init_fpu(struct vcpu *v)
>      return xstate_alloc_save_area(v);
>  }
>  
> -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
> -                    const void *data, unsigned int fcw_default)
> +void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw)
>  {
> -    fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse;
> -
> -    ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
> -
> -    v->fpu_initialised = !!data;
> -
> -    if ( data )
> -    {
> -        memcpy(fpu_sse, data, sizeof(*fpu_sse));
> -        if ( xsave_area )
> -            xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> -    }
> -    else if ( xsave_area && fcw_default == FCW_DEFAULT )
> -    {
> -        xsave_area->xsave_hdr.xstate_bv = 0;
> -        fpu_sse->mxcsr = MXCSR_DEFAULT;
> -    }
> -    else
> -    {
> -        memset(fpu_sse, 0, sizeof(*fpu_sse));
> -        fpu_sse->fcw = fcw_default;
> -        fpu_sse->mxcsr = MXCSR_DEFAULT;
> -        if ( v->arch.xsave_area )
> -        {
> -            v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE;
> -            if ( fcw_default != FCW_DEFAULT )
> -                v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87;
> -        }
> -    }
> +    v->fpu_initialised = false;
> +    *v->arch.xsave_area = (struct xsave_struct) {
> +        .fpu_sse = {
> +            .mxcsr = MXCSR_DEFAULT,
> +            .fcw = fcw,
> +        },
> +        .xsave_hdr.xstate_bv = fcw == FCW_RESET ? X86_XCR0_X87 : 0,
> +    };
> +}

Old code checked against FCW_DEFAULT uniformly. You switching to checking
against FCW_RESET is no functional change only because all callers pass
either of the two values. I wonder whether the new function's parameter
wouldn't want to be a boolean (reset vs init).

> -    if ( xsave_area )
> -        xsave_area->xsave_hdr.xcomp_bv = 0;
> +void vcpu_setup_fpu(struct vcpu *v, const void *data)
> +{
> +    v->fpu_initialised = true;
> +    *v->arch.xsave_area = (struct xsave_struct) {
> +        .fpu_sse = *(fpusse_t*)data,

First of all please never cast away const. See Misra rule 11.8. And then
a nit again: Blank ahead of the latter of the two *-s, please.

> --- a/xen/arch/x86/include/asm/i387.h
> +++ b/xen/arch/x86/include/asm/i387.h
> @@ -31,10 +31,29 @@ 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);
> -
>  int vcpu_init_fpu(struct vcpu *v);
> -struct xsave_struct;
> -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
> -                    const void *data, unsigned int fcw_default);
>  void vcpu_destroy_fpu(struct vcpu *v);
> +
> +/*
> + * Restore `v`'s FPU to known values
> + *
> + * If fcw == FCW_RESET, then the reset state is power-on RESET.
> + *
> + * Otherwise `mxcsr` is set to `MXCSR_DEFAULT`, `fcw` is overriden with the
> + * `fcw` argument and everything else is zeroed out.

Backticks are used for two different purposes here, which I'm afraid is
confusing. You want to make it easy to tell function arguments from other
entities, imo.

> + * @param v   vCPU containing the FPU
> + * @param fcw Intended FPU Control Word
> + */
> +void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw);
> +
> +/*
> + * Load x87/SSE state into `v`'s FPU

Applicable here then as well.

Jan

Re: [PATCH for-4.20 4/4] x86/fpu: Split fpu_setup_fpu() in two

[Alejandro Vallejo]

On Thu Jul 18, 2024 at 1:19 PM BST, Jan Beulich wrote:
> On 09.07.2024 17:52, Alejandro Vallejo wrote:
> > It's doing too many things at once and there's no clear way of defining what
> > it's meant to do. This patch splits the function in two.
> > 
> >   1. A reset function, parameterized by the FCW value. FCW_RESET means to reset
> >      the state to power-on reset values, while FCW_DEFAULT means to reset to the
> >      default values present during vCPU creation.
> >   2. A x87/SSE state loader (equivalent to the old function when it took a data
> >      pointer).
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> > I'm still not sure what the old function tries to do. The state we start vCPUs
> > in is _similar_ to the after-finit, but it's not quite (`ftw` is not -1). I went
> > for the "let's not deviate too much from previous behaviour", but maybe we did
> > intend for vCPUs to start as if `finit` had just been executed?
>
> A relevant aspect here may be that what FSXR and XSAVE area have is only an
> abridged form of the tag word, being only 8 bits in size. 0x00 there is
> equivalent to FTW=0xffff (all st(<N>) empty). That's not quite correct for
> the reset case indeed, where FTW=0x5555 (i.e. all st(<N>) zero, requiring

I missed the tag being abridged. That makes a lot of sense, thanks.

> the abridged form to hold 0xff instead). While no-one has reported issues
> there so far, I think it wouldn't be inappropriate to correct this.

Ack, I'll add it on v2.

>
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1162,10 +1162,17 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> >      seg.attr = ctxt.ldtr_arbytes;
> >      hvm_set_segment_register(v, x86_seg_ldtr, &seg);
> >  
> > -    /* Cover xsave-absent save file restoration on xsave-capable host. */
> > -    vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area,
> > -                   ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL,
> > -                   FCW_RESET);
> > +    /*
> > +     * On Xen 4.1 and later the FPU state is restored on a later HVM context, so
> > +     * what we're doing here is initialising the FPU state for guests from even
> > +     * older versions of Xen. In general such guests only use legacy x87/SSE
> > +     * state, and if they did use XSAVE then our best-effort strategy is to make
> > +     * an XSAVE header for x87 and SSE hoping that's good enough.
> > +     */
> > +    if ( ctxt.flags & XEN_X86_FPU_INITIALISED )
> > +        vcpu_setup_fpu(v, &ctxt.fpu_regs);
> > +    else
> > +        vcpu_reset_fpu(v, FCW_RESET);
>
> I'm struggling with the use of "later" in the comment. What exactly is that
> meant to express? Fundamentally the XSAVE data is fully backwards compatible
> with the FXSR one, I think, so the mentioning of "best-effort" isn't quite
> clear to me either.

I meant that the XSAVE state (including FPU/SSE state) is passed not on the HVM
context struct being process _here_, but another one that will arrive later on
in the stream. There's 3 interesting cases regarding extended states:

  1. If there is an XSAVE context later in the stream, what we do here for the
     FPU doesn't matter because it'll be overriden later. That's fine.
  2. If there isn't and the guest didn't use extended states  it's still fine
     because we have all the information we need here.
  2. If there isn't but the guest DID use extended states (could've happened
     prior to Xen 4.1) then we're in a pickle because we have to make up
     non-existing state. This is what I meant by best effort.

Seeing how you got confused the comment probably needs to be rewritten to
better reflect this.

>
> > --- a/xen/arch/x86/i387.c
> > +++ b/xen/arch/x86/i387.c
> > @@ -310,41 +310,25 @@ int vcpu_init_fpu(struct vcpu *v)
> >      return xstate_alloc_save_area(v);
> >  }
> >  
> > -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
> > -                    const void *data, unsigned int fcw_default)
> > +void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw)
> >  {
> > -    fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse;
> > -
> > -    ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
> > -
> > -    v->fpu_initialised = !!data;
> > -
> > -    if ( data )
> > -    {
> > -        memcpy(fpu_sse, data, sizeof(*fpu_sse));
> > -        if ( xsave_area )
> > -            xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> > -    }
> > -    else if ( xsave_area && fcw_default == FCW_DEFAULT )
> > -    {
> > -        xsave_area->xsave_hdr.xstate_bv = 0;
> > -        fpu_sse->mxcsr = MXCSR_DEFAULT;
> > -    }
> > -    else
> > -    {
> > -        memset(fpu_sse, 0, sizeof(*fpu_sse));
> > -        fpu_sse->fcw = fcw_default;
> > -        fpu_sse->mxcsr = MXCSR_DEFAULT;
> > -        if ( v->arch.xsave_area )
> > -        {
> > -            v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE;
> > -            if ( fcw_default != FCW_DEFAULT )
> > -                v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87;
> > -        }
> > -    }
> > +    v->fpu_initialised = false;
> > +    *v->arch.xsave_area = (struct xsave_struct) {
> > +        .fpu_sse = {
> > +            .mxcsr = MXCSR_DEFAULT,
> > +            .fcw = fcw,
> > +        },
> > +        .xsave_hdr.xstate_bv = fcw == FCW_RESET ? X86_XCR0_X87 : 0,
> > +    };
> > +}
>
> Old code checked against FCW_DEFAULT uniformly. You switching to checking
> against FCW_RESET is no functional change only because all callers pass
> either of the two values. I wonder whether the new function's parameter
> wouldn't want to be a boolean (reset vs init).

I agree, and It's effectively what it is. The problem with the boolean is that
it's utterly unreadable at the call sites.

    vcpu_reset_fpu(v, true); /* Is this reset or set-to-default? */
    vcpu_reset_fpu(v, FCW_RESET); /* Clear to be a reset */

I could also split it in 2, so we end up with these:

  * vcpu_setup_fpu(v, data): Copies x87/SSE state
  * vcpu_reset_fpu(v): Reset to power-on state
  * vcpu_set_default_fpu(v): Reset to default state

Thinking about it, I kind of prefer this second approach. Thoughts?

>
> > -    if ( xsave_area )
> > -        xsave_area->xsave_hdr.xcomp_bv = 0;
> > +void vcpu_setup_fpu(struct vcpu *v, const void *data)
> > +{
> > +    v->fpu_initialised = true;
> > +    *v->arch.xsave_area = (struct xsave_struct) {
> > +        .fpu_sse = *(fpusse_t*)data,
>
> First of all please never cast away const. See Misra rule 11.8. And then
> a nit again: Blank ahead of the latter of the two *-s, please.
>

Bah, yes. You're right. Casting to (const fpusse_t *) instead should appease
the UB gods.

> > --- a/xen/arch/x86/include/asm/i387.h
> > +++ b/xen/arch/x86/include/asm/i387.h
> > @@ -31,10 +31,29 @@ 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);
> > -
> >  int vcpu_init_fpu(struct vcpu *v);
> > -struct xsave_struct;
> > -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
> > -                    const void *data, unsigned int fcw_default);
> >  void vcpu_destroy_fpu(struct vcpu *v);
> > +
> > +/*
> > + * Restore `v`'s FPU to known values
> > + *
> > + * If fcw == FCW_RESET, then the reset state is power-on RESET.
> > + *
> > + * Otherwise `mxcsr` is set to `MXCSR_DEFAULT`, `fcw` is overriden with the
> > + * `fcw` argument and everything else is zeroed out.
>
> Backticks are used for two different purposes here, which I'm afraid is
> confusing. You want to make it easy to tell function arguments from other
> entities, imo.
>

Fair enough, sure.

> > + * @param v   vCPU containing the FPU
> > + * @param fcw Intended FPU Control Word
> > + */
> > +void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw);
> > +
> > +/*
> > + * Load x87/SSE state into `v`'s FPU
>
> Applicable here then as well.
>
> Jan

Cheers,
Alejandro

Re: [PATCH for-4.20 4/4] x86/fpu: Split fpu_setup_fpu() in two

[Jan Beulich]

On 18.07.2024 19:25, Alejandro Vallejo wrote:
> On Thu Jul 18, 2024 at 1:19 PM BST, Jan Beulich wrote:
>> On 09.07.2024 17:52, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/i387.c
>>> +++ b/xen/arch/x86/i387.c
>>> @@ -310,41 +310,25 @@ int vcpu_init_fpu(struct vcpu *v)
>>>      return xstate_alloc_save_area(v);
>>>  }
>>>  
>>> -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
>>> -                    const void *data, unsigned int fcw_default)
>>> +void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw)
>>>  {
>>> -    fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse;
>>> -
>>> -    ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
>>> -
>>> -    v->fpu_initialised = !!data;
>>> -
>>> -    if ( data )
>>> -    {
>>> -        memcpy(fpu_sse, data, sizeof(*fpu_sse));
>>> -        if ( xsave_area )
>>> -            xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
>>> -    }
>>> -    else if ( xsave_area && fcw_default == FCW_DEFAULT )
>>> -    {
>>> -        xsave_area->xsave_hdr.xstate_bv = 0;
>>> -        fpu_sse->mxcsr = MXCSR_DEFAULT;
>>> -    }
>>> -    else
>>> -    {
>>> -        memset(fpu_sse, 0, sizeof(*fpu_sse));
>>> -        fpu_sse->fcw = fcw_default;
>>> -        fpu_sse->mxcsr = MXCSR_DEFAULT;
>>> -        if ( v->arch.xsave_area )
>>> -        {
>>> -            v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE;
>>> -            if ( fcw_default != FCW_DEFAULT )
>>> -                v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87;
>>> -        }
>>> -    }
>>> +    v->fpu_initialised = false;
>>> +    *v->arch.xsave_area = (struct xsave_struct) {
>>> +        .fpu_sse = {
>>> +            .mxcsr = MXCSR_DEFAULT,
>>> +            .fcw = fcw,
>>> +        },
>>> +        .xsave_hdr.xstate_bv = fcw == FCW_RESET ? X86_XCR0_X87 : 0,
>>> +    };
>>> +}
>>
>> Old code checked against FCW_DEFAULT uniformly. You switching to checking
>> against FCW_RESET is no functional change only because all callers pass
>> either of the two values. I wonder whether the new function's parameter
>> wouldn't want to be a boolean (reset vs init).
> 
> I agree, and It's effectively what it is. The problem with the boolean is that
> it's utterly unreadable at the call sites.
> 
>     vcpu_reset_fpu(v, true); /* Is this reset or set-to-default? */

    vcpu_reset_fpu(v, true /* reset */);

and

    vcpu_reset_fpu(v, false /* init */);

would be an option. But I get your point.

>     vcpu_reset_fpu(v, FCW_RESET); /* Clear to be a reset */
> 
> I could also split it in 2, so we end up with these:
> 
>   * vcpu_setup_fpu(v, data): Copies x87/SSE state
>   * vcpu_reset_fpu(v): Reset to power-on state
>   * vcpu_set_default_fpu(v): Reset to default state
> 
> Thinking about it, I kind of prefer this second approach. Thoughts?

I'd be okay with that seeing how small the two functions would end up
being, albeit I don't like the "set_default" part of the name very much.
If I could talk you into using "init" instead ...

Jan