On Tue, Jan 09, 2024 at 03:38:33PM +0000, Alejandro Vallejo wrote:
> Implements the helper for mapping vcpu_id to x2apic_id given a valid
> topology in a policy. The algo is written with the intention of extending
> it to leaves 0x1f and e26 in the future.
>
> Toolstack doesn't set leaf 0xb and the HVM default policy has it cleared,
> so the leaf is not implemented. In that case, the new helper just returns
> the legacy mapping.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vall...@cloud.com>
> ---
> tools/tests/cpu-policy/test-cpu-policy.c | 128 +++++++++++++++++++++++
> xen/include/xen/lib/x86/cpu-policy.h | 2 +
> xen/lib/x86/policy.c | 75 +++++++++++--
> 3 files changed, 199 insertions(+), 6 deletions(-)
>
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c
> b/tools/tests/cpu-policy/test-cpu-policy.c
> index 301df2c002..6ff5c1dd3d 100644
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -650,6 +650,132 @@ static void test_is_compatible_failure(void)
> }
> }
>
> +static void test_x2apic_id_from_vcpu_id_success(void)
> +{
> + static struct test {
const
> + 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.
> + },
> + },
> + {
> + .name = "6v: 3 t/c, 8 c/s", .vcpu_id = 6, .x2apic_id = 2 << 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, },
> + },
> + },
> + },
> + {
> + .name = "24v: 3 t/c, 8 c/s", .vcpu_id = 24, .x2apic_id = 1 << 5,
> + .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, },
> + },
> + },
> + },
> + {
> + .name = "35v: 3 t/c, 8 c/s", .vcpu_id = 35,
> + .x2apic_id = (35 % 3) | (((35 / 3) % 8) << 2) | ((35 / 24) <<
> 5),
> + .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, },
> + },
> + },
> + },
> + {
> + .name = "96v: 7 t/c, 3 c/s", .vcpu_id = 96,
> + .x2apic_id = (96 % 7) | (((96 / 7) % 3) << 3) | ((96 / 21) <<
> 5),
> + .policy = {
> + .x86_vendor = X86_VENDOR_AMD,
> + .topo.subleaf = {
> + [0] = { .nr_logical = 7, .level = 0, .type = 1,
> .id_shift = 3, },
> + [1] = { .nr_logical = 3, .level = 1, .type = 2,
> .id_shift = 5, },
> + },
> + },
> + },
> + {
> + .name = "3v: 3 t/c, 8 c/s", .vcpu_id = 3, .x2apic_id = 1 << 2,
> + .policy = {
> + .x86_vendor = X86_VENDOR_INTEL,
> + .topo.subleaf = {
> + [0] = { .nr_logical = 3, .level = 0, .type = 1,
> .id_shift = 2, },
> + [1] = { .nr_logical = 24, .level = 1, .type = 2,
> .id_shift = 5, },
> + },
> + },
> + },
> + {
> + .name = "6v: 3 t/c, 8 c/s", .vcpu_id = 6, .x2apic_id = 2 << 2,
> + .policy = {
> + .x86_vendor = X86_VENDOR_INTEL,
> + .topo.subleaf = {
> + [0] = { .nr_logical = 3, .level = 0, .type = 1,
> .id_shift = 2, },
> + [1] = { .nr_logical = 24, .level = 1, .type = 2,
> .id_shift = 5, },
> + },
> + },
> + },
> + {
> + .name = "24v: 3 t/c, 8 c/s", .vcpu_id = 24, .x2apic_id = 1 << 5,
> + .policy = {
> + .x86_vendor = X86_VENDOR_INTEL,
> + .topo.subleaf = {
> + [0] = { .nr_logical = 3, .level = 0, .type = 1,
> .id_shift = 2, },
> + [1] = { .nr_logical = 24, .level = 1, .type = 2,
> .id_shift = 5, },
> + },
> + },
> + },
> + {
> + .name = "35v: 3 t/c, 8 c/s", .vcpu_id = 35,
> + .x2apic_id = (35 % 3) | (((35 / 3) % 8) << 2) | ((35 / 24) <<
> 5),
> + .policy = {
> + .x86_vendor = X86_VENDOR_INTEL,
> + .topo.subleaf = {
> + [0] = { .nr_logical = 3, .level = 0, .type = 1,
> .id_shift = 2, },
> + [1] = { .nr_logical = 24, .level = 1, .type = 2,
> .id_shift = 5, },
> + },
> + },
> + },
> + {
> + .name = "96v: 7 t/c, 3 c/s", .vcpu_id = 96,
> + .x2apic_id = (96 % 7) | (((96 / 7) % 3) << 3) | ((96 / 21) <<
> 5),
> + .policy = {
> + .x86_vendor = X86_VENDOR_INTEL,
> + .topo.subleaf = {
> + [0] = { .nr_logical = 7, .level = 0, .type = 1,
> .id_shift = 3, },
> + [1] = { .nr_logical = 21, .level = 1, .type = 2,
> .id_shift = 5, },
> + },
> + },
> + },
> + };
> +
> + printf("Testing x2apic id from vcpu id success:\n");
> +
> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> + {
> + struct test *t = &tests[i];
const
> + uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(&t->policy,
> t->vcpu_id);
Newline preferably.
> + if ( x2apic_id != t->x2apic_id )
> + fail("FAIL - '%s'. bad x2apic_id: expected=%u actual=%u\n",
> + t->name, t->x2apic_id, x2apic_id);
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> printf("CPU Policy unit tests\n");
> @@ -667,6 +793,8 @@ int main(int argc, char **argv)
> test_is_compatible_success();
> test_is_compatible_failure();
>
> + test_x2apic_id_from_vcpu_id_success();
> +
> if ( nr_failures )
> printf("Done: %u failures\n", nr_failures);
> else
> diff --git a/xen/include/xen/lib/x86/cpu-policy.h
> b/xen/include/xen/lib/x86/cpu-policy.h
> index 65f6335b32..d81ae2f47c 100644
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -550,6 +550,8 @@ int x86_cpu_policies_are_compatible(const struct
> cpu_policy *host,
> /**
> * Calculates the x2APIC ID of a vCPU given a CPU policy
> *
> + * If the policy lacks leaf 0xb falls back to legacy mapping of apic_id=cpu*2
> + *
> * @param p CPU policy of the domain.
> * @param vcpu_id vCPU ID of the vCPU.
> * @returns x2APIC ID of the vCPU.
> 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);
> +}
> +
> +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id)
Can you keep the previous vcpu_id parameter name? Or alternatively
adjust the prototype to also be id.
Thanks, Roger.
