| Message ID | 20211214211600.2751-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpuid: Introduce dom0-cpuid command line option | expand |
On 14.12.2021 22:16, Andrew Cooper wrote: > Specifically, this lets the user opt in to non-default for dom0. > > Split features[] out of parse_xen_cpuid(), giving it a lightly more > appropraite name, so it can be shared with parse_xen_cpuid(). With the latter one I guess you mean parse_dom0_cpuid()? > Collect all dom0 settings together in dom0_{en,dis}able_feat[], and apply it > to dom0's policy when other tweaks are being made. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Daniel Smith <dpsmith@apertussolutions.com> > > RFC, because I think we've got a preexisting error with late hwdom here. We > really should not be cobbering a late hwdom's settings (which were provided in > the usual way by the toolstack in dom0). For ITSC I think also covering late hwdom is at least acceptable. For the speculation controls I'm less certain (but as per the comment there they're temporary only anyway), and I agree the command line option here should strictly only apply to Dom0 (or else, as a minor aspect, the option also would better be named "hwdom-cpuid="). > Furthermore, the distinction gets more murky in a hyperlaunch future where > multiple domains may be constructed by Xen, and there is reason to expect that > a full toolstack-like configuration is made available for them. Like above, anything created via the toolstack interfaces should use the toolstack controls. If there was something dom0less-like on x86, domains created that way (without toolstack involvement) would instead want to have another way of controlling their CPUID settings. > One option might be to remove the special case from init_domain_cpuid_policy() > and instead make a call into the cpuid code from create_dom0(). It would have > to be placed between domain_create() and alloc_dom0_vcpu0() for dynamic sizing > of the FPU block to work. Thoughts? As said above, I think the ITSC special case could stay. But apart from this I agree. > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -801,6 +801,22 @@ Controls for how dom0 is constructed on x86 systems. > > If using this option is necessary to fix an issue, please report a bug. > > +### dom0-cpuid > + = List of comma separated booleans > + > + Applicability: x86 > + > +This option allows for fine tuning of the facilities dom0 will use, after > +accounting for hardware capabilities and Xen settings as enumerated via CPUID. > + > +Options are accepted in positive and negative form, to enable or disable > +specific features. All selections via this mechanism are subject to normal > +CPU Policy safety logic. > + > +This option is intended for developers to opt dom0 into non-default features, > +and is not intended for use in production circumstances. If using this option > +is necessary to fix an issue, please report a bug. You may want to state explicitly that disables take priority over enables, as per the present implementation. Personally I would find it better if the item specified last took effect. This is, as mentioned in other contexts, so one can override earlier settings (e.g. in a xen.cfg file used with xen.efi) by simply appending to the command line. > @@ -97,6 +98,73 @@ static int __init parse_xen_cpuid(const char *s) > } > custom_param("cpuid", parse_xen_cpuid); > > +static uint32_t __hwdom_initdata dom0_enable_feat[FSCAPINTS]; > +static uint32_t __hwdom_initdata dom0_disable_feat[FSCAPINTS]; > + > +static int __init parse_dom0_cpuid(const char *s) > +{ > + const char *ss; > + int val, rc = 0; > + > + do { > + const struct feature_name *lhs, *rhs, *mid = NULL /* GCC... */; > + const char *feat; > + > + ss = strchr(s, ','); > + if ( !ss ) > + ss = strchr(s, '\0'); > + > + /* Skip the 'no-' prefix for name comparisons. */ > + feat = s; > + if ( strncmp(s, "no-", 3) == 0 ) > + feat += 3; > + > + /* (Re)initalise lhs and rhs for binary search. */ > + lhs = feature_names; > + rhs = feature_names + ARRAY_SIZE(feature_names); > + > + while ( lhs < rhs ) > + { > + int res; > + > + mid = lhs + (rhs - lhs) / 2; > + res = cmdline_strcmp(feat, mid->name); > + > + if ( res < 0 ) > + { > + rhs = mid; > + continue; > + } > + if ( res > 0 ) > + { > + lhs = mid + 1; > + continue; > + } > + > + if ( (val = parse_boolean(mid->name, s, ss)) >= 0 ) > + { > + __set_bit(mid->bit, > + val ? dom0_enable_feat : dom0_disable_feat); > + mid = NULL; > + } > + > + break; > + } > + > + /* > + * Mid being NULL means that the name and boolean were successfully > + * identified. Everything else is an error. > + */ > + if ( mid ) > + rc = -EINVAL; > + > + s = ss + 1; > + } while ( *ss ); > + > + return rc; > +} > +custom_param("dom0-cpuid", parse_dom0_cpuid); I wonder whether it wouldn't be better (less duplication) if the bulk of the code was also shared with parse_xen_cpuid(). In return moving features[] wouldn't be needed then. Jan
On 12/14/21 4:16 PM, Andrew Cooper wrote: > Specifically, this lets the user opt in to non-default for dom0. > > Split features[] out of parse_xen_cpuid(), giving it a lightly more > appropraite name, so it can be shared with parse_xen_cpuid(). > > Collect all dom0 settings together in dom0_{en,dis}able_feat[], and apply it > to dom0's policy when other tweaks are being made. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Daniel Smith <dpsmith@apertussolutions.com> > > RFC, because I think we've got a preexisting error with late hwdom here. We > really should not be cobbering a late hwdom's settings (which were provided in > the usual way by the toolstack in dom0). > > Furthermore, the distinction gets more murky in a hyperlaunch future where > multiple domains may be constructed by Xen, and there is reason to expect that > a full toolstack-like configuration is made available for them. For hyperlaunch, perhaps Christohper and I should revisit the DTB to add a cpuid field/mask where CPU features can be masked out. > One option might be to remove the special case from init_domain_cpuid_policy() > and instead make a call into the cpuid code from create_dom0(). It would have > to be placed between domain_create() and alloc_dom0_vcpu0() for dynamic sizing > of the FPU block to work. Thoughts? v/r, dps
On 15/12/2021 08:34, Jan Beulich wrote: > On 14.12.2021 22:16, Andrew Cooper wrote: >> Specifically, this lets the user opt in to non-default for dom0. >> >> Split features[] out of parse_xen_cpuid(), giving it a lightly more >> appropraite name, so it can be shared with parse_xen_cpuid(). > With the latter one I guess you mean parse_dom0_cpuid()? I do, yes. This is a copy/paste error. >> Collect all dom0 settings together in dom0_{en,dis}able_feat[], and apply it >> to dom0's policy when other tweaks are being made. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> >> RFC, because I think we've got a preexisting error with late hwdom here. We >> really should not be cobbering a late hwdom's settings (which were provided in >> the usual way by the toolstack in dom0). > For ITSC I think also covering late hwdom is at least acceptable. For the > speculation controls I'm less certain (but as per the comment there they're > temporary only anyway), and I agree the command line option here should > strictly only apply to Dom0 (or else, as a minor aspect, the option also > would better be named "hwdom-cpuid="). > >> Furthermore, the distinction gets more murky in a hyperlaunch future where >> multiple domains may be constructed by Xen, and there is reason to expect that >> a full toolstack-like configuration is made available for them. > Like above, anything created via the toolstack interfaces should use the > toolstack controls. If there was something dom0less-like on x86, domains > created that way (without toolstack involvement) would instead want to > have another way of controlling their CPUID settings. > >> One option might be to remove the special case from init_domain_cpuid_policy() >> and instead make a call into the cpuid code from create_dom0(). It would have >> to be placed between domain_create() and alloc_dom0_vcpu0() for dynamic sizing >> of the FPU block to work. Thoughts? > As said above, I think the ITSC special case could stay. But apart from > this I agree. So I disagree with keeping the ITSC special case. I do agree that a non-dom0 hwdom probably wants ITSC, but ITSC explicitly can be controlled by the toolstack, and therefore Xen should not be overriding the toolstack's decision. IMO, this really does want to remain dom0-cpuid= rather than hwdom-cpuid=. It is specific to the domain which Xen creates as part of bringing the system up. In a future world with hyperlaunch/dom0less/etc, there is (or should be) adequate provision to specify settings like this for all created domains. In this case, perhaps hyperlaunch declares that dom0-cpuid= gets ignored, because we probably don't want random command line settings impacting one N'th of the carefully curated system configuration. (Also, any Xen constructed domains in a hyperlaunch world probably want ITSC, but I'll again argue firmly that Xen should not be special-casing this one feature.) >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -801,6 +801,22 @@ Controls for how dom0 is constructed on x86 systems. >> >> If using this option is necessary to fix an issue, please report a bug. >> >> +### dom0-cpuid >> + = List of comma separated booleans >> + >> + Applicability: x86 >> + >> +This option allows for fine tuning of the facilities dom0 will use, after >> +accounting for hardware capabilities and Xen settings as enumerated via CPUID. >> + >> +Options are accepted in positive and negative form, to enable or disable >> +specific features. All selections via this mechanism are subject to normal >> +CPU Policy safety logic. >> + >> +This option is intended for developers to opt dom0 into non-default features, >> +and is not intended for use in production circumstances. If using this option >> +is necessary to fix an issue, please report a bug. > You may want to state explicitly that disables take priority over enables, > as per the present implementation. Personally I would find it better if the > item specified last took effect. This is, as mentioned in other contexts, > so one can override earlier settings (e.g. in a xen.cfg file used with > xen.efi) by simply appending to the command line. Order of enabled/disabled I feel is an implementation detail. Perhaps what to put in the docs is that specifying both forms is unsupported, but "this is for developers only" is already a fairly big hint. The only way to make a latest-takes-priority scheme work is to use string_param() (creating an arbitrary upper bound limit), and parsing the list during dom0 construction. For a developer-only option, I really don't think the complexity is worth the effort. >> @@ -97,6 +98,73 @@ static int __init parse_xen_cpuid(const char *s) >> } >> custom_param("cpuid", parse_xen_cpuid); >> >> +static uint32_t __hwdom_initdata dom0_enable_feat[FSCAPINTS]; >> +static uint32_t __hwdom_initdata dom0_disable_feat[FSCAPINTS]; >> + >> +static int __init parse_dom0_cpuid(const char *s) >> +{ >> + const char *ss; >> + int val, rc = 0; >> + >> + do { >> + const struct feature_name *lhs, *rhs, *mid = NULL /* GCC... */; >> + const char *feat; >> + >> + ss = strchr(s, ','); >> + if ( !ss ) >> + ss = strchr(s, '\0'); >> + >> + /* Skip the 'no-' prefix for name comparisons. */ >> + feat = s; >> + if ( strncmp(s, "no-", 3) == 0 ) >> + feat += 3; >> + >> + /* (Re)initalise lhs and rhs for binary search. */ >> + lhs = feature_names; >> + rhs = feature_names + ARRAY_SIZE(feature_names); >> + >> + while ( lhs < rhs ) >> + { >> + int res; >> + >> + mid = lhs + (rhs - lhs) / 2; >> + res = cmdline_strcmp(feat, mid->name); >> + >> + if ( res < 0 ) >> + { >> + rhs = mid; >> + continue; >> + } >> + if ( res > 0 ) >> + { >> + lhs = mid + 1; >> + continue; >> + } >> + >> + if ( (val = parse_boolean(mid->name, s, ss)) >= 0 ) >> + { >> + __set_bit(mid->bit, >> + val ? dom0_enable_feat : dom0_disable_feat); >> + mid = NULL; >> + } >> + >> + break; >> + } >> + >> + /* >> + * Mid being NULL means that the name and boolean were successfully >> + * identified. Everything else is an error. >> + */ >> + if ( mid ) >> + rc = -EINVAL; >> + >> + s = ss + 1; >> + } while ( *ss ); >> + >> + return rc; >> +} >> +custom_param("dom0-cpuid", parse_dom0_cpuid); > I wonder whether it wouldn't be better (less duplication) if the bulk > of the code was also shared with parse_xen_cpuid(). In return moving > features[] wouldn't be needed then. I wondered the same, but couldn't think of anything I liked. I suppose an always_inline parse_cpuid(void (*cb)(feat, val)) wouldn't be too bad. We'd need 5 functions in total, but it will optimise back to 2 with no function pointers. ~Andrew
On 15.12.2021 13:13, Andrew Cooper wrote: > On 15/12/2021 08:34, Jan Beulich wrote: >> On 14.12.2021 22:16, Andrew Cooper wrote: >>> RFC, because I think we've got a preexisting error with late hwdom here. We >>> really should not be cobbering a late hwdom's settings (which were provided in >>> the usual way by the toolstack in dom0). >> For ITSC I think also covering late hwdom is at least acceptable. For the >> speculation controls I'm less certain (but as per the comment there they're >> temporary only anyway), and I agree the command line option here should >> strictly only apply to Dom0 (or else, as a minor aspect, the option also >> would better be named "hwdom-cpuid="). >> >>> Furthermore, the distinction gets more murky in a hyperlaunch future where >>> multiple domains may be constructed by Xen, and there is reason to expect that >>> a full toolstack-like configuration is made available for them. >> Like above, anything created via the toolstack interfaces should use the >> toolstack controls. If there was something dom0less-like on x86, domains >> created that way (without toolstack involvement) would instead want to >> have another way of controlling their CPUID settings. >> >>> One option might be to remove the special case from init_domain_cpuid_policy() >>> and instead make a call into the cpuid code from create_dom0(). It would have >>> to be placed between domain_create() and alloc_dom0_vcpu0() for dynamic sizing >>> of the FPU block to work. Thoughts? >> As said above, I think the ITSC special case could stay. But apart from >> this I agree. > > So I disagree with keeping the ITSC special case. > > I do agree that a non-dom0 hwdom probably wants ITSC, but ITSC > explicitly can be controlled by the toolstack, and therefore Xen should > not be overriding the toolstack's decision. Well, fair enough as long as there actually is a tool stack side equivalent of this. >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -801,6 +801,22 @@ Controls for how dom0 is constructed on x86 systems. >>> >>> If using this option is necessary to fix an issue, please report a bug. >>> >>> +### dom0-cpuid >>> + = List of comma separated booleans >>> + >>> + Applicability: x86 >>> + >>> +This option allows for fine tuning of the facilities dom0 will use, after >>> +accounting for hardware capabilities and Xen settings as enumerated via CPUID. >>> + >>> +Options are accepted in positive and negative form, to enable or disable >>> +specific features. All selections via this mechanism are subject to normal >>> +CPU Policy safety logic. >>> + >>> +This option is intended for developers to opt dom0 into non-default features, >>> +and is not intended for use in production circumstances. If using this option >>> +is necessary to fix an issue, please report a bug. >> You may want to state explicitly that disables take priority over enables, >> as per the present implementation. Personally I would find it better if the >> item specified last took effect. This is, as mentioned in other contexts, >> so one can override earlier settings (e.g. in a xen.cfg file used with >> xen.efi) by simply appending to the command line. > > Order of enabled/disabled I feel is an implementation detail. Perhaps > what to put in the docs is that specifying both forms is unsupported, > but "this is for developers only" is already a fairly big hint. > > The only way to make a latest-takes-priority scheme work is to use > string_param() (creating an arbitrary upper bound limit), and parsing > the list during dom0 construction. Why? Simply accompany __set_bit() by a __clear_bit() accessing the opposite array. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index f7797ea233f9..1278f9c27597 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -801,6 +801,22 @@ Controls for how dom0 is constructed on x86 systems. If using this option is necessary to fix an issue, please report a bug. +### dom0-cpuid + = List of comma separated booleans + + Applicability: x86 + +This option allows for fine tuning of the facilities dom0 will use, after +accounting for hardware capabilities and Xen settings as enumerated via CPUID. + +Options are accepted in positive and negative form, to enable or disable +specific features. All selections via this mechanism are subject to normal +CPU Policy safety logic. + +This option is intended for developers to opt dom0 into non-default features, +and is not intended for use in production circumstances. If using this option +is necessary to fix an issue, 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/cpuid.c b/xen/arch/x86/cpuid.c index 151944f65702..896bc1595317 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -26,17 +26,18 @@ static const uint32_t __initconst hvm_hap_def_featuremask[] = INIT_HVM_HAP_DEF_FEATURES; static const uint32_t deep_features[] = INIT_DEEP_FEATURES; +static const struct feature_name { + const char *name; + unsigned int bit; +} feature_names[] __initconstrel = INIT_FEATURE_NAMES; + static int __init parse_xen_cpuid(const char *s) { const char *ss; int val, rc = 0; do { - static const struct feature { - const char *name; - unsigned int bit; - } features[] __initconstrel = INIT_FEATURE_NAMES; - const struct feature *lhs, *rhs, *mid = NULL /* GCC... */; + const struct feature_name *lhs, *rhs, *mid = NULL /* GCC... */; const char *feat; ss = strchr(s, ','); @@ -49,8 +50,8 @@ static int __init parse_xen_cpuid(const char *s) feat += 3; /* (Re)initalise lhs and rhs for binary search. */ - lhs = features; - rhs = features + ARRAY_SIZE(features); + lhs = feature_names; + rhs = feature_names + ARRAY_SIZE(feature_names); while ( lhs < rhs ) { @@ -97,6 +98,73 @@ static int __init parse_xen_cpuid(const char *s) } custom_param("cpuid", parse_xen_cpuid); +static uint32_t __hwdom_initdata dom0_enable_feat[FSCAPINTS]; +static uint32_t __hwdom_initdata dom0_disable_feat[FSCAPINTS]; + +static int __init parse_dom0_cpuid(const char *s) +{ + const char *ss; + int val, rc = 0; + + do { + const struct feature_name *lhs, *rhs, *mid = NULL /* GCC... */; + const char *feat; + + ss = strchr(s, ','); + if ( !ss ) + ss = strchr(s, '\0'); + + /* Skip the 'no-' prefix for name comparisons. */ + feat = s; + if ( strncmp(s, "no-", 3) == 0 ) + feat += 3; + + /* (Re)initalise lhs and rhs for binary search. */ + lhs = feature_names; + rhs = feature_names + ARRAY_SIZE(feature_names); + + while ( lhs < rhs ) + { + int res; + + mid = lhs + (rhs - lhs) / 2; + res = cmdline_strcmp(feat, mid->name); + + if ( res < 0 ) + { + rhs = mid; + continue; + } + if ( res > 0 ) + { + lhs = mid + 1; + continue; + } + + if ( (val = parse_boolean(mid->name, s, ss)) >= 0 ) + { + __set_bit(mid->bit, + val ? dom0_enable_feat : dom0_disable_feat); + mid = NULL; + } + + break; + } + + /* + * Mid being NULL means that the name and boolean were successfully + * identified. Everything else is an error. + */ + if ( mid ) + rc = -EINVAL; + + s = ss + 1; + } while ( *ss ); + + return rc; +} +custom_param("dom0-cpuid", parse_dom0_cpuid); + #define EMPTY_LEAF ((struct cpuid_leaf){}) static void zero_leaves(struct cpuid_leaf *l, unsigned int first, unsigned int last) @@ -727,17 +795,33 @@ int init_domain_cpuid_policy(struct domain *d) if ( !p ) return -ENOMEM; - /* The hardware domain can't migrate. Give it ITSC if available. */ if ( is_hardware_domain(d) ) + { + uint32_t fs[FSCAPINTS]; + unsigned int i; + + /* The hardware domain can't migrate. Give it ITSC if available. */ p->extd.itsc = cpu_has_itsc; - /* - * Expose the "hardware speculation behaviour" bits of ARCH_CAPS to dom0, - * so dom0 can turn off workarounds as appropriate. Temporary, until the - * domain policy logic gains a better understanding of MSRs. - */ - if ( is_hardware_domain(d) && cpu_has_arch_caps ) - p->feat.arch_caps = true; + /* + * Expose the "hardware speculation behaviour" bits of ARCH_CAPS to + * dom0, so dom0 can turn off workarounds as appropriate. Temporary, + * until the domain policy logic gains a better understanding of MSRs. + */ + if ( cpu_has_arch_caps ) + p->feat.arch_caps = true; + + /* Apply dom0-cpuid= command line settings. */ + cpuid_policy_to_featureset(p, fs); + + for ( i = 0; i < ARRAY_SIZE(fs); ++i ) + { + fs[i] |= dom0_enable_feat[i]; + fs[i] &= ~dom0_disable_feat[i]; + } + + cpuid_featureset_to_policy(fs, p); + } d->arch.cpuid = p;
Specifically, this lets the user opt in to non-default for dom0. Split features[] out of parse_xen_cpuid(), giving it a lightly more appropraite name, so it can be shared with parse_xen_cpuid(). Collect all dom0 settings together in dom0_{en,dis}able_feat[], and apply it to dom0's policy when other tweaks are being made. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Daniel Smith <dpsmith@apertussolutions.com> RFC, because I think we've got a preexisting error with late hwdom here. We really should not be cobbering a late hwdom's settings (which were provided in the usual way by the toolstack in dom0). Furthermore, the distinction gets more murky in a hyperlaunch future where multiple domains may be constructed by Xen, and there is reason to expect that a full toolstack-like configuration is made available for them. One option might be to remove the special case from init_domain_cpuid_policy() and instead make a call into the cpuid code from create_dom0(). It would have to be placed between domain_create() and alloc_dom0_vcpu0() for dynamic sizing of the FPU block to work. Thoughts? --- docs/misc/xen-command-line.pandoc | 16 ++++++ xen/arch/x86/cpuid.c | 114 +++++++++++++++++++++++++++++++++----- 2 files changed, 115 insertions(+), 15 deletions(-)