Re: [Xen-devel] [PATCH for-4.7] x86/cpuid: Calculate a guests xfeature_mask from its featureset

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.16 at 18:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -3443,6 +3443,51 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>          switch ( count )
>          {
>          case 0:
> +        {
> +            uint64_t xfeature_mask = XSTATE_FP_SSE;
> +            uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
> +
> +            if ( _ecx & cpufeat_mask(X86_FEATURE_AVX) )
> +            {
> +                xfeature_mask |= XSTATE_YMM;
> +                xstate_size = MAX(xstate_size,
> +                                  xstate_offsets[_XSTATE_YMM] +
> +                                  xstate_sizes[_XSTATE_YMM]);

Any reason not to use the type safe max() here? Also in this first
construct there's no real reason to use MAX() or max() in the first
place.

> @@ -1087,19 +1087,48 @@ void pv_cpuid(struct cpu_user_regs *regs)
>          break;
>  
>      case XSTATE_CPUID:
> -        if ( !((!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 )
> +
> +        if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
> +            domain_cpuid(currd, 1, 0, &tmp, &tmp, &_ecx, &tmp);
> +        else
> +            _ecx = cpuid_ecx(1);
> +        _ecx &= pv_featureset[FEATURESET_1c];
> +
> +        if ( !(_ecx & cpufeat_mask(X86_FEATURE_XSAVE)) || subleaf >= 63 )
>              goto unsupported;
>          switch ( subleaf )
>          {
>          case 0:
> +        {
> +            uint64_t xfeature_mask = XSTATE_FP_SSE;
> +            uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
> +
> +            if ( _ecx & cpufeat_mask(X86_FEATURE_AVX) )
> +            {
> +                xfeature_mask |= XSTATE_YMM;
> +                xstate_size = MAX(xstate_size,
> +                                  xstate_offsets[_XSTATE_YMM] +
> +                                  xstate_sizes[_XSTATE_YMM]);
> +            }
> +
> +            if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
> +                domain_cpuid(currd, 0x80000001, 0, &tmp, &tmp, &_ecx, &tmp);
> +            else
> +                _ecx = cpuid_ecx(0x80000001);
> +            _ecx &= pv_featureset[FEATURESET_e1c];
> +
> +            if ( _ecx & cpufeat_mask(X86_FEATURE_LWP) )
> +            {
> +                xfeature_mask |= XSTATE_LWP;
> +                xstate_size = MAX(xstate_size,
> +                                  xstate_offsets[_XSTATE_LWP] +
> +                                  xstate_sizes[_XSTATE_LWP]);
> +            }

I'm sorry for not noticing in the pre-review that LWP is HVM-only too,
as per cpufeatureset.h.

Jan


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