[2/3] x86: introduce xstate_zero

Commit Message

Fouad Hilly March 4, 2024, 9:13 a.m. UTC

From: Wei Liu <[e-mail]>

Factor out xrstor__ and introduce xstate_zero, which zeros all the
state components specified in the mask.

Signed-off-by: Wei Liu <[e-mail]>
Signed-off-by: Fouad Hilly <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Andrew Cooper <[e-mail]>
CC: "Roger Pau Monné" <[e-mail]>
CC: Wei Liu <[e-mail]>
---
 xen/arch/x86/include/asm/xstate.h |  1 +
 xen/arch/x86/xstate.c             | 39 +++++++++++++++++++++++++------
 2 files changed, 33 insertions(+), 7 deletions(-)

Comments

Jan Beulich March 5, 2024, 9:06 a.m. UTC | #1

On 04.03.2024 10:13, Fouad Hilly wrote:
> +void xstate_zero(uint64_t mask)
> +{
> +    bool ok;
> +    struct xsave_struct tmp;
> +
> +    tmp.fpu_sse.mxcsr = MXCSR_DEFAULT;

This is a clear indication that the function name is wrong. Perhaps it is
"reset" that was meant?

> +    tmp.xsave_hdr.xstate_bv = 0;
> +    tmp.xsave_hdr.xcomp_bv = 0;
> +    memset(tmp.xsave_hdr.reserved, 0, sizeof(tmp.xsave_hdr.reserved));
> +
> +    ok = xrstor__(&tmp, mask, mask);

There's a lot of "tmp" that is left uninitialized, which would introduce
a leak of (stack) data. I think "tmp" instead wants to have an initializer
(consisting of just the explicit setting of MXCSR, leaving everything else
to be default, i.e. zero-initialized).

> +    ASSERT(ok);
>  }
>  
>  bool xsave_enabled(const struct vcpu *v)
> @@ -731,6 +753,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
>      if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>          return -EINVAL;
>  
> +    /* Zero state components before writing new XCR0 */
> +    xstate_zero(get_xcr0());

This change isn't explained in the description, doesn't fit the subject
(i.e. mechanical and functional change likely want splitting), and is
imo wrong: Why would XSETBV gain this kind of side effect, when processed
(emulated) in Xen? What I can see may want clearing are state components
which were never enabled before by a vCPU. That would then need doing
after writing the new XCR0 value, though. And it looks like that's
already in place - is there something wrong with that code?

Or is this about clearing in hardware state components about to be turned
off? If so, it would again need to be only the delta of components that's
reset here, and their state would need saving first. Upon re-enabling of
a component, that state would then need to be available for restoring.
This need for saving of state would then also explain why what's presently
named xstate_zero() can't very well use the vCPU's ->arch.xsave_area, but
needs to have a (relatively big) on-stack variable instead.

Jan

Patch

diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index c08c267884f0..bd767d9cd714 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -94,6 +94,7 @@  uint64_t get_msr_xss(void);
 uint64_t read_bndcfgu(void);
 void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
+void xstate_zero(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 cf94761d0542..92a65bd8d52c 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -368,11 +368,12 @@  void xsave(struct vcpu *v, uint64_t mask)
         ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
 }
 
-void xrstor(struct vcpu *v, uint64_t mask)
+/* True -> no error, false -> failed */
+static bool xrstor__(struct xsave_struct *ptr, uint64_t xcr0_accum,
+                     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;
 
     /*
@@ -412,7 +413,7 @@  void xrstor(struct vcpu *v, uint64_t mask)
                          [ptr] "D" (ptr) )
 
 #define XRSTOR(pfx) \
-        if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY ) \
+        if ( xcr0_accum & XSTATE_XSAVES_ONLY ) \
         { \
             if ( unlikely(!(ptr->xsave_hdr.xcomp_bv & \
                             XSTATE_COMPACTION_ENABLED)) ) \
@@ -461,7 +462,7 @@  void xrstor(struct vcpu *v, uint64_t mask)
                   ((mask & X86_XCR0_YMM) &&
                    !(ptr->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED))) )
                 ptr->fpu_sse.mxcsr &= mxcsr_mask;
-            if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY )
+            if ( xcr0_accum & XSTATE_XSAVES_ONLY )
             {
                 ptr->xsave_hdr.xcomp_bv &= this_cpu(xcr0) | this_cpu(xss);
                 ptr->xsave_hdr.xstate_bv &= ptr->xsave_hdr.xcomp_bv;
@@ -478,14 +479,35 @@  void xrstor(struct vcpu *v, uint64_t mask)
         case 2: /* Stage 2: Reset all state. */
             ptr->fpu_sse.mxcsr = MXCSR_DEFAULT;
             ptr->xsave_hdr.xstate_bv = 0;
-            ptr->xsave_hdr.xcomp_bv = v->arch.xcr0_accum & XSTATE_XSAVES_ONLY
+            ptr->xsave_hdr.xcomp_bv = xcr0_accum & XSTATE_XSAVES_ONLY
                                       ? XSTATE_COMPACTION_ENABLED : 0;
             continue;
         }
 
-        domain_crash(current->domain);
-        return;
+        return false;
     }
+
+    return true;
+}
+
+void xrstor(struct vcpu *v, uint64_t mask)
+{
+    if ( !xrstor__(v->arch.xsave_area, v->arch.xcr0_accum, mask) )
+        domain_crash(current->domain);
+}
+
+void xstate_zero(uint64_t mask)
+{
+    bool ok;
+    struct xsave_struct tmp;
+
+    tmp.fpu_sse.mxcsr = MXCSR_DEFAULT;
+    tmp.xsave_hdr.xstate_bv = 0;
+    tmp.xsave_hdr.xcomp_bv = 0;
+    memset(tmp.xsave_hdr.reserved, 0, sizeof(tmp.xsave_hdr.reserved));
+
+    ok = xrstor__(&tmp, mask, mask);
+    ASSERT(ok);
 }
 
 bool xsave_enabled(const struct vcpu *v)
@@ -731,6 +753,9 @@  int handle_xsetbv(u32 index, u64 new_bv)
     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
         return -EINVAL;
 
+    /* Zero state components before writing new XCR0 */
+    xstate_zero(get_xcr0());
+
     /* By this point, new_bv really should be accepted by hardware. */
     if ( unlikely(!set_xcr0(new_bv)) )
     {