[Xen-devel] [PATCH v2 8/8] x86/cpuid: Enable CPUID Faulting for the control domain by default

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

The domain builder no longer uses local CPUID instructions for policy
decisions.  This resolves a key issue for PVH dom0's.  However, as PV dom0's
have never had faulting enforced, leave a command line option to restore the
old behaviour.

In ctxt_switch_levelling(), invert the first cpu_has_cpuid_faulting condition
to reduce the indentation for the CPUID faulting logic.

Advertise virtualised faulting support to control domains unless the opt-out
has been used.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wl@xxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>

opt_dom0_cpuid_faulting would ideally live in dom0_build.c next to
opt_dom0_verbose, but the file is currently .init

v2:
 * Introduce a command line option to retain old behaviour.
 * Advertise virtualised faulting support to dom0 when it is used.

RFC: The previous logic was slightly buggy in that even PVH dom0's had
virtualised faulting support hidden from them.  Given that this case always
hits the CPUID intercept, how much do we care about retaining the old
behaviour?
---
 xen/arch/x86/cpu/common.c   | 59 +++++++++++++++++++++++----------------------
 xen/arch/x86/dom0_build.c   |  2 ++
 xen/arch/x86/msr.c          |  3 ++-
 xen/include/asm-x86/setup.h |  1 +
 4 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 937d8e82a8..b8d6c4967e 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -10,12 +10,15 @@
 #include <asm/io.h>
 #include <asm/mpspec.h>
 #include <asm/apic.h>
+#include <asm/setup.h>
 #include <mach_apic.h>
 #include <public/sysctl.h> /* for XEN_INVALID_{SOCKET,CORE}_ID */
 
 #include "cpu.h"
 #include "mcheck/x86_mca.h"
 
+bool __read_mostly opt_dom0_cpuid_faulting = true;
+
 bool_t opt_arat = 1;
 boolean_param("arat", opt_arat);
 
@@ -161,38 +164,36 @@ void ctxt_switch_levelling(const struct vcpu *next)
 {
        const struct domain *nextd = next ? next->domain : NULL;
 
-       if (cpu_has_cpuid_faulting) {
-               /*
-                * No need to alter the faulting setting if we are switching
-                * to idle; it won't affect any code running in idle context.
-                */
-               if (nextd && is_idle_domain(nextd))
-                       return;
-               /*
-                * We *should* be enabling faulting for the control domain.
-                *
-                * Unfortunately, the domain builder (having only ever been a
-                * PV guest) expects to be able to see host cpuid state in a
-                * native CPUID instruction, to correctly build a CPUID policy
-                * for HVM guests (notably the xstate leaves).
-                *
-                * This logic is fundimentally broken for HVM toolstack
-                * domains, and faulting causes PV guests to behave like HVM
-                * guests from their point of view.
-                *
-                * Future development plans will move responsibility for
-                * generating the maximum full cpuid policy into Xen, at which
-                * this problem will disappear.
-                */
-               set_cpuid_faulting(nextd && !is_control_domain(nextd) &&
-                                  (is_pv_domain(nextd) ||
-                                   next->arch.msrs->
-                                   misc_features_enables.cpuid_faulting));
+       if (!cpu_has_cpuid_faulting) {
+               if (ctxt_switch_masking)
+                       alternative_vcall(ctxt_switch_masking, next);
                return;
        }
 
-       if (ctxt_switch_masking)
-               alternative_vcall(ctxt_switch_masking, next);
+       /*
+        * No need to alter the faulting setting if we are switching
+        * to idle; it won't affect any code running in idle context.
+        */
+       if (nextd && is_idle_domain(nextd))
+               return;
+
+       /*
+        * We *should* be enabling faulting for the control domain.
+        *
+        * The domain builder has now been updated to not depend on seeing
+        * host CPUID values.  This makes it compatible with PVH toolstack
+        * domains, and lets us enable faulting by default for all PV domains.
+        *
+        * However, as PV control domains have never had faulting enforced on
+        * them before, there might plausibly be other dependenices on host
+        * CPUID data.  Therefore, we have left an interim escape hatch in the
+        * form of `dom0=no-cpuid-faulting` to restore the older behaviour.
+        */
+       set_cpuid_faulting(nextd && (opt_dom0_cpuid_faulting ||
+                                    !is_control_domain(nextd)) &&
+                          (is_pv_domain(nextd) ||
+                           next->arch.msrs->
+                           misc_features_enables.cpuid_faulting));
 }
 
 bool_t opt_cpu_info;
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c69570920c..4b75166db3 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -305,6 +305,8 @@ static int __init parse_dom0_param(const char *s)
 #endif
         else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
             opt_dom0_verbose = val;
+        else if ( (val = parse_boolean("cpuid-faulting", s, ss)) >= 0 )
+            opt_dom0_cpuid_faulting = val;
         else
             rc = -EINVAL;
 
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index e65961fccb..88b882c8cc 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -26,6 +26,7 @@
 
 #include <asm/debugreg.h>
 #include <asm/msr.h>
+#include <asm/setup.h>
 
 DEFINE_PER_CPU(uint32_t, tsc_aux);
 
@@ -92,7 +93,7 @@ int init_domain_msr_policy(struct domain *d)
         return -ENOMEM;
 
     /* See comment in intel_ctxt_switch_levelling() */
-    if ( is_control_domain(d) )
+    if ( !opt_dom0_cpuid_faulting && is_control_domain(d) )
         mp->platform_info.cpuid_faulting = false;
 
     d->arch.msr = mp;
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index 15d6363022..861d46d6ac 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -66,6 +66,7 @@ extern bool opt_dom0_shadow;
 #endif
 extern bool opt_dom0_pvh;
 extern bool opt_dom0_verbose;
+extern bool opt_dom0_cpuid_faulting;
 
 #define max_init_domid (0)
 
-- 
2.11.0


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