Re: [PATCH] x86/cpuid: Introduce dom0-cpuid command line option

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 
> 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