[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 5/6] xen/x86: Derive topologically correct x2APIC IDs from the policy
Hi, Ack and sure to everything on types, constness and variable names. On 20/03/2024 10:15, Roger Pau Monné wrote: >> + const char *name; >> + uint32_t vcpu_id; >> + uint32_t x2apic_id; >> + struct cpu_policy policy; >> + } tests[] = { >> + { >> + .name = "3v: 3 t/c, 8 c/s", .vcpu_id = 3, .x2apic_id = 1 << 2, >> + .policy = { >> + .x86_vendor = X86_VENDOR_AMD, >> + .topo.subleaf = { >> + [0] = { .nr_logical = 3, .level = 0, .type = 1, >> .id_shift = 2, }, >> + [1] = { .nr_logical = 8, .level = 1, .type = 2, >> .id_shift = 5, }, >> + }, > > Don't we need a helper that gets passed the number of cores per > socket and threads per core and fills this up? > > I would introduce this first, add a test for it, and then do this > testing using the helper. Funnily enough that's how I tested it initially. Alas, it felt silly to put it where it will be linked against the hypervisor. I'm happy to put it back there. >> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c >> index a3b24e6879..2a50bc076a 100644 >> --- a/xen/lib/x86/policy.c >> +++ b/xen/lib/x86/policy.c >> @@ -2,15 +2,78 @@ >> >> #include <xen/lib/x86/cpu-policy.h> >> >> -uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t >> vcpu_id) >> +static uint32_t parts_per_higher_scoped_level(const struct cpu_policy *p, >> size_t lvl) >> { >> /* >> - * TODO: Derive x2APIC ID from the topology information inside `p` >> - * rather than from vCPU ID. This bodge is a temporary measure >> - * until all infra is in place to retrieve or derive the initial >> - * x2APIC ID from migrated domains. >> + * `nr_logical` reported by Intel is the number of THREADS contained in >> + * the next topological scope. For example, assuming a system with 2 >> + * threads/core and 3 cores/module in a fully symmetric topology, >> + * `nr_logical` at the core level will report 6. Because it's reporting >> + * the number of threads in a module. >> + * >> + * On AMD/Hygon, nr_logical is already normalized by the higher scoped >> + * level (cores/complex, etc) so we can return it as-is. >> */ >> - return vcpu_id * 2; >> + if ( p->x86_vendor != X86_VENDOR_INTEL || !lvl ) >> + return p->topo.subleaf[lvl].nr_logical; >> + >> + return p->topo.subleaf[lvl].nr_logical / p->topo.subleaf[lvl - >> 1].nr_logical; > > I think this is an optimization because we only have 2 levels, > otherwise you would need a loop like: > > unsigned int nr = p->topo.subleaf[lvl].nr_logical > for ( unsigned int i; i < lvl; i++ ) > nr /= p->topo.subleaf[i].nr_logical; > > If that's the case I think we need a > BUILD_BUG_ON(ARRAY_SIZE(p->topo.subleaf) > 1); It's not meant to be. That division should still hold for leaves 0x1f and e26 where the level count typically exceeds 2. From the SDM. ``` Bits 15-00: The number of logical processors across all instances of this domain within the next higherscoped domain. (For example, in a processor socket/package comprising “M” dies of “N” cores each, where each core has “L” logical processors, the “die” domain sub-leaf value of this field would be M*N*L.) This number reflects configuration as shipped by Intel. Note, software must not use this field to enumerate processor topology*. ``` So. If level1 has nr_logical=X*Y and level2 has nr_logical=X*Y*Z then dividing the latter by the former gives you Z, which is the number of lvl1-parts for a single instance of lvl2 (i.e: cores/package, or whatever). Unless I'm missing something? Cheers, Alejandro
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |