Re: [PATCH v4 26/26] tools/libxc: Calculate xstate cpuid leaf from guest information

["Jan Beulich" <JBeulich@suse.com>]

>>> On 23.03.16 at 17:36, <andrew.cooper3@citrix.com> wrote:
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -398,54 +398,97 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
>      }
>  }
>  
> +/* XSTATE bits in XCR0. */
> +#define X86_XCR0_X87    (1ULL <<  0)
> +#define X86_XCR0_SSE    (1ULL <<  1)
> +#define X86_XCR0_AVX    (1ULL <<  2)
> +#define X86_XCR0_BNDREG (1ULL <<  3)
> +#define X86_XCR0_BNDCSR (1ULL <<  4)
> +#define X86_XCR0_LWP    (1ULL << 62)

Why an incomplete set? At least PKRU should be needed right
away. And I see no reason why the three AVX-512 pieces can't
be put here right away too.

> +#define X86_XSS_MASK    (0) /* No XSS states supported yet. */
> +
> +/* Per-component subleaf flags. */
> +#define XSTATE_XSS      (1ULL <<  0)
> +#define XSTATE_ALIGN64  (1ULL <<  1)
> +
>  /* Configure extended state enumeration leaves (0x0000000D for xsave) */
>  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_MPX, info->featureset) )
> +        guest_xfeature_mask |= X86_XCR0_BNDREG | X86_XCR0_BNDCSR;
> +
> +    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;

This is ugly. For one, your dependency mechanism should be able to
express the dependencies you "manually"enforce above. And beyond
that masking with info->xfeature_mask should be all that's needed,
together with enforcing the XCR0 / XSS split ...

>      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;
>          /* EDX: high 32bits of xfeature_enabled_mask */
> -        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
> +        regs[3] = guest_xfeature_mask >> 32;

... here and ...

>      case 1: /* leaf 1 */
>          regs[0] = info->featureset[featureword_of(X86_FEATURE_XSAVEOPT)];
> -        regs[2] &= info->xfeature_mask;
> -        regs[3] = 0;
> +        regs[2] = guest_xfeature_mask & X86_XSS_MASK;
> +        regs[3] = (guest_xfeature_mask >> 32) & X86_XSS_MASK;

... here. Yet not by a compile time defined mask, but by using
(host) CPUID output: It is clear that once a bit got assigned to XCR0
vs XSS, it won't ever change. Hence it doesn't matter whether you
use the guest or host view of that split. And this will then also - other
than you've said before would be unavoidable - make unnecessary to
always update this code when new states get added.

> -    case 2 ... 63: /* sub-leaves */
> -        if ( !(info->xfeature_mask & (1ULL << input[1])) )
> +
> +    case 2 ... 62: /* per-component sub-leaves */
> +        if ( !(guest_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;

Wouldn't this better also use the "known features" approach, by
adding yet another word in cpufeatureset.h?

Btw., looking at that header again I now wonder whether it
wouldn't have been neater to make XEN_CPUFEATURE() a
3-parameter macro, with word and bit specified separately
and a default definition of

#define XEN_CPUFEATURE(name, word, bit) XEN_X86_FEATURE_##name = (word) * 32 + (bit),

avoiding the ugly repeated "*32" in all macro invocations. Of
course we'd need to adjust this before we release with this new
interface.

Jan

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