Re: [PATCH v6 08/11] xen/lib: Add topology generator for x86

[Jan Beulich <jbeulich@xxxxxxxx>]

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