[PATCH] x86/HVM: correct CPUID leaf 80000008 handling

[PATCH] x86/HVM: correct CPUID leaf 80000008 handling

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4824 bytes --]

CPUID[80000001].EAX[23:16] have been given the meaning of the guest
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@suse.com>

--- 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;
+
+        hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
+        *eax = (*eax & ~0xffff00) | (_edx & cpufeat_mask(X86_FEATURE_LM)
+                                     ? 0x3000 : 0x2000);
+        break;
     }
 }
 
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -145,7 +145,7 @@ bool_t is_var_mtrr_overlapped(struct mtr
 
 static int __init hvm_mtrr_pat_init(void)
 {
-    unsigned int i, j, phys_addr;
+    unsigned int i, j;
 
     for ( i = 0; i < MTRR_NUM_TYPES; i++ )
     {
@@ -170,11 +170,7 @@ static int __init hvm_mtrr_pat_init(void
         }
     }
 
-    phys_addr = 36;
-    if ( cpuid_eax(0x80000000) >= 0x80000008 )
-        phys_addr = (uint8_t)cpuid_eax(0x80000008);
-
-    size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
+    size_or_mask = ~((1 << (paddr_bits - PAGE_SHIFT)) - 1);
 
     return 0;
 }
@@ -463,7 +459,7 @@ bool_t mtrr_fix_range_msr_set(struct mtr
 bool_t mtrr_var_range_msr_set(
     struct domain *d, struct mtrr_state *m, uint32_t msr, uint64_t msr_content)
 {
-    uint32_t index, phys_addr, eax, ebx, ecx, edx;
+    uint32_t index, phys_addr, eax;
     uint64_t msr_mask;
     uint64_t *var_range_base = (uint64_t*)m->var_ranges;
 
@@ -474,16 +470,21 @@ bool_t mtrr_var_range_msr_set(
     if ( unlikely(!valid_mtrr_type((uint8_t)msr_content)) )
         return 0;
 
-    phys_addr = 36;
-    domain_cpuid(d, 0x80000000, 0, &eax, &ebx, &ecx, &edx);
-    if ( eax >= 0x80000008 )
+    if ( d == current->domain )
     {
-        domain_cpuid(d, 0x80000008, 0, &eax, &ebx, &ecx, &edx);
-        phys_addr = (uint8_t)eax;
+        phys_addr = 36;
+        hvm_cpuid(0x80000000, &eax, NULL, NULL, NULL);
+        if ( eax >= 0x80000008 )
+        {
+            hvm_cpuid(0x80000008, &eax, NULL, NULL, NULL);
+            phys_addr = (uint8_t)eax;
+        }
     }
+    else
+        phys_addr = paddr_bits;
     msr_mask = ~((((uint64_t)1) << phys_addr) - 1);
     msr_mask |= (index & 1) ? 0x7ffUL : 0xf00UL;
-    if ( unlikely(msr_content && (msr_content & msr_mask)) )
+    if ( unlikely(msr_content & msr_mask) )
     {
         HVM_DBG_LOG(DBG_LEVEL_MSR, "invalid msr content:%"PRIx64"\n",
                     msr_content);



[-- Attachment #2: x86-guest-paddr-limit.patch --]
[-- Type: text/plain, Size: 4869 bytes --]

x86/HVM: correct CPUID leaf 80000008 handling

CPUID[80000001].EAX[23:16] have been given the meaning of the guest
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@suse.com>

--- 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;
+
+        hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
+        *eax = (*eax & ~0xffff00) | (_edx & cpufeat_mask(X86_FEATURE_LM)
+                                     ? 0x3000 : 0x2000);
+        break;
     }
 }
 
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -145,7 +145,7 @@ bool_t is_var_mtrr_overlapped(struct mtr
 
 static int __init hvm_mtrr_pat_init(void)
 {
-    unsigned int i, j, phys_addr;
+    unsigned int i, j;
 
     for ( i = 0; i < MTRR_NUM_TYPES; i++ )
     {
@@ -170,11 +170,7 @@ static int __init hvm_mtrr_pat_init(void
         }
     }
 
-    phys_addr = 36;
-    if ( cpuid_eax(0x80000000) >= 0x80000008 )
-        phys_addr = (uint8_t)cpuid_eax(0x80000008);
-
-    size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
+    size_or_mask = ~((1 << (paddr_bits - PAGE_SHIFT)) - 1);
 
     return 0;
 }
@@ -463,7 +459,7 @@ bool_t mtrr_fix_range_msr_set(struct mtr
 bool_t mtrr_var_range_msr_set(
     struct domain *d, struct mtrr_state *m, uint32_t msr, uint64_t msr_content)
 {
-    uint32_t index, phys_addr, eax, ebx, ecx, edx;
+    uint32_t index, phys_addr, eax;
     uint64_t msr_mask;
     uint64_t *var_range_base = (uint64_t*)m->var_ranges;
 
@@ -474,16 +470,21 @@ bool_t mtrr_var_range_msr_set(
     if ( unlikely(!valid_mtrr_type((uint8_t)msr_content)) )
         return 0;
 
-    phys_addr = 36;
-    domain_cpuid(d, 0x80000000, 0, &eax, &ebx, &ecx, &edx);
-    if ( eax >= 0x80000008 )
+    if ( d == current->domain )
     {
-        domain_cpuid(d, 0x80000008, 0, &eax, &ebx, &ecx, &edx);
-        phys_addr = (uint8_t)eax;
+        phys_addr = 36;
+        hvm_cpuid(0x80000000, &eax, NULL, NULL, NULL);
+        if ( eax >= 0x80000008 )
+        {
+            hvm_cpuid(0x80000008, &eax, NULL, NULL, NULL);
+            phys_addr = (uint8_t)eax;
+        }
     }
+    else
+        phys_addr = paddr_bits;
     msr_mask = ~((((uint64_t)1) << phys_addr) - 1);
     msr_mask |= (index & 1) ? 0x7ffUL : 0xf00UL;
-    if ( unlikely(msr_content && (msr_content & msr_mask)) )
+    if ( unlikely(msr_content & msr_mask) )
     {
         HVM_DBG_LOG(DBG_LEVEL_MSR, "invalid msr content:%"PRIx64"\n",
                     msr_content);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Tim Deegan]

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@suse.com>
> 
> --- 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.

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

[Jan Beulich]

>>> On 27.03.14 at 12:44, <tim@xen.org> wrote:
> 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/?

Oops, of course.

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

But what we modify isn't what came from hardware, but what was
presented (possibly overridden) by the tool stack.

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

Clearly the lower bounds are implied, and PSE/PAE in fact do
guarantee 36 bits to be supported (and 32 bits have always been
afaict, at least since the introduction of CPUID).

In any event - what alternatives would you suggest?

Jan

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

[Andrew Cooper]

On 27/03/14 15:10, Jan Beulich wrote:
>>>> On 27.03.14 at 12:44, <tim@xen.org> wrote:
>> 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/?
> Oops, of course.
>
>>> @@ -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.
> But what we modify isn't what came from hardware, but what was
> presented (possibly overridden) by the tool stack.

This is just one of the many things in the mess which is the current
cpuid policy/levelling code where libxc and disagree in an unsafe way.

In this particular case, it is not valid for libxc to be able to set any
policy which would make cpuid.0x80000008.eax able to report a physical
width greater than currently supported on the cpu, or less than is
architecturally supported for long mode (which I believe is indeed 36
bits, but cant offhand remember where I read this).

My proposal for this was for there to be very strict sanity checking
when trying to upload the policy leaves, which removes large quantities
of Xen's second-guessing in locations such as this.

~Andrew

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

[Tim Deegan]

At 15:10 +0000 on 27 Mar (1395929407), Jan Beulich wrote:
> >> @@ -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.
> 
> But what we modify isn't what came from hardware, but what was
> presented (possibly overridden) by the tool stack.

Oh right.  Yes, in that case these seem fine.   And until Andrew's
series to do such checking at upload time, doing it here is consistnet
with how other leaves are handled. 

My comment about advertising 64-bit linear addresses is also clearly
incorrect - I was misreading 0x30 as 64 for some reason.

So, Reviewed-by: Tim Deegan <tim@xen.org>

Cheers,

Tim.