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 <jbeul...@suse.com>
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
