Re: [Xen-devel] [PATCH 1/2] x86: flush high xstate CPUID sub-leaves to zero

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/06/16 16:05, Jan Beulich wrote:
> In line with other recent changes, these should be fully white listed,
> requiring us to zero them until the obtain a meaning we support.
>
> Without XSAVE support, all xstate sub-leaves should be zero.
>
> Also move away from checking host XSAVE support - we really ought to
> consider the guest flag for that purpose.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, with one suggestion

>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3433,7 +3433,13 @@ void hvm_cpuid(unsigned int input, unsig
>          *edx = v->vcpu_id * 2;
>          break;
>  
> -    case 0xd:
> +    case XSTATE_CPUID:
> +        hvm_cpuid(1, NULL, NULL, &_ecx, NULL);
> +        if ( !(_ecx & cpufeat_mask(X86_FEATURE_XSAVE)) || count >= 63 )
> +        {
> +            *eax = *ebx = *ecx = *edx = 0;
> +            break;
> +        }
>          /* EBX value of main leaf 0 depends on enabled xsave features */
>          if ( count == 0 && v->arch.xcr0 ) 
>          {
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -928,6 +928,8 @@ void pv_cpuid(struct cpu_user_regs *regs
>  
>      switch ( leaf )
>      {
> +        uint32_t tmp;
> +
>      case 0x00000001:
>          c &= pv_featureset[FEATURESET_1c];
>          d &= pv_featureset[FEATURESET_1d];
> @@ -1085,14 +1087,19 @@ void pv_cpuid(struct cpu_user_regs *regs
>          break;
>  
>      case XSTATE_CPUID:
> -        if ( !cpu_has_xsave )
> +        if ( !((!is_control_domain(currd) && !is_hardware_domain(currd)

I would recommend extra brackets on this line, to avoid the possible
mis-interpretation of !is_control_domain(currd) &&
(!is_hardware_domain(currd) ? ...

> +                ? ({
> +                    uint32_t ecx;
> +
> +                    domain_cpuid(currd, 1, 0, &tmp, &tmp, &ecx, &tmp);
> +                    ecx & pv_featureset[FEATURESET_1c];
> +                  })
> +                : cpuid_ecx(1)) & cpufeat_mask(X86_FEATURE_XSAVE)) ||
> +             subleaf >= 63 )

This is rather nasty code.  I am glad that my longterm plans involve
removing it all.

~Andrew

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