[v2,8/8] x86/cpuid: Enable CPUID Faulting for the control domain by default
diff mbox series

Message ID 20190912185500.21621-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • Untitled series #173549
Related show

Commit Message

Andrew Cooper Sept. 12, 2019, 6:55 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.

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

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

Comments

Jan Beulich Sept. 13, 2019, 6:38 a.m. UTC | #1
On 12.09.2019 20:55, 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.
> 
> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> opt_dom0_cpuid_faulting would ideally live in dom0_build.c next to
> opt_dom0_verbose, but the file is currently .init

And it should remain so imo.

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

I'm having trouble with this statement: Neither the description nor
the actual code change suggest you alter behavior in this regard
(i.e. with the option used PVH would still be treated the same as
PV afaict). Yet here you seem to suggest things change with this
patch.

As to the question, I think I'd consider this a bugfix, and hence
for the behavior to be okay to change. But as per above I would
expect the change to init_domain_msr_policy() to also include
adding is_pv_domain() to the conditional, or alternatively to
override opt_dom0_cpuid_faulting to true if a PVH Dom0 is being
built.

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

Please also update the command line doc.

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

While unrelated to the overall change here, I think the comment
would better be updated too, to drop the intel_ prefix of the
function name.

Jan
Andrew Cooper Sept. 13, 2019, 2:56 p.m. UTC | #2
On 13/09/2019 07:38, Jan Beulich wrote:
>
>> 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?
> I'm having trouble with this statement: Neither the description nor
> the actual code change suggest you alter behavior in this regard
> (i.e. with the option used PVH would still be treated the same as
> PV afaict). Yet here you seem to suggest things change with this
> patch.
>
> As to the question, I think I'd consider this a bugfix, and hence
> for the behavior to be okay to change. But as per above I would
> expect the change to init_domain_msr_policy() to also include
> adding is_pv_domain() to the conditional, or alternatively to
> override opt_dom0_cpuid_faulting to true if a PVH Dom0 is being
> built.

I've pulled the bugfix out into an earlier patch so it can be backported.

>> ---
>>  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(-)
> Please also update the command line doc.

Hmm - I wonder where the hunk went... I did write one.

>
>> @@ -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;
> While unrelated to the overall change here, I think the comment
> would better be updated too, to drop the intel_ prefix of the
> function name.

Fixed in the bugfix patch.

~Andrew

Patch
diff mbox series

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)