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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Wei Liu <w...@xen.org>
>>
>> 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
