[Intel-gfx] [PATCH i-g-t 3/4] hw-tests: Fix and update gem_bad_address

[Chris Wilson]

Quoting Petri Latvala (2017-12-13 12:58:15)
> From: Antonio Argenziano <antonio.argenziano at intel.com>
> 
> When writing to an invalid memory location, the HW should be clever
> enough to silently discard the write without disrupting execution.
> gem_bad_address aim at just that. The test has been updated to move away
> from the libDrm wrappers and use the IOCTL wrappers instead. Also the
> invalid address has been updated to be just outside of the GTT space.

It barely scratch the surfaces of what could be done to feed in a bad
address.

> 
> v2 (Petri): Split the directory changes to separate commits, fix
> indentation
> 
> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
> Signed-off-by: Petri Latvala <petri.latvala at intel.com>
> ---
>  tests/hw-tests/gem_bad_address.c | 69 +++++++++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/tests/hw-tests/gem_bad_address.c b/tests/hw-tests/gem_bad_address.c
> index a970dfa4..2d6112bd 100644
> --- a/tests/hw-tests/gem_bad_address.c
> +++ b/tests/hw-tests/gem_bad_address.c
> @@ -23,37 +23,53 @@
>   * Authors:
>   *    Eric Anholt <eric at anholt.net>
>   *    Jesse Barnes <jbarnes at virtuousgeek.org> (based on gem_bad_blit.c)
> + *    Antonio Argenziano <antonio.argenziano at intel.com>
>   *
>   */
>  
>  #include "igt.h"
> -#include <stdlib.h>
> -#include <stdio.h>
> -#include <string.h>
> -#include <fcntl.h>
> -#include <inttypes.h>
> -#include <errno.h>
> -#include <sys/stat.h>
> -#include <sys/time.h>
> -#include "drm.h"
> -#include "intel_bufmgr.h"
>  
> -static drm_intel_bufmgr *bufmgr;
> -struct intel_batchbuffer *batch;
> -
> -#define BAD_GTT_DEST ((512*1024*1024)) /* past end of aperture */
> +/*
> + * This test aims at verifying that writing to an invalid location in memory,
> + * doesn't cause hangs. The store command should be ignored completely by the
> + * HW and the whole process should be transparent to the user. Therefore,
> + * the test doesn't perform any validation check but expects the wrapping
> + * execution environment to check no hangs have occurred.

igt provides the facilities to detect GPU hangs. Wrap in
igt_fork_hang_detector, finish with gem_sync. Actual machine death sadly
requires an outside body.

> + *
> + * The test needs to send a privileged batch to be able to write to the GTT.
> + */
>  
>  static void
> -bad_store(void)
> +bad_store(uint32_t fd, uint32_t engine)
>  {
> -       BEGIN_BATCH(4, 0);
> -       OUT_BATCH(MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL | 1 << 21);
> -       OUT_BATCH(0);
> -       OUT_BATCH(BAD_GTT_DEST);
> -       OUT_BATCH(0xdeadbeef);
> -       ADVANCE_BATCH();
> +       struct drm_i915_gem_exec_object2 obj;
> +       struct drm_i915_gem_execbuffer2 execbuf;
> +
> +       uint32_t batch[16];
> +       int i = 0;
> +
> +       memset(&obj, 0, sizeof(obj));
> +       memset(&execbuf, 0, sizeof(execbuf));
> +
> +       execbuf.buffers_ptr = to_user_pointer(&obj);
> +       execbuf.buffer_count = 1;
> +       execbuf.flags = engine;
> +       execbuf.flags |= I915_EXEC_SECURE;

That is asking to turn off the HW validator (for the most part), and
requires drm_open_driver_master(). But since you are doing something
illegal in a context that allows you to shoot yourself in the foot,
what's the point?  What does it prove?

> -       intel_batchbuffer_flush(batch);
> +       obj.handle = gem_create(fd, 4096);
> +
> +       batch[i++] = MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL;

Note that MI_STORE_DWORD doesn't even need a bad address to take over
the machine on some platforms. You should start with a validation that a
known good store works, before testing whether a bad one is skipped.
-Chris