[PATCH] libxc/x86: XSAVE related adjustments

[PATCH] libxc/x86: XSAVE related adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

- don't unintentionally increase features reported by sub-leaf 0
  EDX:EAX
- don't discard the known flags in sub-leaves 2..63 ECX
- handle components 32...62 (EDX) in sub-leaf 1 consistently with
  0...31 (ECX)
- zap sub-leaves beyond 62

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While obviously requiring re-basing on either end when taking Andrew's
CPUID levelling series into account, the changes done here appear to
be orthogonal to those done in his series.

--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -281,10 +281,14 @@ static void intel_xc_cpuid_policy(xc_int
     }
 }
 
+/* Leaf 1, EAX: */
 #define XSAVEOPT        (1 << 0)
 #define XSAVEC          (1 << 1)
 #define XGETBV1         (1 << 2)
 #define XSAVES          (1 << 3)
+/* Leaves beyond 1, ECX: */
+#define XSTATE_XSS      (1 << 0)
+#define XSTATE_ALIGN64  (1 << 1)
 /* Configure extended state enumeration leaves (0x0000000D for xsave) */
 static void xc_cpuid_config_xsave(xc_interface *xch,
                                   const struct cpuid_domain_info *info,
@@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
     {
     case 0: 
         /* EAX: low 32bits of xfeature_enabled_mask */
-        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
+        regs[0] &= info->xfeature_mask;
         /* EDX: high 32bits of xfeature_enabled_mask */
-        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
+        regs[3] &= info->xfeature_mask >> 32;
         /* ECX: max size required by all HW features */
         {
             unsigned int _input[2] = {0xd, 0x0}, _regs[4];
@@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int
         if ( !info->hvm )
             regs[0] &= ~XSAVES;
         regs[2] &= info->xfeature_mask;
-        regs[3] = 0;
+        regs[3] &= info->xfeature_mask >> 32;
         break;
-    case 2 ... 63: /* sub-leaves */
+    case 2 ... 62: /* per-component sub-leaves */
         if ( !(info->xfeature_mask & (1ULL << input[1])) )
         {
             regs[0] = regs[1] = regs[2] = regs[3] = 0;
             break;
         }
         /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
-        regs[2] = regs[3] = 0;
+        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
+        regs[3] = 0;
+        break;
+    default:
+        regs[0] = regs[1] = regs[2] = regs[3] = 0;
         break;
     }
 }




[-- Attachment #2: libxc-x86-cpuid-xstate.patch --]
[-- Type: text/plain, Size: 2469 bytes --]

libxc/x86: XSAVE related adjustments

- don't unintentionally increase features reported by sub-leaf 0
  EDX:EAX
- don't discard the known flags in sub-leaves 2..63 ECX
- handle components 32...62 (EDX) in sub-leaf 1 consistently with
  0...31 (ECX)
- zap sub-leaves beyond 62

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While obviously requiring re-basing on either end when taking Andrew's
CPUID levelling series into account, the changes done here appear to
be orthogonal to those done in his series.

--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -281,10 +281,14 @@ static void intel_xc_cpuid_policy(xc_int
     }
 }
 
+/* Leaf 1, EAX: */
 #define XSAVEOPT        (1 << 0)
 #define XSAVEC          (1 << 1)
 #define XGETBV1         (1 << 2)
 #define XSAVES          (1 << 3)
+/* Leaves beyond 1, ECX: */
+#define XSTATE_XSS      (1 << 0)
+#define XSTATE_ALIGN64  (1 << 1)
 /* Configure extended state enumeration leaves (0x0000000D for xsave) */
 static void xc_cpuid_config_xsave(xc_interface *xch,
                                   const struct cpuid_domain_info *info,
@@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
     {
     case 0: 
         /* EAX: low 32bits of xfeature_enabled_mask */
-        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
+        regs[0] &= info->xfeature_mask;
         /* EDX: high 32bits of xfeature_enabled_mask */
-        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
+        regs[3] &= info->xfeature_mask >> 32;
         /* ECX: max size required by all HW features */
         {
             unsigned int _input[2] = {0xd, 0x0}, _regs[4];
@@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int
         if ( !info->hvm )
             regs[0] &= ~XSAVES;
         regs[2] &= info->xfeature_mask;
-        regs[3] = 0;
+        regs[3] &= info->xfeature_mask >> 32;
         break;
-    case 2 ... 63: /* sub-leaves */
+    case 2 ... 62: /* per-component sub-leaves */
         if ( !(info->xfeature_mask & (1ULL << input[1])) )
         {
             regs[0] = regs[1] = regs[2] = regs[3] = 0;
             break;
         }
         /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
-        regs[2] = regs[3] = 0;
+        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
+        regs[3] = 0;
+        break;
+    default:
+        regs[0] = regs[1] = regs[2] = regs[3] = 0;
         break;
     }
 }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc/x86: XSAVE related adjustments

[Andrew Cooper]

On 22/03/16 13:05, Jan Beulich wrote:
> - don't unintentionally increase features reported by sub-leaf 0
>   EDX:EAX
> - don't discard the known flags in sub-leaves 2..63 ECX
> - handle components 32...62 (EDX) in sub-leaf 1 consistently with
>   0...31 (ECX)
> - zap sub-leaves beyond 62
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> While obviously requiring re-basing on either end when taking Andrew's
> CPUID levelling series into account, the changes done here appear to
> be orthogonal to those done in his series.
>
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -281,10 +281,14 @@ static void intel_xc_cpuid_policy(xc_int
>      }
>  }
>  
> +/* Leaf 1, EAX: */
>  #define XSAVEOPT        (1 << 0)
>  #define XSAVEC          (1 << 1)
>  #define XGETBV1         (1 << 2)
>  #define XSAVES          (1 << 3)

Hmm - I should convert these to be X86_FEATURESET_xxx values.

> +/* Leaves beyond 1, ECX: */
> +#define XSTATE_XSS      (1 << 0)
> +#define XSTATE_ALIGN64  (1 << 1)
>  /* Configure extended state enumeration leaves (0x0000000D for xsave) */
>  static void xc_cpuid_config_xsave(xc_interface *xch,
>                                    const struct cpuid_domain_info *info,
> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
>      {
>      case 0: 
>          /* EAX: low 32bits of xfeature_enabled_mask */
> -        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
> +        regs[0] &= info->xfeature_mask;
>          /* EDX: high 32bits of xfeature_enabled_mask */
> -        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
> +        regs[3] &= info->xfeature_mask >> 32;
>          /* ECX: max size required by all HW features */
>          {
>              unsigned int _input[2] = {0xd, 0x0}, _regs[4];

This is an improvement on the code currently present, but is still
superseded by the final patch of my cpuid series.

> @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int

Between these two hunks, there is a loop bound which is also wrong.

>          if ( !info->hvm )
>              regs[0] &= ~XSAVES;
>          regs[2] &= info->xfeature_mask;
> -        regs[3] = 0;
> +        regs[3] &= info->xfeature_mask >> 32;
>          break;
> -    case 2 ... 63: /* sub-leaves */
> +    case 2 ... 62: /* per-component sub-leaves */
>          if ( !(info->xfeature_mask & (1ULL << input[1])) )

Now I think about it, this check is incomplete.  xfeature_mask doesn't
contain xss values.

For now its fine, but it will cause problems when support for Processor
Trace is added.

>          {
>              regs[0] = regs[1] = regs[2] = regs[3] = 0;
>              break;
>          }
>          /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
> -        regs[2] = regs[3] = 0;
> +        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
> +        regs[3] = 0;
> +        break;
> +    default:
> +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
>          break;

If you wish, I can fold this patch into the final patch of my cpuid series.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc/x86: XSAVE related adjustments

[Jan Beulich]

>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> wrote:
>> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
>>      {
>>      case 0: 
>>          /* EAX: low 32bits of xfeature_enabled_mask */
>> -        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
>> +        regs[0] &= info->xfeature_mask;
>>          /* EDX: high 32bits of xfeature_enabled_mask */
>> -        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
>> +        regs[3] &= info->xfeature_mask >> 32;
>>          /* ECX: max size required by all HW features */
>>          {
>>              unsigned int _input[2] = {0xd, 0x0}, _regs[4];
> 
> This is an improvement on the code currently present, but is still
> superseded by the final patch of my cpuid series.

Is it? I did check your tree before sending, and you do only
mechanical adjustments. In particular you don't switch to
&= and you don't drop the pointless and-ing with 0xFFFFFFFF.

>> @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int
> 
> Between these two hunks, there is a loop bound which is also wrong.

But seeing that your patches fix it I didn't bother stealing the fix
from your patches.

>>          if ( !info->hvm )
>>              regs[0] &= ~XSAVES;
>>          regs[2] &= info->xfeature_mask;
>> -        regs[3] = 0;
>> +        regs[3] &= info->xfeature_mask >> 32;
>>          break;
>> -    case 2 ... 63: /* sub-leaves */
>> +    case 2 ... 62: /* per-component sub-leaves */
>>          if ( !(info->xfeature_mask & (1ULL << input[1])) )
> 
> Now I think about it, this check is incomplete.  xfeature_mask doesn't
> contain xss values.

For now the XSS bitmask is blank. Looking at everything together I
do think though that once it becomes non-zero, info->xfeature_mask
will need to become the OR of both masks.

>>          {
>>              regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>              break;
>>          }
>>          /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
>> -        regs[2] = regs[3] = 0;
>> +        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
>> +        regs[3] = 0;
>> +        break;
>> +    default:
>> +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>          break;
> 
> If you wish, I can fold this patch into the final patch of my cpuid series.

I'd be fine with that, albeit (as said in the submission) the changes
are independent of one another despite them causing conflicts.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc/x86: XSAVE related adjustments

[Andrew Cooper]

On 22/03/16 14:46, Jan Beulich wrote:
>>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> wrote:
>>> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
>>>      {
>>>      case 0: 
>>>          /* EAX: low 32bits of xfeature_enabled_mask */
>>> -        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
>>> +        regs[0] &= info->xfeature_mask;
>>>          /* EDX: high 32bits of xfeature_enabled_mask */
>>> -        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
>>> +        regs[3] &= info->xfeature_mask >> 32;
>>>          /* ECX: max size required by all HW features */
>>>          {
>>>              unsigned int _input[2] = {0xd, 0x0}, _regs[4];
>> This is an improvement on the code currently present, but is still
>> superseded by the final patch of my cpuid series.
> Is it? I did check your tree before sending, and you do only
> mechanical adjustments. In particular you don't switch to
> &= and you don't drop the pointless and-ing with 0xFFFFFFFF.

Using &= is specifically wrong and buggy.  My patch replaces
info->xfeature_mask with guest_xfeature_mask, which itself is calculated
from the guest feature availability.

The value in regs[] is dom0's view of the cpuid leaves, and are
inappropriate to be combined to make the guests view.  Observe that I
have specifically been replacing masks with assignments.

Consider the (admittedly contrived scenario of) dom0 being denied access
to xsave, while domU is intended to have access.  A less contrived
scenario is a 32bit dom0 trying to construct a 64bit PV guest.  It only
worked previously because dom0 used native cpuid which bypassed Xen
hiding the LM bit.

>
>>> @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int
>> Between these two hunks, there is a loop bound which is also wrong.
> But seeing that your patches fix it I didn't bother stealing the fix
> from your patches.
>
>>>          if ( !info->hvm )
>>>              regs[0] &= ~XSAVES;
>>>          regs[2] &= info->xfeature_mask;
>>> -        regs[3] = 0;
>>> +        regs[3] &= info->xfeature_mask >> 32;
>>>          break;
>>> -    case 2 ... 63: /* sub-leaves */
>>> +    case 2 ... 62: /* per-component sub-leaves */
>>>          if ( !(info->xfeature_mask & (1ULL << input[1])) )
>> Now I think about it, this check is incomplete.  xfeature_mask doesn't
>> contain xss values.
> For now the XSS bitmask is blank. Looking at everything together I
> do think though that once it becomes non-zero, info->xfeature_mask
> will need to become the OR of both masks.
>
>>>          {
>>>              regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>>              break;
>>>          }
>>>          /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
>>> -        regs[2] = regs[3] = 0;
>>> +        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
>>> +        regs[3] = 0;
>>> +        break;
>>> +    default:
>>> +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>>          break;
>> If you wish, I can fold this patch into the final patch of my cpuid series.
> I'd be fine with that, albeit (as said in the submission) the changes
> are independent of one another despite them causing conflicts.

It would be clearer than having two different patches both fixing part
of the code.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc/x86: XSAVE related adjustments

[Jan Beulich]

>>> On 22.03.16 at 18:23, <andrew.cooper3@citrix.com> wrote:
> On 22/03/16 14:46, Jan Beulich wrote:
>>>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
>>>>      {
>>>>      case 0: 
>>>>          /* EAX: low 32bits of xfeature_enabled_mask */
>>>> -        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
>>>> +        regs[0] &= info->xfeature_mask;
>>>>          /* EDX: high 32bits of xfeature_enabled_mask */
>>>> -        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
>>>> +        regs[3] &= info->xfeature_mask >> 32;
>>>>          /* ECX: max size required by all HW features */
>>>>          {
>>>>              unsigned int _input[2] = {0xd, 0x0}, _regs[4];
>>> This is an improvement on the code currently present, but is still
>>> superseded by the final patch of my cpuid series.
>> Is it? I did check your tree before sending, and you do only
>> mechanical adjustments. In particular you don't switch to
>> &= and you don't drop the pointless and-ing with 0xFFFFFFFF.
> 
> Using &= is specifically wrong and buggy.  My patch replaces
> info->xfeature_mask with guest_xfeature_mask, which itself is calculated
> from the guest feature availability.

Oh, right - if you imply buggy just if it was done with or after your
change.

>>>>          {
>>>>              regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>>>              break;
>>>>          }
>>>>          /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
>>>> -        regs[2] = regs[3] = 0;
>>>> +        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
>>>> +        regs[3] = 0;
>>>> +        break;
>>>> +    default:
>>>> +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>>>          break;
>>> If you wish, I can fold this patch into the final patch of my cpuid series.
>> I'd be fine with that, albeit (as said in the submission) the changes
>> are independent of one another despite them causing conflicts.
> 
> It would be clearer than having two different patches both fixing part
> of the code.

Especially with the above in mind - go ahead.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel