Re: [Xen-devel] [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.02.19 at 12:56, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/01/2019 11:19, Jan Beulich wrote:
>>>>> On 04.01.19 at 16:33, <andrew.cooper3@xxxxxxxxxx> 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.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>>> ---
>>>  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.

Well, I certainly appreciate this, and my comment wasn't meant as
plain opposition. At the time I was simply wondering whether the few
instances we have already aren't more like an accident. And I didn't
recall any discussion towards moving away from the centralized model.

>>> +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.

"Cascade failures" to me are ones where the latter is a result of
something earlier having gone wrong. Iirc this isn't the case with the
subsequent test(s) here, unless something's fundamentally wrong.
Seeing which particular case doesn't work plus all of the ones which
do can also be helpful. But as indicated by me using "perhaps" in the
original reply - I won't insist, as in the end something will need to be
done anyway until everything succeeds.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel