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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2022 11:53, Michal Orzel wrote:
> 
> 
> 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?

Oh, I see my mistake now. Apologies, I take back my request.

Jan