Re: [Xen-devel] [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/01/17 15:58, Jan Beulich wrote:
>>>> On 18.01.17 at 20:40, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
>>      }
>>  }
>>  
>> +static void recalculate_common(struct cpuid_policy *p)
>> +{
>> +    switch ( p->x86_vendor )
>> +    {
>> +    case X86_VENDOR_INTEL:
>> +        p->extd.vendor_ebx = 0;
>> +        p->extd.vendor_ecx = 0;
>> +        p->extd.vendor_edx = 0;
>> +        break;
>> +
>> +    case X86_VENDOR_AMD:
>> +        p->extd.vendor_ebx = p->basic.vendor_ebx;
>> +        p->extd.vendor_ecx = p->basic.vendor_ecx;
>> +        p->extd.vendor_edx = p->basic.vendor_edx;
>> +        break;
>> +    }
>> +}
> I find the word "common" in the name here not very indicative
> of what the function does, especially with ...
>
>> @@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
>>          min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
>>      p->feat.max_subleaf =
>>          min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
>> -    p->extd.max_leaf =
>> -        min_t(uint32_t, p->extd.max_leaf,
>> -              0x80000000u + ARRAY_SIZE(p->extd.raw) - 1);
>> +    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf & 
>> 0xffff,
>> +                                          ARRAY_SIZE(p->extd.raw) - 1);
>>  
>>      cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
>>      recalculate_xstate(p);
>> +    recalculate_common(p);
>>  }
> ... the neighboring call here (which is quite a bit more specific).
> Of course possible alternatives depend on what further uses of
> this function you do (or do not) plan, but by the name of the
> other function here it could be recalculate_extd_vendor().

It adjustments common to the global calculations, and the per-domain
recalculations.

It isn't however just per-vendor adjustments; there are global
adjustments (like clobbering the reserved leaves).

I am open to any naming suggestions.

>
>> @@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>          return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
>>  
>>      case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
>> -        if ( leaf > p->extd.max_leaf )
>> +        ASSERT((p->extd.max_leaf & 0xffff) < ARRAY_SIZE(p->extd.raw));
>> +        if ( (leaf & 0xffff) > min_t(uint32_t, p->extd.max_leaf & 0xffff,
>> +                                     ARRAY_SIZE(p->extd.raw) - 1) )
>>              return;
>> -        goto legacy;
>> +
>> +        switch ( leaf )
>> +        {
>> +        default:
>> +            goto legacy;
>> +
>> +        case 0x80000000:
>> +            *res = p->extd.raw[leaf & 0xffff];
> I take it that the plan is to have further leaves and up here, or else
> the array index could simply be literal zero.

I believe you found your answer in the following patch?  Eventually
(when the legacy path disappears), this entire switch statement will go.

~Andrew

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