On 07/01/2019 11:19, Jan Beulich wrote:
>>>> On 04.01.19 at 16:33, <andrew.coop...@citrix.com> wrote:
>> The AFL harness currently notices that there are cases where we optimse the
>> serialised stream by omitting data beyond the various maximum leaves.
>>
>> Both sets of tests will be extended with further libx86 work.
>>
>> Fix the sorting of the CPUID_GUEST_NR_* constants, noticed while writing the
>> unit tests.
>>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Wei Liu <wei.l...@citrix.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Sergey Dyasli <sergey.dya...@citrix.com>
>> ---
>> tools/fuzz/cpu-policy/.gitignore | 1 +
>> tools/fuzz/cpu-policy/Makefile | 27 ++++
>> tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 117 ++++++++++++++
>> tools/tests/Makefile | 1 +
>> tools/tests/cpu-policy/.gitignore | 1 +
> Did we somehow come to the conclusion that the central .gitignore
> at the root of the tree is not the way to go in the future?
We've already got several examples in the tree.
andrewcoop@andrewcoop:/local/xen.git$ git ls-files | grep gitignore
.gitignore
tools/tests/vhpet/.gitignore
xen/tools/kconfig/.gitignore
xen/tools/kconfig/lxdialog/.gitignore
xen/xsm/flask/.gitignore
As for the pro's of using split ignores, fewer collisions for backported
changes, and no forgetting to update the root .gitconfig when you move
directories.
>
>> --- /dev/null
>> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
>> @@ -0,0 +1,247 @@
>> +#include <assert.h>
>> +#include <errno.h>
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +
>> +#include <xen-tools/libs.h>
>> +#include <xen/lib/x86/cpuid.h>
>> +#include <xen/lib/x86/msr.h>
>> +#include <xen/domctl.h>
>> +
>> +static void test_cpuid_serialise_success(void)
>> +{
>> + static const struct test {
>> + struct cpuid_policy p;
>> + const char *name;
>> + unsigned int nr_leaves;
>> + } tests[] = {
>> + {
>> + .name = "empty policy",
>> + .nr_leaves = 4,
>> + },
>> + };
>> + unsigned int i;
>> +
>> + printf("Testing CPUID serialise success:\n");
>> +
>> + for ( i = 0; i < ARRAY_SIZE(tests); ++i )
>> + {
>> + const struct test *t = &tests[i];
>> + unsigned int nr = t->nr_leaves;
>> + xen_cpuid_leaf_t *leaves = malloc(nr * sizeof(*leaves));
>> + int rc;
>> +
>> + if ( !leaves )
>> + goto test_done;
> Shouldn't you leave some indication of the test not having got run?
I've swapped this for a hard error. Its not going to fail in practice.
>
>> +static void test_cpuid_deserialise_failure(void)
>> +{
>> + static const struct test {
>> + const char *name;
>> + xen_cpuid_leaf_t leaf;
>> + } tests[] = {
>> + {
>> + .name = "incorrect basic subleaf",
>> + .leaf = { .leaf = 0, .subleaf = 0 },
>> + },
>> + {
>> + .name = "incorrect hv1 subleaf",
>> + .leaf = { .leaf = 0x40000000, .subleaf = 0 },
>> + },
>> + {
>> + .name = "incorrect hv2 subleaf",
>> + .leaf = { .leaf = 0x40000100, .subleaf = 0 },
>> + },
>> + {
>> + .name = "incorrect extd subleaf",
>> + .leaf = { .leaf = 0x80000000, .subleaf = 0 },
>> + },
>> + {
>> + .name = "OoB basic leaf",
>> + .leaf = { .leaf = CPUID_GUEST_NR_BASIC },
>> + },
>> + {
>> + .name = "OoB cache leaf",
>> + .leaf = { .leaf = 0x4, .subleaf = CPUID_GUEST_NR_CACHE },
>> + },
>> + {
>> + .name = "OoB feat leaf",
>> + .leaf = { .leaf = 0x7, .subleaf = CPUID_GUEST_NR_FEAT },
>> + },
>> + {
>> + .name = "OoB topo leaf",
>> + .leaf = { .leaf = 0xb, .subleaf = CPUID_GUEST_NR_TOPO },
>> + },
>> + {
>> + .name = "OoB xstate leaf",
>> + .leaf = { .leaf = 0xd, .subleaf = CPUID_GUEST_NR_XSTATE },
>> + },
>> + {
>> + .name = "OoB extd leaf",
>> + .leaf = { .leaf = 0x80000000 | CPUID_GUEST_NR_EXTD },
>> + },
>> + };
>> + unsigned int i;
>> +
>> + printf("Testing CPUID deserialise failure:\n");
>> +
>> + for ( i = 0; i < ARRAY_SIZE(tests); ++i )
>> + {
>> + const struct test *t = &tests[i];
>> + uint32_t err_leaf = ~0u, err_subleaf = ~0u;
>> + int rc;
>> +
>> + rc = x86_cpuid_copy_from_buffer(NULL, &t->leaf, 1,
>> + &err_leaf, &err_subleaf);
>> +
>> + if ( rc != -ERANGE )
>> + {
>> + printf(" Test %s, expected rc %d, got %d\n",
>> + t->name, -ERANGE, rc);
>> + continue;
> Perhaps drop this? The subsequent test ought to apply regardless
> of error code.
The common case is no failures at all. However, once something has gone
wrong, spewing cascade errors gets in the way, rather than being helpful.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
