diff mbox series

[v2,03/10] libx86: Introduce x86_cpu_policies_are_compatible()

Message ID 20190913192759.10795-4-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/cpuid: Switch to using XEN_DOMCTL_set_cpumsr_policy | expand

Commit Message

Andrew Cooper Sept. 13, 2019, 7:27 p.m. UTC
This helper will eventually be the core "can a guest configured like this run
on the CPU?" logic.  For now, it is just enough of a stub to allow us to
replace the hypercall interface while retaining the previous behaviour.

It will be expanded as various other bits of CPUID handling get cleaned up.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over 'plaform' typo fix
 * Proactively initialise the error pointer
 * Expand the function documentation
---
 tools/tests/cpu-policy/Makefile          |   2 +-
 tools/tests/cpu-policy/test-cpu-policy.c | 111 ++++++++++++++++++++++++++++++-
 xen/include/xen/lib/x86/cpu-policy.h     |  26 ++++++++
 xen/lib/x86/Makefile                     |   1 +
 xen/lib/x86/policy.c                     |  54 +++++++++++++++
 5 files changed, 191 insertions(+), 3 deletions(-)
 create mode 100644 xen/lib/x86/policy.c

Comments

Jan Beulich Sept. 16, 2019, 10:59 a.m. UTC | #1
On 13.09.2019 21:27, Andrew Cooper wrote:
> --- /dev/null
> +++ b/xen/lib/x86/policy.c
> @@ -0,0 +1,54 @@
> +#include "private.h"
> +
> +#include <xen/lib/x86/cpu-policy.h>
> +
> +int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> +                                    const struct cpu_policy *guest,
> +                                    struct cpu_policy_errors *err)
> +{
> +    struct cpu_policy_errors e = INIT_CPU_POLICY_ERRORS;
> +    int ret = -EINVAL;
> +
> +    if ( err )
> +        *err = e;

You don't really need this, do you? All paths lead ...

> +#define NA XEN_CPUID_NO_SUBLEAF
> +#define FAIL_CPUID(l, s) \
> +    do { e.leaf = (l); e.subleaf = (s); goto out; } while ( 0 )
> +#define FAIL_MSR(m) \
> +    do { e.msr = (m); goto out; } while ( 0 )
> +
> +    if ( guest->cpuid->basic.max_leaf > host->cpuid->basic.max_leaf )
> +        FAIL_CPUID(0, NA);
> +
> +    if ( guest->cpuid->extd.max_leaf > host->cpuid->extd.max_leaf )
> +        FAIL_CPUID(0x80000008, NA);
> +
> +    /* TODO: Audit more CPUID data. */
> +
> +    if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
> +        FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
> +
> +#undef FAIL_MSR
> +#undef FAIL_CPUID
> +#undef NA
> +
> +    /* Success. */
> +    ret = 0;
> +
> + out:
> +    if ( ret && err )
> +        *err = e;

... here, and hence you could simply drop the "ret &&" part of the
condition.

Jan
Andrew Cooper Sept. 16, 2019, 3:31 p.m. UTC | #2
On 16/09/2019 11:59, Jan Beulich wrote:
> On 13.09.2019 21:27, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/xen/lib/x86/policy.c
>> @@ -0,0 +1,54 @@
>> +#include "private.h"
>> +
>> +#include <xen/lib/x86/cpu-policy.h>
>> +
>> +int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> +                                    const struct cpu_policy *guest,
>> +                                    struct cpu_policy_errors *err)
>> +{
>> +    struct cpu_policy_errors e = INIT_CPU_POLICY_ERRORS;
>> +    int ret = -EINVAL;
>> +
>> +    if ( err )
>> +        *err = e;
> You don't really need this, do you? All paths lead ...
>
>> +#define NA XEN_CPUID_NO_SUBLEAF
>> +#define FAIL_CPUID(l, s) \
>> +    do { e.leaf = (l); e.subleaf = (s); goto out; } while ( 0 )
>> +#define FAIL_MSR(m) \
>> +    do { e.msr = (m); goto out; } while ( 0 )
>> +
>> +    if ( guest->cpuid->basic.max_leaf > host->cpuid->basic.max_leaf )
>> +        FAIL_CPUID(0, NA);
>> +
>> +    if ( guest->cpuid->extd.max_leaf > host->cpuid->extd.max_leaf )
>> +        FAIL_CPUID(0x80000008, NA);
>> +
>> +    /* TODO: Audit more CPUID data. */
>> +
>> +    if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>> +        FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>> +
>> +#undef FAIL_MSR
>> +#undef FAIL_CPUID
>> +#undef NA
>> +
>> +    /* Success. */
>> +    ret = 0;
>> +
>> + out:
>> +    if ( ret && err )
>> +        *err = e;
> ... here, and hence you could simply drop the "ret &&" part of the
> condition.

Hmm - so they do.  I'll adjust as you suggest.

~Andrew
diff mbox series

Patch

diff --git a/tools/tests/cpu-policy/Makefile b/tools/tests/cpu-policy/Makefile
index fb548c9b9a..70ff154da6 100644
--- a/tools/tests/cpu-policy/Makefile
+++ b/tools/tests/cpu-policy/Makefile
@@ -39,7 +39,7 @@  CFLAGS += $(APPEND_CFLAGS)
 
 vpath %.c ../../../xen/lib/x86
 
-test-cpu-policy: test-cpu-policy.o msr.o cpuid.o
+test-cpu-policy: test-cpu-policy.o msr.o cpuid.o policy.o
 	$(CC) $(CFLAGS) $^ -o $@
 
 -include $(DEPS_INCLUDE)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 201358d210..20ebed923b 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -9,8 +9,7 @@ 
 
 #include <xen-tools/libs.h>
 #include <xen/asm/x86-vendors.h>
-#include <xen/lib/x86/cpuid.h>
-#include <xen/lib/x86/msr.h>
+#include <xen/lib/x86/cpu-policy.h>
 #include <xen/domctl.h>
 
 static unsigned int nr_failures;
@@ -503,6 +502,111 @@  static void test_cpuid_out_of_range_clearing(void)
     }
 }
 
+static void test_is_compatible_success(void)
+{
+    static struct test {
+        const char *name;
+        struct cpuid_policy host_cpuid;
+        struct cpuid_policy guest_cpuid;
+        struct msr_policy host_msr;
+        struct msr_policy guest_msr;
+    } tests[] = {
+        {
+            .name = "Host CPUID faulting, Guest not",
+            .host_msr = {
+                .platform_info.cpuid_faulting = true,
+            },
+        },
+        {
+            .name = "Host CPUID faulting, Guest wanted",
+            .host_msr = {
+                .platform_info.cpuid_faulting = true,
+            },
+            .guest_msr = {
+                .platform_info.cpuid_faulting = true,
+            },
+        },
+    };
+    struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
+
+    printf("Testing policy compatibility success:\n");
+
+    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+    {
+        struct test *t = &tests[i];
+        struct cpu_policy sys = {
+            &t->host_cpuid,
+            &t->host_msr,
+        }, new = {
+            &t->guest_cpuid,
+            &t->guest_msr,
+        };
+        struct cpu_policy_errors e;
+        int res = x86_cpu_policies_are_compatible(&sys, &new, &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);
+    }
+}
+
+static void test_is_compatible_failure(void)
+{
+    static struct test {
+        const char *name;
+        struct cpuid_policy host_cpuid;
+        struct cpuid_policy guest_cpuid;
+        struct msr_policy host_msr;
+        struct msr_policy guest_msr;
+        struct cpu_policy_errors e;
+    } tests[] = {
+        {
+            .name = "Host basic.max_leaf out of range",
+            .guest_cpuid.basic.max_leaf = 1,
+            .e = { 0, -1, -1 },
+        },
+        {
+            .name = "Host extd.max_leaf out of range",
+            .guest_cpuid.extd.max_leaf = 1,
+            .e = { 0x80000008, -1, -1 },
+        },
+        {
+            .name = "Host no CPUID faulting, Guest wanted",
+            .guest_msr = {
+                .platform_info.cpuid_faulting = true,
+            },
+            .e = { -1, -1, 0xce },
+        },
+    };
+
+    printf("Testing policy compatibility failure:\n");
+
+    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+    {
+        struct test *t = &tests[i];
+        struct cpu_policy sys = {
+            &t->host_cpuid,
+            &t->host_msr,
+        }, new = {
+            &t->guest_cpuid,
+            &t->guest_msr,
+        };
+        struct cpu_policy_errors e;
+        int res = x86_cpu_policies_are_compatible(&sys, &new, &e);
+
+        /* Check the expected error output. */
+        if ( res == 0 || memcmp(&t->e, &e, sizeof(t->e)) )
+            fail("  Test '%s' res %d\n"
+                 "    expected { leaf %08x, subleaf %08x, msr %08x }\n"
+                 "    got      { leaf %08x, subleaf %08x, msr %08x }\n",
+                 t->name, res,
+                 t->e.leaf, t->e.subleaf, t->e.msr,
+                 e.leaf, e.subleaf, e.msr);
+    }
+}
+
 int main(int argc, char **argv)
 {
     printf("CPU Policy unit tests\n");
@@ -516,6 +620,9 @@  int main(int argc, char **argv)
     test_msr_serialise_success();
     test_msr_deserialise_failure();
 
+    test_is_compatible_success();
+    test_is_compatible_failure();
+
     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 6f07c4b493..b7e38732a0 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -11,6 +11,32 @@  struct cpu_policy
     struct msr_policy *msr;
 };
 
+struct cpu_policy_errors
+{
+    uint32_t leaf, subleaf;
+    uint32_t msr;
+};
+
+#define INIT_CPU_POLICY_ERRORS { ~0u, ~0u, ~0u }
+
+/*
+ * Calculate whether two policies are compatible.
+ *
+ * i.e. Can a VM configured with @guest run on a CPU supporting @host.
+ *
+ * @param host     A cpu_policy describing the hardware capabilities.
+ * @param guest    A cpu_policy describing the intended VM configuration.
+ * @param err      Optional hint for error diagnostics.
+ * @returns -errno
+ *
+ * For typical usage, @host should be a system policy.  In the case that an
+ * incompatibility is detected, the optional err pointer may identify the
+ * problematic leaf/subleaf and/or MSR.
+ */
+int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
+                                    const struct cpu_policy *guest,
+                                    struct cpu_policy_errors *err);
+
 #endif /* !XEN_LIB_X86_POLICIES_H */
 
 /*
diff --git a/xen/lib/x86/Makefile b/xen/lib/x86/Makefile
index 2f9691e964..780ea05db1 100644
--- a/xen/lib/x86/Makefile
+++ b/xen/lib/x86/Makefile
@@ -1,2 +1,3 @@ 
 obj-y += cpuid.o
 obj-y += msr.o
+obj-y += policy.o
diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
new file mode 100644
index 0000000000..33a347ff9b
--- /dev/null
+++ b/xen/lib/x86/policy.c
@@ -0,0 +1,54 @@ 
+#include "private.h"
+
+#include <xen/lib/x86/cpu-policy.h>
+
+int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
+                                    const struct cpu_policy *guest,
+                                    struct cpu_policy_errors *err)
+{
+    struct cpu_policy_errors e = INIT_CPU_POLICY_ERRORS;
+    int ret = -EINVAL;
+
+    if ( err )
+        *err = e;
+
+#define NA XEN_CPUID_NO_SUBLEAF
+#define FAIL_CPUID(l, s) \
+    do { e.leaf = (l); e.subleaf = (s); goto out; } while ( 0 )
+#define FAIL_MSR(m) \
+    do { e.msr = (m); goto out; } while ( 0 )
+
+    if ( guest->cpuid->basic.max_leaf > host->cpuid->basic.max_leaf )
+        FAIL_CPUID(0, NA);
+
+    if ( guest->cpuid->extd.max_leaf > host->cpuid->extd.max_leaf )
+        FAIL_CPUID(0x80000008, NA);
+
+    /* TODO: Audit more CPUID data. */
+
+    if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
+        FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
+
+#undef FAIL_MSR
+#undef FAIL_CPUID
+#undef NA
+
+    /* Success. */
+    ret = 0;
+
+ out:
+    if ( ret && err )
+        *err = e;
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */