diff mbox

[v2,16/25] x86/pv: Use per-domain policy information in pv_cpuid()

Message ID 1483959822-30484-17-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Jan. 9, 2017, 11:03 a.m. UTC 
... rather than performing runtime adjustments.  This is safe now that
recalculate_cpuid_policy() perfoms suitable sanitisation when the policy data
is loaded.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/traps.c | 44 ++++++++++++--------------------------------
 1 file changed, 12 insertions(+), 32 deletions(-)

Comments

Boris Ostrovsky Jan. 12, 2017, 3:22 p.m. UTC | #1 
>      case 0x80000001:
> -        c &= pv_featureset[FEATURESET_e1c];
> -        d &= pv_featureset[FEATURESET_e1d];
> +        c = p->extd.e1c;

This appears to crash guests Intel, at least for dom0.

p->extd.e1c is 0x3 and bit 1 is reserved on Intel.

I haven't traced it yet to exact place that causes dom0 to crash but
clearing this bit make dom0 boot.


-boris
Andrew Cooper Jan. 12, 2017, 3:31 p.m. UTC | #2 
On 12/01/17 15:22, Boris Ostrovsky wrote:
>>      case 0x80000001:
>> -        c &= pv_featureset[FEATURESET_e1c];
>> -        d &= pv_featureset[FEATURESET_e1d];
>> +        c = p->extd.e1c;
> This appears to crash guests Intel, at least for dom0.

Is this a PVH dom0?  I can't see from this snippet which function you
are in.

>
> p->extd.e1c is 0x3 and bit 1 is reserved on Intel.

>
> I haven't traced it yet to exact place that causes dom0 to crash but
> clearing this bit make dom0 boot.

The logic immediately below the snippet should clean out the common bits
if vendor != AMD.  Do we perhaps have a bad vendor setting?

~Andrew
Boris Ostrovsky Jan. 12, 2017, 3:50 p.m. UTC | #3 
On 01/12/2017 10:31 AM, Andrew Cooper wrote:
> On 12/01/17 15:22, Boris Ostrovsky wrote:
>>>      case 0x80000001:
>>> -        c &= pv_featureset[FEATURESET_e1c];
>>> -        d &= pv_featureset[FEATURESET_e1d];
>>> +        c = p->extd.e1c;
>> This appears to crash guests Intel, at least for dom0.
> Is this a PVH dom0?  I can't see from this snippet which function you
> are in.

No, this is normal PV dom0.

I may have gone too far trimming the patch. It's this chunk:


@@ -1291,15 +1281,15 @@ void pv_cpuid(struct cpu_user_regs *regs)
         }
 
         case 1:
-            a &= pv_featureset[FEATURESET_Da1];
+            a = p->xstate.Da1;
             b = c = d = 0;
             break;
         }
         break;
 
     case 0x80000001:
-        c &= pv_featureset[FEATURESET_e1c];
-        d &= pv_featureset[FEATURESET_e1d];
+        c = p->extd.e1c;
+        d = p->extd.e1d;


>
>> p->extd.e1c is 0x3 and bit 1 is reserved on Intel.
>> I haven't traced it yet to exact place that causes dom0 to crash but
>> clearing this bit make dom0 boot.
> The logic immediately below the snippet should clean out the common bits
> if vendor != AMD.  Do we perhaps have a bad vendor setting?
>

-bash-4.1# ./cpuid 0
CPUID 0x00000000: eax = 0x0000000d ebx = 0x756e6547 ecx = 0x6c65746e edx
= 0x49656e69
-bash-4.1#

This is machine that I run my nightly tests on and it failed this
morning so it's not a new HW.

As far as adjusting the bits based on vendor --- don't you only do this
for edx:

arch/x86/cpuid.c: pv_cpuid():

   case 0x80000001:
        res->c = p->extd.e1c;
        res->c &= ~2U; // My workaround
        res->d = p->extd.e1d;

        /* If not emulating AMD, clear the duplicated features in e1d. */
        if ( currd->arch.x86_vendor != X86_VENDOR_AMD )
            res->d &= ~CPUID_COMMON_1D_FEATURES;


-boris
diff mbox

Patch

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 47c7ce7..360b10d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1064,11 +1064,8 @@  void pv_cpuid(struct cpu_user_regs *regs)
         uint32_t tmp;
 
     case 0x00000001:
-        c &= pv_featureset[FEATURESET_1c];
-        d &= pv_featureset[FEATURESET_1d];
-
-        if ( is_pv_32bit_domain(currd) )
-            c &= ~cpufeat_mask(X86_FEATURE_CX16);
+        c = p->basic._1c;
+        d = p->basic._1d;
 
         if ( !is_pvh_domain(currd) )
         {
@@ -1127,7 +1124,7 @@  void pv_cpuid(struct cpu_user_regs *regs)
              *    Emulated vs Faulted CPUID is distinguised based on whether a
              *    #UD or #GP is currently being serviced.
              */
-            /* OSXSAVE cleared by pv_featureset.  Fast-forward CR4 back in. */
+            /* OSXSAVE clear in policy.  Fast-forward CR4 back in. */
             if ( (curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_OSXSAVE) ||
                  (regs->entry_vector == TRAP_invalid_op &&
                   guest_kernel_mode(curr, regs) &&
@@ -1203,21 +1200,14 @@  void pv_cpuid(struct cpu_user_regs *regs)
             if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
                 c |= cpufeat_mask(X86_FEATURE_DSCPL);
         }
-
-        c |= cpufeat_mask(X86_FEATURE_HYPERVISOR);
         break;
 
     case 0x00000007:
         if ( subleaf == 0 )
         {
-            /* Fold host's FDP_EXCP_ONLY and NO_FPU_SEL into guest's view. */
-            b &= (pv_featureset[FEATURESET_7b0] &
-                  ~special_features[FEATURESET_7b0]);
-            b |= (host_featureset[FEATURESET_7b0] &
-                  special_features[FEATURESET_7b0]);
-
-            c &= pv_featureset[FEATURESET_7c0];
-            d &= pv_featureset[FEATURESET_7d0];
+            b = currd->arch.cpuid->feat._7b0;
+            c = currd->arch.cpuid->feat._7c0;
+            d = currd->arch.cpuid->feat._7d0;
 
             if ( !is_pvh_domain(currd) )
             {
@@ -1226,7 +1216,7 @@  void pv_cpuid(struct cpu_user_regs *regs)
                  * and HVM guests no longer enter a PV codepath.
                  */
 
-                /* OSPKE cleared by pv_featureset.  Fast-forward CR4 back in. */
+                /* OSPKE clear in policy.  Fast-forward CR4 back in. */
                 if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_PKE )
                     c |= cpufeat_mask(X86_FEATURE_OSPKE);
             }
@@ -1291,15 +1281,15 @@  void pv_cpuid(struct cpu_user_regs *regs)
         }
 
         case 1:
-            a &= pv_featureset[FEATURESET_Da1];
+            a = p->xstate.Da1;
             b = c = d = 0;
             break;
         }
         break;
 
     case 0x80000001:
-        c &= pv_featureset[FEATURESET_e1c];
-        d &= pv_featureset[FEATURESET_e1d];
+        c = p->extd.e1c;
+        d = p->extd.e1d;
 
         /* If not emulating AMD, clear the duplicated features in e1d. */
         if ( currd->arch.x86_vendor != X86_VENDOR_AMD )
@@ -1317,25 +1307,15 @@  void pv_cpuid(struct cpu_user_regs *regs)
         if ( is_hardware_domain(currd) && guest_kernel_mode(curr, regs) &&
              cpu_has_mtrr )
             d |= cpufeat_mask(X86_FEATURE_MTRR);
-
-        if ( is_pv_32bit_domain(currd) )
-        {
-            d &= ~cpufeat_mask(X86_FEATURE_LM);
-            c &= ~cpufeat_mask(X86_FEATURE_LAHF_LM);
-
-            if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD )
-                d &= ~cpufeat_mask(X86_FEATURE_SYSCALL);
-        }
         break;
 
     case 0x80000007:
-        d &= (pv_featureset[FEATURESET_e7d] |
-              (host_featureset[FEATURESET_e7d] & cpufeat_mask(X86_FEATURE_ITSC)));
+        d = p->extd.e7d;
         break;
 
     case 0x80000008:
         a = paddr_bits | (vaddr_bits << 8);
-        b &= pv_featureset[FEATURESET_e8b];
+        b = p->extd.e8b;
         break;
 
     case 0x00000005: /* MONITOR/MWAIT */