Re: [Xen-devel] [PATCH for-4.5 v7 1/7] xen: Add support for VMware cpuid leaves

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.10.14 at 23:30, <dslutz@xxxxxxxxxxx> wrote:
> @@ -5536,6 +5540,11 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>                  if ( curr_d == d )
>                      break;
>  
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] )
> +                {
> +                    rc = -EXDEV;

That's a pretty strange error code here. -EOPNOTSUPP perhaps?

> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/cpuid.c

Whether adding another subdirectory here is really the way to go
heavily depends on how much of this new code we really want to
take into the tree. There sheer size of the series makes me
hesitant to consider taking it all.

> +/*
> + * VMware hardware version 7 defines some of these cpuid levels,
> + * below is a brief description about those.
> + *
> + *     Leaf 0x40000000, Hypervisor CPUID information
> + * # EAX: The maximum input value for hypervisor CPUID info (0x40000010).
> + * # EBX, ECX, EDX: Hypervisor vendor ID signature. E.g. "VMwareVMware"
> + *
> + *     Leaf 0x40000010, Timing information.
> + * # EAX: (Virtual) TSC frequency in kHz.
> + * # EBX: (Virtual) Bus (local apic timer) frequency in kHz.
> + * # ECX, EDX: RESERVED
> + */
> +
> +int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
> +                        uint32_t *ecx, uint32_t *edx)
> +{
> +    struct domain *d = current->domain;
> +
> +    if ( !is_vmware_domain(d) )
> +        return 0;
> +
> +    switch ( idx - 0x40000000 )
> +    {
> +    case 0x0:
> +        if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] >= 7 )
> +        {
> +            *eax = 0x40000010;  /* Largest leaf */
> +            *ebx = 0x61774d56;  /* "VMwa" */
> +            *ecx = 0x4d566572;  /* "reVM" */
> +            *edx = 0x65726177;  /* "ware" */
> +            break;
> +        }
> +        /* fallthrough */
> +    case 0x10:
> +        if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] >= 7 )
> +        {
> +            /* (Virtual) TSC frequency in kHz. */
> +            *eax =  d->arch.tsc_khz;
> +            /* (Virtual) Bus (local apic timer) frequency in kHz. */
> +            *ebx = 1000000ull / APIC_BUS_CYCLE_NS;
> +            *ecx = 0;          /* Reserved */
> +            *edx = 0;          /* Reserved */
> +            break;
> +        }
> +        /* fallthrough */

So for versions < 7 there's effectively no CPUID support at all?
Wouldn't it then make more sense to check for the version together
with the is_vmware_domain() check at the top?

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -346,6 +346,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct 
> vcpu *v)
>  #define is_viridian_domain(d) \
>      (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq))
>  
> +#define is_vmware_domain(_d)                                             \
> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))

Indentation. Also please use d, not _d. I.e. take the macro above
as reference.

> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -189,6 +189,9 @@
>  /* Location of the VM Generation ID in guest physical address space. */
>  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>  
> -#define HVM_NR_PARAMS          35
> +/* Params for VMware */
> +#define HVM_PARAM_VMWARE_HW                 35

The comment seems wrong - after all it's the version, not some
arbitrary parameters/flags.

Jan


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