[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 08/11] xen/lib: Add topology generator for x86
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |