tools/include/xen-tools/libs.h | 5 ++ tools/libs/light/libxl_internal.h | 2 - tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++ xen/include/xen/lib/x86/cpu-policy.h | 28 ++++++ xen/lib/x86/policy.c | 49 ++++++++++ 5 files changed, 232 insertions(+), 2 deletions(-)
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>
v2:
* Don't break the libxl build.
* Extend the function description to discuss why levelling the max policy
doesn't make sense, and that we (will) pick some data from @a specifically.
* Ifdefary for now until a Xen caller arrives.
---
tools/include/xen-tools/libs.h | 5 ++
tools/libs/light/libxl_internal.h | 2 -
tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++
xen/include/xen/lib/x86/cpu-policy.h | 28 ++++++
xen/lib/x86/policy.c | 49 ++++++++++
5 files changed, 232 insertions(+), 2 deletions(-)
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/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 0b4671318c..1816c9704f 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -126,8 +126,6 @@
#define PVSHIM_CMDLINE "pv-shim console=xen,pv"
/* Size macros. */
-#define __AC(X,Y) (X##Y)
-#define _AC(X,Y) __AC(X,Y)
#define MB(_mb) (_AC(_mb, ULL) << 20)
#define GB(_gb) (_AC(_gb, ULL) << 30)
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..0e8ff1194a 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -37,6 +37,34 @@ 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, if successful.
+ * @param err Optional hint for error diagnostics.
+ * @returns -errno
+ *
+ * For typical usage, @a and @b should be default system policies of the same
+ * type (i.e. PV or HVM) from different hosts. It does not make sense to try
+ * and level max policies, as they contain the non-migrateable features.
+ *
+ * Some data (e.g. the long CPU brand string) cannot be levelled. Such data
+ * will be taken from @a, and the content in @b will be discaraded.
+ *
+ * It is possible that @a and @b cannot be resolved to migration-compatible
+ * new policy. In this case, 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..de14fe4912 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,52 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
return ret;
}
+#ifndef __XEN__
+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;
+}
+#endif /* !__XEN__ */
+
/*
* Local variables:
* mode: C
--
2.11.0
On 28.06.2021 17:00, 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 { It's only testing code, so it doesn't matter all this much, but elsewhere such static struct-s are const. > + 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, > + }, > + }, For RDCL_NO you go through quite a few more variations, and given the accumulating nature of RSBA habing a similar set for it would imo be quite valuable, not the least for people like me to see clearly what behavior is expected there. > + }; > + 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, Hmm, I guess these two struct instances are the reason for tests[] to be non-const. I vaguely recall discussion about having a const- correct variant of struct cpu_policy; if you don't think this is warranted, may I ask that you add a respective brief comment to tests[]? > + }, 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)) ) While this memcmp() has precedents, ... > + { > + 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)) ) ... I'm worried that this and ... > + { > + fail(" Test '%s' resulting CPUID policy not as expected\n", > + t->name); > + goto test_done; > + } > + > + if ( memcmp(&t->out.m, m, sizeof(*m)) ) ... this may (down the road) suffer from mismatches on uninitialized padding fields. Otoh I've meanwhile found that the new function clears both output buffers first thinhg. > --- a/xen/include/xen/lib/x86/cpu-policy.h > +++ b/xen/include/xen/lib/x86/cpu-policy.h > @@ -37,6 +37,34 @@ 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, if successful. > + * @param err Optional hint for error diagnostics. > + * @returns -errno > + * > + * For typical usage, @a and @b should be default system policies of the same > + * type (i.e. PV or HVM) from different hosts. Given this property, what use do you anticipate for the new function within libxl? Or asked differently, where from would libxl obtain a remote host's policy? > It does not make sense to try > + * and level max policies, as they contain the non-migrateable features. > + * > + * Some data (e.g. the long CPU brand string) cannot be levelled. Such data > + * will be taken from @a, and the content in @b will be discaraded. I'm afraid I can't spot this "taking from @a" in the code. Also, nit: "discarded" > + * It is possible that @a and @b cannot be resolved to migration-compatible Nit: Missing "a" after "to"? > @@ -43,6 +46,52 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, > return ret; > } > > +#ifndef __XEN__ > +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; Is there a particular reason to not handle this in full, i.e. for all of the subleaves? If there is, I'd still have expected you to at least handle _7a1 that we already know about. Failing that I'd have hoped for a justifying comment (or maybe a TODO item beyond ... > + } > + > + /* TODO: Far more. */ ... this one. > + 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 > + } Related to my respective request on the set of tests performed, this really is partial accumulation, as ARCH_CAPS are still taken as a prereq feature. That is, one host with RSBA and another without ARCH_CAPS will result in a policy without RSBA. Is this really what's intended? Jan
© 2016 - 2024 Red Hat, Inc.