[XTF-ARM] tests: Hypercall xen_version testing

[XTF-ARM] tests: Hypercall xen_version testing

[Michal Orzel]

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.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
This is a patch for XTF fork https://gitlab.com/xen-project/fusa/xtf
(branch xtf-arm) under which XTF on Arm port is maintained.

Pushed to xen-devel together with patches to Xen CI automation, so that it is
clear what the test is doing.
---
 build/arm64/arch-tests.mk      |   2 +-
 docs/all-tests.dox             |   2 +
 tests/hyp-xen-version/Makefile |  10 ++++
 tests/hyp-xen-version/main.c   | 105 +++++++++++++++++++++++++++++++++
 4 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 tests/hyp-xen-version/Makefile
 create mode 100644 tests/hyp-xen-version/main.c

diff --git a/build/arm64/arch-tests.mk b/build/arm64/arch-tests.mk
index 1a13ba923625..c85e28bbea9e 100644
--- a/build/arm64/arch-tests.mk
+++ b/build/arm64/arch-tests.mk
@@ -1,4 +1,4 @@
 # Supported tests by arm64
 
-# Currently only example test is supported
 TESTS := $(ROOT)/tests/example
+TESTS += $(ROOT)/tests/hyp-xen-version
diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index 322f078db09e..48a407593463 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -20,6 +20,8 @@ and functionality.
 
 @subpage test-fpu-exception-emulation - FPU Exception Emulation.  Covers XSA-190.
 
+@subpage test-hyp-xen-version - Hypercall xen_version testing.
+
 @subpage test-invlpg - `invlpg` instruction behaviour.
 
 @subpage test-lbr-tsx-vmentry - Haswell and later LBR/TSX Vmentry failure test.
diff --git a/tests/hyp-xen-version/Makefile b/tests/hyp-xen-version/Makefile
new file mode 100644
index 000000000000..2d7db8fa6ad9
--- /dev/null
+++ b/tests/hyp-xen-version/Makefile
@@ -0,0 +1,10 @@
+include $(ROOT)/build/common.mk
+
+NAME      := hyp-xen-version
+CATEGORY  := functional
+TEST-ARCH := arm64
+TEST-ENVS := $(ALL_ENVIRONMENTS)
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/hyp-xen-version/main.c b/tests/hyp-xen-version/main.c
new file mode 100644
index 000000000000..bda591ca5c29
--- /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);
+
+        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);
+
+        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);
+
+        printk(" changeset: %s\n", xen_cs);
+    }
+
+    printk("Checking XENVER_get_features:\n");
+    {
+        for ( unsigned int i = 0; i < XENFEAT_NR_SUBMAPS; i++ )
+        {
+            xen_feature_info_t xen_fi = { .submap_idx = i };
+
+            ret = hypercall_xen_version(XENVER_get_features, &xen_fi);
+            if ( ret < 0 )
+                return xtf_error("Error %d for submap[%u]\n", ret, i);
+
+            printk(" submap[%u]: %#x\n", i, xen_fi.submap);
+        }
+    }
+
+    printk("Checking invalid command:\n");
+    {
+        /* Invalid cmd should result in returning -ENOSYS. */
+        ret = hypercall_xen_version(INVALID_CMD, NULL);
+        if ( ret != -ENOSYS )
+            return xtf_error("Error %d\n", ret);
+
+        printk(" ok\n");
+    }
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.25.1

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

[Jan Beulich]

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?

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

Jan

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

[Stefano Stabellini]

On Thu, 15 Dec 2022, 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?

Actually the tests are not arm64 specific. Michal, I think you should
remove the comment above from the commit message. (What/if/when gets
enabled as a test in gitlab-ci is a different matter.)


> > --- /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.
> 
> Jan
> 

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

[Michal Orzel]

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

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

[Jan Beulich]

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

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

[Michal Orzel]

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

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

[Jan Beulich]

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