[Xen-devel] [PATCH RFC] x86/amd: Avoid cpu_has_hypervisor evaluating true on native hardware

[Andrew Cooper <andrew.cooper3@citrix.com>]

Currently when booting native on AMD hardware, cpuidmask_defaults._1cd gets
configured with the HYPERVISOR bit before native CPUID is scanned for feature
bits.

This results in cpu_has_hypervisor becoming set as part of identify_cpu(), and
ends up appearing in the raw and host CPU policies.  Nothing has really cared
in the past.

Alter amd_init_levelling() to exclude the HYPERVISOR bit from
cpumask_defaults, and update domain_cpu_policy_changed() to allow it to be
explicitly forwarded.

This in turn highlighted that dom0 construction was asymetric with domU
construction, by not having any calls to domain_cpu_policy_changed().  Extend
arch_domain_create() to always call domain_cpu_policy_changed().

Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Igor Druzhinin <igor.druzhinin@citrix.com>

Without this fix, there is apparently a problem with Roger's "[PATCH v3 7/7]
x86/tlb: use Xen L0 assisted TLB flush when available" on native AMD hardware.
I haven't investgiated the issue with that patch specifically, because
cpu_has_hypervisor being wrong is obviously a bug.

This is one of two possible approaches, and both have their downsides.  This
one takes an extra hit on context switches between PV vcpus and idle/hvm, as
they will usually differ in HYPERVISOR bit.

The other approach is to order things more carefully so levelling is
configured after scanning for cpuid bits, but that has the downside that it is
very easy to regress.

Thoughts on which is the least-bad approach to take?  Having written this
patch, I'm now erring on the side of doing it the other way.
---
 xen/arch/x86/cpu/amd.c       | 3 ---
 xen/arch/x86/domain.c        | 2 ++
 xen/arch/x86/domctl.c        | 9 ++++++++-
 xen/include/asm-x86/domain.h | 2 ++
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 8b5f0f2e4c..0906b23582 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -297,9 +297,6 @@ static void __init noinline amd_init_levelling(void)
 			ecx |= cpufeat_mask(X86_FEATURE_OSXSAVE);
 		edx |= cpufeat_mask(X86_FEATURE_APIC);
 
-		/* Allow the HYPERVISOR bit to be set via guest policy. */
-		ecx |= cpufeat_mask(X86_FEATURE_HYPERVISOR);
-
 		cpuidmask_defaults._1cd = ((uint64_t)ecx << 32) | edx;
 	}
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 28fefa1f81..316b801597 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -665,6 +665,8 @@ int arch_domain_create(struct domain *d,
      */
     d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
 
+    domain_cpu_policy_changed(d);
+
     return 0;
 
  fail:
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 5ed63ac10a..0627eb4e06 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -48,7 +48,7 @@ static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
 }
 #endif
 
-static void domain_cpu_policy_changed(struct domain *d)
+void domain_cpu_policy_changed(struct domain *d)
 {
     const struct cpuid_policy *p = d->arch.cpuid;
     struct vcpu *v;
@@ -106,6 +106,13 @@ static void domain_cpu_policy_changed(struct domain *d)
                     ecx = 0;
                 edx = cpufeat_mask(X86_FEATURE_APIC);
 
+                /*
+                 * If the Hypervisor bit is set in the policy, we can also
+                 * forward it into real CPUID.
+                 */
+                if ( p->basic.hypervisor )
+                    ecx |= cpufeat_mask(X86_FEATURE_HYPERVISOR);
+
                 mask |= ((uint64_t)ecx << 32) | edx;
                 break;
             }
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index a3ae5d9a20..817065bf81 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -624,6 +624,8 @@ struct guest_memory_policy
 void update_guest_memory_policy(struct vcpu *v,
                                 struct guest_memory_policy *policy);
 
+void domain_cpu_policy_changed(struct domain *d);
+
 bool update_runstate_area(struct vcpu *);
 bool update_secondary_system_time(struct vcpu *,
                                   struct vcpu_time_info *);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel