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 5:14 pm, Jan Beulich wrote:
> 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>

Thanks.

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

Hmm, maybe.

But honestly, I want to keep the code as it is because this is trying to
only be code-movement, and because it's currently symmetric between the
two cases.

~Andrew