On 01.10.2024 14:38, Alejandro Vallejo wrote:
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -542,6 +542,22 @@ int x86_cpu_policies_are_compatible(const struct
> cpu_policy *host,
> const struct cpu_policy *guest,
> struct cpu_policy_errors *err);
>
> +/**
> + * Synthesise topology information in `p` given high-level constraints
> + *
> + * Topology is given in various fields accross several leaves, some of
> + * which are vendor-specific. This function uses the policy itself to
> + * derive such leaves from threads/core and cores/package.
Isn't it more like s/uses/fills/ (and the rest of the sentence then
possibly adjust some to match)? The policy looks to be purely an output
here (except for the vendor field).
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -2,6 +2,94 @@
>
> #include <xen/lib/x86/cpu-policy.h>
>
> +static unsigned int order(unsigned int n)
> +{
> + ASSERT(n); /* clz(0) is UB */
> +
> + return 8 * sizeof(n) - __builtin_clz(n);
> +}
> +
> +int x86_topo_from_parts(struct cpu_policy *p,
> + unsigned int threads_per_core,
> + unsigned int cores_per_pkg)
> +{
> + unsigned int threads_per_pkg = threads_per_core * cores_per_pkg;
What about the (admittedly absurd) case of this overflowing?
> + unsigned int apic_id_size;
> +
> + if ( !p || !threads_per_core || !cores_per_pkg )
> + return -EINVAL;
> +
> + p->basic.max_leaf = MAX(0xb, p->basic.max_leaf);
Better use the type-safe max() (and min() further down)?
> + memset(p->topo.raw, 0, sizeof(p->topo.raw));
> +
> + /* thread level */
> + p->topo.subleaf[0].nr_logical = threads_per_core;
> + p->topo.subleaf[0].id_shift = 0;
> + p->topo.subleaf[0].level = 0;
> + p->topo.subleaf[0].type = 1;
> + if ( threads_per_core > 1 )
> + p->topo.subleaf[0].id_shift = order(threads_per_core - 1);
> +
> + /* core level */
> + p->topo.subleaf[1].nr_logical = cores_per_pkg;
> + if ( p->x86_vendor == X86_VENDOR_INTEL )
> + p->topo.subleaf[1].nr_logical = threads_per_pkg;
> + p->topo.subleaf[1].id_shift = p->topo.subleaf[0].id_shift;
> + p->topo.subleaf[1].level = 1;
> + p->topo.subleaf[1].type = 2;
> + if ( cores_per_pkg > 1 )
> + p->topo.subleaf[1].id_shift += order(cores_per_pkg - 1);
> +
> + apic_id_size = p->topo.subleaf[1].id_shift;
> +
> + /*
> + * Contrary to what the name might seem to imply. HTT is an enabler for
> + * SMP and there's no harm in setting it even with a single vCPU.
> + */
> + p->basic.htt = true;
> + p->basic.lppp = MIN(0xff, threads_per_pkg);
> +
> + switch ( p->x86_vendor )
> + {
> + case X86_VENDOR_INTEL: {
> + struct cpuid_cache_leaf *sl = p->cache.subleaf;
> +
> + for ( size_t i = 0; sl->type &&
> + i < ARRAY_SIZE(p->cache.raw); i++, sl++ )
> + {
> + sl->cores_per_package = cores_per_pkg - 1;
> + sl->threads_per_cache = threads_per_core - 1;
> + if ( sl->type == 3 /* unified cache */ )
> + sl->threads_per_cache = threads_per_pkg - 1;
I wasn't able to find documentation for this, well, anomaly. Can you please
point me at where this is spelled out?
Jan
