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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Wed Oct 9, 2024 at 3:45 PM BST, Jan Beulich wrote:
> 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).

Sure.

>
> > --- 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?

Each of them individually could overflow the fields in which they are used.

Does returning EINVAL if either threads_per_core or cores_per_pkg overflow the
INTEL structure j

>
> > +    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)?

Sure

>
> > +    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?

That's showing all unified caches as caches covering the whole package. We
could do it the other way around (but I don't want to reverse engineer what the
host policy says because that's irrelevant). There's nothing in the SDM (AFAIK)
forcing L2 or L3 to behave one way or another, so we get to choose. I thought
it more helpful to make all unified caches unified across the package. to give
more information in the leaf.

My own system exposes 2 unified caches (data trimmed for space):

``` cpuid

   deterministic cache parameters (4):
      --- cache 0 ---
      cache type                         = data cache (1)
      cache level                        = 0x1 (1)
      maximum IDs for CPUs sharing cache = 0x1 (1)
      maximum IDs for cores in pkg       = 0xf (15)
      --- cache 1 ---
      cache type                         = instruction cache (2)
      cache level                        = 0x1 (1)
      maximum IDs for CPUs sharing cache = 0x1 (1)
      maximum IDs for cores in pkg       = 0xf (15)
      --- cache 2 ---
      cache type                         = unified cache (3)
      cache level                        = 0x2 (2)
      maximum IDs for CPUs sharing cache = 0x1 (1)
      maximum IDs for cores in pkg       = 0xf (15)
      --- cache 3 ---
      cache type                         = unified cache (3)
      cache level                        = 0x3 (3)
      maximum IDs for CPUs sharing cache = 0x1f (31)
      maximum IDs for cores in pkg       = 0xf (15)
      --- cache 4 ---
      cache type                         = no more caches (0)
```

>
> Jan

Cheers,
Alejandro