Re: [Xen-devel] [PATCH] x86/HVM: correct CPUID leaf 80000008 handling

[Tim Deegan <tim@xxxxxxx>]

At 11:48 +0000 on 26 Mar (1395830898), Jan Beulich wrote:
> CPUID[80000001].EAX[23:16] have been given the meaning of the guest

s/80000001/80000008/?

> physical address restriction (in case it needs to be smaller than the
> host's), hence we need to mirror that into vCPUID[80000008].EAX[7:0].
> 
> Enforce a lower limit at the same time, as well as a fixed value for
> the virtual address bits, and zero for the guest physical address ones.
> 
> In order for the vMTRR code to see these overrides we need to make it
> call hvm_cpuid() instead of domain_cpuid(), which in turn requires
> special casing (and relaxing) the controlling domain.
> 
> This additionally should hide an ordering problem in the tools: Both
> xend and xl appear to be restoring a guest from its image before
> setting up the CPUID policy in the hypervisor, resulting in
> domain_cpuid() returning all zeros and hence the check in
> mtrr_var_range_msr_set() failing if the guest previously had more than
> the minimum 36 physical address bits.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3012,6 +3012,8 @@ void hvm_cpuid(unsigned int input, unsig
>  
>      switch ( input )
>      {
> +        unsigned int sub_leaf, _eax, _ebx, _ecx, _edx;
> +
>      case 0x1:
>          /* Fix up VLAPIC details. */
>          *ebx &= 0x00FFFFFFu;
> @@ -3051,8 +3053,6 @@ void hvm_cpuid(unsigned int input, unsig
>          *edx = v->vcpu_id * 2;
>          break;
>      case 0xd:
> -    {
> -        unsigned int sub_leaf, _eax, _ebx, _ecx, _edx;
>          /* EBX value of main leaf 0 depends on enabled xsave features */
>          if ( count == 0 && v->arch.xcr0 ) 
>          {
> @@ -3069,7 +3069,7 @@ void hvm_cpuid(unsigned int input, unsig
>              }
>          }
>          break;
> -    }
> +
>      case 0x80000001:
>          /* We expose RDTSCP feature to guest only when
>             tsc_mode == TSC_MODE_DEFAULT and host_tsc_is_safe() returns 1 */
> @@ -3083,6 +3083,23 @@ void hvm_cpuid(unsigned int input, unsig
>          if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
>              *edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
>          break;
> +
> +    case 0x80000008:
> +        count = cpuid_eax(0x80000008);
> +        count = (count >> 16) & 0xff ?: count & 0xff;
> +        if ( (*eax & 0xff) > count )
> +            *eax = (*eax & ~0xff) | count;
> +
> +        hvm_cpuid(1, NULL, NULL, NULL, &_edx);
> +        count = _edx & (cpufeat_mask(X86_FEATURE_PAE) |
> +                        cpufeat_mask(X86_FEATURE_PSE36)) ? 36 : 32;
> +        if ( (*eax & 0xff) < count )
> +            *eax = (*eax & ~0xff) | count;

What is this for?  Surely if the CPU really claims to have a paddr
limit lower than 32 then (a) it's buggy and (b) we can't just pretend
it doesn't say that.  And architecturally, the presence of PSE/PAE
doesn't guarantee that the CPU will support a given paddr width.
(My reading of the AMD docs says you get up-to-40/up-to-52 but gives
no lower bound).

> +        hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
> +        *eax = (*eax & ~0xffff00) | (_edx & cpufeat_mask(X86_FEATURE_LM)
> +                                     ? 0x3000 : 0x2000);

But we don't support 64-bit linear addresses in long mode -- shouldn't
we be capping this at what the hardware actually allows?

Tim.

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