Re: [RFC PATCH 03/10] Add cpuid_vmware_leaves

[Don Slutz <Don@CloudSwitch.Com>]

On 12/12/13 17:27, Andrew Cooper wrote:
> On 12/12/2013 19:15, Don Slutz wrote:
>> From: Don Slutz<dslutz@verizon.com>
>>
>> Signed-off-by: Don Slutz<dslutz@verizon.com>
>> ---
>>   xen/arch/x86/hvm/hvm.c          |  3 +++
>>   xen/arch/x86/traps.c            | 58 ++++++++++++++++++++++++++++++++++++++++-
>>   xen/include/asm-x86/hvm/hvm.h   |  3 +++
>>   xen/include/asm-x86/processor.h |  2 ++
>>   4 files changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 69f7e74..6a7a781 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2878,6 +2878,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>>       if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
>>           return;
>>   
>> +    if ( cpuid_vmware_leaves(input, count, eax, ebx, ecx, edx) )
>> +        return;
>> +
>>       if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) )
>>           return;
>>   
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 940bc33..71a76df 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -671,14 +671,70 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>>       return 0;
>>   }
>>   
>> +int cpuid_vmware_leaves(uint32_t idx, uint32_t sub_idx,
>> +                        uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>> +{
>> +    struct domain *d = current->domain;
>> +    /* Optionally shift out of the way of Viridian architectural leaves. */
>> +    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> +    uint32_t limit;
>> +    const uint32_t apic_khz = 1000000L;
> Please use the proper constant for this.

Is this SYSTEM_TIME_HZ / 1000 ?  Or is not a constant.  All my old testing on amazon showed it to be a constant (but that is an much older xen; most likely did not use virtual apic hardware, but used qemu).  I just checked and the hardware I am testing on has:

(XEN) calibrating APIC timer ...
(XEN) ..... CPU clock speed is 2400.0122 MHz.
(XEN) ..... host bus clock speed is 100.0003 MHz.
(XEN) ..... bus_scale = 0x6669

So it may be safer to pass on the detected host bus clock speed.

>> +
>> +    if ( !is_vmware_domain(d) )
>> +        return 0;
>> +
>> +    idx -= base;
>> +
>> +    limit = 0x10;
>> +
>> +    if ( idx > limit )
>> +        return 0;
> This `limit` is pointless and the BUG() below is dead code (which
> Coverity will complain about).
>
> Please see 839b966e3f587bbb1a0d954230fb3904330dccb6 and follow its
> style, rather than propagating this bad style (which admittedly you have
> gotten from following cpuid_hypervisor_leaves() )
Will do.
>> +
>> +    switch ( idx )
>> +    {
>> +    case 0:
>> +        *eax = base + limit; /* Largest leaf */
>> +        *ebx = 0x61774d56; /* "VMwa" */
>> +        *ecx = 0x4d566572; /* "reVM" */
>> +        *edx = 0x65726177; /* "ware" */
>> +        break;
>> +
>> +    case 1 ... 0xf:
>> +        *eax = 0;          /* Reserved */
>> +        *ebx = 0;          /* Reserved */
>> +        *ecx = 0;          /* Reserved */
>> +        *edx = 0;          /* Reserved */
>> +        break;
>> +
>> +    case 0x10:
>> +        /* (Virtual) TSC frequency in kHz. */
>> +        *eax =  d->arch.tsc_khz;
>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +        *ebx = apic_khz;
>> +        *ecx = 0;          /* Reserved */
>> +        *edx = 0;          /* Reserved */
>> +        break;
>> +
>> +    default:
>> +        BUG();
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>>   int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>>   {
>>       struct domain *d = current->domain;
>>       /* Optionally shift out of the way of Viridian architectural leaves. */
>> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> +    uint32_t base = 0x40000000;
>>       uint32_t limit;
>>   
>> +    if ( is_viridian_domain(d) )
>> +        base += 0x100;
>> +    if ( is_vmware_domain(d) )
>> +        base += 0x100;
>> +
> These bases need a far more scalable solution, especially as the result
> of each of these clauses can be changed at runtime with a cunning
> hvm_param_set hypercall.
Will work on a redesign for these bases.
    -Don Slutz
> I think that both "is pretending to be HyperV" and "is pretending to be
> VMware" need to be domain creation flags which are strictly static for
> the lifetime of the domain.
>
> ~Andrew
>
>>       idx -= base;
>>   
>>       /*
>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
>> index ccca5df..ae3768c 100644
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -332,6 +332,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>>   #define is_viridian_domain(_d)                                             \
>>    (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
>>   
>> +#define is_vmware_domain(_d)                                             \
>> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
>> +
>>   void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>>                                      unsigned int *ecx, unsigned int *edx);
>>   void hvm_migrate_timers(struct vcpu *v);
>> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
>> index c120460..6c53e45 100644
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -559,6 +559,8 @@ void int80_direct_trap(void);
>>   
>>   extern int hypercall(void);
>>   
>> +int cpuid_vmware_leaves( uint32_t idx, uint32_t sub_idx,
>> +          uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
>>   int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>             uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
>>   int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val);