[PATCH v2] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling

Andrew Cooper posted 1 patch 2 years, 9 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210628150011.13106-1-andrew.cooper3@citrix.com
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(-)
[PATCH v2] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling
Posted by Andrew Cooper 2 years, 9 months ago
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


Re: [PATCH v2] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling
Posted by Jan Beulich 2 years, 9 months ago
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