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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 16/12/2022 11:21, Jan Beulich wrote:
> 
> 
> 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.
Ok but in any case, in the current xen_version implementation, it will just
result in storing "<denied>". No -EPERM will be returned. So do you think it
would make sense to add handling for it in the test even though it cannot be
triggered?

> 
> Jan

~Michal