diff mbox series

[v2,10/10] x86/cpuid: Enable CPUID Faulting for PV control domains by default

Message ID 20190913192759.10795-11-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/cpuid: Switch to using XEN_DOMCTL_set_cpumsr_policy | expand

Commit Message

Andrew Cooper Sept. 13, 2019, 7:27 p.m. UTC
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.

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

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>

v2:
 * Introduce a command line option to retain old behaviour.
 * Advertise virtualised faulting support to dom0 when it is used.
v2.1:
 * Split the PVH adjustment out.  Rebase.
 * Recover the docs/ hunk which was accidentally missing.
---
 docs/misc/xen-command-line.pandoc | 19 ++++++++++++++++++-
 xen/arch/x86/cpu/common.c         | 26 ++++++++++++++------------
 xen/arch/x86/dom0_build.c         |  2 ++
 xen/arch/x86/msr.c                |  3 ++-
 xen/include/asm-x86/setup.h       |  1 +
 5 files changed, 37 insertions(+), 14 deletions(-)

Comments

Jan Beulich Sept. 16, 2019, 11:22 a.m. UTC | #1
On 13.09.2019 21:27, Andrew Cooper wrote:
> 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.
> 
> Advertise virtualised faulting support to control domains unless the opt-out
> has been used.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
perhaps with ...

> --- 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;

... this wrapped in "#ifdef CONFIG_PV" or IS_ENABLED(CONFIG_PV)
added to the condition?

Jan
Andrew Cooper Sept. 16, 2019, 3:52 p.m. UTC | #2
On 16/09/2019 12:22, Jan Beulich wrote:
> On 13.09.2019 21:27, Andrew Cooper wrote:
>> 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.
>>
>> Advertise virtualised faulting support to control domains unless the opt-out
>> has been used.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

> perhaps with ...
>
>> --- 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;
> ... this wrapped in "#ifdef CONFIG_PV" or IS_ENABLED(CONFIG_PV)
> added to the condition?

Done.

~Andrew
diff mbox series

Patch

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 832797e2e2..fc64429064 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -658,7 +658,8 @@  The debug trace feature is only enabled in debugging builds of Xen.
 Specify the bit width of the DMA heap.
 
 ### dom0
-    = List of [ pv | pvh, shadow=<bool>, verbose=<bool> ]
+    = List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
+                cpuid-faulting=<bool> ]
 
     Applicability: x86
 
@@ -691,6 +692,22 @@  Controls for how dom0 is constructed on x86 systems.
     information during the dom0 build.  It defaults to the compile time choice
     of `CONFIG_VERBOSE_DEBUG`.
 
+*   The `cpuid-faulting` boolean is an interim option, is only applicable to
+    PV dom0, and defaults to true.
+
+    Before Xen 4.13, the domain builder logic for guest construction depended
+    on seeing host CPUID values to function correctly.  As a result, CPUID
+    Faulting was never activated for PV dom0's, even on capable hardware.
+
+    In Xen 4.13, the domain builder logic has been fixed, and no longer has
+    this dependency.  As a consequence, CPUID Faulting is activated by default
+    even for PV dom0's.
+
+    However, as PV dom0's have always seen host CPUID data in the past, there
+    is a chance that further dependencies exist.  This boolean can be used to
+    restore the pre-4.13 behaviour.  If specifying `no-cpuid-faulting` fixes
+    an issue in dom0, please report a bug.
+
 ### dom0-iommu
     = List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
                 map-reserved=<bool>, none ]
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 4bf852c948..6c6bd63301 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);
 
@@ -171,20 +174,19 @@  void ctxt_switch_levelling(const struct vcpu *next)
 		/*
 		 * We *should* be enabling faulting for PV control domains.
 		 *
-		 * 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.
+		 * 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.
 		 *
-		 * Future development plans will move responsibility for
-		 * generating the maximum full cpuid policy into Xen, at which
-		 * this problem will disappear.
+		 * 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 && (!is_control_domain(nextd) ||
+		set_cpuid_faulting(nextd && (opt_dom0_cpuid_faulting ||
+					     !is_control_domain(nextd) ||
 					     !is_pv_domain(nextd)) &&
 				   (is_pv_domain(nextd) ||
 				    next->arch.msrs->
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 a6c8cc7627..4698d2bba1 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 ctxt_switch_levelling() */
-    if ( is_control_domain(d) && is_pv_domain(d) )
+    if ( !opt_dom0_cpuid_faulting && is_control_domain(d) && is_pv_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)