| Message ID | 20190912185500.21621-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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)
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(-)