Re: [PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, May 08, 2024 at 01:39:26PM +0100, 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.

Using 0x1f and e26 is kind of confusing.  I would word as "0x1f and
extended leaf 0x26" to avoid confusion.

> 
> 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.vallejo@cloud.com>
> ---
> v2:
>   * const-ify the test definitions
>   * Cosmetic changes (newline + parameter name in prototype)
> ---
>  tools/tests/cpu-policy/test-cpu-policy.c | 63 ++++++++++++++++++++
>  xen/include/xen/lib/x86/cpu-policy.h     |  2 +
>  xen/lib/x86/policy.c                     | 73 ++++++++++++++++++++++--
>  3 files changed, 133 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
> index 0ba8c418b1b3..82a6aeb23317 100644
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -776,6 +776,68 @@ static void test_topo_from_parts(void)
>      }
>  }
>  
> +static void test_x2apic_id_from_vcpu_id_success(void)
> +{
> +    static const struct test {
> +        unsigned int vcpu_id;
> +        unsigned int threads_per_core;
> +        unsigned int cores_per_pkg;
> +        uint32_t x2apic_id;
> +        uint8_t x86_vendor;
> +    } tests[] = {
> +        {
> +            .vcpu_id = 3, .threads_per_core = 3, .cores_per_pkg = 8,
> +            .x2apic_id = 1 << 2,
> +        },
> +        {
> +            .vcpu_id = 6, .threads_per_core = 3, .cores_per_pkg = 8,
> +            .x2apic_id = 2 << 2,
> +        },
> +        {
> +            .vcpu_id = 24, .threads_per_core = 3, .cores_per_pkg = 8,
> +            .x2apic_id = 1 << 5,
> +        },
> +        {
> +            .vcpu_id = 35, .threads_per_core = 3, .cores_per_pkg = 8,
> +            .x2apic_id = (35 % 3) | (((35 / 3) % 8)  << 2) | ((35 / 24) << 5),
> +        },
> +        {
> +            .vcpu_id = 96, .threads_per_core = 7, .cores_per_pkg = 3,
> +            .x2apic_id = (96 % 7) | (((96 / 7) % 3)  << 3) | ((96 / 21) << 5),
                                                      ^ extra space (same above)

> +        },
> +    };
> +
> +    const uint8_t vendors[] = {
> +        X86_VENDOR_INTEL,
> +        X86_VENDOR_AMD,
> +        X86_VENDOR_CENTAUR,
> +        X86_VENDOR_SHANGHAI,
> +        X86_VENDOR_HYGON,
> +    };
> +
> +    printf("Testing x2apic id from vcpu id success:\n");
> +
> +    /* Perform the test run on every vendor we know about */
> +    for ( size_t i = 0; i < ARRAY_SIZE(vendors); ++i )
> +    {
> +        struct cpu_policy policy = { .x86_vendor = vendors[i] };

Newline.

> +        for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> +        {
> +            const struct test *t = &tests[i];
> +            uint32_t x2apic_id;
> +            int rc = x86_topo_from_parts(&policy, t->threads_per_core, t->cores_per_pkg);

Overly long line.

Won't it be better to define `policy` in this scope, so that for each
test you start with a clean policy, rather than having leftover data
from the previous test?

Also you could initialize x2apic_id at definition:

const struct test *t = &tests[j];
struct cpu_policy policy = { .x86_vendor = vendors[i] };
int rc = x86_topo_from_parts(&policy, t->threads_per_core, t->cores_per_pkg);
uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(&policy, t->vcpu_id);

> +
> +            x2apic_id = x86_x2apic_id_from_vcpu_id(&policy, t->vcpu_id);
> +            if ( rc || x2apic_id != t->x2apic_id )
> +                fail("FAIL[%d] - '%s cpu%u %u t/c %u c/p'. bad x2apic_id: expected=%u actual=%u\n",
> +                     rc,
> +                     x86_cpuid_vendor_to_str(policy.x86_vendor),
> +                     t->vcpu_id, t->threads_per_core, t->cores_per_pkg,
> +                     t->x2apic_id, x2apic_id);
> +        }
> +    }
> +}
> +
>  int main(int argc, char **argv)
>  {
>      printf("CPU Policy unit tests\n");
> @@ -794,6 +856,7 @@ int main(int argc, char **argv)
>      test_is_compatible_failure();
>  
>      test_topo_from_parts();
> +    test_x2apic_id_from_vcpu_id_success();
>  
>      if ( nr_failures )
>          printf("Done: %u failures\n", nr_failures);
> diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
> index f5df18e9f77c..2cbc2726a861 100644
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -545,6 +545,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 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 d033ee5398dd..e498e32f8fd7 100644
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -2,15 +2,78 @@
>  
>  #include <xen/lib/x86/cpu-policy.h>
>  
> +static uint32_t parts_per_higher_scoped_level(const struct cpu_policy *p, size_t lvl)
> +{
> +    /*
> +     * `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.
> +     */
> +    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;

Line length here and in the function declaration.

> +}
> +
>  uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id)
>  {
> +    uint32_t shift = 0, x2apic_id = 0;
> +
> +    /* In the absence of topology leaves, fallback to traditional mapping */
> +    if ( !p->topo.subleaf[0].type )
> +        return id * 2;
> +
>      /*
> -     * 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.

I'm a bit confused with this, the policy is domain wide, so we will
always need to pass the vCPU ID into x86_x2apic_id_from_vcpu_id()?
IOW: the x2APIC ID will always be derived from the vCPU ID.

Thanks, Roger.