Re: [XTF-ARM] tests: Hypercall xen_version testing

[Michal Orzel <michal.orzel@amd.com>]

Hi Jan,

Thanks for the feedback.

On 15/12/2022 16:48, Jan Beulich wrote:
> 
> 
> On 15.12.2022 16:25, Michal Orzel wrote:
>> Add a new test hyp-xen-version to perform functional testing of
>> xen_version hypercall. Check the following commands (more can be added
>> later on):
>>  - XENVER_version,
>>  - XENVER_extraversion,
>>  - XENVER_compile_info,
>>  - XENVER_changeset
>>  - XENVER_get_features,
>>  - passing invalid command.
>>
>> For now, enable this test only for arm64.
> 
> What's wrong with exposing this uniformly?
There is nothing wrong. I can remove the ARCH restriction.

> 
>> --- /dev/null
>> +++ b/tests/hyp-xen-version/main.c
>> @@ -0,0 +1,105 @@
>> +/**
>> + * @file tests/hyp-xen-version/main.c
>> + * @ref test-hyp-xen-version
>> + *
>> + * @page test-hyp-xen-version Hypercall xen_version
>> + *
>> + * Functional testing of xen_version hypercall.
>> + *
>> + * @see tests/hyp-xen-version/main.c
>> + */
>> +#include <xtf.h>
>> +
>> +const char test_title[] = "Hypercall xen_version testing";
>> +
>> +#define INVALID_CMD -1
>> +
>> +void test_main(void)
>> +{
>> +    int ret;
>> +
>> +    printk("Checking XENVER_version:\n");
>> +    {
>> +        /*
>> +        * Version is returned directly in format: ((major << 16) | minor),
>> +        * so no need to check the return value for an error.
>> +        */
>> +        ret = hypercall_xen_version(XENVER_version, NULL);
>> +        printk(" version: %u.%u\n", ret >> 16, ret & 0xFFFF);
>> +    }
>> +
>> +    printk("Checking XENVER_extraversion:\n");
>> +    {
>> +        xen_extraversion_t xen_ev;
>> +        memset(&xen_ev, 0, sizeof(xen_ev));
>> +
>> +        ret = hypercall_xen_version(XENVER_extraversion, xen_ev);
>> +        if ( ret < 0 )
>> +            return xtf_error("Error %d\n", ret);
> 
> This, ...
> 
>> +        printk(" extraversion: %s\n", xen_ev);
>> +    }
>> +
>> +    printk("Checking XENVER_compile_info:\n");
>> +    {
>> +        xen_compile_info_t xen_ci;
>> +        memset(&xen_ci, 0, sizeof(xen_ci));
>> +
>> +        ret = hypercall_xen_version(XENVER_compile_info, &xen_ci);
>> +        if ( ret < 0 )
>> +            return xtf_error("Error %d\n", ret);
> 
> ... this, and ...
> 
>> +        printk(" compiler:       %s\n", xen_ci.compiler);
>> +        printk(" compile_by:     %s\n", xen_ci.compile_by);
>> +        printk(" compile_domain: %s\n", xen_ci.compile_domain);
>> +        printk(" compile_date:   %s\n", xen_ci.compile_date);
>> +    }
>> +
>> +    printk("Checking XENVER_changeset:\n");
>> +    {
>> +        xen_changeset_info_t xen_cs;
>> +        memset(&xen_cs, 0, sizeof(xen_cs));
>> +
>> +        ret = hypercall_xen_version(XENVER_changeset, &xen_cs);
>> +        if ( ret < 0 )
>> +            return xtf_error("Error %d\n", ret);
> 
> ... this can fail because of XSM denying access. (Others can of course
> also fail for this reason, but here possible failure is kind of
> "intended" - see the dummy xsm_xen_version() handling.) Therefore I
> would like to suggest that you also special case getting back -EPERM,
> resulting in e.g. just a warning instead of an error.
When writing a test I did make sure to check xsm_xen_version *for the operations that I covered*
and my understanding is as follows:
For XENVER_version and XENVER_get_features, it returns 0 so deny is false.
For other commands I test, xsm_default_action is called with XSM_HOOK which returns 0 as well.
So AFAICT nothing can result in setting deny to true.
But even in case of setting deny to true, it would just result in copying "<denied>" into
the respective buffer. It would not alter the hypercall return value.

The only command causing the return value to be -EPERM in case deny is set to true is XENVER_build_id
which I did not covered in my test.

> 
> Jan

~Michal