[04/13] libx86: introduce helper to fetch msr entry

Commit Message

Roger Pau Monne June 16, 2023, 1:10 p.m. UTC

Use such helper in order to replace the code in
x86_msr_copy_from_buffer. Note the introduced helper should not be
directly called and instead x86_msr_get_entry should be used that will
properly deal with const and non-const inputs.

Note this requires making the raw fields uint64_t so that it can
accommodate the maximum size of MSRs values, and in turn removing the
truncation tests.

Signed-off-by: Roger Pau Monné <[e-mail]>
---
Changes since v4:
 - Rename _x86_msr_get_entry to x86_msr_get_entry_const.
 - Add newline before endif.

Changes since v3:
 - New in this version.
---
 tools/tests/cpu-policy/test-cpu-policy.c | 43 +++++++++++++++++++++---
 xen/include/xen/lib/x86/cpu-policy.h     | 18 +++++++++-
 xen/lib/x86/msr.c                        | 41 +++++++++++-----------
 3 files changed, 75 insertions(+), 27 deletions(-)

Comments

Jan Beulich June 20, 2023, 2:28 p.m. UTC | #1

On 16.06.2023 15:10, Roger Pau Monne wrote:
> @@ -119,6 +103,21 @@ int x86_msr_copy_from_buffer(struct cpu_policy *p,
>      return rc;
>  }
>  
> +const uint64_t *x86_msr_get_entry_const(const struct cpu_policy *p,
> +                                        uint32_t idx)

I don't think idx needs to be of a fixed width type; unsigned int
ought to be quite fine.

> +{
> +    switch ( idx )
> +    {
> +    case MSR_INTEL_PLATFORM_INFO:
> +        return &p->platform_info.raw;
> +
> +    case MSR_ARCH_CAPABILITIES:
> +        return &p->arch_caps.raw;

Earlier on (also for the CPUID counterpart) a risky aspect of this
didn't occur to me: The validity of the returned pointer is tied
to the scope of the object the address of which was passed in. I'm
afraid this is an aspect which is easy to miss, and hence wants
calling out in the comments in cpu-policy.h (provided we're okay
to accept this risk in the first place).

Jan

Patch

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index a11c8f067aad..2ce6ee96f91f 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -387,11 +387,6 @@  static void test_msr_deserialise_failure(void)
             .msr = { .idx = 0xce, .flags = 1 },
             .rc = -EINVAL,
         },
-        {
-            .name = "truncated val",
-            .msr = { .idx = 0xce, .val = ~0ull },
-            .rc = -EOVERFLOW,
-        },
     };
 
     printf("Testing MSR deserialise failure:\n");
@@ -740,6 +735,43 @@  static void test_cpuid_get_leaf(void)
     }
 }
 
+static void test_msr_get_entry(void)
+{
+    static const struct test {
+        const char *name;
+        unsigned int idx;
+        bool success;
+    } tests[] = {
+        {
+            .name = "bad msr index",
+            .idx = -1,
+        },
+        {
+            .name = "good msr index",
+            .idx = 0xce,
+            .success = true,
+        },
+    };
+    const struct cpu_policy pc = { };
+    const uint64_t *ec;
+    struct cpu_policy p = { };
+    uint64_t *e;
+
+    /* Constness build test. */
+    ec = x86_msr_get_entry(&pc, 0);
+    e = x86_msr_get_entry(&p, 0);
+
+    printf("Testing MSR get leaf:\n");
+
+    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+    {
+        const struct test *t = &tests[i];
+
+        if ( !!x86_msr_get_entry(&pc, t->idx) != t->success )
+            fail("  Test %s failed\n", t->name);
+    }
+}
+
 static void test_is_compatible_success(void)
 {
     static struct test {
@@ -840,6 +872,7 @@  int main(int argc, char **argv)
 
     test_msr_serialise_success();
     test_msr_deserialise_failure();
+    test_msr_get_entry();
 
     test_is_compatible_success();
     test_is_compatible_failure();
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index 3fcc02c729db..877879fc8c16 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -337,7 +337,7 @@  struct cpu_policy
      * is dependent on real hardware support.
      */
     union {
-        uint32_t raw;
+        uint64_t raw;
         struct {
             uint32_t :31;
             bool cpuid_faulting:1;
@@ -559,6 +559,22 @@  const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct cpu_policy *p,
 #define x86_cpuid_get_leaf(p, l, s) \
     ((__typeof__(&(p)->basic.raw[0]))x86_cpuid_get_leaf_const(p, l, s))
 
+/**
+ * Get a MSR entry from a policy object.
+ *
+ * @param policy      The msr_policy object.
+ * @param idx         The index.
+ * @returns a pointer to the requested leaf or NULL in case of error.
+ *
+ * Do not call this function directly and instead use x86_msr_get_entry that
+ * will deal with both const and non-const policies returning a pointer with
+ * constness matching that of the input.
+ */
+const uint64_t *x86_msr_get_entry_const(const struct cpu_policy *p,
+                                        uint32_t idx);
+#define x86_msr_get_entry(p, i) \
+    ((__typeof__(&(p)->platform_info.raw))x86_msr_get_entry_const(p, i))
+
 #endif /* !XEN_LIB_X86_POLICIES_H */
 
 /*
diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c
index e04b9ca01302..25f6e8e0be92 100644
--- a/xen/lib/x86/msr.c
+++ b/xen/lib/x86/msr.c
@@ -74,6 +74,8 @@  int x86_msr_copy_from_buffer(struct cpu_policy *p,
 
     for ( i = 0; i < nr_entries; i++ )
     {
+        uint64_t *val;
+
         if ( copy_from_buffer_offset(&data, msrs, i, 1) )
             return -EFAULT;
 
@@ -83,31 +85,13 @@  int x86_msr_copy_from_buffer(struct cpu_policy *p,
             goto err;
         }
 
-        switch ( data.idx )
+        val = x86_msr_get_entry(p, data.idx);
+        if ( !val )
         {
-            /*
-             * Assign data.val to p->field, checking for truncation if the
-             * backing storage for field is smaller than uint64_t
-             */
-#define ASSIGN(field)                             \
-({                                                \
-    if ( (typeof(p->field))data.val != data.val ) \
-    {                                             \
-        rc = -EOVERFLOW;                          \
-        goto err;                                 \
-    }                                             \
-    p->field = data.val;                          \
-})
-
-        case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break;
-        case MSR_ARCH_CAPABILITIES:   ASSIGN(arch_caps.raw);     break;
-
-#undef ASSIGN
-
-        default:
             rc = -ERANGE;
             goto err;
         }
+        *val = data.val;
     }
 
     return 0;
@@ -119,6 +103,21 @@  int x86_msr_copy_from_buffer(struct cpu_policy *p,
     return rc;
 }
 
+const uint64_t *x86_msr_get_entry_const(const struct cpu_policy *p,
+                                        uint32_t idx)
+{
+    switch ( idx )
+    {
+    case MSR_INTEL_PLATFORM_INFO:
+        return &p->platform_info.raw;
+
+    case MSR_ARCH_CAPABILITIES:
+        return &p->arch_caps.raw;
+    }
+
+    return NULL;
+}
+
 /*
  * Local variables:
  * mode: C