[PATCH v3 00/12] x86: Address Space Isolation FPU preparations

[PATCH v3 00/12] x86: Address Space Isolation FPU preparations

[Alejandro Vallejo]

See original cover letter in v1

v2: https://lore.kernel.org/xen-devel/20241105143310.28301-1-alejandro.vallejo@cloud.com/
v2->v3:
  * Evaluated `v` argument in UNMAP operation (see patch 1)
  * Fixed bug on (de)compressing xstates by not unmapping them on early return.
  * Constified more when converting (f)xrstor and (f)xsave

v1: https://lore.kernel.org/xen-devel/20241028154932.6797-1-alejandro.vallejo@cloud.com/
v1->v2:
  * Turned v1/patch1 into an assert removal
  * Dropped v1/patch11: "x86/mpx: Adjust read_bndcfgu() to clean after itself"
  * Other minor changes out of feedback. Explained in each patch.

Alejandro Vallejo (12):
  x86/xstate: Create map/unmap primitives for xsave areas
  x86/hvm: Map/unmap xsave area in hvm_save_cpu_ctxt()
  x86/fpu: Map/umap xsave area in vcpu_{reset,setup}_fpu()
  x86/xstate: Map/unmap xsave area in xstate_set_init() and
    handle_setbv()
  x86/hvm: Map/unmap xsave area in hvmemul_{get,put}_fpu()
  x86/domctl: Map/unmap xsave area in arch_get_info_guest()
  x86/xstate: Map/unmap xsave area in {compress,expand}_xsave_states()
  x86/emulator: Refactor FXSAVE_AREA to use wrappers
  x86/mpx: Map/unmap xsave area in in read_bndcfgu()
  x86/fpu: Pass explicit xsave areas to fpu_(f)xsave()
  x86/fpu: Pass explicit xsave areas to fpu_(f)xrstor()
  x86/xstate: Make xstate_all() and vcpu_xsave_mask() take explicit
    xstate

 xen/arch/x86/domctl.c             |  9 ++--
 xen/arch/x86/hvm/emulate.c        | 12 +++++-
 xen/arch/x86/hvm/hvm.c            |  8 ++--
 xen/arch/x86/i387.c               | 69 ++++++++++++++++++++-----------
 xen/arch/x86/include/asm/xstate.h | 51 +++++++++++++++++++++--
 xen/arch/x86/x86_emulate/blk.c    | 11 ++++-
 xen/arch/x86/xstate.c             | 53 +++++++++++++++++-------
 7 files changed, 157 insertions(+), 56 deletions(-)

-- 
2.47.1

[PATCH v3 01/12] x86/xstate: Create map/unmap primitives for xsave areas

[Alejandro Vallejo]

Add infrastructure to simplify ASI handling. With ASI in the picture
we'll have several different means of accessing the XSAVE area of a
given vCPU, depending on whether a domain is covered by ASI or not and
whether the vCPU is question is scheduled on the current pCPU or not.

Having these complexities exposed at the call sites becomes unwieldy
very fast. These wrappers are intended to be used in a similar way to
map_domain_page() and unmap_domain_page(); The map operation will
dispatch the appropriate pointer for each case in a future patch, while
unmap will remain a no-op where no unmap is required (e.g: when there's
no ASI) and remove the transient maping if one was required.

Follow-up patches replace all uses of raw v->arch.xsave_area by this
mechanism in preparation to add the beforementioned dispatch logic to be
added at a later time.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * Evaluate `v` in UNMAP (casted to void)

v1->v2:
  * Comment macros more heavily to show their performance characteristics.
  * Addressed various nits in the macro comments.
  * Macro names to uppercase.
---
 xen/arch/x86/include/asm/xstate.h | 42 +++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index 07017cc4edfd..ab81a4c8527e 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -143,4 +143,46 @@ static inline bool xstate_all(const struct vcpu *v)
            (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
 }
 
+/*
+ * Fetch a pointer to a vCPU's XSAVE area
+ *
+ * TL;DR: If v == current, the mapping is guaranteed to already exist.
+ *
+ * Despite the name, this macro might not actually map anything. The only case
+ * in which a mutation of page tables is strictly required is when ASI==on &&
+ * v!=current. For everything else the mapping already exists and needs not
+ * be created nor destroyed.
+ *
+ *                         +-----------------+--------------+
+ *                         |   v == current  | v != current |
+ *          +--------------+-----------------+--------------+
+ *          | ASI  enabled | per-vCPU fixmap |  actual map  |
+ *          +--------------+-----------------+--------------+
+ *          | ASI disabled |             directmap          |
+ *          +--------------+--------------------------------+
+ *
+ * There MUST NOT be outstanding maps of XSAVE areas of the non-current vCPU
+ * at the point of context switch. Otherwise, the unmap operation will
+ * misbehave.
+ *
+ * TODO: Expand the macro to the ASI cases after infra to do so is in place.
+ *
+ * @param v Owner of the XSAVE area
+ */
+#define VCPU_MAP_XSAVE_AREA(v) ((v)->arch.xsave_area)
+
+/*
+ * Drops the mapping of a vCPU's XSAVE area and nullifies its pointer on exit
+ *
+ * See VCPU_MAP_XSAVE_AREA() for additional information on the persistence of
+ * these mappings. This macro only tears down the mappings in the ASI=on &&
+ * v!=current case.
+ *
+ * TODO: Expand the macro to the ASI cases after infra to do so is in place.
+ *
+ * @param v Owner of the XSAVE area
+ * @param x XSAVE blob of v
+ */
+#define VCPU_UNMAP_XSAVE_AREA(v, x) do { (void)(v); (x) = NULL; } while(0)
+
 #endif /* __ASM_XSTATE_H */
-- 
2.47.1

[PATCH v3 02/12] x86/hvm: Map/unmap xsave area in hvm_save_cpu_ctxt()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v2->v3:
  * Added A-by

v1->v2:
  * No change
---
 xen/arch/x86/hvm/hvm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 922c9b3af64d..884b2b9d32cd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -914,11 +914,11 @@ static int cf_check hvm_save_cpu_ctxt(struct vcpu *v, hvm_domain_context_t *h)
 
     if ( v->fpu_initialised )
     {
-        BUILD_BUG_ON(sizeof(ctxt.fpu_regs) !=
-                     sizeof(v->arch.xsave_area->fpu_sse));
-        memcpy(ctxt.fpu_regs, &v->arch.xsave_area->fpu_sse,
-               sizeof(ctxt.fpu_regs));
+        const struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(v);
 
+        BUILD_BUG_ON(sizeof(ctxt.fpu_regs) != sizeof(xsave_area->fpu_sse));
+        memcpy(ctxt.fpu_regs, &xsave_area->fpu_sse, sizeof(ctxt.fpu_regs));
+        VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
         ctxt.flags = XEN_X86_FPU_INITIALISED;
     }
 
-- 
2.47.1

[PATCH v3 03/12] x86/fpu: Map/umap xsave area in vcpu_{reset,setup}_fpu()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v2->v3:
  * Added A-by

v1->v2:
  * No change
---
 xen/arch/x86/i387.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 8fba0aef4284..5429531ddd5f 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -304,24 +304,32 @@ int vcpu_init_fpu(struct vcpu *v)
 
 void vcpu_reset_fpu(struct vcpu *v)
 {
+    struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(v);
+
     v->fpu_initialised = false;
-    *v->arch.xsave_area = (struct xsave_struct) {
+    *xsave_area = (struct xsave_struct) {
         .xsave_hdr.xstate_bv = X86_XCR0_X87,
     };
 
     /* Old gcc doesn't permit these to be part of the initializer. */
-    v->arch.xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
-    v->arch.xsave_area->fpu_sse.fcw = FCW_RESET;
-    v->arch.xsave_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
+    xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
+    xsave_area->fpu_sse.fcw = FCW_RESET;
+    xsave_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
+
+    VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
 }
 
 void vcpu_setup_fpu(struct vcpu *v, const void *data)
 {
+    struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(v);
+
     v->fpu_initialised = true;
-    *v->arch.xsave_area = (struct xsave_struct) {
+    *xsave_area = (struct xsave_struct) {
         .fpu_sse = *(const fpusse_t*)data,
         .xsave_hdr.xstate_bv = XSTATE_FP_SSE,
     };
+
+    VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
 }
 
 /* Free FPU's context save area */
-- 
2.47.1

[PATCH v3 04/12] x86/xstate: Map/unmap xsave area in xstate_set_init() and handle_setbv()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v2->v3:
  * style: Capitalized first letter of the comment.
  * Added A-by

v1->v2:
  * Added comment highlighting fastpath for current
---
 xen/arch/x86/xstate.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index af9e345a7ace..12004d7db24b 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -993,7 +993,13 @@ int handle_xsetbv(u32 index, u64 new_bv)
 
         clts();
         if ( curr->fpu_dirtied )
-            asm ( "stmxcsr %0" : "=m" (curr->arch.xsave_area->fpu_sse.mxcsr) );
+        {
+            /* Has a fastpath for `current`, so there's no actual map */
+            struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(curr);
+
+            asm ( "stmxcsr %0" : "=m" (xsave_area->fpu_sse.mxcsr) );
+            VCPU_UNMAP_XSAVE_AREA(curr, xsave_area);
+        }
         else if ( xstate_all(curr) )
         {
             /* See the comment in i387.c:vcpu_restore_fpu_eager(). */
@@ -1048,7 +1054,7 @@ void xstate_set_init(uint64_t mask)
     unsigned long cr0 = read_cr0();
     unsigned long xcr0 = this_cpu(xcr0);
     struct vcpu *v = idle_vcpu[smp_processor_id()];
-    struct xsave_struct *xstate = v->arch.xsave_area;
+    struct xsave_struct *xstate;
 
     if ( ~xfeature_mask & mask )
     {
@@ -1061,8 +1067,10 @@ void xstate_set_init(uint64_t mask)
 
     clts();
 
+    xstate = VCPU_MAP_XSAVE_AREA(v);
     memset(&xstate->xsave_hdr, 0, sizeof(xstate->xsave_hdr));
     xrstor(v, mask);
+    VCPU_UNMAP_XSAVE_AREA(v, xstate);
 
     if ( cr0 & X86_CR0_TS )
         write_cr0(cr0);
-- 
2.47.1

[PATCH v3 05/12] x86/hvm: Map/unmap xsave area in hvmemul_{get,put}_fpu()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v2->v3:
  * style: Capitalized first letter of the fastpath comment
  * Added A-by

v1->v2:
  * Added comments highlighting fastpath for current
---
 xen/arch/x86/hvm/emulate.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index a1935a174830..d5011612aff0 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2371,7 +2371,9 @@ 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.xsave_area->fpu_sse;
+        /* Has a fastpath for `current`, so there's no actual map */
+        const struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(curr);
+        const fpusse_t *fpu_ctxt = &xsave_area->fpu_sse;
 
         /*
          * Latch current register state so that we can back out changes
@@ -2397,6 +2399,8 @@ static int cf_check hvmemul_get_fpu(
             else
                 ASSERT(fcw == fpu_ctxt->fcw);
         }
+
+        VCPU_UNMAP_XSAVE_AREA(curr, xsave_area);
     }
 
     return X86EMUL_OKAY;
@@ -2411,7 +2415,9 @@ static void cf_check hvmemul_put_fpu(
 
     if ( aux )
     {
-        fpusse_t *fpu_ctxt = &curr->arch.xsave_area->fpu_sse;
+        /* Has a fastpath for `current`, so there's no actual map */
+        struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(curr);
+        fpusse_t *fpu_ctxt = &xsave_area->fpu_sse;
         bool dval = aux->dval;
         int mode = hvm_guest_x86_mode(curr);
 
@@ -2467,6 +2473,8 @@ static void cf_check hvmemul_put_fpu(
 
         fpu_ctxt->fop = aux->op;
 
+        VCPU_UNMAP_XSAVE_AREA(curr, xsave_area);
+
         /* Re-use backout code below. */
         backout = X86EMUL_FPU_fpu;
     }
-- 
2.47.1

[PATCH v3 06/12] x86/domctl: Map/unmap xsave area in arch_get_info_guest()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v1->v2:
  * Added A-by

v1->v2:
  * No change
---
 xen/arch/x86/domctl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 5f01111619da..3044f706de1c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1377,16 +1377,17 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
     unsigned int i;
     const struct domain *d = v->domain;
     bool compat = is_pv_32bit_domain(d);
+    const struct xsave_struct *xsave_area;
 #ifdef CONFIG_COMPAT
 #define c(fld) (!compat ? (c.nat->fld) : (c.cmp->fld))
 #else
 #define c(fld) (c.nat->fld)
 #endif
 
-    BUILD_BUG_ON(sizeof(c.nat->fpu_ctxt) !=
-                 sizeof(v->arch.xsave_area->fpu_sse));
-    memcpy(&c.nat->fpu_ctxt, &v->arch.xsave_area->fpu_sse,
-           sizeof(c.nat->fpu_ctxt));
+    xsave_area = VCPU_MAP_XSAVE_AREA(v);
+    BUILD_BUG_ON(sizeof(c.nat->fpu_ctxt) != sizeof(xsave_area->fpu_sse));
+    memcpy(&c.nat->fpu_ctxt, &xsave_area->fpu_sse, sizeof(c.nat->fpu_ctxt));
+    VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
 
     if ( is_pv_domain(d) )
         c(flags = v->arch.pv.vgc_flags & ~(VGCF_i387_valid|VGCF_in_kernel));
-- 
2.47.1

[PATCH v3 07/12] x86/xstate: Map/unmap xsave area in {compress,expand}_xsave_states()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * Unmap xsave area also before the early return.

v1->v2:
  * No change
---
 xen/arch/x86/xstate.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 12004d7db24b..3d249518a1b7 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -177,7 +177,7 @@ static void setup_xstate_comp(uint16_t *comp_offsets,
  */
 void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
 {
-    const struct xsave_struct *xstate = v->arch.xsave_area;
+    const struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);
     const void *src;
     uint16_t comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv = xstate->xsave_hdr.xstate_bv;
@@ -191,7 +191,7 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
     if ( !(xstate->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
     {
         memcpy(dest, xstate, size);
-        return;
+        goto out;
     }
 
     ASSERT(xsave_area_compressed(xstate));
@@ -228,6 +228,9 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
 
         valid &= ~feature;
     }
+
+ out:
+    VCPU_UNMAP_XSAVE_AREA(v, xstate);
 }
 
 /*
@@ -242,7 +245,7 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
  */
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 {
-    struct xsave_struct *xstate = v->arch.xsave_area;
+    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);
     void *dest;
     uint16_t comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv, valid;
@@ -256,7 +259,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {
         memcpy(xstate, src, size);
-        return;
+        goto out;
     }
 
     /*
@@ -294,6 +297,9 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 
         valid &= ~feature;
     }
+
+ out:
+    VCPU_UNMAP_XSAVE_AREA(v, xstate);
 }
 
 void xsave(struct vcpu *v, uint64_t mask)
-- 
2.47.1

[PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Alejandro Vallejo]

Adds an UNMAP primitive to make use of vcpu_unmap_xsave_area() when
linked into xen. unmap is a no-op during tests.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * Fixed typo in first parameter of UNMAP_FXSAVE_AREA.
  * Added Parenthesis around "x" in #else's UNMAP_FXSAVE_AREA(x)

v1->v2:
  * Added comments highlighting fastpath on `current`
---
 xen/arch/x86/x86_emulate/blk.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
index 08a05f8453f7..11b16aeaec39 100644
--- a/xen/arch/x86/x86_emulate/blk.c
+++ b/xen/arch/x86/x86_emulate/blk.c
@@ -11,9 +11,12 @@
     !defined(X86EMUL_NO_SIMD)
 # ifdef __XEN__
 #  include <asm/xstate.h>
-#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
+/* Has a fastpath for `current`, so there's no actual map */
+#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
+#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
 # else
 #  define FXSAVE_AREA get_fpu_save_area()
+#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
 # endif
 #endif
 
@@ -292,6 +295,9 @@ int x86_emul_blk(
         }
         else
             asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
+
+        UNMAP_FXSAVE_AREA(fxsr);
+
         break;
     }
 
@@ -320,6 +326,9 @@ int x86_emul_blk(
 
         if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
             memcpy(ptr, fxsr, s->op_bytes);
+
+        UNMAP_FXSAVE_AREA(fxsr);
+
         break;
     }
 
-- 
2.47.1

[PATCH v3 09/12] x86/mpx: Map/unmap xsave area in in read_bndcfgu()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * No change

v1->v2:
  * s/ret/bndcfgu
---
 xen/arch/x86/xstate.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 3d249518a1b7..2003ba664594 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -1024,9 +1024,10 @@ int handle_xsetbv(u32 index, u64 new_bv)
 
 uint64_t read_bndcfgu(void)
 {
+    uint64_t bndcfgu = 0;
     unsigned long cr0 = read_cr0();
-    struct xsave_struct *xstate
-        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
+    struct vcpu *v = idle_vcpu[smp_processor_id()];
+    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);
     const struct xstate_bndcsr *bndcsr;
 
     ASSERT(cpu_has_mpx);
@@ -1052,7 +1053,12 @@ uint64_t read_bndcfgu(void)
     if ( cr0 & X86_CR0_TS )
         write_cr0(cr0);
 
-    return xstate->xsave_hdr.xstate_bv & X86_XCR0_BNDCSR ? bndcsr->bndcfgu : 0;
+    if ( xstate->xsave_hdr.xstate_bv & X86_XCR0_BNDCSR )
+        bndcfgu = bndcsr->bndcfgu;
+
+    VCPU_UNMAP_XSAVE_AREA(v, xstate);
+
+    return bndcfgu;
 }
 
 void xstate_set_init(uint64_t mask)
-- 
2.47.1

[PATCH v3 10/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xsave()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * const-ified v in fpu_fxsave() (missing in v2)

v1->v2:
  * const-ified v
---
 xen/arch/x86/i387.c               | 16 ++++++++++------
 xen/arch/x86/include/asm/xstate.h |  2 +-
 xen/arch/x86/xstate.c             |  3 +--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 5429531ddd5f..11d06f921269 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -129,7 +129,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
 }
 
 /* Save x87 extended state */
-static inline void fpu_xsave(struct vcpu *v)
+static inline void fpu_xsave(const struct vcpu *v, struct xsave_struct *xsave_area)
 {
     bool ok;
     uint64_t mask = vcpu_xsave_mask(v);
@@ -141,15 +141,14 @@ static inline void fpu_xsave(struct vcpu *v)
      */
     ok = set_xcr0(v->arch.xcr0_accum | XSTATE_FP_SSE);
     ASSERT(ok);
-    xsave(v, mask);
+    xsave(v, xsave_area, mask);
     ok = set_xcr0(v->arch.xcr0 ?: XSTATE_FP_SSE);
     ASSERT(ok);
 }
 
 /* Save x87 FPU, MMX, SSE and SSE2 state */
-static inline void fpu_fxsave(struct vcpu *v)
+static inline void fpu_fxsave(const struct vcpu *v, fpusse_t *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 )
@@ -264,6 +263,8 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
  */
 static bool _vcpu_save_fpu(struct vcpu *v)
 {
+    struct xsave_struct *xsave_area;
+
     if ( !v->fpu_dirtied && !v->arch.nonlazy_xstate_used )
         return false;
 
@@ -272,11 +273,14 @@ static bool _vcpu_save_fpu(struct vcpu *v)
     /* This can happen, if a paravirtualised guest OS has set its CR0.TS. */
     clts();
 
+    xsave_area = VCPU_MAP_XSAVE_AREA(v);
+
     if ( cpu_has_xsave )
-        fpu_xsave(v);
+        fpu_xsave(v, xsave_area);
     else
-        fpu_fxsave(v);
+        fpu_fxsave(v, &xsave_area->fpu_sse);
 
+    VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
     v->fpu_dirtied = 0;
 
     return true;
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index ab81a4c8527e..87f05dbca6f4 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -97,7 +97,7 @@ uint64_t get_xcr0(void);
 void set_msr_xss(u64 xss);
 uint64_t get_msr_xss(void);
 uint64_t read_bndcfgu(void);
-void xsave(struct vcpu *v, uint64_t mask);
+void xsave(const struct vcpu *v, struct xsave_struct *ptr, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 void xstate_set_init(uint64_t mask);
 bool xsave_enabled(const struct vcpu *v);
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 2003ba664594..24053b394200 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -302,9 +302,8 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     VCPU_UNMAP_XSAVE_AREA(v, xstate);
 }
 
-void xsave(struct vcpu *v, uint64_t mask)
+void xsave(const struct vcpu *v, struct xsave_struct *ptr, uint64_t mask)
 {
-    struct xsave_struct *ptr = v->arch.xsave_area;
     uint32_t hmask = mask >> 32;
     uint32_t lmask = mask;
     unsigned int fip_width = v->domain->arch.x87_fip_width;
-- 
2.47.1

[PATCH v3 11/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xrstor()

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * const-ified v in fpu_xrstor()
  * Removed v in fpu_fxrstor()

v1->v2:
  * const-ified v in xrstor()
    ( it was incorrectly reported in v2 as it being fpu_xrstor() )
---
 xen/arch/x86/i387.c               | 26 ++++++++++++++++----------
 xen/arch/x86/include/asm/xstate.h |  2 +-
 xen/arch/x86/xstate.c             | 10 ++++++----
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 11d06f921269..943ae668606f 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -20,7 +20,8 @@
 /*     FPU Restore Functions   */
 /*******************************/
 /* Restore x87 extended state */
-static inline void fpu_xrstor(struct vcpu *v, uint64_t mask)
+static inline void fpu_xrstor(const struct vcpu *v,
+                              struct xsave_struct *xsave_area, uint64_t mask)
 {
     bool ok;
 
@@ -30,16 +31,14 @@ static inline void fpu_xrstor(struct vcpu *v, uint64_t mask)
      */
     ok = set_xcr0(v->arch.xcr0_accum | XSTATE_FP_SSE);
     ASSERT(ok);
-    xrstor(v, mask);
+    xrstor(v, xsave_area, mask);
     ok = set_xcr0(v->arch.xcr0 ?: XSTATE_FP_SSE);
     ASSERT(ok);
 }
 
 /* Restore x87 FPU, MMX, SSE and SSE2 state */
-static inline void fpu_fxrstor(struct vcpu *v)
+static inline void fpu_fxrstor(const fpusse_t *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
      * is pending. Clear the x87 state here by setting it to fixed
@@ -195,6 +194,8 @@ static inline void fpu_fxsave(const struct vcpu *v, fpusse_t *fpu_ctxt)
 /* Restore FPU state whenever VCPU is schduled in. */
 void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts)
 {
+    struct xsave_struct *xsave_area;
+
     /* Restore nonlazy extended state (i.e. parts not tracked by CR0.TS). */
     if ( !v->arch.fully_eager_fpu && !v->arch.nonlazy_xstate_used )
         goto maybe_stts;
@@ -209,12 +210,13 @@ 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.
      */
+    xsave_area = VCPU_MAP_XSAVE_AREA(v);
     if ( v->arch.fully_eager_fpu || xstate_all(v) )
     {
         if ( cpu_has_xsave )
-            fpu_xrstor(v, XSTATE_ALL);
+            fpu_xrstor(v, xsave_area, XSTATE_ALL);
         else
-            fpu_fxrstor(v);
+            fpu_fxrstor(&xsave_area->fpu_sse);
 
         v->fpu_initialised = 1;
         v->fpu_dirtied = 1;
@@ -224,9 +226,10 @@ void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts)
     }
     else
     {
-        fpu_xrstor(v, XSTATE_NONLAZY);
+        fpu_xrstor(v, xsave_area, XSTATE_NONLAZY);
         need_stts = true;
     }
+    VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
 
  maybe_stts:
     if ( need_stts )
@@ -238,6 +241,7 @@ void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts)
  */
 void vcpu_restore_fpu_lazy(struct vcpu *v)
 {
+    struct xsave_struct *xsave_area;
     ASSERT(!is_idle_vcpu(v));
 
     /* Avoid recursion. */
@@ -248,10 +252,12 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
 
     ASSERT(!v->arch.fully_eager_fpu);
 
+    xsave_area = VCPU_MAP_XSAVE_AREA(v);
     if ( cpu_has_xsave )
-        fpu_xrstor(v, XSTATE_LAZY);
+        fpu_xrstor(v, xsave_area, XSTATE_LAZY);
     else
-        fpu_fxrstor(v);
+        fpu_fxrstor(&xsave_area->fpu_sse);
+    VCPU_UNMAP_XSAVE_AREA(v, xsave_area);
 
     v->fpu_initialised = 1;
     v->fpu_dirtied = 1;
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index 87f05dbca6f4..7d160d2b54be 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -98,7 +98,7 @@ void set_msr_xss(u64 xss);
 uint64_t get_msr_xss(void);
 uint64_t read_bndcfgu(void);
 void xsave(const struct vcpu *v, struct xsave_struct *ptr, uint64_t mask);
-void xrstor(struct vcpu *v, uint64_t mask);
+void xrstor(const struct vcpu *v, struct xsave_struct *ptr, uint64_t mask);
 void xstate_set_init(uint64_t mask);
 bool xsave_enabled(const struct vcpu *v);
 int __must_check validate_xstate(const struct domain *d,
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 24053b394200..3d4fb7664c5f 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -376,11 +376,10 @@ void xsave(const struct vcpu *v, struct xsave_struct *ptr, uint64_t mask)
         ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
 }
 
-void xrstor(struct vcpu *v, uint64_t mask)
+void xrstor(const struct vcpu *v, struct xsave_struct *ptr, uint64_t mask)
 {
     uint32_t hmask = mask >> 32;
     uint32_t lmask = mask;
-    struct xsave_struct *ptr = v->arch.xsave_area;
     unsigned int faults, prev_faults;
 
     /*
@@ -994,6 +993,7 @@ int handle_xsetbv(u32 index, u64 new_bv)
     mask &= curr->fpu_dirtied ? ~XSTATE_FP_SSE : XSTATE_NONLAZY;
     if ( mask )
     {
+        struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(curr);
         unsigned long cr0 = read_cr0();
 
         clts();
@@ -1013,7 +1013,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
             curr->fpu_dirtied = 1;
             cr0 &= ~X86_CR0_TS;
         }
-        xrstor(curr, mask);
+        xrstor(curr, xsave_area, mask);
+        VCPU_UNMAP_XSAVE_AREA(curr, xsave_area);
+
         if ( cr0 & X86_CR0_TS )
             write_cr0(cr0);
     }
@@ -1080,7 +1082,7 @@ void xstate_set_init(uint64_t mask)
 
     xstate = VCPU_MAP_XSAVE_AREA(v);
     memset(&xstate->xsave_hdr, 0, sizeof(xstate->xsave_hdr));
-    xrstor(v, mask);
+    xrstor(v, xstate, mask);
     VCPU_UNMAP_XSAVE_AREA(v, xstate);
 
     if ( cr0 & X86_CR0_TS )
-- 
2.47.1

[PATCH v3 12/12] x86/xstate: Make xstate_all() and vcpu_xsave_mask() take explicit xstate

[Alejandro Vallejo]

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v2->v3:
  * Added A-by
---
 xen/arch/x86/i387.c               | 9 +++++----
 xen/arch/x86/include/asm/xstate.h | 5 +++--
 xen/arch/x86/xstate.c             | 2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 943ae668606f..8120a6a4afc0 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -107,7 +107,8 @@ static inline void fpu_fxrstor(const fpusse_t *fpu_ctxt)
 /*      FPU Save Functions     */
 /*******************************/
 
-static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
+static inline uint64_t vcpu_xsave_mask(const struct vcpu *v,
+                                       const struct xsave_struct *xsave_area)
 {
     if ( v->fpu_dirtied )
         return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
@@ -124,14 +125,14 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
      * XSTATE_FP_SSE), vcpu_xsave_mask will return XSTATE_ALL. Otherwise
      * return XSTATE_NONLAZY.
      */
-    return xstate_all(v) ? XSTATE_ALL : XSTATE_NONLAZY;
+    return xstate_all(v, xsave_area) ? XSTATE_ALL : XSTATE_NONLAZY;
 }
 
 /* Save x87 extended state */
 static inline void fpu_xsave(const struct vcpu *v, struct xsave_struct *xsave_area)
 {
     bool ok;
-    uint64_t mask = vcpu_xsave_mask(v);
+    uint64_t mask = vcpu_xsave_mask(v, xsave_area);
 
     ASSERT(mask);
     /*
@@ -211,7 +212,7 @@ void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts)
      * saving state belonging to another vCPU.
      */
     xsave_area = VCPU_MAP_XSAVE_AREA(v);
-    if ( v->arch.fully_eager_fpu || xstate_all(v) )
+    if ( v->arch.fully_eager_fpu || xstate_all(v, xsave_area) )
     {
         if ( cpu_has_xsave )
             fpu_xrstor(v, xsave_area, XSTATE_ALL);
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index 7d160d2b54be..a1b188537c15 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -132,14 +132,15 @@ 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)
+static inline bool xstate_all(const struct vcpu *v,
+                              const struct xsave_struct *xsave_area)
 {
     /*
      * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
      * (in the legacy region of xsave area) are fixed, so saving
      * XSTATE_FP_SSE will not cause overwriting problem with XSAVES/XSAVEC.
      */
-    return xsave_area_compressed(v->arch.xsave_area) &&
+    return xsave_area_compressed(xsave_area) &&
            (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
 }
 
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 3d4fb7664c5f..b204147815c3 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -1005,7 +1005,7 @@ int handle_xsetbv(u32 index, u64 new_bv)
             asm ( "stmxcsr %0" : "=m" (xsave_area->fpu_sse.mxcsr) );
             VCPU_UNMAP_XSAVE_AREA(curr, xsave_area);
         }
-        else if ( xstate_all(curr) )
+        else if ( xstate_all(curr, xsave_area) )
         {
             /* See the comment in i387.c:vcpu_restore_fpu_eager(). */
             mask |= XSTATE_LAZY;
-- 
2.47.1

Re: [PATCH v3 01/12] x86/xstate: Create map/unmap primitives for xsave areas

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> Add infrastructure to simplify ASI handling. With ASI in the picture
> we'll have several different means of accessing the XSAVE area of a
> given vCPU, depending on whether a domain is covered by ASI or not and
> whether the vCPU is question is scheduled on the current pCPU or not.
> 
> Having these complexities exposed at the call sites becomes unwieldy
> very fast. These wrappers are intended to be used in a similar way to
> map_domain_page() and unmap_domain_page(); The map operation will
> dispatch the appropriate pointer for each case in a future patch, while
> unmap will remain a no-op where no unmap is required (e.g: when there's
> no ASI) and remove the transient maping if one was required.
> 
> Follow-up patches replace all uses of raw v->arch.xsave_area by this
> mechanism in preparation to add the beforementioned dispatch logic to be
> added at a later time.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with one nit below.

> --- a/xen/arch/x86/include/asm/xstate.h
> +++ b/xen/arch/x86/include/asm/xstate.h
> @@ -143,4 +143,46 @@ static inline bool xstate_all(const struct vcpu *v)
>             (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
>  }
>  
> +/*
> + * Fetch a pointer to a vCPU's XSAVE area
> + *
> + * TL;DR: If v == current, the mapping is guaranteed to already exist.
> + *
> + * Despite the name, this macro might not actually map anything. The only case
> + * in which a mutation of page tables is strictly required is when ASI==on &&
> + * v!=current. For everything else the mapping already exists and needs not
> + * be created nor destroyed.
> + *
> + *                         +-----------------+--------------+
> + *                         |   v == current  | v != current |
> + *          +--------------+-----------------+--------------+
> + *          | ASI  enabled | per-vCPU fixmap |  actual map  |
> + *          +--------------+-----------------+--------------+
> + *          | ASI disabled |             directmap          |
> + *          +--------------+--------------------------------+
> + *
> + * There MUST NOT be outstanding maps of XSAVE areas of the non-current vCPU
> + * at the point of context switch. Otherwise, the unmap operation will
> + * misbehave.
> + *
> + * TODO: Expand the macro to the ASI cases after infra to do so is in place.
> + *
> + * @param v Owner of the XSAVE area
> + */
> +#define VCPU_MAP_XSAVE_AREA(v) ((v)->arch.xsave_area)
> +
> +/*
> + * Drops the mapping of a vCPU's XSAVE area and nullifies its pointer on exit
> + *
> + * See VCPU_MAP_XSAVE_AREA() for additional information on the persistence of
> + * these mappings. This macro only tears down the mappings in the ASI=on &&
> + * v!=current case.
> + *
> + * TODO: Expand the macro to the ASI cases after infra to do so is in place.
> + *
> + * @param v Owner of the XSAVE area
> + * @param x XSAVE blob of v
> + */
> +#define VCPU_UNMAP_XSAVE_AREA(v, x) do { (void)(v); (x) = NULL; } while(0)

The "while" still wants to conform to style, despite it being a kind of
degenerate form. The overwhelming majority of instances we've got have at
least a blank before the opening parenthesis. Many also have the ones
inside.

Jan

Re: [PATCH v3 07/12] x86/xstate: Map/unmap xsave area in {compress,expand}_xsave_states()

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

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

Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> Adds an UNMAP primitive to make use of vcpu_unmap_xsave_area() when
> linked into xen. unmap is a no-op during tests.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

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

> --- a/xen/arch/x86/x86_emulate/blk.c
> +++ b/xen/arch/x86/x86_emulate/blk.c
> @@ -11,9 +11,12 @@
>      !defined(X86EMUL_NO_SIMD)
>  # ifdef __XEN__
>  #  include <asm/xstate.h>
> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> +/* Has a fastpath for `current`, so there's no actual map */
> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)

... the comment here kind of suggesting that ...

>  # else
>  #  define FXSAVE_AREA get_fpu_save_area()
> +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))

... use of this new construct is solely decoration, and could hence as
well be omitted.

Jan

Re: [PATCH v3 09/12] x86/mpx: Map/unmap xsave area in in read_bndcfgu()

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -1024,9 +1024,10 @@ int handle_xsetbv(u32 index, u64 new_bv)
>  
>  uint64_t read_bndcfgu(void)
>  {
> +    uint64_t bndcfgu = 0;
>      unsigned long cr0 = read_cr0();
> -    struct xsave_struct *xstate
> -        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
> +    struct vcpu *v = idle_vcpu[smp_processor_id()];

Question on this one remains: Can it be pointer-to-const (in the longer
run; certainly in can be right now)?

> +    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);

I realize my similar remark on this one was actually wrong; the asm()s
clearly modify what is being pointed top.

Jan

Re: [PATCH v3 10/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xsave()

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> v2->v3:
>   * const-ified v in fpu_fxsave() (missing in v2)

Sadly this has rendered ...

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -129,7 +129,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
>  }
>  
>  /* Save x87 extended state */
> -static inline void fpu_xsave(struct vcpu *v)
> +static inline void fpu_xsave(const struct vcpu *v, struct xsave_struct *xsave_area)

... this line too long now. With it suitably wrapped (possibly doable while
committing, if no other reason for a v4 appears)
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v3 11/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xrstor()

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

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

> ---
> v2->v3:
>   * const-ified v in fpu_xrstor()
>   * Removed v in fpu_fxrstor()

On this basis the parameter could also be removed from fpu_fxsave(), by
passing in fip_width instead.

Jan

Re: [PATCH v3 09/12] x86/mpx: Map/unmap xsave area in in read_bndcfgu()

[Alejandro Vallejo]

Hi,

Thanks for reviewing this and the other patches.

On Mon Jan 27, 2025 at 10:57 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/xstate.c
> > +++ b/xen/arch/x86/xstate.c
> > @@ -1024,9 +1024,10 @@ int handle_xsetbv(u32 index, u64 new_bv)
> >  
> >  uint64_t read_bndcfgu(void)
> >  {
> > +    uint64_t bndcfgu = 0;
> >      unsigned long cr0 = read_cr0();
> > -    struct xsave_struct *xstate
> > -        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
> > +    struct vcpu *v = idle_vcpu[smp_processor_id()];
>
> Question on this one remains: Can it be pointer-to-const (in the longer
> run; certainly in can be right now)?

I have no idea where I got the idea the C constness was transitive (quite
likely from Rust, as its illegal to grab a &mut from a &). Const being
non-transitive means I can constify the vcpu as you suggest/ask. The rationale
was that getting a pointer to non-const from a pointer to const seemed wrong.

But C is bizarre and its finer details have a way of biting. Oh well.

FWIW, this ought to hold even in the long run. There won't be anything special
in the map/unmap macros besides some indirection, so it'll work in a very
similar fashion as it does now.

I'll adjust it as you propose.

>
> > +    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);
>
> I realize my similar remark on this one was actually wrong; the asm()s
> clearly modify what is being pointed top.

Indeed, xstate can definitely not be const.

> Jan

Cheers,
Alejandro

Re: [PATCH v3 01/12] x86/xstate: Create map/unmap primitives for xsave areas

[Alejandro Vallejo]

Hi,

On Mon Jan 27, 2025 at 10:44 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > Add infrastructure to simplify ASI handling. With ASI in the picture
> > we'll have several different means of accessing the XSAVE area of a
> > given vCPU, depending on whether a domain is covered by ASI or not and
> > whether the vCPU is question is scheduled on the current pCPU or not.
> > 
> > Having these complexities exposed at the call sites becomes unwieldy
> > very fast. These wrappers are intended to be used in a similar way to
> > map_domain_page() and unmap_domain_page(); The map operation will
> > dispatch the appropriate pointer for each case in a future patch, while
> > unmap will remain a no-op where no unmap is required (e.g: when there's
> > no ASI) and remove the transient maping if one was required.
> > 
> > Follow-up patches replace all uses of raw v->arch.xsave_area by this
> > mechanism in preparation to add the beforementioned dispatch logic to be
> > added at a later time.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> with one nit below.
>
> > --- a/xen/arch/x86/include/asm/xstate.h
> > +++ b/xen/arch/x86/include/asm/xstate.h
> > @@ -143,4 +143,46 @@ static inline bool xstate_all(const struct vcpu *v)
> >             (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
> >  }
> >  
> > +/*
> > + * Fetch a pointer to a vCPU's XSAVE area
> > + *
> > + * TL;DR: If v == current, the mapping is guaranteed to already exist.
> > + *
> > + * Despite the name, this macro might not actually map anything. The only case
> > + * in which a mutation of page tables is strictly required is when ASI==on &&
> > + * v!=current. For everything else the mapping already exists and needs not
> > + * be created nor destroyed.
> > + *
> > + *                         +-----------------+--------------+
> > + *                         |   v == current  | v != current |
> > + *          +--------------+-----------------+--------------+
> > + *          | ASI  enabled | per-vCPU fixmap |  actual map  |
> > + *          +--------------+-----------------+--------------+
> > + *          | ASI disabled |             directmap          |
> > + *          +--------------+--------------------------------+
> > + *
> > + * There MUST NOT be outstanding maps of XSAVE areas of the non-current vCPU
> > + * at the point of context switch. Otherwise, the unmap operation will
> > + * misbehave.
> > + *
> > + * TODO: Expand the macro to the ASI cases after infra to do so is in place.
> > + *
> > + * @param v Owner of the XSAVE area
> > + */
> > +#define VCPU_MAP_XSAVE_AREA(v) ((v)->arch.xsave_area)
> > +
> > +/*
> > + * Drops the mapping of a vCPU's XSAVE area and nullifies its pointer on exit
> > + *
> > + * See VCPU_MAP_XSAVE_AREA() for additional information on the persistence of
> > + * these mappings. This macro only tears down the mappings in the ASI=on &&
> > + * v!=current case.
> > + *
> > + * TODO: Expand the macro to the ASI cases after infra to do so is in place.
> > + *
> > + * @param v Owner of the XSAVE area
> > + * @param x XSAVE blob of v
> > + */
> > + #define VCPU_UNMAP_XSAVE_AREA(v, x) do { (void)(v); (x) = NULL; } while(0)
>
> The "while" still wants to conform to style, despite it being a kind of
> degenerate form. The overwhelming majority of instances we've got have at
> least a blank before the opening parenthesis. Many also have the ones
> inside.
>
> Jan

Sure, makes sense. I'll adjust it.

Cheers
Alejandro

Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Alejandro Vallejo]

On Mon Jan 27, 2025 at 10:52 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > Adds an UNMAP primitive to make use of vcpu_unmap_xsave_area() when
> > linked into xen. unmap is a no-op during tests.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,

> despite ...
>
> > --- a/xen/arch/x86/x86_emulate/blk.c
> > +++ b/xen/arch/x86/x86_emulate/blk.c
> > @@ -11,9 +11,12 @@
> >      !defined(X86EMUL_NO_SIMD)
> >  # ifdef __XEN__
> >  #  include <asm/xstate.h>
> > -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> > +/* Has a fastpath for `current`, so there's no actual map */
> > +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> > +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
>
> ... the comment here kind of suggesting that ...
>
> >  # else
> >  #  define FXSAVE_AREA get_fpu_save_area()
> > +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>
> ... use of this new construct is solely decoration, and could hence as
> well be omitted.
>
> Jan

It seems like a dangerous proposition to abuse knowledge of an implementation
in order to skip parts of its interface. The fact that no such map is required
at this point in x86_emulate does not mean it never will be. Predicting the
future is hard, but being consistent today is less so (imo).

Cheers,
Alejandro

Re: [PATCH v3 10/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xsave()

[Alejandro Vallejo]

On Mon Jan 27, 2025 at 11:01 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > No functional change.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> > v2->v3:
> >   * const-ified v in fpu_fxsave() (missing in v2)
>
> Sadly this has rendered ...
>
> > --- a/xen/arch/x86/i387.c
> > +++ b/xen/arch/x86/i387.c
> > @@ -129,7 +129,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
> >  }
> >  
> >  /* Save x87 extended state */
> > -static inline void fpu_xsave(struct vcpu *v)
> > +static inline void fpu_xsave(const struct vcpu *v, struct xsave_struct *xsave_area)
>
> ... this line too long now. With it suitably wrapped (possibly doable while
> committing, if no other reason for a v4 appears)

Bah, yes. You're right. I don't mind it being adjusted on commit.

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

Cheers,
Alejandro

Re: [PATCH v3 11/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xrstor()

[Alejandro Vallejo]

Hi,

On Mon Jan 27, 2025 at 11:05 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > No functional change.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> > ---
> > v2->v3:
> >   * const-ified v in fpu_xrstor()
> >   * Removed v in fpu_fxrstor()
>
> On this basis the parameter could also be removed from fpu_fxsave(), by
> passing in fip_width instead.
>
> Jan

Could be, but there's not a whole lot of gain to be had? The access must be
done either way before or after the fpu_fxsave() call, and a parameter must be
passed (be it fip_width or v). Passing the vCPU encapsulates the access of
fip_width where its actually used, which seems more desirable, I'd say.

Cheers,
Alejandro

Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Jan Beulich]

On 27.01.2025 16:42, Alejandro Vallejo wrote:
> On Mon Jan 27, 2025 at 10:52 AM GMT, Jan Beulich wrote:
>> On 10.01.2025 14:28, Alejandro Vallejo wrote:
>>> Adds an UNMAP primitive to make use of vcpu_unmap_xsave_area() when
>>> linked into xen. unmap is a no-op during tests.
>>>
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks,
> 
>> despite ...
>>
>>> --- a/xen/arch/x86/x86_emulate/blk.c
>>> +++ b/xen/arch/x86/x86_emulate/blk.c
>>> @@ -11,9 +11,12 @@
>>>      !defined(X86EMUL_NO_SIMD)
>>>  # ifdef __XEN__
>>>  #  include <asm/xstate.h>
>>> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
>>> +/* Has a fastpath for `current`, so there's no actual map */
>>> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
>>> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
>>
>> ... the comment here kind of suggesting that ...
>>
>>>  # else
>>>  #  define FXSAVE_AREA get_fpu_save_area()
>>> +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>>
>> ... use of this new construct is solely decoration, and could hence as
>> well be omitted.
> 
> It seems like a dangerous proposition to abuse knowledge of an implementation
> in order to skip parts of its interface. The fact that no such map is required
> at this point in x86_emulate does not mean it never will be. Predicting the
> future is hard, but being consistent today is less so (imo).

Entirely true. How likely do you consider it though that with a future
change altering that property, the comment above would be left in place
(and hence then be stale)? My take: Very likely, as long as the two
"current" uses wouldn't need altering.

Yet FTAOD: I'm not asking for any adjustment here, I'm merely mentioning
an observation.

Jan

Re: [PATCH v3 11/12] x86/fpu: Pass explicit xsave areas to fpu_(f)xrstor()

[Jan Beulich]

On 27.01.2025 16:48, Alejandro Vallejo wrote:
> On Mon Jan 27, 2025 at 11:05 AM GMT, Jan Beulich wrote:
>> On 10.01.2025 14:28, Alejandro Vallejo wrote:
>>> No functional change.
>>>
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>>> ---
>>> v2->v3:
>>>   * const-ified v in fpu_xrstor()
>>>   * Removed v in fpu_fxrstor()
>>
>> On this basis the parameter could also be removed from fpu_fxsave(), by
>> passing in fip_width instead.
> 
> Could be, but there's not a whole lot of gain to be had? The access must be
> done either way before or after the fpu_fxsave() call, and a parameter must be
> passed (be it fip_width or v). Passing the vCPU encapsulates the access of
> fip_width where its actually used, which seems more desirable, I'd say.

Not much of a gain indeed, largely for symmetry between the two sibling
functions.

Jan

Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Jan Beulich]

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/x86_emulate/blk.c
> +++ b/xen/arch/x86/x86_emulate/blk.c
> @@ -11,9 +11,12 @@
>      !defined(X86EMUL_NO_SIMD)
>  # ifdef __XEN__
>  #  include <asm/xstate.h>
> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> +/* Has a fastpath for `current`, so there's no actual map */
> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
>  # else
>  #  define FXSAVE_AREA get_fpu_save_area()
> +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>  # endif
>  #endif

While preparing to commit this I felt a little uneasy. The mapping aspect
is ...

> @@ -292,6 +295,9 @@ int x86_emul_blk(
>          }
>          else
>              asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
> +
> +        UNMAP_FXSAVE_AREA(fxsr);
> +
>          break;
>      }
>  
> @@ -320,6 +326,9 @@ int x86_emul_blk(
>  
>          if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
>              memcpy(ptr, fxsr, s->op_bytes);
> +
> +        UNMAP_FXSAVE_AREA(fxsr);
> +
>          break;
>      }
>  

... is entirely invisible at the use sites. This imo calls for making
mistakes, and hence the existing macro better would be adjusted to become
MAP_FXSAVE_AREA().

Jan

Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Alejandro Vallejo]

On Wed Mar 5, 2025 at 3:29 PM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/x86_emulate/blk.c
> > +++ b/xen/arch/x86/x86_emulate/blk.c
> > @@ -11,9 +11,12 @@
> >      !defined(X86EMUL_NO_SIMD)
> >  # ifdef __XEN__
> >  #  include <asm/xstate.h>
> > -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> > +/* Has a fastpath for `current`, so there's no actual map */
> > +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> > +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
> >  # else
> >  #  define FXSAVE_AREA get_fpu_save_area()
> > +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
> >  # endif
> >  #endif
>
> While preparing to commit this I felt a little uneasy. The mapping aspect
> is ...

Thanks for coming back to it :)

>
> > @@ -292,6 +295,9 @@ int x86_emul_blk(
> >          }
> >          else
> >              asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
> > +
> > +        UNMAP_FXSAVE_AREA(fxsr);
> > +
> >          break;
> >      }
> >  
> > @@ -320,6 +326,9 @@ int x86_emul_blk(
> >  
> >          if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
> >              memcpy(ptr, fxsr, s->op_bytes);
> > +
> > +        UNMAP_FXSAVE_AREA(fxsr);
> > +
> >          break;
> >      }
> >  
>
> ... is entirely invisible at the use sites. This imo calls for making
> mistakes, and hence the existing macro better would be adjusted to become
> MAP_FXSAVE_AREA().
>
> Jan

I prefer it that way too; I was simply trying to minimize the diff. Would you
like me to resend it with that adjustment?

Cheers,
Alejandro

Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Jan Beulich]

On 05.03.2025 17:16, Alejandro Vallejo wrote:
> On Wed Mar 5, 2025 at 3:29 PM GMT, Jan Beulich wrote:
>> On 10.01.2025 14:28, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/x86_emulate/blk.c
>>> +++ b/xen/arch/x86/x86_emulate/blk.c
>>> @@ -11,9 +11,12 @@
>>>      !defined(X86EMUL_NO_SIMD)
>>>  # ifdef __XEN__
>>>  #  include <asm/xstate.h>
>>> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
>>> +/* Has a fastpath for `current`, so there's no actual map */
>>> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
>>> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
>>>  # else
>>>  #  define FXSAVE_AREA get_fpu_save_area()
>>> +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>>>  # endif
>>>  #endif
>>
>> While preparing to commit this I felt a little uneasy. The mapping aspect
>> is ...
> 
> Thanks for coming back to it :)
> 
>>
>>> @@ -292,6 +295,9 @@ int x86_emul_blk(
>>>          }
>>>          else
>>>              asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
>>> +
>>> +        UNMAP_FXSAVE_AREA(fxsr);
>>> +
>>>          break;
>>>      }
>>>  
>>> @@ -320,6 +326,9 @@ int x86_emul_blk(
>>>  
>>>          if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
>>>              memcpy(ptr, fxsr, s->op_bytes);
>>> +
>>> +        UNMAP_FXSAVE_AREA(fxsr);
>>> +
>>>          break;
>>>      }
>>>  
>>
>> ... is entirely invisible at the use sites. This imo calls for making
>> mistakes, and hence the existing macro better would be adjusted to become
>> MAP_FXSAVE_AREA().
> 
> I prefer it that way too; I was simply trying to minimize the diff. Would you
> like me to resend it with that adjustment?

Yes please. Plus whatever else adjustments are needed in the subsequent patches
(sorry, it has been a while, so I don't recall the state of those later ones).

Jan