tools/include/xen-tools/libs.h | 5 ++ tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++ xen/include/xen/lib/x86/cpu-policy.h | 22 +++++ xen/lib/x86/policy.c | 47 ++++++++++ 4 files changed, 224 insertions(+)
Just as with x86_cpu_policies_are_compatible(), make a start on this function
with some token handling.
Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting
properties, and introduce test_calculate_compatible_success() to the unit
tests, covering various cases where the arch_caps CPUID bit falls out, and
with RSBA accumulating rather than intersecting across the two.
Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which
was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for
MSR_ARCH_CAPS".
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
tools/include/xen-tools/libs.h | 5 ++
tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++
xen/include/xen/lib/x86/cpu-policy.h | 22 +++++
xen/lib/x86/policy.c | 47 ++++++++++
4 files changed, 224 insertions(+)
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index a16e0c3807..4de10efdea 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -63,4 +63,9 @@
#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
#endif
+#ifndef _AC
+#define __AC(X, Y) (X ## Y)
+#define _AC(X, Y) __AC(X, Y)
+#endif
+
#endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 75973298df..455b4fe3c0 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
}
}
+static void test_calculate_compatible_success(void)
+{
+ static struct test {
+ const char *name;
+ struct {
+ struct cpuid_policy p;
+ struct msr_policy m;
+ } a, b, out;
+ } tests[] = {
+ {
+ "arch_caps, b short max_leaf",
+ .a = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .b = {
+ .p.basic.max_leaf = 6,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .out = {
+ .p.basic.max_leaf = 6,
+ },
+ },
+ {
+ "arch_caps, b feat missing",
+ .a = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .b = {
+ .p.basic.max_leaf = 7,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .out = {
+ .p.basic.max_leaf = 7,
+ },
+ },
+ {
+ "arch_caps, b rdcl_no missing",
+ .a = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .b = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ },
+ .out = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ },
+ },
+ {
+ "arch_caps, rdcl_no ok",
+ .a = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .b = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ .out = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rdcl_no = true,
+ },
+ },
+ {
+ "arch_caps, rsba accum",
+ .a = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rsba = true,
+ },
+ .b = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ },
+ .out = {
+ .p.basic.max_leaf = 7,
+ .p.feat.arch_caps = true,
+ .m.arch_caps.rsba = true,
+ },
+ },
+ };
+ struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
+
+ printf("Testing calculate compatibility success:\n");
+
+ for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+ {
+ struct test *t = &tests[i];
+ struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy));
+ struct msr_policy *m = malloc(sizeof(struct msr_policy));
+ struct cpu_policy a = {
+ &t->a.p,
+ &t->a.m,
+ }, b = {
+ &t->b.p,
+ &t->b.m,
+ }, out = {
+ p,
+ m,
+ };
+ struct cpu_policy_errors e;
+ int res;
+
+ if ( !p || !m )
+ err(1, "%s() malloc failure", __func__);
+
+ res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e);
+
+ /* Check the expected error output. */
+ if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) )
+ {
+ fail(" Test '%s' expected no errors\n"
+ " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
+ t->name, res, e.leaf, e.subleaf, e.msr);
+ goto test_done;
+ }
+
+ if ( memcmp(&t->out.p, p, sizeof(*p)) )
+ {
+ fail(" Test '%s' resulting CPUID policy not as expected\n",
+ t->name);
+ goto test_done;
+ }
+
+ if ( memcmp(&t->out.m, m, sizeof(*m)) )
+ {
+ fail(" Test '%s' resulting MSR policy not as expected\n",
+ t->name);
+ goto test_done;
+ }
+
+ test_done:
+ free(p);
+ free(m);
+ }
+}
+
int main(int argc, char **argv)
{
printf("CPU Policy unit tests\n");
@@ -793,6 +941,8 @@ int main(int argc, char **argv)
test_is_compatible_success();
test_is_compatible_failure();
+ test_calculate_compatible_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 5a2c4c7b2d..0422a15557 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
const struct cpu_policy *guest,
struct cpu_policy_errors *err);
+/*
+ * Given two policies, calculate one which is compatible with each.
+ *
+ * i.e. Given host @a and host @b, calculate what to give a VM so it can live
+ * migrate between the two.
+ *
+ * @param a A cpu_policy.
+ * @param b Another cpu_policy.
+ * @param out A policy compatible with @a and @b.
+ * @param err Optional hint for error diagnostics.
+ * @returns -errno
+ *
+ * For typical usage, @a and @b should be system policies of the same type
+ * (i.e. PV/HVM default/max) from different hosts. In the case that an
+ * incompatibility is detected, the optional err pointer may identify the
+ * problematic leaf/subleaf and/or MSR.
+ */
+int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
+ const struct cpu_policy *b,
+ struct cpu_policy *out,
+ struct cpu_policy_errors *err);
+
#endif /* !XEN_LIB_X86_POLICIES_H */
/*
diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
index f6cea4e2f9..06039e8aa8 100644
--- a/xen/lib/x86/policy.c
+++ b/xen/lib/x86/policy.c
@@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
+ if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
+ FAIL_MSR(MSR_ARCH_CAPABILITIES);
+
#undef FAIL_MSR
#undef FAIL_CPUID
#undef NA
@@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
return ret;
}
+int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
+ const struct cpu_policy *b,
+ struct cpu_policy *out,
+ struct cpu_policy_errors *err)
+{
+ const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
+ const struct msr_policy *am = a->msr, *bm = b->msr;
+ struct cpuid_policy *cp = out->cpuid;
+ struct msr_policy *mp = out->msr;
+
+ memset(cp, 0, sizeof(*cp));
+ memset(mp, 0, sizeof(*mp));
+
+ cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
+
+ if ( cp->basic.max_leaf >= 7 )
+ {
+ cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf);
+
+ cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b;
+ cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c;
+ cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d;
+ }
+
+ /* TODO: Far more. */
+
+ mp->platform_info.raw = am->platform_info.raw & bm->platform_info.raw;
+
+ if ( cp->feat.arch_caps )
+ {
+ /*
+ * RSBA means "RSB Alternative", i.e. RSB stuffing not necesserily
+ * safe. It needs to accumulate rather than intersect across a
+ * resource pool.
+ */
+#define POL_MASK ARCH_CAPS_RSBA
+ mp->arch_caps.raw = ((am->arch_caps.raw ^ POL_MASK) &
+ (bm->arch_caps.raw ^ POL_MASK)) ^ POL_MASK;
+#undef POL_MASK
+ }
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
--
2.11.0
On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote:
> Just as with x86_cpu_policies_are_compatible(), make a start on this function
> with some token handling.
>
> Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting
> properties, and introduce test_calculate_compatible_success() to the unit
> tests, covering various cases where the arch_caps CPUID bit falls out, and
> with RSBA accumulating rather than intersecting across the two.
>
> Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which
> was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for
> MSR_ARCH_CAPS".
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
> tools/include/xen-tools/libs.h | 5 ++
> tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++
> xen/include/xen/lib/x86/cpu-policy.h | 22 +++++
> xen/lib/x86/policy.c | 47 ++++++++++
> 4 files changed, 224 insertions(+)
>
> diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
> index a16e0c3807..4de10efdea 100644
> --- a/tools/include/xen-tools/libs.h
> +++ b/tools/include/xen-tools/libs.h
> @@ -63,4 +63,9 @@
> #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
> #endif
>
> +#ifndef _AC
> +#define __AC(X, Y) (X ## Y)
> +#define _AC(X, Y) __AC(X, Y)
> +#endif
You need to remove those definitions from libxl_internal.h.
> #endif /* __XEN_TOOLS_LIBS__ */
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
> index 75973298df..455b4fe3c0 100644
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
> }
> }
>
> +static void test_calculate_compatible_success(void)
> +{
> + static struct test {
> + const char *name;
> + struct {
> + struct cpuid_policy p;
> + struct msr_policy m;
> + } a, b, out;
> + } tests[] = {
> + {
> + "arch_caps, b short max_leaf",
> + .a = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .b = {
> + .p.basic.max_leaf = 6,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .out = {
> + .p.basic.max_leaf = 6,
> + },
> + },
> + {
> + "arch_caps, b feat missing",
> + .a = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .b = {
> + .p.basic.max_leaf = 7,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .out = {
> + .p.basic.max_leaf = 7,
> + },
> + },
> + {
> + "arch_caps, b rdcl_no missing",
> + .a = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .b = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + },
> + .out = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + },
> + },
> + {
> + "arch_caps, rdcl_no ok",
> + .a = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .b = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .out = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + },
> + {
> + "arch_caps, rsba accum",
> + .a = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rsba = true,
> + },
> + .b = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + },
> + .out = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rsba = true,
> + },
> + },
> + };
> + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
> +
> + printf("Testing calculate compatibility success:\n");
> +
> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> + {
> + struct test *t = &tests[i];
> + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy));
> + struct msr_policy *m = malloc(sizeof(struct msr_policy));
> + struct cpu_policy a = {
> + &t->a.p,
> + &t->a.m,
> + }, b = {
> + &t->b.p,
> + &t->b.m,
> + }, out = {
> + p,
> + m,
> + };
> + struct cpu_policy_errors e;
> + int res;
> +
> + if ( !p || !m )
> + err(1, "%s() malloc failure", __func__);
> +
> + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e);
> +
> + /* Check the expected error output. */
> + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) )
> + {
> + fail(" Test '%s' expected no errors\n"
> + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> + t->name, res, e.leaf, e.subleaf, e.msr);
> + goto test_done;
> + }
> +
> + if ( memcmp(&t->out.p, p, sizeof(*p)) )
> + {
> + fail(" Test '%s' resulting CPUID policy not as expected\n",
> + t->name);
> + goto test_done;
> + }
> +
> + if ( memcmp(&t->out.m, m, sizeof(*m)) )
> + {
> + fail(" Test '%s' resulting MSR policy not as expected\n",
> + t->name);
> + goto test_done;
> + }
In order to assert that we don't mess things up, I would also add a
x86_cpu_policies_are_compatible check here:
res = x86_cpu_policies_are_compatible(&a, &out, &e);
if ( res )
fail(" Test '%s' created incompatible policy\n"
" got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
t->name, res, e.leaf, e.subleaf, e.msr);
res = x86_cpu_policies_are_compatible(&b, &out, &e);
if ( res )
fail(" Test '%s' created incompatible policy\n"
" got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
t->name, res, e.leaf, e.subleaf, e.msr);
> +
> + test_done:
> + free(p);
> + free(m);
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> printf("CPU Policy unit tests\n");
> @@ -793,6 +941,8 @@ int main(int argc, char **argv)
> test_is_compatible_success();
> test_is_compatible_failure();
>
> + test_calculate_compatible_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 5a2c4c7b2d..0422a15557 100644
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> const struct cpu_policy *guest,
> struct cpu_policy_errors *err);
>
> +/*
> + * Given two policies, calculate one which is compatible with each.
> + *
> + * i.e. Given host @a and host @b, calculate what to give a VM so it can live
> + * migrate between the two.
> + *
> + * @param a A cpu_policy.
> + * @param b Another cpu_policy.
> + * @param out A policy compatible with @a and @b.
> + * @param err Optional hint for error diagnostics.
> + * @returns -errno
> + *
> + * For typical usage, @a and @b should be system policies of the same type
> + * (i.e. PV/HVM default/max) from different hosts. In the case that an
> + * incompatibility is detected, the optional err pointer may identify the
> + * problematic leaf/subleaf and/or MSR.
> + */
> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
> + const struct cpu_policy *b,
> + struct cpu_policy *out,
> + struct cpu_policy_errors *err);
> +
> #endif /* !XEN_LIB_X86_POLICIES_H */
>
> /*
> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
> index f6cea4e2f9..06039e8aa8 100644
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>
> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
It might be nice to expand test_is_compatible_{success,failure} to
account for arch_caps being checked now.
Shouldn't this check also take into account that host might not have
RSBA set, but it's legit for a guest policy to have it?
if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK )
FAIL_MSR(MSR_ARCH_CAPABILITIES);
Maybe POL_MASK should be renamed and defined in a header so it's
widely available?
> +
> #undef FAIL_MSR
> #undef FAIL_CPUID
> #undef NA
> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> return ret;
> }
>
> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
> + const struct cpu_policy *b,
> + struct cpu_policy *out,
> + struct cpu_policy_errors *err)
I think this should be in an #ifndef __XEN__ protected region?
There's no need to expose this to the hypervisor, as I would expect it
will never have to do compatible policy generation? (ie: it will
always be done by the toolstack?)
> +{
> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
> + const struct msr_policy *am = a->msr, *bm = b->msr;
> + struct cpuid_policy *cp = out->cpuid;
> + struct msr_policy *mp = out->msr;
> +
> + memset(cp, 0, sizeof(*cp));
> + memset(mp, 0, sizeof(*mp));
> +
> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
> +
> + if ( cp->basic.max_leaf >= 7 )
> + {
> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf);
> +
> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b;
> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c;
> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d;
> + }
> +
> + /* TODO: Far more. */
Right, my proposed patch (07/13) went a bit further and also leveled
1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level
a couple of max_leaf fields.
I'm happy for this to go in first, and I can rebase the extra logic I
have on top of this one.
Thanks, Roger.
On 05/05/2021 11:04, Roger Pau Monné wrote:
> On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote:
>> Just as with x86_cpu_policies_are_compatible(), make a start on this function
>> with some token handling.
>>
>> Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting
>> properties, and introduce test_calculate_compatible_success() to the unit
>> tests, covering various cases where the arch_caps CPUID bit falls out, and
>> with RSBA accumulating rather than intersecting across the two.
>>
>> Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which
>> was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for
>> MSR_ARCH_CAPS".
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> ---
>> tools/include/xen-tools/libs.h | 5 ++
>> tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++
>> xen/include/xen/lib/x86/cpu-policy.h | 22 +++++
>> xen/lib/x86/policy.c | 47 ++++++++++
>> 4 files changed, 224 insertions(+)
>>
>> diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
>> index a16e0c3807..4de10efdea 100644
>> --- a/tools/include/xen-tools/libs.h
>> +++ b/tools/include/xen-tools/libs.h
>> @@ -63,4 +63,9 @@
>> #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
>> #endif
>>
>> +#ifndef _AC
>> +#define __AC(X, Y) (X ## Y)
>> +#define _AC(X, Y) __AC(X, Y)
>> +#endif
> You need to remove those definitions from libxl_internal.h.
Ok.
>
>> #endif /* __XEN_TOOLS_LIBS__ */
>> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
>> index 75973298df..455b4fe3c0 100644
>> --- a/tools/tests/cpu-policy/test-cpu-policy.c
>> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
>> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
>> }
>> }
>>
>> +static void test_calculate_compatible_success(void)
>> +{
>> + static struct test {
>> + const char *name;
>> + struct {
>> + struct cpuid_policy p;
>> + struct msr_policy m;
>> + } a, b, out;
>> + } tests[] = {
>> + {
>> + "arch_caps, b short max_leaf",
>> + .a = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .b = {
>> + .p.basic.max_leaf = 6,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .out = {
>> + .p.basic.max_leaf = 6,
>> + },
>> + },
>> + {
>> + "arch_caps, b feat missing",
>> + .a = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .b = {
>> + .p.basic.max_leaf = 7,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .out = {
>> + .p.basic.max_leaf = 7,
>> + },
>> + },
>> + {
>> + "arch_caps, b rdcl_no missing",
>> + .a = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .b = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + },
>> + .out = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + },
>> + },
>> + {
>> + "arch_caps, rdcl_no ok",
>> + .a = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .b = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .out = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + },
>> + {
>> + "arch_caps, rsba accum",
>> + .a = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rsba = true,
>> + },
>> + .b = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + },
>> + .out = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rsba = true,
>> + },
>> + },
>> + };
>> + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
>> +
>> + printf("Testing calculate compatibility success:\n");
>> +
>> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
>> + {
>> + struct test *t = &tests[i];
>> + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy));
>> + struct msr_policy *m = malloc(sizeof(struct msr_policy));
>> + struct cpu_policy a = {
>> + &t->a.p,
>> + &t->a.m,
>> + }, b = {
>> + &t->b.p,
>> + &t->b.m,
>> + }, out = {
>> + p,
>> + m,
>> + };
>> + struct cpu_policy_errors e;
>> + int res;
>> +
>> + if ( !p || !m )
>> + err(1, "%s() malloc failure", __func__);
>> +
>> + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e);
>> +
>> + /* Check the expected error output. */
>> + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) )
>> + {
>> + fail(" Test '%s' expected no errors\n"
>> + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
>> + t->name, res, e.leaf, e.subleaf, e.msr);
>> + goto test_done;
>> + }
>> +
>> + if ( memcmp(&t->out.p, p, sizeof(*p)) )
>> + {
>> + fail(" Test '%s' resulting CPUID policy not as expected\n",
>> + t->name);
>> + goto test_done;
>> + }
>> +
>> + if ( memcmp(&t->out.m, m, sizeof(*m)) )
>> + {
>> + fail(" Test '%s' resulting MSR policy not as expected\n",
>> + t->name);
>> + goto test_done;
>> + }
> In order to assert that we don't mess things up, I would also add a
> x86_cpu_policies_are_compatible check here:
>
> res = x86_cpu_policies_are_compatible(&a, &out, &e);
> if ( res )
> fail(" Test '%s' created incompatible policy\n"
> " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> t->name, res, e.leaf, e.subleaf, e.msr);
> res = x86_cpu_policies_are_compatible(&b, &out, &e);
> if ( res )
> fail(" Test '%s' created incompatible policy\n"
> " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> t->name, res, e.leaf, e.subleaf, e.msr);
That's potentially problematic, hence not including it the first time
around. See the discussion below.
>> +
>> + test_done:
>> + free(p);
>> + free(m);
>> + }
>> +}
>> +
>> int main(int argc, char **argv)
>> {
>> printf("CPU Policy unit tests\n");
>> @@ -793,6 +941,8 @@ int main(int argc, char **argv)
>> test_is_compatible_success();
>> test_is_compatible_failure();
>>
>> + test_calculate_compatible_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 5a2c4c7b2d..0422a15557 100644
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> const struct cpu_policy *guest,
>> struct cpu_policy_errors *err);
>>
>> +/*
>> + * Given two policies, calculate one which is compatible with each.
>> + *
>> + * i.e. Given host @a and host @b, calculate what to give a VM so it can live
>> + * migrate between the two.
>> + *
>> + * @param a A cpu_policy.
>> + * @param b Another cpu_policy.
>> + * @param out A policy compatible with @a and @b.
>> + * @param err Optional hint for error diagnostics.
>> + * @returns -errno
>> + *
>> + * For typical usage, @a and @b should be system policies of the same type
>> + * (i.e. PV/HVM default/max) from different hosts. In the case that an
>> + * incompatibility is detected, the optional err pointer may identify the
>> + * problematic leaf/subleaf and/or MSR.
>> + */
>> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
>> + const struct cpu_policy *b,
>> + struct cpu_policy *out,
>> + struct cpu_policy_errors *err);
>> +
>> #endif /* !XEN_LIB_X86_POLICIES_H */
>>
>> /*
>> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
>> index f6cea4e2f9..06039e8aa8 100644
>> --- a/xen/lib/x86/policy.c
>> +++ b/xen/lib/x86/policy.c
>> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>>
>> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
>> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
> It might be nice to expand test_is_compatible_{success,failure} to
> account for arch_caps being checked now.
At some point we're going to need to stop unit testing "does the AND
operator work", and limit testing to the interesting corner cases.
> Shouldn't this check also take into account that host might not have
> RSBA set, but it's legit for a guest policy to have it?
When we expose this properly to guests, the max policies will have RSBA
set, and the default policies will have RSBA forwarded from hardware
and/or the model table.
Therefore, we can accept any VM RSBA configuration, irrespective of the
particulars of this host, but if you e.g. have a pool of haswell's, the
default policy will have RSBA clear across the board, and the VM won't
see it.
> if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK )
> FAIL_MSR(MSR_ARCH_CAPABILITIES);
>
> Maybe POL_MASK should be renamed and defined in a header so it's
> widely available?
No - this would be incorrect. The polarity of certain bits only matters
for levelling calculations, not for "can this VM run on this host"
calculations.
If the VM has seen RSBA, and Xen doesn't know about it, the VM cannot run.
>
>> +
>> #undef FAIL_MSR
>> #undef FAIL_CPUID
>> #undef NA
>> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> return ret;
>> }
>>
>> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
>> + const struct cpu_policy *b,
>> + struct cpu_policy *out,
>> + struct cpu_policy_errors *err)
> I think this should be in an #ifndef __XEN__ protected region?
>
> There's no need to expose this to the hypervisor, as I would expect it
> will never have to do compatible policy generation? (ie: it will
> always be done by the toolstack?)
As indicated previously, I still think we want this in Xen for the boot
paths, but I suppose the guard was my suggestion to you, so is only fair
at this point.
>
>> +{
>> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
>> + const struct msr_policy *am = a->msr, *bm = b->msr;
>> + struct cpuid_policy *cp = out->cpuid;
>> + struct msr_policy *mp = out->msr;
>> +
>> + memset(cp, 0, sizeof(*cp));
>> + memset(mp, 0, sizeof(*mp));
>> +
>> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
>> +
>> + if ( cp->basic.max_leaf >= 7 )
>> + {
>> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf);
>> +
>> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b;
>> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c;
>> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d;
>> + }
>> +
>> + /* TODO: Far more. */
> Right, my proposed patch (07/13) went a bit further and also leveled
> 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level
> a couple of max_leaf fields.
>
> I'm happy for this to go in first, and I can rebase the extra logic I
> have on top of this one.
There is a lot of work to do.
One thing I haven't addressed yet is the fact is things which don't
level, e.g. vendor. You've got to pick one, and there isn't a
mathematical relationship to use between a and b.
I think for that, we ought to document that we strictly take from a.
This makes the operation not commutative, and in particular, I don't
think we want to waste too much time/effort trying to make cross-vendor
cases work - it was a stunt a decade ago, with a huge number of sharp
corners, as well as creating a number of XSAs due to poor implementation.
For v1, I suggest we firmly stick to the same-vendor case. It's not as
if there is a lack of things to do to make this work.
~Andrew
On Wed, May 05, 2021 at 01:37:48PM +0100, Andrew Cooper wrote:
> On 05/05/2021 11:04, Roger Pau Monné wrote:
> > On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote:
> >> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
> >> index 75973298df..455b4fe3c0 100644
> >> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> >> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> >> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
> >> }
> >> }
> >>
> >> +static void test_calculate_compatible_success(void)
> >> +{
> >> + static struct test {
> >> + const char *name;
> >> + struct {
> >> + struct cpuid_policy p;
> >> + struct msr_policy m;
> >> + } a, b, out;
> >> + } tests[] = {
> >> + {
> >> + "arch_caps, b short max_leaf",
> >> + .a = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .b = {
> >> + .p.basic.max_leaf = 6,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .out = {
> >> + .p.basic.max_leaf = 6,
> >> + },
> >> + },
> >> + {
> >> + "arch_caps, b feat missing",
> >> + .a = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .b = {
> >> + .p.basic.max_leaf = 7,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .out = {
> >> + .p.basic.max_leaf = 7,
> >> + },
> >> + },
> >> + {
> >> + "arch_caps, b rdcl_no missing",
> >> + .a = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .b = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + },
> >> + .out = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + },
> >> + },
> >> + {
> >> + "arch_caps, rdcl_no ok",
> >> + .a = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .b = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + .out = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rdcl_no = true,
> >> + },
> >> + },
> >> + {
> >> + "arch_caps, rsba accum",
> >> + .a = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rsba = true,
> >> + },
> >> + .b = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + },
> >> + .out = {
> >> + .p.basic.max_leaf = 7,
> >> + .p.feat.arch_caps = true,
> >> + .m.arch_caps.rsba = true,
> >> + },
> >> + },
> >> + };
> >> + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
> >> +
> >> + printf("Testing calculate compatibility success:\n");
> >> +
> >> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> >> + {
> >> + struct test *t = &tests[i];
> >> + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy));
> >> + struct msr_policy *m = malloc(sizeof(struct msr_policy));
> >> + struct cpu_policy a = {
> >> + &t->a.p,
> >> + &t->a.m,
> >> + }, b = {
> >> + &t->b.p,
> >> + &t->b.m,
> >> + }, out = {
> >> + p,
> >> + m,
> >> + };
> >> + struct cpu_policy_errors e;
> >> + int res;
> >> +
> >> + if ( !p || !m )
> >> + err(1, "%s() malloc failure", __func__);
> >> +
> >> + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e);
> >> +
> >> + /* Check the expected error output. */
> >> + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) )
> >> + {
> >> + fail(" Test '%s' expected no errors\n"
> >> + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> >> + t->name, res, e.leaf, e.subleaf, e.msr);
> >> + goto test_done;
> >> + }
> >> +
> >> + if ( memcmp(&t->out.p, p, sizeof(*p)) )
> >> + {
> >> + fail(" Test '%s' resulting CPUID policy not as expected\n",
> >> + t->name);
> >> + goto test_done;
> >> + }
> >> +
> >> + if ( memcmp(&t->out.m, m, sizeof(*m)) )
> >> + {
> >> + fail(" Test '%s' resulting MSR policy not as expected\n",
> >> + t->name);
> >> + goto test_done;
> >> + }
> > In order to assert that we don't mess things up, I would also add a
> > x86_cpu_policies_are_compatible check here:
> >
> > res = x86_cpu_policies_are_compatible(&a, &out, &e);
> > if ( res )
> > fail(" Test '%s' created incompatible policy\n"
> > " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> > t->name, res, e.leaf, e.subleaf, e.msr);
> > res = x86_cpu_policies_are_compatible(&b, &out, &e);
> > if ( res )
> > fail(" Test '%s' created incompatible policy\n"
> > " got res %d { leaf %08x, subleaf %08x, msr %08x }\n",
> > t->name, res, e.leaf, e.subleaf, e.msr);
>
> That's potentially problematic, hence not including it the first time
> around. See the discussion below.
Right, I think being able to do what I propose would also allow to
detect missing bits between x86_cpu_policy_calculate_compatible and
x86_cpu_policies_are_compatible.
> >> +
> >> + test_done:
> >> + free(p);
> >> + free(m);
> >> + }
> >> +}
> >> +
> >> int main(int argc, char **argv)
> >> {
> >> printf("CPU Policy unit tests\n");
> >> @@ -793,6 +941,8 @@ int main(int argc, char **argv)
> >> test_is_compatible_success();
> >> test_is_compatible_failure();
> >>
> >> + test_calculate_compatible_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 5a2c4c7b2d..0422a15557 100644
> >> --- a/xen/include/xen/lib/x86/cpu-policy.h
> >> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> >> @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> >> const struct cpu_policy *guest,
> >> struct cpu_policy_errors *err);
> >>
> >> +/*
> >> + * Given two policies, calculate one which is compatible with each.
> >> + *
> >> + * i.e. Given host @a and host @b, calculate what to give a VM so it can live
> >> + * migrate between the two.
> >> + *
> >> + * @param a A cpu_policy.
> >> + * @param b Another cpu_policy.
> >> + * @param out A policy compatible with @a and @b.
> >> + * @param err Optional hint for error diagnostics.
> >> + * @returns -errno
> >> + *
> >> + * For typical usage, @a and @b should be system policies of the same type
> >> + * (i.e. PV/HVM default/max) from different hosts. In the case that an
> >> + * incompatibility is detected, the optional err pointer may identify the
> >> + * problematic leaf/subleaf and/or MSR.
> >> + */
> >> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
> >> + const struct cpu_policy *b,
> >> + struct cpu_policy *out,
> >> + struct cpu_policy_errors *err);
> >> +
> >> #endif /* !XEN_LIB_X86_POLICIES_H */
> >>
> >> /*
> >> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
> >> index f6cea4e2f9..06039e8aa8 100644
> >> --- a/xen/lib/x86/policy.c
> >> +++ b/xen/lib/x86/policy.c
> >> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> >> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
> >> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
> >>
> >> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
> >> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
> > It might be nice to expand test_is_compatible_{success,failure} to
> > account for arch_caps being checked now.
>
> At some point we're going to need to stop unit testing "does the AND
> operator work", and limit testing to the interesting corner cases.
>
> > Shouldn't this check also take into account that host might not have
> > RSBA set, but it's legit for a guest policy to have it?
>
> When we expose this properly to guests, the max policies will have RSBA
> set, and the default policies will have RSBA forwarded from hardware
> and/or the model table.
>
> Therefore, we can accept any VM RSBA configuration, irrespective of the
> particulars of this host, but if you e.g. have a pool of haswell's, the
> default policy will have RSBA clear across the board, and the VM won't
> see it.
>
> > if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK )
> > FAIL_MSR(MSR_ARCH_CAPABILITIES);
> >
> > Maybe POL_MASK should be renamed and defined in a header so it's
> > widely available?
>
> No - this would be incorrect. The polarity of certain bits only matters
> for levelling calculations, not for "can this VM run on this host"
> calculations.
>
> If the VM has seen RSBA, and Xen doesn't know about it, the VM cannot run.
But then the logic relation between
x86_cpu_policy_calculate_compatible and
x86_cpu_policies_are_compatible is broken AFAICT.
If you give x86_cpu_policy_calculate_compatible one policy with RSBA set
and one without it will generate a compatible policy, yet that output
will be regarded as not compatible if feed into
x86_cpu_policies_are_compatible against the policy that doesn't have
RSBA set.
I think the output from x86_cpu_policy_calculate_compatible should
strictly return true when checked against any of the inputs using
x86_cpu_policies_are_compatible, or else we need to note it somewhere
because I think it's not the expected behavior.
> >
> >> +
> >> #undef FAIL_MSR
> >> #undef FAIL_CPUID
> >> #undef NA
> >> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> >> return ret;
> >> }
> >>
> >> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
> >> + const struct cpu_policy *b,
> >> + struct cpu_policy *out,
> >> + struct cpu_policy_errors *err)
> > I think this should be in an #ifndef __XEN__ protected region?
> >
> > There's no need to expose this to the hypervisor, as I would expect it
> > will never have to do compatible policy generation? (ie: it will
> > always be done by the toolstack?)
>
> As indicated previously, I still think we want this in Xen for the boot
> paths, but I suppose the guard was my suggestion to you, so is only fair
> at this point.
TBH I replied before seeing your email that also had this suggestion.
If it's indeed going to be used by Xen itself then that's fine, but I
couldn't figure out why the hypervisor would need to generate
compatible policies itself.
Maybe it will be used to generate the initial policies?
> >
> >> +{
> >> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
> >> + const struct msr_policy *am = a->msr, *bm = b->msr;
> >> + struct cpuid_policy *cp = out->cpuid;
> >> + struct msr_policy *mp = out->msr;
> >> +
> >> + memset(cp, 0, sizeof(*cp));
> >> + memset(mp, 0, sizeof(*mp));
> >> +
> >> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
> >> +
> >> + if ( cp->basic.max_leaf >= 7 )
> >> + {
> >> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf);
> >> +
> >> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b;
> >> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c;
> >> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d;
> >> + }
> >> +
> >> + /* TODO: Far more. */
> > Right, my proposed patch (07/13) went a bit further and also leveled
> > 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level
> > a couple of max_leaf fields.
> >
> > I'm happy for this to go in first, and I can rebase the extra logic I
> > have on top of this one.
>
> There is a lot of work to do.
>
> One thing I haven't addressed yet is the fact is things which don't
> level, e.g. vendor. You've got to pick one, and there isn't a
> mathematical relationship to use between a and b.
>
> I think for that, we ought to document that we strictly take from a.
> This makes the operation not commutative, and in particular, I don't
> think we want to waste too much time/effort trying to make cross-vendor
> cases work - it was a stunt a decade ago, with a huge number of sharp
> corners, as well as creating a number of XSAs due to poor implementation.
>
> For v1, I suggest we firmly stick to the same-vendor case. It's not as
> if there is a lack of things to do to make this work.
OK, so level all the feature fields and pick the non feature parts of
cpuid strictly from one of the inputs.
Thanks, Roger.
On 05/05/2021 14:02, Roger Pau Monné wrote:
> On Wed, May 05, 2021 at 01:37:48PM +0100, Andrew Cooper wrote:
>> On 05/05/2021 11:04, Roger Pau Monné wrote:
>>> On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote:
>>>> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
>>>> index f6cea4e2f9..06039e8aa8 100644
>>>> --- a/xen/lib/x86/policy.c
>>>> +++ b/xen/lib/x86/policy.c
>>>> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>>> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>>>> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>>>>
>>>> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
>>>> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
>>> It might be nice to expand test_is_compatible_{success,failure} to
>>> account for arch_caps being checked now.
>> At some point we're going to need to stop unit testing "does the AND
>> operator work", and limit testing to the interesting corner cases.
>>
>>> Shouldn't this check also take into account that host might not have
>>> RSBA set, but it's legit for a guest policy to have it?
>> When we expose this properly to guests, the max policies will have RSBA
>> set, and the default policies will have RSBA forwarded from hardware
>> and/or the model table.
>>
>> Therefore, we can accept any VM RSBA configuration, irrespective of the
>> particulars of this host, but if you e.g. have a pool of haswell's, the
>> default policy will have RSBA clear across the board, and the VM won't
>> see it.
>>
>>> if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK )
>>> FAIL_MSR(MSR_ARCH_CAPABILITIES);
>>>
>>> Maybe POL_MASK should be renamed and defined in a header so it's
>>> widely available?
>> No - this would be incorrect. The polarity of certain bits only matters
>> for levelling calculations, not for "can this VM run on this host"
>> calculations.
>>
>> If the VM has seen RSBA, and Xen doesn't know about it, the VM cannot run.
> But then the logic relation between
> x86_cpu_policy_calculate_compatible and
> x86_cpu_policies_are_compatible is broken AFAICT.
>
> If you give x86_cpu_policy_calculate_compatible one policy with RSBA set
> and one without it will generate a compatible policy, yet that output
> will be regarded as not compatible if feed into
> x86_cpu_policies_are_compatible against the policy that doesn't have
> RSBA set.
>
> I think the output from x86_cpu_policy_calculate_compatible should
> strictly return true when checked against any of the inputs using
> x86_cpu_policies_are_compatible, or else we need to note it somewhere
> because I think it's not the expected behavior.
Welcome to the monumental complexity, and the reason why this isn't 5
minutes of work. This is just the tip of the iceberg.
"Please create me a policy for a VM" is conducted across PV/HVM default
policies, and/or user settings, while "Can this VM run on this host" is
checked against the max policy. This split is necessary to cope with
the corner cases.
So no - levelling max policies isn't expected to result in anything
useful, and calling is_compatible with a default (rather than max) host
setting also isn't going result in a useful answer.
And yes - for some changes, RSBA included, you're going to need to
update all your Xen's across the pool before migration is going to work,
but that's already the case now.
Tangentially, we haven't started yet on
struct irritating_corner_cases {
bool vm_not_using_fcs_fds;
bool vm_not_using_lbr;
...
};
which will require explicit user opt-in to override the "No - you can't
migration across the IvyBridge/Haswell, or pre-Zen/Zen boundary".
Technically, MCXSR_MASK is also a hard blocker to migration, but we
don't even have that data in a consumable form, and we just might be
extremely lucky and discover that it is restricted to non-64-bit CPUs.
Migration with a VM having turned on LBR is still a disaster. For now,
we drop everything on the floor, and let the VM explode if the
LBR_FORMAT has changed, or if the number of stack entries changes (which
does change with Hyperthreading enabled/disabled in firmware).
>>>> +
>>>> #undef FAIL_MSR
>>>> #undef FAIL_CPUID
>>>> #undef NA
>>>> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>>> return ret;
>>>> }
>>>>
>>>> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
>>>> + const struct cpu_policy *b,
>>>> + struct cpu_policy *out,
>>>> + struct cpu_policy_errors *err)
>>> I think this should be in an #ifndef __XEN__ protected region?
>>>
>>> There's no need to expose this to the hypervisor, as I would expect it
>>> will never have to do compatible policy generation? (ie: it will
>>> always be done by the toolstack?)
>> As indicated previously, I still think we want this in Xen for the boot
>> paths, but I suppose the guard was my suggestion to you, so is only fair
>> at this point.
> TBH I replied before seeing your email that also had this suggestion.
> If it's indeed going to be used by Xen itself then that's fine, but I
> couldn't figure out why the hypervisor would need to generate
> compatible policies itself.
>
> Maybe it will be used to generate the initial policies?
Yes.
>
>>>> +{
>>>> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
>>>> + const struct msr_policy *am = a->msr, *bm = b->msr;
>>>> + struct cpuid_policy *cp = out->cpuid;
>>>> + struct msr_policy *mp = out->msr;
>>>> +
>>>> + memset(cp, 0, sizeof(*cp));
>>>> + memset(mp, 0, sizeof(*mp));
>>>> +
>>>> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
>>>> +
>>>> + if ( cp->basic.max_leaf >= 7 )
>>>> + {
>>>> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf);
>>>> +
>>>> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b;
>>>> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c;
>>>> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d;
>>>> + }
>>>> +
>>>> + /* TODO: Far more. */
>>> Right, my proposed patch (07/13) went a bit further and also leveled
>>> 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level
>>> a couple of max_leaf fields.
>>>
>>> I'm happy for this to go in first, and I can rebase the extra logic I
>>> have on top of this one.
>> There is a lot of work to do.
>>
>> One thing I haven't addressed yet is the fact is things which don't
>> level, e.g. vendor. You've got to pick one, and there isn't a
>> mathematical relationship to use between a and b.
>>
>> I think for that, we ought to document that we strictly take from a.
>> This makes the operation not commutative, and in particular, I don't
>> think we want to waste too much time/effort trying to make cross-vendor
>> cases work - it was a stunt a decade ago, with a huge number of sharp
>> corners, as well as creating a number of XSAs due to poor implementation.
>>
>> For v1, I suggest we firmly stick to the same-vendor case. It's not as
>> if there is a lack of things to do to make this work.
> OK, so level all the feature fields and pick the non feature parts of
> cpuid strictly from one of the inputs.
The awkward part to address is that we've still got simultaneous
equations with feature handling. I'm fairly certain that the simple
and's which both you and I did won't be sufficient in due course.
~Andrew
On 05.05.2021 16:29, Andrew Cooper wrote:
> Technically, MCXSR_MASK is also a hard blocker to migration, but we
> don't even have that data in a consumable form, and we just might be
> extremely lucky and discover that it is restricted to non-64-bit CPUs.
"it" being what here? The value's presence / absence in an {F,}XSAVE
image? Or the precise value of it?
Jan
On 05/05/2021 15:48, Jan Beulich wrote:
> On 05.05.2021 16:29, Andrew Cooper wrote:
>> Technically, MCXSR_MASK is also a hard blocker to migration, but we
>> don't even have that data in a consumable form, and we just might be
>> extremely lucky and discover that it is restricted to non-64-bit CPUs.
> "it" being what here? The value's presence / absence in an {F,}XSAVE
> image? Or the precise value of it?
The precise value of it. Migrating across the boundary where the
default changed will cause {F,}RSTOR instructions to #GP.
~Andrew
On 05.05.2021 16:50, Andrew Cooper wrote:
> On 05/05/2021 15:48, Jan Beulich wrote:
>> On 05.05.2021 16:29, Andrew Cooper wrote:
>>> Technically, MCXSR_MASK is also a hard blocker to migration, but we
>>> don't even have that data in a consumable form, and we just might be
>>> extremely lucky and discover that it is restricted to non-64-bit CPUs.
>> "it" being what here? The value's presence / absence in an {F,}XSAVE
>> image? Or the precise value of it?
>
> The precise value of it.
Not sure whether DAZ is new enough, but relatively sure MM isn't.
> Migrating across the boundary where the
> default changed will cause {F,}RSTOR instructions to #GP.
That's understood. Is there actually anything standing in the way
of treating MXCSR_MASK like yet another feature flag group?
Jan
On 05/05/2021 16:00, Jan Beulich wrote:
> On 05.05.2021 16:50, Andrew Cooper wrote:
>> On 05/05/2021 15:48, Jan Beulich wrote:
>>> On 05.05.2021 16:29, Andrew Cooper wrote:
>>>> Technically, MCXSR_MASK is also a hard blocker to migration, but we
>>>> don't even have that data in a consumable form, and we just might be
>>>> extremely lucky and discover that it is restricted to non-64-bit CPUs.
>>> "it" being what here? The value's presence / absence in an {F,}XSAVE
>>> image? Or the precise value of it?
>> The precise value of it.
> Not sure whether DAZ is new enough, but relatively sure MM isn't.
>
>> Migrating across the boundary where the
>> default changed will cause {F,}RSTOR instructions to #GP.
> That's understood. Is there actually anything standing in the way
> of treating MXCSR_MASK like yet another feature flag group?
Well - we'd need to find somewhere to put it. It doesn't fit in
architectural CPUID or MSR information.
We could add a 3rd category of information in "a cpu policy", and that
is always an option.
However, this issue hasn't been a problem so far, is only for very
legacy CPUs at this point, and isn't high on my list of worries.
~Andrew
On 04.05.2021 23:31, Andrew Cooper wrote:
> --- a/tools/include/xen-tools/libs.h
> +++ b/tools/include/xen-tools/libs.h
> @@ -63,4 +63,9 @@
> #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
> #endif
>
> +#ifndef _AC
> +#define __AC(X, Y) (X ## Y)
> +#define _AC(X, Y) __AC(X, Y)
> +#endif
Somewhere in Roger's recent / pending work I recall he moved these
from somewhere, instead of adding new instances.
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
> }
> }
>
> +static void test_calculate_compatible_success(void)
> +{
> + static struct test {
> + const char *name;
> + struct {
> + struct cpuid_policy p;
> + struct msr_policy m;
> + } a, b, out;
> + } tests[] = {
> + {
> + "arch_caps, b short max_leaf",
> + .a = {
> + .p.basic.max_leaf = 7,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
> + },
> + .b = {
> + .p.basic.max_leaf = 6,
> + .p.feat.arch_caps = true,
> + .m.arch_caps.rdcl_no = true,
Is this legitimate input in the first place?
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>
> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
Doesn't this need special treatment of RSBA, just like it needs specially
treating below?
> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> return ret;
> }
>
> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
> + const struct cpu_policy *b,
> + struct cpu_policy *out,
> + struct cpu_policy_errors *err)
> +{
> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
> + const struct msr_policy *am = a->msr, *bm = b->msr;
> + struct cpuid_policy *cp = out->cpuid;
> + struct msr_policy *mp = out->msr;
Hmm, okay - this would not work with my proposal in reply to your
other patch. The output would instead need to have pointers
allocated here then.
> + memset(cp, 0, sizeof(*cp));
> + memset(mp, 0, sizeof(*mp));
> +
> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
Any reason you don't do the same right away for the max extended
leaf?
Jan
On 05/05/2021 07:39, Jan Beulich wrote:
> On 04.05.2021 23:31, Andrew Cooper wrote:
>> --- a/tools/tests/cpu-policy/test-cpu-policy.c
>> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
>> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
>> }
>> }
>>
>> +static void test_calculate_compatible_success(void)
>> +{
>> + static struct test {
>> + const char *name;
>> + struct {
>> + struct cpuid_policy p;
>> + struct msr_policy m;
>> + } a, b, out;
>> + } tests[] = {
>> + {
>> + "arch_caps, b short max_leaf",
>> + .a = {
>> + .p.basic.max_leaf = 7,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
>> + },
>> + .b = {
>> + .p.basic.max_leaf = 6,
>> + .p.feat.arch_caps = true,
>> + .m.arch_caps.rdcl_no = true,
> Is this legitimate input in the first place?
I've been debating this for a long time, and have decided "yes".
We have the xend format, and libxl format, and
cpuid=["host:max_leaf=6"]
ought not to be rejected simply because it hasn't also gone and zeroed
every higher piece of information as well.
In production, this function will be used once per host in the pool, and
once more if any custom configuration is specified.
Requiring both inputs to be of the normal form isn't necessary for this
to function correctly (and indeed, would only add extra overhead), but
the eventual result will be cleaned/shrunk/etc as appropriate.
>> --- a/xen/lib/x86/policy.c
>> +++ b/xen/lib/x86/policy.c
>> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>>
>> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
>> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
> Doesn't this need special treatment of RSBA, just like it needs specially
> treating below?
No. If RSBA is clear in 'host', then Xen doesn't know about it, and it
cannot be set in the policy, and cannot be offered to guests.
At the moment, our ARCH_CAPS in the policy is special for dom0 alone to
see, which is why RSBA isn't unilaterally set, but it will just as soon
as the toolstack logic for handling MSRs properly is in place.
>> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> return ret;
>> }
>>
>> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
>> + const struct cpu_policy *b,
>> + struct cpu_policy *out,
>> + struct cpu_policy_errors *err)
>> +{
>> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
>> + const struct msr_policy *am = a->msr, *bm = b->msr;
>> + struct cpuid_policy *cp = out->cpuid;
>> + struct msr_policy *mp = out->msr;
> Hmm, okay - this would not work with my proposal in reply to your
> other patch. The output would instead need to have pointers
> allocated here then.
>
>> + memset(cp, 0, sizeof(*cp));
>> + memset(mp, 0, sizeof(*mp));
>> +
>> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
> Any reason you don't do the same right away for the max extended
> leaf?
I did the minimum for RSBA testing. The line needs to be drawn somewhere.
~Andrew
On 05.05.2021 14:15, Andrew Cooper wrote:
> On 05/05/2021 07:39, Jan Beulich wrote:
>> On 04.05.2021 23:31, Andrew Cooper wrote:
>>> --- a/tools/tests/cpu-policy/test-cpu-policy.c
>>> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
>>> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
>>> }
>>> }
>>>
>>> +static void test_calculate_compatible_success(void)
>>> +{
>>> + static struct test {
>>> + const char *name;
>>> + struct {
>>> + struct cpuid_policy p;
>>> + struct msr_policy m;
>>> + } a, b, out;
>>> + } tests[] = {
>>> + {
>>> + "arch_caps, b short max_leaf",
>>> + .a = {
>>> + .p.basic.max_leaf = 7,
>>> + .p.feat.arch_caps = true,
>>> + .m.arch_caps.rdcl_no = true,
>>> + },
>>> + .b = {
>>> + .p.basic.max_leaf = 6,
>>> + .p.feat.arch_caps = true,
>>> + .m.arch_caps.rdcl_no = true,
>> Is this legitimate input in the first place?
>
> I've been debating this for a long time, and have decided "yes".
>
> We have the xend format, and libxl format, and
>
> cpuid=["host:max_leaf=6"]
>
> ought not to be rejected simply because it hasn't also gone and zeroed
> every higher piece of information as well.
>
> In production, this function will be used once per host in the pool, and
> once more if any custom configuration is specified.
>
> Requiring both inputs to be of the normal form isn't necessary for this
> to function correctly (and indeed, would only add extra overhead), but
> the eventual result will be cleaned/shrunk/etc as appropriate.
Makes sense to me.
>>> --- a/xen/lib/x86/policy.c
>>> +++ b/xen/lib/x86/policy.c
>>> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>> if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>>> FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>>>
>>> + if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
>>> + FAIL_MSR(MSR_ARCH_CAPABILITIES);
>> Doesn't this need special treatment of RSBA, just like it needs specially
>> treating below?
>
> No. If RSBA is clear in 'host', then Xen doesn't know about it, and it
> cannot be set in the policy, and cannot be offered to guests.
How does this play together with the comment in
x86_cpu_policy_calculate_compatible() saying "accumulate"? If it's
clear in one policy because it has to be clear in the host's, how
can it be valid for it to get set there in the result, just because
it was set in the other input?
>>> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>> return ret;
>>> }
>>>
>>> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
>>> + const struct cpu_policy *b,
>>> + struct cpu_policy *out,
>>> + struct cpu_policy_errors *err)
>>> +{
>>> + const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
>>> + const struct msr_policy *am = a->msr, *bm = b->msr;
>>> + struct cpuid_policy *cp = out->cpuid;
>>> + struct msr_policy *mp = out->msr;
>> Hmm, okay - this would not work with my proposal in reply to your
>> other patch. The output would instead need to have pointers
>> allocated here then.
>>
>>> + memset(cp, 0, sizeof(*cp));
>>> + memset(mp, 0, sizeof(*mp));
>>> +
>>> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);
>> Any reason you don't do the same right away for the max extended
>> leaf?
>
> I did the minimum for RSBA testing. The line needs to be drawn somewhere.
I see. I was thinking that it would be nice if the various related
pieces could remain in sync (or at least new code not staying behind
what we already handle elsewhere), i.e. this one doing at least the
equivalent of what x86_cpu_policies_are_compatible() is currently
capable of dealing with. This, again, would make it easier to spot
all the places that need adjusting when something new is to be added.
But I'm not going to insist - this is largely your realm anyway.
Jan
© 2016 - 2024 Red Hat, Inc.