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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/06/16 11:19, Jan Beulich wrote:
>>>> 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?

Compiler says no.  Fixed.

>  Also in this first
> construct there's no real reason to use MAX() or max() in the first
> place.

I put it in for code consistency only.

>
>> @@ -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.

So it is.  I will triple check all of this.

~Andrew

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