[PATCH i-g-t] tests/intel/xe_create: Check negative cases for exec_queue create/destroy ioctl
Cavitt, Jonathan
jonathan.cavitt at intel.com
Fri Jan 17 23:25:17 UTC 2025
I left some notes below. The NITs can be ignored safely, but everything else should be corrected.
To summarize the requested changes:
1. Swap the names of the invalid_width and invalid_num_placements tests, or test the product of
those two values instead (as it should not be zero and should be less than XE_HW_ENGINE_MAX_INSTANCE).
2. Add width = 1 and num_placements = 1 to the drm_xe_exec_queue_create struct in invalid_extensions.
3. Check for EFAULT instead of EINVAL in the invalid_extensions test.
Once the above changes are applied (or refuted), you can add my RB:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-----Original Message-----
From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of nakshtra.goyal at intel.com
Sent: Friday, January 17, 2025 12:53 PM
To: igt-dev at lists.freedesktop.org; Gandi, Ramadevi <ramadevi.gandi at intel.com>
Cc: Gurram, Pravalika <pravalika.gurram at intel.com>
Subject: [PATCH i-g-t] tests/intel/xe_create: Check negative cases for exec_queue create/destroy ioctl
>
> From: Nakshtra Goyal <nakshtra.goyal at intel.com>
>
> Negetive test cases to check expected errors using invalid
> flags, width, pad, reserved bits, num_placements, extensions, exec_queue_id
NIT:
I'm always a bit concerned with these error-case tests that we might be hitting
false-negatives, or cases where the tests should fail but end up passing regardless.
We don't have a unique error code for every possible failure in the execution,
so one of two things could happen that would cause the test to pass without
correctly identifying the exercised issue:
1. The test fails earlier than expected, but with the expected error value. The test would
still pass, but we wouldn't be able to catch it until the same failure is picked up again
during regular execution. This would mean the test isn't exercising what it's supposed to
be exercising, which is bad on its own.
2. The test fails later than expected, but with the expected error value. This would
arguably be worse than the first case, because that would definitely mean that the
error case we're trying to exercise isn't properly being handled.
In both cases, the issue could bleed into regular execution.
>
> Signed-off-by: Nakshtra Goyal <nakshtra.goyal at intel.com>
> ---
> tests/intel/xe_create.c | 141 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
>
> diff --git a/tests/intel/xe_create.c b/tests/intel/xe_create.c
> index 07e11036d..edc2a6178 100644
> --- a/tests/intel/xe_create.c
> +++ b/tests/intel/xe_create.c
> @@ -59,6 +59,123 @@ static int __create_bo(int fd, uint32_t vm, uint64_t size, uint32_t placement,
> return ret;
> }
>
> +/**
> + * TEST: Negative test for exec-queue create/destroy ioctl
> + * Category: Core
> + * Mega feature: General Core features
> + * Sub-category: Synchronization
> + * Functionality: exec-queue create
> + * Test category: negative test
> + */
> +
> +/**
> + * SUBTEST: invalid-flag
> + * Description: Check ioctl with invalid flag returns expected error code
> + *
> + * SUBTEST: exec-queue-create-invalid-reserved
> + * Description: Send query with invalid reserved value for exec_queue_destroy ioctl
> + *
> + * SUBTEST: invalid-width
> + * Description: Check query with invalid width returns expected error code
> + *
> + * SUBTEST: invalid-num-placements
> + * Description: Check query with invalid num-placements returns expected error code
> + *
> + * SUBTEST: invalid-extensions
> + * Description: Check query with invalid extensions returns expected error code
> + */
> +
> +static void invalid_flag(int fd)
> +{
> + struct drm_xe_exec_queue_create create = {
> + .flags = -1,
NIT:
I'm fairly certain that for right now, any non-zero flag is invalid. We could
probably just use 1 here, but maybe this is to future-proof this test for when
flag support is added?
It's not particularly important to change, mind: -1 is just as invalid as 1 is.
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
> +}
> +
> +static void exec_queue_create_invalid_reserved(int fd)
> +{
> + struct drm_xe_exec_queue_create create = {
> + .reserved[0] = 0xffff,
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
> +
> + create.reserved[0] = 0;
> + create.reserved[1] = 0xffff;
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
> +}
> +
> +static void invalid_width(int fd)
> +{
> + struct drm_xe_exec_queue_create create = {
> + .width = 1,
I think that we only get an error with width and num placements if
width * num_placements == 0 or width * num_placements > XE_HW_ENGINE_MAX_INSTANCE.
So, here, I think it's technically the num_placements that is invalid because it should not be zero.
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
> +}
> +
> +static void invalid_num_placements(int fd)
> +{
> + struct drm_xe_exec_queue_create create = {
> + .num_placements = 1,
See above comment. I think it's technically the width that is invalid here.
NIT:
Maybe this test and the above invalid_width tests can be concatenated? Something like:
"""
static void invalid_len(int fd)
{
struct drm_xe_exec_queue_create create = {
.width = 1,
.num_placements = 1,
}
/* Test width = 0 */
create.width = 0;
do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
create.width = 1;
/* Test num_placements = 0 */
create.num_placements = 0;
do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
create.num_placements = 1;
/* Test length overflow (length = width * num_placements) */
create.width = 16; // FIXME: Refer to XE_HW_ENGINE_MAX_INSTANCE here.
create.num_placements = 16;
do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
}
"""
Or perhaps we should be testing length = 0 and length > XE_HW_ENGINE_MAX_INSTANCE
as the tests here instead of exercising width and num_placements separately?
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
> +}
> +
> +static void invalid_extensions(int fd)
> +{
> + struct drm_xe_exec_queue_create create = {
> + .extensions = -1,
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_CREATE, &create, EINVAL);
I ran this test on my end, and while the error value here is correct, I'm fairly certain
the ioctl is only reporting EINVAL because it's hitting the earlier invalid_num_placements
and invalid_width cases. With width and num_placements set to 1, we hit EFAULT
instead, which I think is what's expected to be hit when we run to
exec_queue_user_extensions in the execution stack (namely, due to the error reported
by __copy_from_user).
Though I can see why EINVAL was chosen here: we test something similar in
create-invalid-mbz, and there, passing any extensions returns EINVAL. However,
xe_exec_queue_create returns a different error value in this case.
Also, this is the only error test that goes beyond the width and num_placements check
during execution, so this should be the only place we need to set width and
num_placements values.
> +}
> +
> +/**
> + * SUBTEST: invalid-pad
> + * Description: Check query with invalid pad returns expected error code
> + *
> + * SUBTEST: exec-queue-destroy-invalid-reserved
> + * Description: Send query with invalid reserved value for exec_queue_destroy ioctl
> + *
> + * SUBTEST: invalid-exec-queue-id
> + * Description: Check query with invalid exec_queue_id returns expected error code
> + */
> +
> +static void invalid_pad(int fd)
> +{
> + struct drm_xe_exec_queue_destroy destroy = {
> + .pad = 1,
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_DESTROY, &destroy, EINVAL);
> +}
> +
> +static void exec_queue_destroy_invalid_reserved(int fd)
NIT:
Hmm...
I was originally going to point out that we probably don't need to declare "exec_queue_"
here and could just shorten the test name to "destroy_invalid_reserved", as then the
naming convention would match better with the create-invalid-mbz test. But then I
checked create-invalid-mbz more thoroughly, and realized that these tests and the
create-invalid-mbz test exercise two different utilities (exec queues and bos, respectively).
With that in mind, I'm not sure if this test suite (xe_create.c) is the correct place to be
exercising the exec_queue_create and exec_queue_destroy error functionalities.
Though on the other hand, I don't see us exercising anything even remotely similar in
any of the other test suites, so this is probably as good of a place as any to perform this
exercise.
-Jonathan Cavitt
> +{
> + struct drm_xe_exec_queue_destroy destroy = {
> + .reserved[0] = 0xffff,
> + };
> +
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_DESTROY, &destroy, EINVAL);
> +
> + destroy.reserved[0] = 0;
> + destroy.reserved[1] = 0xffff;
> + do_ioctl_err(fd, DRM_IOCTL_XE_EXEC_QUEUE_DESTROY, &destroy, EINVAL);
> +}
> +
> +static void invalid_exec_queue_id(int xe)
> +{
> + struct drm_xe_exec_queue_destroy args = {
> + .exec_queue_id = 0xffff,
> + };
> +
> + do_ioctl_err(xe, DRM_IOCTL_XE_EXEC_QUEUE_DESTROY, &args, ENOENT);
> +}
> +
> /**
> * SUBTEST: create-invalid-size
> * Functionality: ioctl
> @@ -392,6 +509,30 @@ igt_main_args("Q:p:", NULL, help_str, opt_handler, NULL)
> igt_fixture
> xe = drm_open_driver(DRIVER_XE);
>
> + igt_subtest("invalid-flag")
> + invalid_flag(xe);
> +
> + igt_subtest("exec-queue-create-invalid-reserved")
> + exec_queue_create_invalid_reserved(xe);
> +
> + igt_subtest("invalid-width")
> + invalid_width(xe);
> +
> + igt_subtest("invalid-num-placements")
> + invalid_num_placements(xe);
> +
> + igt_subtest("invalid-extensions")
> + invalid_extensions(xe);
> +
> + igt_subtest("invalid-pad")
> + invalid_pad(xe);
> +
> + igt_subtest("exec-queue-destroy-invalid-reserved")
> + exec_queue_destroy_invalid_reserved(xe);
> +
> + igt_subtest("invalid-exec-queue-id")
> + invalid_exec_queue_id(xe);
> +
> igt_subtest("create-invalid-mbz")
> create_invalid_mbz(xe);
>
> --
> 2.34.1
>
>
More information about the igt-dev
mailing list