Re: [PATCH v5 21/21] tools/libxc: Calculate xstate cpuid leaf from guest information

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

>>> On 07.04.16 at 13:57, <andrew.cooper3@citrix.com> wrote:
> The existing logic is broken for heterogeneous migration.  By always
> advertising the host maximum xstate, a migration to a less capable host always
> fails as Xen cannot accomodate the xcr0_accum in the migration stream.

I don't understand this - xcr0_accum is definitely part of the
migration stream.

> v5:
>  * Reintroduce PKRU, (again, lost due to rebasing).
>  * Rewrite the commit message and comments to try and better explain why I am
>    deliberatly removing host-specific information from the xstate calculation.
>  * Reintroduce 0xFFFFFFFF masks for EAX, to avoid Coverity complaining about
>    truncation on assignment.

Urgh - I don't think ugliness like this can be justified by Coverity
complaining. That would be different if the compiler complained
(like compilers other than gcc do).

>  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_PKU, info->featureset) )
> +        guest_xfeature_mask |= X86_XCR0_PKRU;
> +
> +    if ( test_bit(X86_FEATURE_LWP, info->featureset) )
> +        guest_xfeature_mask |= X86_XCR0_LWP;

I continue to be unhappy about that white listing, but well...

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

This is correct only because X86_XSS_MASK == 0(or else >> and &
need to be swapped). Yet if you assume this, the and-ing with
0xFFFFFFFF makes even less sense here than above.

> +    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;

I'm sorry for realizing this only now, but shouldn't we hide XSTATE_XSS
from PV guests? Or is this being taken care of elsewhere?

Jan


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