diff mbox series

[v2,2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()

Message ID 20240429182823.1130436-3-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series x86/xstate: Fixes to size calculations | expand

Commit Message

Andrew Cooper April 29, 2024, 6:28 p.m. UTC
We're soon going to need a compressed helper of the same form.

The size of the uncompressed image depends on the single element with the
largest offset+size.  Sadly this isn't always the element with the largest
index.

Retain the cross-check with hardware in debug builds, but forgo it normal
builds.  In particular, this means that the migration paths don't need to mess
with XCR0 just to sanity check the buffer size.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Scan all features.  LWP/APX_F are out-of-order.
---
 xen/arch/x86/domctl.c             |  2 +-
 xen/arch/x86/hvm/hvm.c            |  2 +-
 xen/arch/x86/include/asm/xstate.h |  2 +-
 xen/arch/x86/xstate.c             | 45 +++++++++++++++++++++++++++----
 4 files changed, 43 insertions(+), 8 deletions(-)

Comments

Jan Beulich May 2, 2024, 12:19 p.m. UTC | #1
On 29.04.2024 20:28, Andrew Cooper wrote:
> @@ -567,16 +567,51 @@ static unsigned int hw_uncompressed_size(uint64_t xcr0)
>      return size;
>  }
>  
> -/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
> -unsigned int xstate_ctxt_size(u64 xcr0)
> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
>  {
> +    unsigned int size = XSTATE_AREA_MIN_SIZE, i;
> +
>      if ( xcr0 == xfeature_mask )
>          return xsave_cntxt_size;
>  
>      if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
>          return 0;
>  
> -    return hw_uncompressed_size(xcr0);
> +    if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )

This is open-coded XSTATE_FP_SSE, which I wouldn't mind if ...

> +        return size;
> +
> +    /*
> +     * For the non-legacy states, search all activate states and find the
> +     * maximum offset+size.  Some states (e.g. LWP, APX_F) are out-of-order
> +     * with respect their index.
> +     */
> +    xcr0 &= ~XSTATE_FP_SSE;

... you didn't use that macro here (and once further down). IOW please
be consistent, no matter which way round.

> +    for_each_set_bit ( i, &xcr0, 63 )
> +    {
> +        unsigned int s;
> +
> +        ASSERT(xstate_offsets[i] && xstate_sizes[i]);
> +
> +        s = xstate_offsets[i] && xstate_sizes[i];

You mean + here, don't you? Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm also inclined to suggest making this the initializer of s.

Jan
Andrew Cooper May 2, 2024, 1:17 p.m. UTC | #2
On 02/05/2024 1:19 pm, Jan Beulich wrote:
> On 29.04.2024 20:28, Andrew Cooper wrote:
>> @@ -567,16 +567,51 @@ static unsigned int hw_uncompressed_size(uint64_t xcr0)
>>      return size;
>>  }
>>  
>> -/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
>> -unsigned int xstate_ctxt_size(u64 xcr0)
>> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
>>  {
>> +    unsigned int size = XSTATE_AREA_MIN_SIZE, i;
>> +
>>      if ( xcr0 == xfeature_mask )
>>          return xsave_cntxt_size;
>>  
>>      if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
>>          return 0;
>>  
>> -    return hw_uncompressed_size(xcr0);
>> +    if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )
> This is open-coded XSTATE_FP_SSE, which I wouldn't mind if ...
>
>> +        return size;
>> +
>> +    /*
>> +     * For the non-legacy states, search all activate states and find the
>> +     * maximum offset+size.  Some states (e.g. LWP, APX_F) are out-of-order
>> +     * with respect their index.
>> +     */
>> +    xcr0 &= ~XSTATE_FP_SSE;
> ... you didn't use that macro here (and once further down). IOW please
> be consistent, no matter which way round.

Oh yes - that's a consequence of these hunks having between written 3
years apart.

It's important for the first one (logical comparison against a bitmap)
that it's split apart.

>
>> +    for_each_set_bit ( i, &xcr0, 63 )
>> +    {
>> +        unsigned int s;
>> +
>> +        ASSERT(xstate_offsets[i] && xstate_sizes[i]);
>> +
>> +        s = xstate_offsets[i] && xstate_sizes[i];
> You mean + here, don't you?

Yes I do...  That was a victim of a last minute refactor.

It also shows that even the cross-check with hardware isn't terribly
effective.  More on that in the other thread.

>  Then:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I'm also inclined to suggest making this the initializer of s.

Hmm, good point.  Will change.

Thanks,

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9a72d57333e9..c2f2016ed45a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -833,7 +833,7 @@  long arch_do_domctl(
         uint32_t offset = 0;
 
 #define PV_XSAVE_HDR_SIZE (2 * sizeof(uint64_t))
-#define PV_XSAVE_SIZE(xcr0) (PV_XSAVE_HDR_SIZE + xstate_ctxt_size(xcr0))
+#define PV_XSAVE_SIZE(xcr0) (PV_XSAVE_HDR_SIZE + xstate_uncompressed_size(xcr0))
 
         ret = -ESRCH;
         if ( (evc->vcpu >= d->max_vcpus) ||
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 9594e0a5c530..9e677d891d6c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1191,7 +1191,7 @@  HVM_REGISTER_SAVE_RESTORE(CPU, hvm_save_cpu_ctxt, NULL, hvm_load_cpu_ctxt, 1,
 
 #define HVM_CPU_XSAVE_SIZE(xcr0) (offsetof(struct hvm_hw_cpu_xsave, \
                                            save_area) + \
-                                  xstate_ctxt_size(xcr0))
+                                  xstate_uncompressed_size(xcr0))
 
 static int cf_check hvm_save_cpu_xsave_states(
     struct vcpu *v, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index c08c267884f0..f5115199d4f9 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -107,7 +107,7 @@  void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size);
 void xstate_free_save_area(struct vcpu *v);
 int xstate_alloc_save_area(struct vcpu *v);
 void xstate_init(struct cpuinfo_x86 *c);
-unsigned int xstate_ctxt_size(u64 xcr0);
+unsigned int xstate_uncompressed_size(uint64_t xcr0);
 
 static inline uint64_t xgetbv(unsigned int index)
 {
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 99cedb4f5e24..a94f4025fce5 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -183,7 +183,7 @@  void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
     /* Check there is state to serialise (i.e. at least an XSAVE_HDR) */
     BUG_ON(!v->arch.xcr0_accum);
     /* Check there is the correct room to decompress into. */
-    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+    BUG_ON(size != xstate_uncompressed_size(v->arch.xcr0_accum));
 
     if ( !(xstate->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
     {
@@ -245,7 +245,7 @@  void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     u64 xstate_bv, valid;
 
     BUG_ON(!v->arch.xcr0_accum);
-    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+    BUG_ON(size != xstate_uncompressed_size(v->arch.xcr0_accum));
     ASSERT(!xsave_area_compressed(src));
 
     xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
@@ -567,16 +567,51 @@  static unsigned int hw_uncompressed_size(uint64_t xcr0)
     return size;
 }
 
-/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
-unsigned int xstate_ctxt_size(u64 xcr0)
+unsigned int xstate_uncompressed_size(uint64_t xcr0)
 {
+    unsigned int size = XSTATE_AREA_MIN_SIZE, i;
+
     if ( xcr0 == xfeature_mask )
         return xsave_cntxt_size;
 
     if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
         return 0;
 
-    return hw_uncompressed_size(xcr0);
+    if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )
+        return size;
+
+    /*
+     * For the non-legacy states, search all activate states and find the
+     * maximum offset+size.  Some states (e.g. LWP, APX_F) are out-of-order
+     * with respect their index.
+     */
+    xcr0 &= ~XSTATE_FP_SSE;
+    for_each_set_bit ( i, &xcr0, 63 )
+    {
+        unsigned int s;
+
+        ASSERT(xstate_offsets[i] && xstate_sizes[i]);
+
+        s = xstate_offsets[i] && xstate_sizes[i];
+
+        size = max(size, s);
+    }
+
+    /* In debug builds, cross-check our calculation with hardware. */
+    if ( IS_ENABLED(CONFIG_DEBUG) )
+    {
+        unsigned int hwsize;
+
+        xcr0 |= XSTATE_FP_SSE;
+        hwsize = hw_uncompressed_size(xcr0);
+
+        if ( size != hwsize )
+            printk_once(XENLOG_ERR "%s(%#"PRIx64") size %#x != hwsize %#x\n",
+                        __func__, xcr0, size, hwsize);
+        size = hwsize;
+    }
+
+    return size;
 }
 
 static bool valid_xcr0(uint64_t xcr0)