[v2,30/30] tools/libxc: Calculate xstate cpuid leaf from guest information

Commit Message

Andrew Cooper Feb. 5, 2016, 1:42 p.m. UTC

It is unsafe to generate the guests xstate leaves from host information, as it
prevents the differences between hosts from being hidden.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Ian Campbell <[e-mail]>
CC: Ian Jackson <[e-mail]>
CC: Wei Liu <[e-mail]>
---
 tools/libxc/xc_cpuid_x86.c | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

Comments

Jan Beulich Feb. 5, 2016, 2:28 p.m. UTC | #1

>>> On 05.02.16 at 14:42, <[e-mail]> wrote:
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -380,6 +380,11 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
>      }
>  }
>  
> +#define X86_XCR0_X87    (1ULL <<  0)
> +#define X86_XCR0_SSE    (1ULL <<  1)
> +#define X86_XCR0_AVX    (1ULL <<  2)
> +#define X86_XCR0_LWP    (1ULL << 62)

What about all the other bits we meanwhile know?

Jan

Andrew Cooper Feb. 5, 2016, 3:22 p.m. UTC | #2

On 05/02/16 14:28, Jan Beulich wrote:
>>>> On 05.02.16 at 14:42, <[e-mail]> wrote:
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -380,6 +380,11 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
>>      }
>>  }
>>  
>> +#define X86_XCR0_X87    (1ULL <<  0)
>> +#define X86_XCR0_SSE    (1ULL <<  1)
>> +#define X86_XCR0_AVX    (1ULL <<  2)
>> +#define X86_XCR0_LWP    (1ULL << 62)
> What about all the other bits we meanwhile know?

These are the only ones used.

... which leads me to wonder where MPX support went.  I think I may have
accidentally lost it in the rebase.

~Andrew

Patch

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 0e79812..810377c 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -380,6 +380,11 @@  static void intel_xc_cpuid_policy(xc_interface *xch,
     }
 }
 
+#define X86_XCR0_X87    (1ULL <<  0)
+#define X86_XCR0_SSE    (1ULL <<  1)
+#define X86_XCR0_AVX    (1ULL <<  2)
+#define X86_XCR0_LWP    (1ULL << 62)
+
 #define XSAVEOPT        (1 << 0)
 #define XSAVEC          (1 << 1)
 #define XGETBV1         (1 << 2)
@@ -389,34 +394,53 @@  static void xc_cpuid_config_xsave(xc_interface *xch,
                                   const struct cpuid_domain_info *info,
                                   const unsigned int *input, unsigned int *regs)
 {
-    if ( info->xfeature_mask == 0 )
+    uint64_t guest_xfeature_mask;
+
+    if ( info->xfeature_mask == 0 ||
+         !test_bit(X86_FEATURE_XSAVE, info->featureset) )
     {
         regs[0] = regs[1] = regs[2] = regs[3] = 0;
         return;
     }
 
+    guest_xfeature_mask = X86_XCR0_SSE | X86_XCR0_X87;
+
+    if ( test_bit(X86_FEATURE_AVX, info->featureset) )
+        guest_xfeature_mask |= X86_XCR0_AVX;
+
+    if ( test_bit(X86_FEATURE_LWP, info->featureset) )
+        guest_xfeature_mask |= X86_XCR0_LWP;
+
+    /*
+     * Clamp to host mask.  Should be no-op, as guest_xfeature_mask should not
+     * be able to be calculated as larger than info->xfeature_mask.
+     *
+     * TODO - see about making this a harder error.
+     */
+    guest_xfeature_mask &= info->xfeature_mask;
+
     switch ( input[1] )
     {
-    case 0: 
+    case 0:
         /* EAX: low 32bits of xfeature_enabled_mask */
-        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
+        regs[0] = guest_xfeature_mask & 0xFFFFFFFF;
         /* EDX: high 32bits of xfeature_enabled_mask */
-        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
+        regs[3] = (guest_xfeature_mask >> 32) & 0xFFFFFFFF;
         /* ECX: max size required by all HW features */
         {
             unsigned int _input[2] = {0xd, 0x0}, _regs[4];
             regs[2] = 0;
-            for ( _input[1] = 2; _input[1] < 64; _input[1]++ )
+            for ( _input[1] = 2; _input[1] <= 62; _input[1]++ )
             {
                 cpuid(_input, _regs);
                 if ( (_regs[0] + _regs[1]) > regs[2] )
                     regs[2] = _regs[0] + _regs[1];
             }
         }
-        /* EBX: max size required by enabled features. 
-         * This register contains a dynamic value, which varies when a guest 
-         * enables or disables XSTATE features (via xsetbv). The default size 
-         * after reset is 576. */ 
+        /* EBX: max size required by enabled features.
+         * This register contains a dynamic value, which varies when a guest
+         * enables or disables XSTATE features (via xsetbv). The default size
+         * after reset is 576. */
         regs[1] = 512 + 64; /* FP/SSE + XSAVE.HEADER */
         break;
     case 1: /* leaf 1 */
@@ -424,7 +448,7 @@  static void xc_cpuid_config_xsave(xc_interface *xch,
         regs[1] = regs[2] = regs[3] = 0;
         break;
     case 2 ... 63: /* sub-leaves */
-        if ( !(info->xfeature_mask & (1ULL << input[1])) )
+        if ( !(guest_xfeature_mask & (1ULL << input[1])) )
         {
             regs[0] = regs[1] = regs[2] = regs[3] = 0;
             break;