Re: [RFC PATCH 03/10] Add cpuid_vmware_leaves

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

> +
> +    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() )

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

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);