Re: [Xen-devel] [PATCH] libxc/x86: XSAVE related adjustments

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel