Re: [PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy
On Fri, May 24, 2024 at 06:03:22PM +0100, Alejandro Vallejo wrote:
> On 24/05/2024 09:39, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 01:39:26PM +0100, Alejandro Vallejo wrote:
> >
> > Also you could initialize x2apic_id at definition:
> >
> > const struct test *t = [j];
> > struct cpu_policy policy = { .x86_vendor = vendors[i] };
> > int rc = x86_topo_from_parts(, t->threads_per_core,
> > t->cores_per_pkg);
> > uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(, t->vcpu_id);
>
> Seeing this snippet I just realized there's a bug. The second loop
> should use j rather than i. Ugh.
Well, you shadow the outer variable with the inner one, which makes it
still fine. Yet I don't like that shadowing much. I was going to
comment, but for the requested change you need to not shadow the outer
loop variable (in the example chunk I've used 'j' to signal the outer
loop index).
> >> +}
> >> +
> >> 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..."
Yup, that's clearer :).
Thanks, Roger.
Re: [PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy
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
>> ---
>> 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 = [i];
>> +uint32_t x2apic_id;
>> +int rc = x86_topo_from_parts(, 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 = [j];
> struct cpu_policy policy = { .x86_vendor = vendors[i] };
> int rc = x86_topo_from_parts(, t->threads_per_core, t->cores_per_pkg);
> uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(, 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(, 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
Re: [PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy
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
> ---
> 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 = [i];
> +uint32_t x2apic_id;
> +int rc = x86_topo_from_parts(, 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 = [j];
struct cpu_policy policy = { .x86_vendor = vendors[i] };
int rc = x86_topo_from_parts(, t->threads_per_core, t->cores_per_pkg);
uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(, t->vcpu_id);
> +
> +x2apic_id = x86_x2apic_id_from_vcpu_id(, 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
[PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy
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
---
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),
+},
+};
+
+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] };
+for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+{
+const struct test *t = [i];
+uint32_t x2apic_id;
+int rc = x86_topo_from_parts(, t->threads_per_core,
t->cores_per_pkg);
+
+x2apic_id = x86_x2apic_id_from_vcpu_id(, 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
+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
