Re: [Xen-devel] [PATCH 09/27] x86/cpuid: Dispatch cpuid_hypervisor_leaves() from guest_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/01/17 15:34, Jan Beulich wrote:
>>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -332,6 +332,9 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf,
>>      case 0x40000000 ... 0x400000ff:
>>          if ( is_viridian_domain(d) )
>>              return cpuid_viridian_leaves(v, leaf, subleaf, res);
>> +        /* Fallthrough. */
>> +    case 0x40000100 ... 0x4fffffff:
>> +        return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
>>      }
> Ah - that's why you didn't have a break statement there. But: Is this
> correct? You're now returning Xen leaves in two windows for non-
> Viridian domains.

Oh - good point.  I think for now I will retain the viridian_domain()
check in cpuid_hypervisor_leaves()

The awkard issue is that the toolstack can provide the xen max leaf
field.  I was considering switching the interface around to having the
toolstack choose all of leaf 0 for the virtualualised leaves, and I am
looking longterm to have unions for the these leaves in struct cpuid_policy.

>
>> @@ -929,83 +927,71 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t 
>> sub_idx,
>>              limit = XEN_CPUID_MAX_NUM_LEAVES;
>>      }
>>  
>> -    if ( idx > limit ) 
>> -        return 0;
>> +    if ( idx > limit )
>> +        return;
>>  
>>      switch ( idx )
>>      {
>>      case 0:
>> -        *eax = base + limit; /* Largest leaf */
>> -        *ebx = XEN_CPUID_SIGNATURE_EBX;
>> -        *ecx = XEN_CPUID_SIGNATURE_ECX;
>> -        *edx = XEN_CPUID_SIGNATURE_EDX;
>> +        res->a = base + limit; /* Largest leaf */
>> +        res->b = XEN_CPUID_SIGNATURE_EBX;
>> +        res->c = XEN_CPUID_SIGNATURE_ECX;
>> +        res->d = XEN_CPUID_SIGNATURE_EDX;
>>          break;
>>  
>>      case 1:
>> -        *eax = (xen_major_version() << 16) | xen_minor_version();
>> -        *ebx = 0;          /* Reserved */
>> -        *ecx = 0;          /* Reserved */
>> -        *edx = 0;          /* Reserved */
>> +        res->a = (xen_major_version() << 16) | xen_minor_version();
>>          break;
>>  
>>      case 2:
>> -        *eax = 1;          /* Number of hypercall-transfer pages */
>> -        *ebx = 0x40000000; /* MSR base address */
>> -        if ( is_viridian_domain(currd) )
>> -            *ebx = 0x40000200;
>> -        *ecx = 0;          /* Features 1 */
>> -        *edx = 0;          /* Features 2 */
>> -        if ( is_pv_domain(currd) )
>> -            *ecx |= XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD;
>> +        res->a = 1;          /* Number of hypercall-transfer pages */
>> +        res->b = 0x40000000; /* MSR base address */
>> +        if ( is_viridian_domain(d) )
>> +            res->b = 0x40000200;
> Could I talk you into making this a conditional expression, as you're
> touching it anyway?

Ok.  I did find the value of 0x40000200 particularly odd (given that we
split the CPUID leaves on the 100 boundary), but it has been like that
for ages.

>
>> +        if ( is_pv_domain(d) )
>> +            res->c |= XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD;
>>          break;
>>  
>>      case 3: /* Time leaf. */
>> -        switch ( sub_idx )
>> +        switch ( subleaf )
>>          {
>>          case 0: /* features */
>> -            *eax = ((!!currd->arch.vtsc << 0) |
>> -                    (!!host_tsc_is_safe() << 1) |
>> -                    (!!boot_cpu_has(X86_FEATURE_RDTSCP) << 2));
>> -            *ebx = currd->arch.tsc_mode;
>> -            *ecx = currd->arch.tsc_khz;
>> -            *edx = currd->arch.incarnation;
>> +            res->a = ((!!d->arch.vtsc << 0) |
>> +                      (!!host_tsc_is_safe() << 1) |
>> +                      (!!boot_cpu_has(X86_FEATURE_RDTSCP) << 2));
> The latter two !! appear to still be necessary, but the first can go
> away now that we use bool, and bool_t is an alias thereof.

Ok.

>
>> +            res->b = d->arch.tsc_mode;
>> +            res->c = d->arch.tsc_khz;
>> +            res->d = d->arch.incarnation;
>>              break;
>>  
>>          case 1: /* scale and offset */
>>          {
>>              uint64_t offset;
>>  
>> -            if ( !currd->arch.vtsc )
>> -                offset = currd->arch.vtsc_offset;
>> +            if ( !d->arch.vtsc )
>> +                offset = d->arch.vtsc_offset;
>>              else
>>                  /* offset already applied to value returned by virtual 
>> rdtscp */
>>                  offset = 0;
>> -            *eax = (uint32_t)offset;
>> -            *ebx = (uint32_t)(offset >> 32);
>> -            *ecx = currd->arch.vtsc_to_ns.mul_frac;
>> -            *edx = (s8)currd->arch.vtsc_to_ns.shift;
>> +            res->a = (uint32_t)offset;
>> +            res->b = (uint32_t)(offset >> 32);
> The casts aren't really necessary.

Will drop.

~Andrew

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