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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2023 17:45, Andrew Cooper wrote:
> On 04/04/2023 4:16 pm, Jan Beulich wrote:
>> On 04.04.2023 11:52, Andrew Cooper wrote:
>>> @@ -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.

Hmm, yes:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

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

Why? hvm_enabled is a #define (to false) only when !HVM.

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

Right, and this will hold when HVM=y but no VMX/SVM was found.

Jan