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

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

On Thu Oct 10, 2024 at 8:54 AM BST, Jan Beulich wrote:
> On 09.10.2024 19:57, Alejandro Vallejo wrote:
> > On Wed Oct 9, 2024 at 3:45 PM BST, Jan Beulich wrote:
> >> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> >>> --- 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
>
> The sentence looks unfinished, so I can only vaguely say that my answer to
> the question would likely be "yes".

It was indeed. Regardless, the number of bits available in Intel's cache
subleaves is rather limited, so I'll be clipping those to the maximum on
overflow and...

>
> >>> +    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)
>
> Note how this is different ...
>
> >       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)
>
> ... from this, whereas your code would make it the same.
>
> Especially if this is something you do beyond / outside the spec, it imo
> needs reasoning about in fair detail in the description.

... given the risk of clipping, I'll get rid of that conditional too to make it
easier for a non-clipped number to be reported.

I'll write in the commit message the behaviour on overflow for these leaves.

>
> Jan

Cheers,
Alejandro