diff mbox series

[2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted

Message ID 20200615141532.1927-3-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series XSA-320 follow for IvyBridge | expand

Commit Message

Andrew Cooper June 15, 2020, 2:15 p.m. UTC
The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
about to start relying on this.  Extend the unit tests.

As test_cpuid_serialise_success() is a fairly limited set of synthetic
examples right now, introduce test_cpuid_current() to operate on the full
policy for the current CPU.

Tweak the fail() macro to allow for simplified control flow.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
---
 tools/tests/cpu-policy/test-cpu-policy.c | 49 +++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

Comments

Ian Jackson June 15, 2020, 2:52 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
> The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
> about to start relying on this.  Extend the unit tests.
> 
> As test_cpuid_serialise_success() is a fairly limited set of synthetic
> examples right now, introduce test_cpuid_current() to operate on the full
> policy for the current CPU.
> 
> Tweak the fail() macro to allow for simplified control flow.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I don't think anything in our normal dev workflow runs this
automatically ?  Maybe this would be something for us to think
about...

Ian.
Andrew Cooper June 15, 2020, 3 p.m. UTC | #2
On 15/06/2020 15:52, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
>> The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
>> about to start relying on this.  Extend the unit tests.
>>
>> As test_cpuid_serialise_success() is a fairly limited set of synthetic
>> examples right now, introduce test_cpuid_current() to operate on the full
>> policy for the current CPU.
>>
>> Tweak the fail() macro to allow for simplified control flow.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> I don't think anything in our normal dev workflow runs this
> automatically ?  Maybe this would be something for us to think
> about...

Nothing runs it by default, but it is part of my prepush testing for
anything in the relevant area.

We should do something better, but its not totally trivial.  The x86
emulator test for example needs a fairly bleeding edge version of
binutils, given that we routinely add support for bleeding edge
instruction groups.

~Andrew
Ian Jackson June 15, 2020, 3:34 p.m. UTC | #3
Andrew Cooper writes ("Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
> Nothing runs it by default, but it is part of my prepush testing for
> anything in the relevant area.
> 
> We should do something better, but its not totally trivial.  The x86
> emulator test for example needs a fairly bleeding edge version of
> binutils, given that we routinely add support for bleeding edge
> instruction groups.

Hmmm.  Is it likely that installing the version from Debian testing
(say) would work ?  Or do we want to build it from source ?  These are
all possibilities.

We could build binutils from source with a job-specific --prefix
setting and that wouldn't stop it sharing the build host with other
jobs.  Maybe it could be a seperate job which provides an anointed
binutils build.

What other awkward requirements are there ?

Ian.
Andrew Cooper June 15, 2020, 4:12 p.m. UTC | #4
On 15/06/2020 16:34, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
>> Nothing runs it by default, but it is part of my prepush testing for
>> anything in the relevant area.
>>
>> We should do something better, but its not totally trivial.  The x86
>> emulator test for example needs a fairly bleeding edge version of
>> binutils, given that we routinely add support for bleeding edge
>> instruction groups.
> Hmmm.  Is it likely that installing the version from Debian testing
> (say) would work ?  Or do we want to build it from source ?  These are
> all possibilities.

Pulling from Sid may work, if they're fairly prompt to update to new
binutils releases.  (They're certainly up to date ATM)

Jan: are we ever in a position where we need something more bleeding
edge than the latest binutils release?

>
> We could build binutils from source with a job-specific --prefix
> setting and that wouldn't stop it sharing the build host with other
> jobs.  Maybe it could be a seperate job which provides an anointed
> binutils build.
>
> What other awkward requirements are there ?

Most of the tests require running under a suitably configured Xen, and
even then, require doing some fairly custom things with the guest.

Perhaps a good start would be to split our "unit test like" tests away
from our "need a specifically configured Xen" tests.  The former would
be rather more amenable to running by default.

~Andrew
Jan Beulich June 16, 2020, 6:51 a.m. UTC | #5
On 15.06.2020 18:12, Andrew Cooper wrote:
> On 15/06/2020 16:34, Ian Jackson wrote:
>> Andrew Cooper writes ("Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
>>> Nothing runs it by default, but it is part of my prepush testing for
>>> anything in the relevant area.
>>>
>>> We should do something better, but its not totally trivial.  The x86
>>> emulator test for example needs a fairly bleeding edge version of
>>> binutils, given that we routinely add support for bleeding edge
>>> instruction groups.
>> Hmmm.  Is it likely that installing the version from Debian testing
>> (say) would work ?  Or do we want to build it from source ?  These are
>> all possibilities.
> 
> Pulling from Sid may work, if they're fairly prompt to update to new
> binutils releases.  (They're certainly up to date ATM)
> 
> Jan: are we ever in a position where we need something more bleeding
> edge than the latest binutils release?

Gcc needs to be about as recent: Right now the minimum is gcc 8 I
think, and I have a few hacks in place locally to make things
somewhat work back to gcc 5, as on my laptop (and hence when
working from home) I don't have any custom gcc builds.

Jan
Jan Beulich June 16, 2020, 9:01 a.m. UTC | #6
On 15.06.2020 16:15, Andrew Cooper wrote:
> The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
> about to start relying on this.  Extend the unit tests.
> 
> As test_cpuid_serialise_success() is a fairly limited set of synthetic
> examples right now, introduce test_cpuid_current() to operate on the full
> policy for the current CPU.
> 
> Tweak the fail() macro to allow for simplified control flow.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff mbox series

Patch

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index fe8cdf6ea9..7ba9707236 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -16,7 +16,7 @@  static unsigned int nr_failures;
 #define fail(fmt, ...)                          \
 ({                                              \
     nr_failures++;                              \
-    printf(fmt, ##__VA_ARGS__);                 \
+    (void)printf(fmt, ##__VA_ARGS__);           \
 })
 
 #define memdup(ptr)                             \
@@ -66,6 +66,45 @@  static void test_vendor_identification(void)
     }
 }
 
+static bool leaves_are_sorted(const xen_cpuid_leaf_t *leaves, unsigned int nr)
+{
+    for ( unsigned int i = 1; i < nr; ++i )
+    {
+        /* leaf index went backwards => not sorted. */
+        if ( leaves[i - 1].leaf > leaves[i].leaf )
+            return false;
+
+        /* leaf index went forwards => ok */
+        if ( leaves[i - 1].leaf < leaves[i].leaf )
+            continue;
+
+        /* leave index the same, subleaf didn't increase => not sorted. */
+        if ( leaves[i - 1].subleaf >= leaves[i].subleaf )
+            return false;
+    }
+
+    return true;
+}
+
+static void test_cpuid_current(void)
+{
+    struct cpuid_policy p;
+    xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
+    unsigned int nr = ARRAY_SIZE(leaves);
+    int rc;
+
+    printf("Testing CPUID on current CPU\n");
+
+    x86_cpuid_policy_fill_native(&p);
+
+    rc = x86_cpuid_copy_to_buffer(&p, leaves, &nr);
+    if ( rc != 0 )
+        return fail("  Serialise, expected rc 0, got %d\n", rc);
+
+    if ( !leaves_are_sorted(leaves, nr) )
+        return fail("  Leaves not sorted\n");
+}
+
 static void test_cpuid_serialise_success(void)
 {
     static const struct test {
@@ -178,6 +217,13 @@  static void test_cpuid_serialise_success(void)
             goto test_done;
         }
 
+        if ( !leaves_are_sorted(leaves, nr) )
+        {
+            fail("  Test %s, leaves not sorted\n",
+                 t->name);
+            goto test_done;
+        }
+
     test_done:
         free(leaves);
     }
@@ -613,6 +659,7 @@  int main(int argc, char **argv)
 
     test_vendor_identification();
 
+    test_cpuid_current();
     test_cpuid_serialise_success();
     test_cpuid_deserialise_failure();
     test_cpuid_out_of_range_clearing();