[igt-dev] [PATCH i-g-t] tests/xe add invalid va access tests
Chang, Yu bruce
yu.bruce.chang at intel.com
Fri Sep 1 23:56:40 UTC 2023
> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: Friday, September 1, 2023 5:16 AM
> To: igt-dev at lists.freedesktop.org
> Cc: Chang, Yu bruce <yu.bruce.chang at intel.com>; Zeng, Oak
> <oak.zeng at intel.com>; Welty, Brian <brian.welty at intel.com>
> Subject: Re: [igt-dev] [PATCH i-g-t] tests/xe add invalid va access tests
>
> Hi Bruce,
>
> On 2023-08-31 at 23:46:54 +0000, Chang, Bruce wrote:
> > add the following invalid va access test cases:
> >
> > gpu fault scratch page command timeout
> > 1) no no yes
> > 2) no yes no
> > 3) yes no yes
> > 4) yes yes no
> >
> > TODO: with futher improvement of xe_wait_ufence, the test can check
> > more specific error code from it.
> >
> > v2: async vm/bind, and re-bind valid address
> > v3: added xe_wait_ufence check
> >
> > Signed-off-by: Bruce Chang <yu.bruce.chang at intel.com>
> > Cc: Oak Zeng <oak.zeng at intel.com>
> > Cc: Brian Welty <brian.welty at intel.com>
> > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
> > Cc: Matthew Brost <matthew.brost at intel.com>
> > ---
> > lib/xe/xe_ioctl.c | 15 +++-
> > lib/xe/xe_ioctl.h | 3 +
> > tests/meson.build | 1 +
> > tests/xe/xe_exec_invalid_va.c | 164
> > ++++++++++++++++++++++++++++++++++
>
> Why not placing this in xe_vm.c as a subtest? We have too much xe_exec tests.
> Or use other name like xe_vm_invalid.
>
xe_vm_invalid makes sense to me since this is really testing vm feature.
> > 4 files changed, 180 insertions(+), 3 deletions(-) create mode
> > 100644 tests/xe/xe_exec_invalid_va.c
> >
> > diff --git a/lib/xe/xe_ioctl.c b/lib/xe/xe_ioctl.c index
> > 730dcfd16..022f0cf04 100644
> > --- a/lib/xe/xe_ioctl.c
> > +++ b/lib/xe/xe_ioctl.c
> > @@ -415,9 +415,9 @@ void xe_exec_wait(int fd, uint32_t exec_queue,
> uint64_t addr)
> > syncobj_destroy(fd, sync.handle);
> > }
> >
> > -int64_t xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
> > +int64_t _xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
> > struct drm_xe_engine_class_instance *eci,
> > - int64_t timeout)
> > + int64_t timeout, bool timeout_assert)
> ------------------------------- ^
> Do not instroduce a bool parameter, rather make __function which will return
> errno in case of ioctl error, so later you may use it like:
> ret = __xe_wait_ufence(..., &val);
> igt_assert_f(!ret || ret == -ETIME, "error %d\n", ret);
>
Agree, was just following the original function convention. I can make this change.
>
> int __xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
> struct drm_xe_engine_class_instance *eci,
> int64_t timeout)
> int64_t timeout,
> int64_t *retval)
> {
> ...
> ret = igt_ioctl(fd, DRM_IOCTL_XE_WAIT_USER_FENCE, &wait);
> if (!ret)
> *retval = wait.timeout;
>
> return ret;
> }
>
> uint64_t xe_wait_ufence(...)
> {
> uint64_t val;
>
> igt_assert_eq(__xe_wait_ufence(..., &val), 0);
>
> return val;
> }
>
> Add description to each new/old library function you re/write.
>
Sure.
> > {
> > struct drm_xe_wait_user_fence wait = {
> > .addr = to_user_pointer(addr),
> > @@ -430,11 +430,20 @@ int64_t xe_wait_ufence(int fd, uint64_t *addr,
> uint64_t value,
> > .instances = eci ? to_user_pointer(eci) : 0,
> > };
> >
> > - igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_WAIT_USER_FENCE,
> &wait), 0);
> > + if (igt_ioctl(fd, DRM_IOCTL_XE_WAIT_USER_FENCE, &wait))
> > + igt_assert (!timeout_assert && errno == ETIME);
> >
> > return wait.timeout;
> > }
> >
> > +
> > +int64_t xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
> > + struct drm_xe_engine_class_instance *eci,
> > + int64_t timeout)
> > +{
> > + return _xe_wait_ufence(fd, addr, value, eci, timeout, true); }
> > +
> > /**
> > * xe_wait_ufence_abstime:
> > * @fd: xe device fd
> > diff --git a/lib/xe/xe_ioctl.h b/lib/xe/xe_ioctl.h index
> > 6c281b3bf..4bf7410a4 100644
> > --- a/lib/xe/xe_ioctl.h
> > +++ b/lib/xe/xe_ioctl.h
> > @@ -82,6 +82,9 @@ void xe_exec(int fd, struct drm_xe_exec *exec);
> > void xe_exec_sync(int fd, uint32_t exec_queue, uint64_t addr,
> > struct drm_xe_sync *sync, uint32_t num_syncs); void
> > xe_exec_wait(int fd, uint32_t exec_queue, uint64_t addr);
> > +int64_t _xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
> > + struct drm_xe_engine_class_instance *eci,
> > + int64_t timeout, bool timeout_assert);
> > int64_t xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
> > struct drm_xe_engine_class_instance *eci,
> > int64_t timeout);
> > diff --git a/tests/meson.build b/tests/meson.build index
> > 4d325bed1..8861b6c5b 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -276,6 +276,7 @@ xe_progs = [
> > 'xe_exec_reset',
> > 'xe_exec_store',
> > 'xe_exec_threads',
> > + 'xe_exec_invalid_va',
> > 'xe_exercise_blt',
> > 'xe_gpgpu_fill',
> > 'xe_guc_pc',
> > diff --git a/tests/xe/xe_exec_invalid_va.c
> > b/tests/xe/xe_exec_invalid_va.c new file mode 100644 index
> > 000000000..468b90c8a
> > --- /dev/null
> > +++ b/tests/xe/xe_exec_invalid_va.c
> > @@ -0,0 +1,164 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2023 Intel Corporation */
> > +
> > +/**
> > + * TEST: invalid va tests
> > + * Category: Hardware building block
> > + * Sub-category: execbuf
> > + * Functionality: fault mode
> > + * Test category: functionality test
> > + * GPU requirements: GPU needs support for
> > +DRM_XE_VM_CREATE_FAULT_MODE */
> > +
> > +#include <fcntl.h>
> > +
> > +#include "igt.h"
> > +#include "lib/igt_syncobj.h"
> > +#include "lib/intel_reg.h"
> > +#include "xe_drm.h"
> > +
> > +#include "xe/xe_ioctl.h"
> > +#include "xe/xe_query.h"
> > +#include <string.h>
> ----------- ^
> Move above after <fcntl.h>
>
Will do.
> > +
> > +static void insert_store(uint32_t *bb, uint64_t va, uint64_t data) {
> > + *bb++ = MI_STORE_DWORD_IMM_GEN4;
> > + *bb++ = lower_32_bits(va);
> > + *bb++ = upper_32_bits(va);
> > + *bb++ = data;
> > + *bb++ = MI_BATCH_BUFFER_END;
> > +}
> > +
> > +/**
> > + * SUBTEST: invalid-va
> > + * Description: Check driver handling of invalid va access
> > + * Run type: FULL
> > + *
> > + * SUBTEST: invalid-va-scratch
> > + * Description: Check driver handling of invalid va access with
> > +scratch page
> > + * Run type: FULL
> > + *
> > + * SUBTEST: invalid-va-fault
> > + * Description: Check driver handling of invalid va access with fault
> > +enabled
> > + * Run type: FULL
> > + *
> > + * SUBTEST: invalid-va-fault-scratch
> > + * Description: Check driver handling of invalid va access with fault
> > ++ scratch page
> > + * Run type: FULL
> > + *
> > + * arg[1]: for vm create flags
> > + */
> > +static void test_exec(int fd, uint32_t flags) {
> > + const uint64_t inv_addr = 0x20000000;
> > + const uint64_t addr = 0x1a0000;
> > +#define USER_FENCE_VALUE 0xdeadbeefdeadbeefull
> ------------------------------^
> imho better in uppercase
>
> > +#define ONE_SEC MS_TO_NS(1000)
> > +#define STORE_DATA 0xDEADBEAF
> ----------------------------- ^
> Inconsistent, use either upper or lower case.
>
Sure, will use upper case for both case.
Thanks,
Bruce
> Regards,
> Kamil
More information about the igt-dev
mailing list