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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> @@ -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?

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

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

Jan

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