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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2022 10:30, Michal Orzel wrote:
> On 15/12/2022 16:48, Jan Beulich wrote:
>> On 15.12.2022 16:25, Michal Orzel wrote:
>>> --- /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.

For dummy itself all is fine; arrangements there suggest to me though
that the intention was that an actual Flask policy may be written such
that some of these might actually be refused. My recollection actually
is that when the distinction for the sub-ops was introduced, quite a
bit of discussion happened as to what may or may not be (optionally
or uniformly) be rejected.

Jan