On 24/05/2024 09:39, Roger Pau Monné wrote:
> 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.vall...@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.
Ack
>
>> + 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?
The leftover data is overridden during setup by x86_topo_from_parts(),
but I can see the appeal. Sure.
>
> 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);
Seeing this snippet I just realized there's a bug. The second loop
should use j rather than i. Ugh.
As for the initialization, I want to prevent feeding garbage into
x86_x2apic_id_from_vcpu_id(). For which there's an "if ( !rc )" missing
to gate the call.
I'll sort both of those.
>
>> +
>> + 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.
>
Ack
>> +}
>> +
>> 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.
The x2APIC ID is derived (after the series) from the vCPU ID _and_ the
topology information. The vCPU alone will work out in all cases because
it'll be cached in the vlapic hvm structure.
I guess the comment could be rewritten as "... rather than from the vCPU
ID alone..."
Cheers,
Alejandro
