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
