[v2,4/4] x86/cpuid: Fix handling of xsave dynamic leaves

Commit Message

Andrew Cooper April 29, 2024, 6:28 p.m. UTC

If max leaf is greater than 0xd but xsave not available to the guest, then the
current XSAVE size should not be filled in.  This is a latent bug for now as
the guest max leaf is 0xd, but will become problematic in the future.

The comment concerning XSS state is wrong.  VT-x doesn't manage host/guest
state automatically, but there is provision for "host only" bits to be set, so
the implications are still accurate.

Introduce {xstate,hw}_compressed_size() helpers to mirror the uncompressed
ones.

This in turn higlights a bug in xstate_init().  Defaulting this_cpu(xss) to ~0
requires a forced write to clear it back out.  This in turn highlights that
it's only a safe default on systems with XSAVES.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Roger Pau Monné <[e-mail]>

The more I think about it, the more I think that cross-checking with hardware
is a bad move.  It's horribly expensive, and for supervisor states in
particular, liable to interfere with functionality.

v2:
 * Cope with blind reads of CPUID.0xD[1] prior to %xcr0 having been set up.
---
 xen/arch/x86/cpuid.c              | 24 ++++--------
 xen/arch/x86/include/asm/xstate.h |  1 +
 xen/arch/x86/xstate.c             | 64 ++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 17 deletions(-)

Comments

Jan Beulich May 2, 2024, 1:04 p.m. UTC | #1

On 29.04.2024 20:28, Andrew Cooper wrote:
> If max leaf is greater than 0xd but xsave not available to the guest, then the
> current XSAVE size should not be filled in.  This is a latent bug for now as
> the guest max leaf is 0xd, but will become problematic in the future.

Why would this not be an issue when .max_leaf == 0xd, but .xsave == 0? Without
my "x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents"
I don't think we shrink max_leaf to below 0xd when there's no xsave for the
guest?

> The comment concerning XSS state is wrong.  VT-x doesn't manage host/guest
> state automatically, but there is provision for "host only" bits to be set, so
> the implications are still accurate.
> 
> Introduce {xstate,hw}_compressed_size() helpers to mirror the uncompressed
> ones.
> 
> This in turn higlights a bug in xstate_init().  Defaulting this_cpu(xss) to ~0
> requires a forced write to clear it back out.  This in turn highlights that
> it's only a safe default on systems with XSAVES.

Well, yes, such an explicit write was expected to appear when some xsaves-
based component would actually be enabled. Much like the set_xcr0() there.

> Signed-off-by: Andrew Cooper <[e-mail]>
> ---
> CC: Jan Beulich <[e-mail]>
> CC: Roger Pau Monné <[e-mail]>
> 
> The more I think about it, the more I think that cross-checking with hardware
> is a bad move.  It's horribly expensive, and for supervisor states in
> particular, liable to interfere with functionality.

I agree, but I'd also like to see the cross checking not dropped entirely.
Can't we arrange for such to happen during boot, before we enable any such
functionality? After all even in debug builds it's not overly useful to do
the same cross-checking (i.e. for identical inputs) over and over again.
Of course doing in an exhaustive manner may be okay for the uncompressed
values, but might be a little too much for all possible combinations in
order to check compressed values, too.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -614,6 +614,65 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
>      return size;
>  }
>  
> +static unsigned int hw_compressed_size(uint64_t xstates)
> +{
> +    uint64_t curr_xcr0 = get_xcr0(), curr_xss = get_msr_xss();
> +    unsigned int size;
> +    bool ok;
> +
> +    ok = set_xcr0(xstates & ~XSTATE_XSAVES_ONLY);
> +    ASSERT(ok);
> +    set_msr_xss(xstates & XSTATE_XSAVES_ONLY);
> +
> +    size = cpuid_count_ebx(XSTATE_CPUID, 1);
> +
> +    ok = set_xcr0(curr_xcr0);
> +    ASSERT(ok);
> +    set_msr_xss(curr_xss);
> +
> +    return size;
> +}
> +
> +unsigned int xstate_compressed_size(uint64_t xstates)
> +{
> +    unsigned int i, size = XSTATE_AREA_MIN_SIZE;
> +
> +    if ( xstates == 0 ) /* TODO: clean up paths passing 0 in here. */
> +        return 0;
> +
> +    if ( xstates <= (X86_XCR0_SSE | X86_XCR0_FP) )

Same comment as on the earlier change regarding the (lack of) use of ....

> +        return size;
> +
> +    /*
> +     * For the compressed size, every component matters.  Some are
> +     * automatically rounded up to 64 first.
> +     */
> +    xstates &= ~XSTATE_FP_SSE;

... this constant up there.

> +    for_each_set_bit ( i, &xstates, 63 )
> +    {
> +        if ( test_bit(i, &xstate_align) )
> +            size = ROUNDUP(size, 64);
> +
> +        size += xstate_sizes[i];
> +    }

The comment is a little misleading: As you have it in code, it is not the
component's size that is rounded up, but its position. Maybe "Some have
their start automatically rounded up to 64 first"?

Jan

Andrew Cooper May 2, 2024, 2:32 p.m. UTC | #2

On 02/05/2024 2:04 pm, Jan Beulich wrote:
> On 29.04.2024 20:28, Andrew Cooper wrote:
>> If max leaf is greater than 0xd but xsave not available to the guest, then the
>> current XSAVE size should not be filled in.  This is a latent bug for now as
>> the guest max leaf is 0xd, but will become problematic in the future.
> Why would this not be an issue when .max_leaf == 0xd, but .xsave == 0?

Hmm, true.  I'll adjust the description.

>
>> The comment concerning XSS state is wrong.  VT-x doesn't manage host/guest
>> state automatically, but there is provision for "host only" bits to be set, so
>> the implications are still accurate.
>>
>> Introduce {xstate,hw}_compressed_size() helpers to mirror the uncompressed
>> ones.
>>
>> This in turn higlights a bug in xstate_init().  Defaulting this_cpu(xss) to ~0
>> requires a forced write to clear it back out.  This in turn highlights that
>> it's only a safe default on systems with XSAVES.
> Well, yes, such an explicit write was expected to appear when some xsaves-
> based component would actually be enabled. Much like the set_xcr0() there.

I stumbled over this because the debug logic ended up trying to restore
0xffffffff (the thing it read out as the "last" value) back into XSS. 
This works about as well as you'd expect.
>> Signed-off-by: Andrew Cooper <[e-mail]>
>> ---
>> CC: Jan Beulich <[e-mail]>
>> CC: Roger Pau Monné <[e-mail]>
>>
>> The more I think about it, the more I think that cross-checking with hardware
>> is a bad move.  It's horribly expensive, and for supervisor states in
>> particular, liable to interfere with functionality.
> I agree, but I'd also like to see the cross checking not dropped entirely.
> Can't we arrange for such to happen during boot, before we enable any such
> functionality? After all even in debug builds it's not overly useful to do
> the same cross-checking (i.e. for identical inputs) over and over again.
> Of course doing in an exhaustive manner may be okay for the uncompressed
> values, but might be a little too much for all possible combinations in
> order to check compressed values, too.

Given the observation of patch 2 being buggy and my final sanity test
before posting didn't notice, I think doing this all at boot would be a
much better idea.

I think I'm going to do a new patch early in the series as an adjustment
to xstate_init().

We can't feasibly test every combination, but what ought to do is
linearly accumulate the xstates Xen knows about and checking that in
each case the size(s) increase as appropriate.

This will have a substantial impact on the remainder of the series, but
I think the end result will be all the better for it.
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -614,6 +614,65 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
>>      return size;
>>  }
>>  
>> +static unsigned int hw_compressed_size(uint64_t xstates)
>> +{
>> +    uint64_t curr_xcr0 = get_xcr0(), curr_xss = get_msr_xss();
>> +    unsigned int size;
>> +    bool ok;
>> +
>> +    ok = set_xcr0(xstates & ~XSTATE_XSAVES_ONLY);
>> +    ASSERT(ok);
>> +    set_msr_xss(xstates & XSTATE_XSAVES_ONLY);
>> +
>> +    size = cpuid_count_ebx(XSTATE_CPUID, 1);
>> +
>> +    ok = set_xcr0(curr_xcr0);
>> +    ASSERT(ok);
>> +    set_msr_xss(curr_xss);
>> +
>> +    return size;
>> +}
>> +
>> +unsigned int xstate_compressed_size(uint64_t xstates)
>> +{
>> +    unsigned int i, size = XSTATE_AREA_MIN_SIZE;
>> +
>> +    if ( xstates == 0 ) /* TODO: clean up paths passing 0 in here. */
>> +        return 0;
>> +
>> +    if ( xstates <= (X86_XCR0_SSE | X86_XCR0_FP) )
> Same comment as on the earlier change regarding the (lack of) use of ....
>
>> +        return size;
>> +
>> +    /*
>> +     * For the compressed size, every component matters.  Some are
>> +     * automatically rounded up to 64 first.
>> +     */
>> +    xstates &= ~XSTATE_FP_SSE;
> ... this constant up there.
>
>> +    for_each_set_bit ( i, &xstates, 63 )
>> +    {
>> +        if ( test_bit(i, &xstate_align) )
>> +            size = ROUNDUP(size, 64);
>> +
>> +        size += xstate_sizes[i];
>> +    }
> The comment is a little misleading: As you have it in code, it is not the
> component's size that is rounded up, but its position. Maybe "Some have
> their start automatically rounded up to 64 first"?

Size in that sentence referees to the compressed size of everything, not
the size of the component.

But I'll try to make it clearer.

~Andrew

Patch

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 7a38e032146a..a822e80c7ea7 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -330,23 +330,15 @@  void guest_cpuid(const struct vcpu *v, uint32_t leaf,
     case XSTATE_CPUID:
         switch ( subleaf )
         {
-        case 1:
-            if ( !p->xstate.xsavec && !p->xstate.xsaves )
-                break;
-
-            /*
-             * TODO: Figure out what to do for XSS state.  VT-x manages host
-             * vs guest MSR_XSS automatically, so as soon as we start
-             * supporting any XSS states, the wrong XSS will be in context.
-             */
-            BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
-            fallthrough;
         case 0:
-            /*
-             * Read CPUID[0xD,0/1].EBX from hardware.  They vary with enabled
-             * XSTATE, and appropriate XCR0|XSS are in context.
-             */
-            res->b = cpuid_count_ebx(leaf, subleaf);
+            if ( p->basic.xsave )
+                res->b = xstate_uncompressed_size(v->arch.xcr0);
+            break;
+
+        case 1:
+            if ( p->xstate.xsavec )
+                res->b = xstate_compressed_size(v->arch.xcr0 |
+                                                v->arch.msrs->xss.raw);
             break;
         }
         break;
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index bfb66dd766b6..da1d89d2f416 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -109,6 +109,7 @@  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_uncompressed_size(uint64_t xcr0);
+unsigned int xstate_compressed_size(uint64_t xstates);
 
 static inline uint64_t xgetbv(unsigned int index)
 {
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index a94f4025fce5..b2cf3ae6acee 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -614,6 +614,65 @@  unsigned int xstate_uncompressed_size(uint64_t xcr0)
     return size;
 }
 
+static unsigned int hw_compressed_size(uint64_t xstates)
+{
+    uint64_t curr_xcr0 = get_xcr0(), curr_xss = get_msr_xss();
+    unsigned int size;
+    bool ok;
+
+    ok = set_xcr0(xstates & ~XSTATE_XSAVES_ONLY);
+    ASSERT(ok);
+    set_msr_xss(xstates & XSTATE_XSAVES_ONLY);
+
+    size = cpuid_count_ebx(XSTATE_CPUID, 1);
+
+    ok = set_xcr0(curr_xcr0);
+    ASSERT(ok);
+    set_msr_xss(curr_xss);
+
+    return size;
+}
+
+unsigned int xstate_compressed_size(uint64_t xstates)
+{
+    unsigned int i, size = XSTATE_AREA_MIN_SIZE;
+
+    if ( xstates == 0 ) /* TODO: clean up paths passing 0 in here. */
+        return 0;
+
+    if ( xstates <= (X86_XCR0_SSE | X86_XCR0_FP) )
+        return size;
+
+    /*
+     * For the compressed size, every component matters.  Some are
+     * automatically rounded up to 64 first.
+     */
+    xstates &= ~XSTATE_FP_SSE;
+    for_each_set_bit ( i, &xstates, 63 )
+    {
+        if ( test_bit(i, &xstate_align) )
+            size = ROUNDUP(size, 64);
+
+        size += xstate_sizes[i];
+    }
+
+    /* In debug builds, cross-check our calculation with hardware. */
+    if ( IS_ENABLED(CONFIG_DEBUG) )
+    {
+        unsigned int hwsize;
+
+        xstates |= XSTATE_FP_SSE;
+        hwsize = hw_compressed_size(xstates);
+
+        if ( size != hwsize )
+            printk_once(XENLOG_ERR "%s(%#"PRIx64") size %#x != hwsize %#x\n",
+                        __func__, xstates, size, hwsize);
+        size = hwsize;
+    }
+
+    return size;
+}
+
 static bool valid_xcr0(uint64_t xcr0)
 {
     /* FP must be unconditionally set. */
@@ -681,7 +740,8 @@  void xstate_init(struct cpuinfo_x86 *c)
      * write it.
      */
     this_cpu(xcr0) = 0;
-    this_cpu(xss) = ~0;
+    if ( cpu_has_xsaves )
+        this_cpu(xss) = ~0;
 
     cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
     feature_mask = (((u64)edx << 32) | eax) & XCNTXT_MASK;
@@ -694,6 +754,8 @@  void xstate_init(struct cpuinfo_x86 *c)
     set_in_cr4(X86_CR4_OSXSAVE);
     if ( !set_xcr0(feature_mask) )
         BUG();
+    if ( cpu_has_xsaves )
+        set_msr_xss(0);
 
     if ( bsp )
     {