Re: [PATCH v2 11/15] x86/boot: Merge CPUID policy initialisation logic into cpu-policy.c

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/2023 4:16 pm, Jan Beulich wrote:
> On 04.04.2023 11:52, Andrew Cooper wrote:
>> Switch to the newer cpu_policy nomenclature.  Do some easy cleanup of
>> includes.
>>
>> No practical change.
>>
>> 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>
>>
>> v2:
>>  * New
>> ---
>>  xen/arch/x86/cpu-policy.c             | 752 ++++++++++++++++++++++++
>>  xen/arch/x86/cpuid.c                  | 817 +-------------------------
>>  xen/arch/x86/hvm/hvm.c                |   1 -
>>  xen/arch/x86/include/asm/cpu-policy.h |   6 +
>>  xen/arch/x86/include/asm/cpuid.h      |  11 +-
>>  xen/arch/x86/pv/domain.c              |   1 +
>>  xen/arch/x86/setup.c                  |   2 -
>>  7 files changed, 764 insertions(+), 826 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
>> index f6a2317ed7bd..83186e940ca7 100644
>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -1,13 +1,19 @@
>>  /* SPDX-License-Identifier: GPL-2.0-or-later */
>>  #include <xen/cache.h>
>>  #include <xen/kernel.h>
>> +#include <xen/param.h>
>>  #include <xen/sched.h>
>>  
>>  #include <xen/lib/x86/cpu-policy.h>
>>  
>> +#include <asm/amd.h>
>>  #include <asm/cpu-policy.h>
>> +#include <asm/hvm/nestedhvm.h>
>> +#include <asm/hvm/svm/svm.h>
>>  #include <asm/msr-index.h>
>> +#include <asm/paging.h>
>>  #include <asm/setup.h>
>> +#include <asm/xstate.h>
>>  
>>  struct cpu_policy __ro_after_init     raw_cpu_policy;
>>  struct cpu_policy __ro_after_init    host_cpu_policy;
>> @@ -20,10 +26,332 @@ struct cpu_policy __ro_after_init hvm_max_cpu_policy;
>>  struct cpu_policy __ro_after_init hvm_def_cpu_policy;
>>  #endif
>>  
>> +const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>> +
>> +static const uint32_t __initconst pv_max_featuremask[] = 
>> INIT_PV_MAX_FEATURES;
>> +static const uint32_t hvm_shadow_max_featuremask[] = 
>> INIT_HVM_SHADOW_MAX_FEATURES;
>> +static const uint32_t __initconst hvm_hap_max_featuremask[] =
>> +    INIT_HVM_HAP_MAX_FEATURES;
>> +static const uint32_t __initconst pv_def_featuremask[] = 
>> INIT_PV_DEF_FEATURES;
>> +static const uint32_t __initconst hvm_shadow_def_featuremask[] =
>> +    INIT_HVM_SHADOW_DEF_FEATURES;
>> +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;
>> +
>> +/*
>> + * Parse a list of cpuid feature names -> bool, calling the callback for any
>> + * matches found.
>> + *
>> + * always_inline, because this is init code only and we really don't want a
>> + * function pointer call in the middle of the loop.
>> + */
>> +static int __init always_inline parse_cpuid(
>> +    const char *s, void (*callback)(unsigned int feat, bool val))
>> +{
>> +    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 )
>> +            {
>> +                callback(mid->bit, val);
>> +                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;
>> +}
>> +
>> +static void __init cf_check _parse_xen_cpuid(unsigned int feat, bool val)
>> +{
>> +    if ( !val )
>> +        setup_clear_cpu_cap(feat);
>> +    else if ( feat == X86_FEATURE_RDRAND &&
>> +              (cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_RDRAND)) )
>> +        setup_force_cpu_cap(X86_FEATURE_RDRAND);
>> +}
>> +
>> +static int __init cf_check parse_xen_cpuid(const char *s)
>> +{
>> +    return parse_cpuid(s, _parse_xen_cpuid);
>> +}
>> +custom_param("cpuid", parse_xen_cpuid);
>> +
>> +static bool __initdata dom0_cpuid_cmdline;
>> +static uint32_t __initdata dom0_enable_feat[FSCAPINTS];
>> +static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
>> +
>> +static void __init cf_check _parse_dom0_cpuid(unsigned int feat, bool val)
>> +{
>> +    __set_bit  (feat, val ? dom0_enable_feat  : dom0_disable_feat);
>> +    __clear_bit(feat, val ? dom0_disable_feat : dom0_enable_feat );
>> +}
>> +
>> +static int __init cf_check parse_dom0_cpuid(const char *s)
>> +{
>> +    dom0_cpuid_cmdline = true;
>> +
>> +    return parse_cpuid(s, _parse_dom0_cpuid);
>> +}
>> +custom_param("dom0-cpuid", parse_dom0_cpuid);
> Unless the plan is to completely remove cpuid.c, this command line
> handling would imo better fit there. I understand that to keep
> dom0_{en,dis}able_feat[] static, the _parse_dom0_cpuid() helper
> would then need to be exposed (under a different name), but I think
> that's quite okay, the more that it's an __init function.

I'm not sure I agree.  (I did debate this for a while before moving the
cmdline parsing.)

I do have some cleanup plans which will move code into cpuid.c, and
guest_cpuid() absolutely still lives there, but for these options
specifically, the moment I add MSR_ARCH_CAPS into a featureset, their
bit names names will work here too.

So arguably {dom0-}cpuid= don't be a great name moving forwards, but it
is is absolutely more cpu-policy.c content than cpuid.c content.

We can't get rid of the existing cmdline names, and I think documenting
our way out of the "it's not only CPUID bits any more" is better than
adding yet a new name.

>> @@ -149,3 +716,188 @@ int init_domain_cpu_policy(struct domain *d)
>>  
>>      return 0;
>>  }
>> +
>> +void recalculate_cpuid_policy(struct domain *d)
>> +{
>> +    struct cpu_policy *p = d->arch.cpuid;
>> +    const struct cpu_policy *max = is_pv_domain(d)
>> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_cpu_policy : NULL)
>> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_cpu_policy : NULL);
> While this is how the original code was, wouldn't this want to use
> hvm_enabled, just like init_guest_cpu_policies() does (patch 10)?

No.  That will fail to link.

This trickery is necessary to drop the compiler-visible reference to
hvm_max_cpu_policy in !CONFIG_HVM builds.

This function is only called after the domain type has already been
established, which precludes calling it in a case where max will
evaluate to NULL, hence the ASSERT_UNREACHABLE() just below.

~Andrew