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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

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

Jan