All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
@ 2018-06-27 19:44 ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-06-27 19:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev

Setup a userptr object that only has a read-only mapping back to a file
store (memfd). Then attempt to write into that mapping using the GPU and
assert that those writes do not land (while also writing via a writable
userptr mapping into the same memfd to verify that the GPU is working!)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 configure.ac              |   1 +
 lib/ioctl_wrappers.c      |   4 +-
 lib/ioctl_wrappers.h      |   4 +-
 lib/meson.build           |   1 +
 meson.build               |   1 +
 tests/Makefile.am         |   4 +-
 tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
 7 files changed, 342 insertions(+), 10 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1ee4e90e9..195963d4f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
 PKG_CHECK_MODULES(KMOD, [libkmod])
 PKG_CHECK_MODULES(PROCPS, [libprocps])
 PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
+PKG_CHECK_MODULES(SSL, [openssl])
 PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
 
 if test x$have_valgrind = xyes; then
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 79db44a8c..d5d2a4e4c 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
 	return madv.retained;
 }
 
-int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
+int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
 {
 	struct drm_i915_gem_userptr userptr;
 
@@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
  *
  * Returns userptr handle for the GEM object.
  */
-void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
+void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
 {
 	igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
 }
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index b966f72c9..8e2cd380b 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
 #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
 	uint32_t handle;
 };
-void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
-int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
+void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
+int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
 
 void gem_sw_finish(int fd, uint32_t handle);
 
diff --git a/lib/meson.build b/lib/meson.build
index 1a355414e..939167f91 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -62,6 +62,7 @@ lib_deps = [
 	pthreads,
 	math,
 	realtime,
+	ssl,
 ]
 
 if libdrm_intel.found()
diff --git a/meson.build b/meson.build
index 4d15d6238..638c01066 100644
--- a/meson.build
+++ b/meson.build
@@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
 libkmod = dependency('libkmod')
 libprocps = dependency('libprocps', required : true)
 libunwind = dependency('libunwind', required : true)
+ssl = dependency('openssl', required : true)
 
 valgrind = null_dep
 valgrindinfo = 'No'
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f41ad5096..ba307b220 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
 gem_tiled_swapping_LDADD = $(LDADD) -lpthread
 prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
 prime_self_import_LDADD = $(LDADD) -lpthread
-gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
-gem_userptr_blits_LDADD = $(LDADD) -lpthread
+gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
+gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
 perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
 
 gem_eio_LDADD = $(LDADD) -lrt
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 7e3b6ef38..30c6bc48c 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -43,13 +43,17 @@
 #include <fcntl.h>
 #include <inttypes.h>
 #include <errno.h>
+#include <setjmp.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/mman.h>
+#include <openssl/sha.h>
 #include <signal.h>
 #include <pthread.h>
 #include <time.h>
 
+#include <linux/memfd.h>
+
 #include "drm.h"
 #include "i915_drm.h"
 
@@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
 	return ret;
 }
 
+static void store_dword(int fd, uint32_t target,
+			uint32_t offset, uint32_t value)
+{
+	const int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16];
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = 0;
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = target;
+	obj[1].handle = gem_create(fd, 4096);
+
+	memset(&reloc, 0, sizeof(reloc));
+	reloc.target_handle = obj[0].handle;
+	reloc.presumed_offset = 0;
+	reloc.offset = sizeof(uint32_t);
+	reloc.delta = offset;
+	reloc.read_domains = I915_GEM_DOMAIN_RENDER;
+	reloc.write_domain = I915_GEM_DOMAIN_RENDER;
+	obj[1].relocs_ptr = to_user_pointer(&reloc);
+	obj[1].relocation_count = 1;
+
+	i = 0;
+	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+	if (gen >= 8) {
+		batch[++i] = offset;
+		batch[++i] = 0;
+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = offset;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = offset;
+	}
+	batch[++i] = value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[1].handle);
+}
+
 static uint32_t
 create_userptr(int fd, uint32_t val, uint32_t *ptr)
 {
@@ -941,6 +996,275 @@ static int test_dmabuf(void)
 	return 0;
 }
 
+static void test_readonly(int i915)
+{
+	unsigned char orig[SHA_DIGEST_LENGTH];
+	uint64_t aperture_size;
+	uint32_t whandle, rhandle;
+	size_t sz, total;
+	void *pages, *space;
+	int memfd;
+
+	/*
+	 * A small batch of pages; small enough to cheaply check for stray
+	 * writes but large enough that we don't create too many VMA pointing
+	 * back to this set from the large arena. The limit on total number
+	 * of VMA for a process is 65,536 (at least on this kernel).
+	 */
+	sz = 16 << 12;
+	memfd = memfd_create("pages", 0);
+	igt_require(memfd != -1);
+	igt_require(ftruncate(memfd, sz) == 0);
+
+	pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
+	igt_assert(pages != MAP_FAILED);
+
+	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
+	gem_close(i915, rhandle);
+
+	gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
+
+	total = 2048ull << 20;
+	aperture_size = gem_aperture_size(i915) / 2;
+	if (aperture_size < total)
+		total = aperture_size;
+	total = total / sz * sz;
+	igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
+		 total, total >> 12, sz >> 12);
+
+	/* Create an arena all pointing to the same set of pages */
+	space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
+	igt_require(space != MAP_FAILED);
+	for (size_t offset = 0; offset < total; offset += sz) {
+		igt_assert(mmap(space + offset, sz,
+				PROT_WRITE, MAP_SHARED | MAP_FIXED,
+				memfd, 0) != MAP_FAILED);
+		*(uint32_t *)(space + offset) = offset;
+	}
+	igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
+	igt_assert(mlock(space, total) == 0);
+	close(memfd);
+
+	/* Check we can create a normal userptr bo wrapping the wrapper */
+	gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
+	gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
+	for (size_t offset = 0; offset < total; offset += sz)
+		store_dword(i915, rhandle, offset + 4, offset / sz);
+	gem_sync(i915, rhandle);
+	igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
+	igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
+	gem_close(i915, rhandle);
+
+	/* Now enforce read-only henceforth */
+	igt_assert(mprotect(space, total, PROT_READ) == 0);
+
+	SHA1(pages, sz, orig);
+	igt_fork(child, 1) {
+		const int gen = intel_gen(intel_get_drm_devid(i915));
+		const int nreloc = 1024;
+		struct drm_i915_gem_relocation_entry *reloc;
+		struct drm_i915_gem_exec_object2 obj[2];
+		struct drm_i915_gem_execbuffer2 exec;
+		unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
+		uint32_t *batch;
+		int i;
+
+		reloc = calloc(sizeof(*reloc), nreloc);
+		gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
+
+
+		memset(obj, 0, sizeof(obj));
+		obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
+		obj[1].handle = gem_create(i915, 4096*16);
+		obj[1].relocation_count = nreloc;
+		obj[1].relocs_ptr = to_user_pointer(reloc);
+
+		batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
+
+		memset(&exec, 0, sizeof(exec));
+		exec.buffer_count =2;
+		exec.buffers_ptr = to_user_pointer(obj);
+
+		for_each_engine(i915, exec.flags) {
+			/* First tweak the backing store through the write */
+			i = 0;
+			obj[0].handle = whandle;
+			for (int n = 0; n < nreloc; n++) {
+				uint64_t offset;
+
+				reloc[n].target_handle = obj[0].handle;
+				reloc[n].delta = 4*(rand() % (sz/4));
+				reloc[n].offset = (i+1) * sizeof(uint32_t);
+				reloc[n].presumed_offset = obj[0].offset;
+				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
+				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
+
+				offset = reloc[n].presumed_offset + reloc[n].delta;
+
+				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+				if (gen >= 8) {
+					batch[++i] = offset;
+					batch[++i] = offset >> 32;
+				} else if (gen >= 4) {
+					batch[++i] = 0;
+					batch[++i] = offset;
+					reloc[n].offset += sizeof(uint32_t);
+				} else {
+					batch[i]--;
+					batch[++i] = offset;
+				}
+				batch[++i] = rand();
+				i++;
+			}
+			batch[i] = MI_BATCH_BUFFER_END;
+
+			gem_execbuf(i915, &exec);
+			gem_sync(i915, obj[0].handle);
+			SHA1(pages, sz, ref);
+
+			igt_assert(memcmp(ref, orig, sizeof(ref)));
+			memcpy(orig, ref, sizeof(orig));
+
+			/* Now try the same through the read-only handle */
+			i = 0;
+			obj[0].handle = rhandle;
+			for (int n = 0; n < nreloc; n++) {
+				uint64_t offset;
+
+				reloc[n].target_handle = obj[0].handle;
+				reloc[n].delta = 4*(rand() % (total/4));
+				reloc[n].offset = (i+1) * sizeof(uint32_t);
+				reloc[n].presumed_offset = obj[0].offset;
+				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
+				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
+
+				offset = reloc[n].presumed_offset + reloc[n].delta;
+
+				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+				if (gen >= 8) {
+					batch[++i] = offset;
+					batch[++i] = offset >> 32;
+				} else if (gen >= 4) {
+					batch[++i] = 0;
+					batch[++i] = offset;
+					reloc[n].offset += sizeof(uint32_t);
+				} else {
+					batch[i]--;
+					batch[++i] = offset;
+				}
+				batch[++i] = rand();
+				i++;
+			}
+			batch[i] = MI_BATCH_BUFFER_END;
+
+			gem_execbuf(i915, &exec);
+			gem_sync(i915, obj[0].handle);
+			SHA1(pages, sz, result);
+
+			/*
+			 * As the writes into the read-only GPU bo should fail,
+			 * the SHA1 hash of the backing store should be
+			 * unaffected.
+			 */
+			igt_assert(memcmp(ref, result, SHA_DIGEST_LENGTH) == 0);
+		}
+
+		munmap(batch, 16*4096);
+		gem_close(i915, obj[1].handle);
+		gem_close(i915, rhandle);
+	}
+	igt_waitchildren();
+
+	munmap(space, total);
+	munmap(pages, sz);
+}
+
+static jmp_buf sigjmp;
+static void sigjmp_handler(int sig)
+{
+	siglongjmp(sigjmp, sig);
+}
+
+static void test_readonly_mmap(int i915)
+{
+	unsigned char original[SHA_DIGEST_LENGTH];
+	unsigned char result[SHA_DIGEST_LENGTH];
+	uint32_t handle;
+	uint32_t sz;
+	void *pages;
+	void *ptr;
+	int sig;
+
+	igt_require(igt_setup_clflush());
+
+	sz = 16 << 12;
+	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
+	igt_assert(pages != MAP_FAILED);
+
+	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
+	gem_set_caching(i915, handle, 0);
+
+	memset(pages, 0xa5, sz);
+	igt_clflush_range(pages, sz);
+	SHA1(pages, sz, original);
+
+	ptr = __gem_mmap__gtt(i915, handle, sz, PROT_WRITE);
+	igt_assert(ptr == NULL);
+
+	ptr = gem_mmap__gtt(i915, handle, sz, PROT_READ);
+	gem_close(i915, handle);
+
+	if (!(sig = sigsetjmp(sigjmp, 1))) {
+		signal(SIGBUS, sigjmp_handler);
+		signal(SIGSEGV, sigjmp_handler);
+		memset(ptr, 0x5a, sz);
+		igt_assert(0);
+	}
+	igt_assert_eq(sig, SIGSEGV);
+
+	igt_assert(mprotect(ptr, sz, PROT_WRITE));
+	munmap(ptr, sz);
+
+	igt_clflush_range(pages, sz);
+	SHA1(pages, sz, result);
+	igt_assert(!memcmp(original, result, sizeof(original)));
+
+	munmap(pages, sz);
+}
+
+static void test_readonly_pwrite(int i915)
+{
+	unsigned char original[SHA_DIGEST_LENGTH];
+	unsigned char result[SHA_DIGEST_LENGTH];
+	uint32_t handle;
+	uint32_t sz;
+	void *pages;
+
+	igt_require(igt_setup_clflush());
+
+	sz = 16 << 12;
+	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
+	igt_assert(pages != MAP_FAILED);
+
+	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
+	memset(pages, 0xa5, sz);
+	SHA1(pages, sz, original);
+
+	for (int page = 0; page < 16; page++) {
+		char data[4096];
+
+		memset(data, page, sizeof(data));
+		igt_assert_eq(__gem_write(i915, handle, page << 12, data, sizeof(data)), -EINVAL);
+	}
+
+	gem_close(i915, handle);
+
+	SHA1(pages, sz, result);
+	igt_assert(!memcmp(original, result, sizeof(original)));
+
+	munmap(pages, sz);
+}
+
 static int test_usage_restrictions(int fd)
 {
 	void *ptr;
@@ -961,10 +1285,6 @@ static int test_usage_restrictions(int fd)
 	ret = __gem_userptr(fd, (char *)ptr + 1, PAGE_SIZE - 1, 0, userptr_flags, &handle);
 	igt_assert_neq(ret, 0);
 
-	/* Read-only not supported. */
-	ret = __gem_userptr(fd, (char *)ptr, PAGE_SIZE, 1, userptr_flags, &handle);
-	igt_assert_neq(ret, 0);
-
 	free(ptr);
 
 	return 0;
@@ -1502,6 +1822,15 @@ int main(int argc, char **argv)
 		igt_subtest("dmabuf-unsync")
 			test_dmabuf();
 
+		igt_subtest("readonly-unsync")
+			test_readonly(fd);
+
+		igt_subtest("readonly-mmap-unsync")
+			test_readonly_mmap(fd);
+
+		igt_subtest("readonly-pwrite-unsync")
+			test_readonly_pwrite(fd);
+
 		for (unsigned flags = 0; flags < ALL_FORKING_EVICTIONS + 1; flags++) {
 			igt_subtest_f("forked-unsync%s%s%s-%s",
 					flags & FORKING_EVICTIONS_SWAPPING ? "-swapping" : "",
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [igt-dev] [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
@ 2018-06-27 19:44 ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-06-27 19:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Tvrtko Ursulin

Setup a userptr object that only has a read-only mapping back to a file
store (memfd). Then attempt to write into that mapping using the GPU and
assert that those writes do not land (while also writing via a writable
userptr mapping into the same memfd to verify that the GPU is working!)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 configure.ac              |   1 +
 lib/ioctl_wrappers.c      |   4 +-
 lib/ioctl_wrappers.h      |   4 +-
 lib/meson.build           |   1 +
 meson.build               |   1 +
 tests/Makefile.am         |   4 +-
 tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
 7 files changed, 342 insertions(+), 10 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1ee4e90e9..195963d4f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
 PKG_CHECK_MODULES(KMOD, [libkmod])
 PKG_CHECK_MODULES(PROCPS, [libprocps])
 PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
+PKG_CHECK_MODULES(SSL, [openssl])
 PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
 
 if test x$have_valgrind = xyes; then
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 79db44a8c..d5d2a4e4c 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
 	return madv.retained;
 }
 
-int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
+int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
 {
 	struct drm_i915_gem_userptr userptr;
 
@@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
  *
  * Returns userptr handle for the GEM object.
  */
-void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
+void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
 {
 	igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
 }
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index b966f72c9..8e2cd380b 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
 #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
 	uint32_t handle;
 };
-void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
-int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
+void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
+int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
 
 void gem_sw_finish(int fd, uint32_t handle);
 
diff --git a/lib/meson.build b/lib/meson.build
index 1a355414e..939167f91 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -62,6 +62,7 @@ lib_deps = [
 	pthreads,
 	math,
 	realtime,
+	ssl,
 ]
 
 if libdrm_intel.found()
diff --git a/meson.build b/meson.build
index 4d15d6238..638c01066 100644
--- a/meson.build
+++ b/meson.build
@@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
 libkmod = dependency('libkmod')
 libprocps = dependency('libprocps', required : true)
 libunwind = dependency('libunwind', required : true)
+ssl = dependency('openssl', required : true)
 
 valgrind = null_dep
 valgrindinfo = 'No'
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f41ad5096..ba307b220 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
 gem_tiled_swapping_LDADD = $(LDADD) -lpthread
 prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
 prime_self_import_LDADD = $(LDADD) -lpthread
-gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
-gem_userptr_blits_LDADD = $(LDADD) -lpthread
+gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
+gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
 perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
 
 gem_eio_LDADD = $(LDADD) -lrt
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 7e3b6ef38..30c6bc48c 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -43,13 +43,17 @@
 #include <fcntl.h>
 #include <inttypes.h>
 #include <errno.h>
+#include <setjmp.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/mman.h>
+#include <openssl/sha.h>
 #include <signal.h>
 #include <pthread.h>
 #include <time.h>
 
+#include <linux/memfd.h>
+
 #include "drm.h"
 #include "i915_drm.h"
 
@@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
 	return ret;
 }
 
+static void store_dword(int fd, uint32_t target,
+			uint32_t offset, uint32_t value)
+{
+	const int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16];
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = 0;
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = target;
+	obj[1].handle = gem_create(fd, 4096);
+
+	memset(&reloc, 0, sizeof(reloc));
+	reloc.target_handle = obj[0].handle;
+	reloc.presumed_offset = 0;
+	reloc.offset = sizeof(uint32_t);
+	reloc.delta = offset;
+	reloc.read_domains = I915_GEM_DOMAIN_RENDER;
+	reloc.write_domain = I915_GEM_DOMAIN_RENDER;
+	obj[1].relocs_ptr = to_user_pointer(&reloc);
+	obj[1].relocation_count = 1;
+
+	i = 0;
+	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+	if (gen >= 8) {
+		batch[++i] = offset;
+		batch[++i] = 0;
+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = offset;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = offset;
+	}
+	batch[++i] = value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[1].handle);
+}
+
 static uint32_t
 create_userptr(int fd, uint32_t val, uint32_t *ptr)
 {
@@ -941,6 +996,275 @@ static int test_dmabuf(void)
 	return 0;
 }
 
+static void test_readonly(int i915)
+{
+	unsigned char orig[SHA_DIGEST_LENGTH];
+	uint64_t aperture_size;
+	uint32_t whandle, rhandle;
+	size_t sz, total;
+	void *pages, *space;
+	int memfd;
+
+	/*
+	 * A small batch of pages; small enough to cheaply check for stray
+	 * writes but large enough that we don't create too many VMA pointing
+	 * back to this set from the large arena. The limit on total number
+	 * of VMA for a process is 65,536 (at least on this kernel).
+	 */
+	sz = 16 << 12;
+	memfd = memfd_create("pages", 0);
+	igt_require(memfd != -1);
+	igt_require(ftruncate(memfd, sz) == 0);
+
+	pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
+	igt_assert(pages != MAP_FAILED);
+
+	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
+	gem_close(i915, rhandle);
+
+	gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
+
+	total = 2048ull << 20;
+	aperture_size = gem_aperture_size(i915) / 2;
+	if (aperture_size < total)
+		total = aperture_size;
+	total = total / sz * sz;
+	igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
+		 total, total >> 12, sz >> 12);
+
+	/* Create an arena all pointing to the same set of pages */
+	space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
+	igt_require(space != MAP_FAILED);
+	for (size_t offset = 0; offset < total; offset += sz) {
+		igt_assert(mmap(space + offset, sz,
+				PROT_WRITE, MAP_SHARED | MAP_FIXED,
+				memfd, 0) != MAP_FAILED);
+		*(uint32_t *)(space + offset) = offset;
+	}
+	igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
+	igt_assert(mlock(space, total) == 0);
+	close(memfd);
+
+	/* Check we can create a normal userptr bo wrapping the wrapper */
+	gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
+	gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
+	for (size_t offset = 0; offset < total; offset += sz)
+		store_dword(i915, rhandle, offset + 4, offset / sz);
+	gem_sync(i915, rhandle);
+	igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
+	igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
+	gem_close(i915, rhandle);
+
+	/* Now enforce read-only henceforth */
+	igt_assert(mprotect(space, total, PROT_READ) == 0);
+
+	SHA1(pages, sz, orig);
+	igt_fork(child, 1) {
+		const int gen = intel_gen(intel_get_drm_devid(i915));
+		const int nreloc = 1024;
+		struct drm_i915_gem_relocation_entry *reloc;
+		struct drm_i915_gem_exec_object2 obj[2];
+		struct drm_i915_gem_execbuffer2 exec;
+		unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
+		uint32_t *batch;
+		int i;
+
+		reloc = calloc(sizeof(*reloc), nreloc);
+		gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
+
+
+		memset(obj, 0, sizeof(obj));
+		obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
+		obj[1].handle = gem_create(i915, 4096*16);
+		obj[1].relocation_count = nreloc;
+		obj[1].relocs_ptr = to_user_pointer(reloc);
+
+		batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
+
+		memset(&exec, 0, sizeof(exec));
+		exec.buffer_count =2;
+		exec.buffers_ptr = to_user_pointer(obj);
+
+		for_each_engine(i915, exec.flags) {
+			/* First tweak the backing store through the write */
+			i = 0;
+			obj[0].handle = whandle;
+			for (int n = 0; n < nreloc; n++) {
+				uint64_t offset;
+
+				reloc[n].target_handle = obj[0].handle;
+				reloc[n].delta = 4*(rand() % (sz/4));
+				reloc[n].offset = (i+1) * sizeof(uint32_t);
+				reloc[n].presumed_offset = obj[0].offset;
+				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
+				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
+
+				offset = reloc[n].presumed_offset + reloc[n].delta;
+
+				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+				if (gen >= 8) {
+					batch[++i] = offset;
+					batch[++i] = offset >> 32;
+				} else if (gen >= 4) {
+					batch[++i] = 0;
+					batch[++i] = offset;
+					reloc[n].offset += sizeof(uint32_t);
+				} else {
+					batch[i]--;
+					batch[++i] = offset;
+				}
+				batch[++i] = rand();
+				i++;
+			}
+			batch[i] = MI_BATCH_BUFFER_END;
+
+			gem_execbuf(i915, &exec);
+			gem_sync(i915, obj[0].handle);
+			SHA1(pages, sz, ref);
+
+			igt_assert(memcmp(ref, orig, sizeof(ref)));
+			memcpy(orig, ref, sizeof(orig));
+
+			/* Now try the same through the read-only handle */
+			i = 0;
+			obj[0].handle = rhandle;
+			for (int n = 0; n < nreloc; n++) {
+				uint64_t offset;
+
+				reloc[n].target_handle = obj[0].handle;
+				reloc[n].delta = 4*(rand() % (total/4));
+				reloc[n].offset = (i+1) * sizeof(uint32_t);
+				reloc[n].presumed_offset = obj[0].offset;
+				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
+				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
+
+				offset = reloc[n].presumed_offset + reloc[n].delta;
+
+				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+				if (gen >= 8) {
+					batch[++i] = offset;
+					batch[++i] = offset >> 32;
+				} else if (gen >= 4) {
+					batch[++i] = 0;
+					batch[++i] = offset;
+					reloc[n].offset += sizeof(uint32_t);
+				} else {
+					batch[i]--;
+					batch[++i] = offset;
+				}
+				batch[++i] = rand();
+				i++;
+			}
+			batch[i] = MI_BATCH_BUFFER_END;
+
+			gem_execbuf(i915, &exec);
+			gem_sync(i915, obj[0].handle);
+			SHA1(pages, sz, result);
+
+			/*
+			 * As the writes into the read-only GPU bo should fail,
+			 * the SHA1 hash of the backing store should be
+			 * unaffected.
+			 */
+			igt_assert(memcmp(ref, result, SHA_DIGEST_LENGTH) == 0);
+		}
+
+		munmap(batch, 16*4096);
+		gem_close(i915, obj[1].handle);
+		gem_close(i915, rhandle);
+	}
+	igt_waitchildren();
+
+	munmap(space, total);
+	munmap(pages, sz);
+}
+
+static jmp_buf sigjmp;
+static void sigjmp_handler(int sig)
+{
+	siglongjmp(sigjmp, sig);
+}
+
+static void test_readonly_mmap(int i915)
+{
+	unsigned char original[SHA_DIGEST_LENGTH];
+	unsigned char result[SHA_DIGEST_LENGTH];
+	uint32_t handle;
+	uint32_t sz;
+	void *pages;
+	void *ptr;
+	int sig;
+
+	igt_require(igt_setup_clflush());
+
+	sz = 16 << 12;
+	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
+	igt_assert(pages != MAP_FAILED);
+
+	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
+	gem_set_caching(i915, handle, 0);
+
+	memset(pages, 0xa5, sz);
+	igt_clflush_range(pages, sz);
+	SHA1(pages, sz, original);
+
+	ptr = __gem_mmap__gtt(i915, handle, sz, PROT_WRITE);
+	igt_assert(ptr == NULL);
+
+	ptr = gem_mmap__gtt(i915, handle, sz, PROT_READ);
+	gem_close(i915, handle);
+
+	if (!(sig = sigsetjmp(sigjmp, 1))) {
+		signal(SIGBUS, sigjmp_handler);
+		signal(SIGSEGV, sigjmp_handler);
+		memset(ptr, 0x5a, sz);
+		igt_assert(0);
+	}
+	igt_assert_eq(sig, SIGSEGV);
+
+	igt_assert(mprotect(ptr, sz, PROT_WRITE));
+	munmap(ptr, sz);
+
+	igt_clflush_range(pages, sz);
+	SHA1(pages, sz, result);
+	igt_assert(!memcmp(original, result, sizeof(original)));
+
+	munmap(pages, sz);
+}
+
+static void test_readonly_pwrite(int i915)
+{
+	unsigned char original[SHA_DIGEST_LENGTH];
+	unsigned char result[SHA_DIGEST_LENGTH];
+	uint32_t handle;
+	uint32_t sz;
+	void *pages;
+
+	igt_require(igt_setup_clflush());
+
+	sz = 16 << 12;
+	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
+	igt_assert(pages != MAP_FAILED);
+
+	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
+	memset(pages, 0xa5, sz);
+	SHA1(pages, sz, original);
+
+	for (int page = 0; page < 16; page++) {
+		char data[4096];
+
+		memset(data, page, sizeof(data));
+		igt_assert_eq(__gem_write(i915, handle, page << 12, data, sizeof(data)), -EINVAL);
+	}
+
+	gem_close(i915, handle);
+
+	SHA1(pages, sz, result);
+	igt_assert(!memcmp(original, result, sizeof(original)));
+
+	munmap(pages, sz);
+}
+
 static int test_usage_restrictions(int fd)
 {
 	void *ptr;
@@ -961,10 +1285,6 @@ static int test_usage_restrictions(int fd)
 	ret = __gem_userptr(fd, (char *)ptr + 1, PAGE_SIZE - 1, 0, userptr_flags, &handle);
 	igt_assert_neq(ret, 0);
 
-	/* Read-only not supported. */
-	ret = __gem_userptr(fd, (char *)ptr, PAGE_SIZE, 1, userptr_flags, &handle);
-	igt_assert_neq(ret, 0);
-
 	free(ptr);
 
 	return 0;
@@ -1502,6 +1822,15 @@ int main(int argc, char **argv)
 		igt_subtest("dmabuf-unsync")
 			test_dmabuf();
 
+		igt_subtest("readonly-unsync")
+			test_readonly(fd);
+
+		igt_subtest("readonly-mmap-unsync")
+			test_readonly_mmap(fd);
+
+		igt_subtest("readonly-pwrite-unsync")
+			test_readonly_pwrite(fd);
+
 		for (unsigned flags = 0; flags < ALL_FORKING_EVICTIONS + 1; flags++) {
 			igt_subtest_f("forked-unsync%s%s%s-%s",
 					flags & FORKING_EVICTIONS_SWAPPING ? "-swapping" : "",
-- 
2.18.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for igt/gem_userptr: Check read-only mappings
  2018-06-27 19:44 ` [igt-dev] " Chris Wilson
  (?)
@ 2018-06-27 20:10 ` Patchwork
  -1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-06-27 20:10 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: igt/gem_userptr: Check read-only mappings
URL   : https://patchwork.freedesktop.org/series/45522/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4373 -> IGTPW_1504 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45522/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1504 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-no-display:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#105395) +2

    igt@gem_ringfill@basic-default-hang:
      fi-pnv-d510:        NOTRUN -> DMESG-WARN (fdo#101600)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       FAIL (fdo#106744) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128) -> PASS

    
  fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105395 https://bugs.freedesktop.org/show_bug.cgi?id=105395
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744


== Participating hosts (43 -> 40) ==

  Additional (2): fi-cnl-psr fi-pnv-d510 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * IGT: IGT_4529 -> IGTPW_1504

  CI_DRM_4373: be7193758db79443ad5dc45072a166746819ba7e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1504: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1504/
  IGT_4529: 23d50a49413aff619d00ec50fc2e051e9b45baa5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@gem_userptr_blits@readonly-mmap-unsync
+igt@gem_userptr_blits@readonly-pwrite-unsync
+igt@gem_userptr_blits@readonly-unsync

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1504/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for igt/gem_userptr: Check read-only mappings
  2018-06-27 19:44 ` [igt-dev] " Chris Wilson
  (?)
  (?)
@ 2018-06-28  0:17 ` Patchwork
  -1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-06-28  0:17 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: igt/gem_userptr: Check read-only mappings
URL   : https://patchwork.freedesktop.org/series/45522/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4529_full -> IGTPW_1504_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1504_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1504_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45522/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1504_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_gtt:
      {shard-glk9}:       NOTRUN -> INCOMPLETE

    igt@drv_selftest@mock_scatterlist:
      {shard-glk9}:       NOTRUN -> DMESG-WARN

    
    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          PASS -> SKIP +2

    
== Known issues ==

  Here are the changes found in IGTPW_1504_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@fbc-1p-indfb-fliptrack:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@kms_sysfs_edid_timing:
      {shard-glk9}:       NOTRUN -> WARN (fdo#100047)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-apl:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@gem_exec_params@rs-invalid-on-bsd-ring:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    igt@kms_cursor_crc@cursor-64x64-onscreen:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          FAIL (fdo#106509, fdo#105454) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@dpms-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render:
      shard-snb:          FAIL (fdo#103167, fdo#104724) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      shard-glk:          DMESG-WARN (fdo#106247) -> PASS

    igt@kms_rotation_crc@primary-rotation-180:
      shard-hsw:          FAIL (fdo#104724, fdo#103925) -> PASS

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-snb:          FAIL (fdo#104724, fdo#103925) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106247 https://bugs.freedesktop.org/show_bug.cgi?id=106247
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 6) ==

  Additional (1): shard-glk9 


== Build changes ==

    * IGT: IGT_4529 -> IGTPW_1504
    * Linux: CI_DRM_4371 -> CI_DRM_4373

  CI_DRM_4371: 9094e9d97a6e13db8c1a444d08c0988adad9a002 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4373: be7193758db79443ad5dc45072a166746819ba7e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1504: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1504/
  IGT_4529: 23d50a49413aff619d00ec50fc2e051e9b45baa5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1504/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
  2018-06-27 19:44 ` [igt-dev] " Chris Wilson
@ 2018-06-28 14:25   ` Mika Kuoppala
  -1 siblings, 0 replies; 12+ messages in thread
From: Mika Kuoppala @ 2018-06-28 14:25 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Setup a userptr object that only has a read-only mapping back to a file
> store (memfd). Then attempt to write into that mapping using the GPU and
> assert that those writes do not land (while also writing via a writable
> userptr mapping into the same memfd to verify that the GPU is working!)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  configure.ac              |   1 +
>  lib/ioctl_wrappers.c      |   4 +-
>  lib/ioctl_wrappers.h      |   4 +-
>  lib/meson.build           |   1 +
>  meson.build               |   1 +
>  tests/Makefile.am         |   4 +-
>  tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
>  7 files changed, 342 insertions(+), 10 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 1ee4e90e9..195963d4f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
>  PKG_CHECK_MODULES(KMOD, [libkmod])
>  PKG_CHECK_MODULES(PROCPS, [libprocps])
>  PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
> +PKG_CHECK_MODULES(SSL, [openssl])
>  PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
>  
>  if test x$have_valgrind = xyes; then
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 79db44a8c..d5d2a4e4c 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
>  	return madv.retained;
>  }
>  
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>  {
>  	struct drm_i915_gem_userptr userptr;
>  
> @@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
>   *
>   * Returns userptr handle for the GEM object.
>   */
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>  {
>  	igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
>  }
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b966f72c9..8e2cd380b 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
>  #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
>  	uint32_t handle;
>  };
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
>  
>  void gem_sw_finish(int fd, uint32_t handle);
>  
> diff --git a/lib/meson.build b/lib/meson.build
> index 1a355414e..939167f91 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -62,6 +62,7 @@ lib_deps = [
>  	pthreads,
>  	math,
>  	realtime,
> +	ssl,
>  ]
>  
>  if libdrm_intel.found()
> diff --git a/meson.build b/meson.build
> index 4d15d6238..638c01066 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
>  libkmod = dependency('libkmod')
>  libprocps = dependency('libprocps', required : true)
>  libunwind = dependency('libunwind', required : true)
> +ssl = dependency('openssl', required : true)
>  
>  valgrind = null_dep
>  valgrindinfo = 'No'
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index f41ad5096..ba307b220 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>  gem_tiled_swapping_LDADD = $(LDADD) -lpthread
>  prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>  prime_self_import_LDADD = $(LDADD) -lpthread
> -gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> -gem_userptr_blits_LDADD = $(LDADD) -lpthread
> +gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
> +gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
>  perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
>  
>  gem_eio_LDADD = $(LDADD) -lrt
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 7e3b6ef38..30c6bc48c 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -43,13 +43,17 @@
>  #include <fcntl.h>
>  #include <inttypes.h>
>  #include <errno.h>
> +#include <setjmp.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <sys/mman.h>
> +#include <openssl/sha.h>
>  #include <signal.h>
>  #include <pthread.h>
>  #include <time.h>
>  
> +#include <linux/memfd.h>
> +
>  #include "drm.h"
>  #include "i915_drm.h"
>  
> @@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
>  	return ret;
>  }
>  
> +static void store_dword(int fd, uint32_t target,
> +			uint32_t offset, uint32_t value)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_exec_object2 obj[2];
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	uint32_t batch[16];
> +	int i;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	execbuf.flags = 0;
> +	if (gen < 6)
> +		execbuf.flags |= I915_EXEC_SECURE;
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[0].handle = target;
> +	obj[1].handle = gem_create(fd, 4096);
> +
> +	memset(&reloc, 0, sizeof(reloc));
> +	reloc.target_handle = obj[0].handle;
> +	reloc.presumed_offset = 0;
> +	reloc.offset = sizeof(uint32_t);
> +	reloc.delta = offset;
> +	reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +	obj[1].relocs_ptr = to_user_pointer(&reloc);
> +	obj[1].relocation_count = 1;
> +
> +	i = 0;
> +	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +	if (gen >= 8) {
> +		batch[++i] = offset;
> +		batch[++i] = 0;
> +	} else if (gen >= 4) {
> +		batch[++i] = 0;
> +		batch[++i] = offset;
> +		reloc.offset += sizeof(uint32_t);
> +	} else {
> +		batch[i]--;
> +		batch[++i] = offset;
> +	}
> +	batch[++i] = value;
> +	batch[++i] = MI_BATCH_BUFFER_END;
> +	gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
> +	gem_execbuf(fd, &execbuf);
> +	gem_close(fd, obj[1].handle);
> +}
> +
>  static uint32_t
>  create_userptr(int fd, uint32_t val, uint32_t *ptr)
>  {
> @@ -941,6 +996,275 @@ static int test_dmabuf(void)
>  	return 0;
>  }
>  
> +static void test_readonly(int i915)
> +{
> +	unsigned char orig[SHA_DIGEST_LENGTH];
> +	uint64_t aperture_size;
> +	uint32_t whandle, rhandle;
> +	size_t sz, total;
> +	void *pages, *space;
> +	int memfd;
> +
> +	/*
> +	 * A small batch of pages; small enough to cheaply check for stray
> +	 * writes but large enough that we don't create too many VMA pointing
> +	 * back to this set from the large arena. The limit on total number
> +	 * of VMA for a process is 65,536 (at least on this kernel).
> +	 */
> +	sz = 16 << 12;
> +	memfd = memfd_create("pages", 0);
> +	igt_require(memfd != -1);
> +	igt_require(ftruncate(memfd, sz) == 0);
> +
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> +	gem_close(i915, rhandle);
> +
> +	gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> +
> +	total = 2048ull << 20;
> +	aperture_size = gem_aperture_size(i915) / 2;
> +	if (aperture_size < total)
> +		total = aperture_size;
> +	total = total / sz * sz;
> +	igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> +		 total, total >> 12, sz >> 12);
> +
> +	/* Create an arena all pointing to the same set of pages */
> +	space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
> +	igt_require(space != MAP_FAILED);
> +	for (size_t offset = 0; offset < total; offset += sz) {
> +		igt_assert(mmap(space + offset, sz,
> +				PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +				memfd, 0) != MAP_FAILED);
> +		*(uint32_t *)(space + offset) = offset;
> +	}
> +	igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
> +	igt_assert(mlock(space, total) == 0);
> +	close(memfd);
> +
> +	/* Check we can create a normal userptr bo wrapping the wrapper */
> +	gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> +	gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> +	for (size_t offset = 0; offset < total; offset += sz)
> +		store_dword(i915, rhandle, offset + 4, offset / sz);
> +	gem_sync(i915, rhandle);
> +	igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> +	igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
> +	gem_close(i915, rhandle);
> +
> +	/* Now enforce read-only henceforth */
> +	igt_assert(mprotect(space, total, PROT_READ) == 0);
> +
> +	SHA1(pages, sz, orig);
> +	igt_fork(child, 1) {
> +		const int gen = intel_gen(intel_get_drm_devid(i915));
> +		const int nreloc = 1024;
> +		struct drm_i915_gem_relocation_entry *reloc;
> +		struct drm_i915_gem_exec_object2 obj[2];
> +		struct drm_i915_gem_execbuffer2 exec;
> +		unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> +		uint32_t *batch;
> +		int i;
> +
> +		reloc = calloc(sizeof(*reloc), nreloc);
> +		gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> +
> +
> +		memset(obj, 0, sizeof(obj));
> +		obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> +		obj[1].handle = gem_create(i915, 4096*16);
> +		obj[1].relocation_count = nreloc;
> +		obj[1].relocs_ptr = to_user_pointer(reloc);
> +
> +		batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> +
> +		memset(&exec, 0, sizeof(exec));
> +		exec.buffer_count =2;
> +		exec.buffers_ptr = to_user_pointer(obj);
> +
> +		for_each_engine(i915, exec.flags) {
> +			/* First tweak the backing store through the write */
> +			i = 0;
> +			obj[0].handle = whandle;
> +			for (int n = 0; n < nreloc; n++) {
> +				uint64_t offset;
> +
> +				reloc[n].target_handle = obj[0].handle;
> +				reloc[n].delta = 4*(rand() % (sz/4));
> +				reloc[n].offset = (i+1) * sizeof(uint32_t);
> +				reloc[n].presumed_offset = obj[0].offset;
> +				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +
> +				offset = reloc[n].presumed_offset + reloc[n].delta;
> +
> +				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +				if (gen >= 8) {
> +					batch[++i] = offset;
> +					batch[++i] = offset >> 32;
> +				} else if (gen >= 4) {
> +					batch[++i] = 0;
> +					batch[++i] = offset;
> +					reloc[n].offset += sizeof(uint32_t);
> +				} else {
> +					batch[i]--;
> +					batch[++i] = offset;
> +				}
> +				batch[++i] = rand();
> +				i++;
> +			}
> +			batch[i] = MI_BATCH_BUFFER_END;
> +
> +			gem_execbuf(i915, &exec);
> +			gem_sync(i915, obj[0].handle);
> +			SHA1(pages, sz, ref);
> +
> +			igt_assert(memcmp(ref, orig, sizeof(ref)));
> +			memcpy(orig, ref, sizeof(orig));

This memcpy seems obsolete.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
@ 2018-06-28 14:25   ` Mika Kuoppala
  0 siblings, 0 replies; 12+ messages in thread
From: Mika Kuoppala @ 2018-06-28 14:25 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Setup a userptr object that only has a read-only mapping back to a file
> store (memfd). Then attempt to write into that mapping using the GPU and
> assert that those writes do not land (while also writing via a writable
> userptr mapping into the same memfd to verify that the GPU is working!)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  configure.ac              |   1 +
>  lib/ioctl_wrappers.c      |   4 +-
>  lib/ioctl_wrappers.h      |   4 +-
>  lib/meson.build           |   1 +
>  meson.build               |   1 +
>  tests/Makefile.am         |   4 +-
>  tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
>  7 files changed, 342 insertions(+), 10 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 1ee4e90e9..195963d4f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
>  PKG_CHECK_MODULES(KMOD, [libkmod])
>  PKG_CHECK_MODULES(PROCPS, [libprocps])
>  PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
> +PKG_CHECK_MODULES(SSL, [openssl])
>  PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
>  
>  if test x$have_valgrind = xyes; then
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 79db44a8c..d5d2a4e4c 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
>  	return madv.retained;
>  }
>  
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>  {
>  	struct drm_i915_gem_userptr userptr;
>  
> @@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
>   *
>   * Returns userptr handle for the GEM object.
>   */
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>  {
>  	igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
>  }
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b966f72c9..8e2cd380b 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
>  #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
>  	uint32_t handle;
>  };
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
>  
>  void gem_sw_finish(int fd, uint32_t handle);
>  
> diff --git a/lib/meson.build b/lib/meson.build
> index 1a355414e..939167f91 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -62,6 +62,7 @@ lib_deps = [
>  	pthreads,
>  	math,
>  	realtime,
> +	ssl,
>  ]
>  
>  if libdrm_intel.found()
> diff --git a/meson.build b/meson.build
> index 4d15d6238..638c01066 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
>  libkmod = dependency('libkmod')
>  libprocps = dependency('libprocps', required : true)
>  libunwind = dependency('libunwind', required : true)
> +ssl = dependency('openssl', required : true)
>  
>  valgrind = null_dep
>  valgrindinfo = 'No'
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index f41ad5096..ba307b220 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>  gem_tiled_swapping_LDADD = $(LDADD) -lpthread
>  prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>  prime_self_import_LDADD = $(LDADD) -lpthread
> -gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> -gem_userptr_blits_LDADD = $(LDADD) -lpthread
> +gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
> +gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
>  perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
>  
>  gem_eio_LDADD = $(LDADD) -lrt
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 7e3b6ef38..30c6bc48c 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -43,13 +43,17 @@
>  #include <fcntl.h>
>  #include <inttypes.h>
>  #include <errno.h>
> +#include <setjmp.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <sys/mman.h>
> +#include <openssl/sha.h>
>  #include <signal.h>
>  #include <pthread.h>
>  #include <time.h>
>  
> +#include <linux/memfd.h>
> +
>  #include "drm.h"
>  #include "i915_drm.h"
>  
> @@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
>  	return ret;
>  }
>  
> +static void store_dword(int fd, uint32_t target,
> +			uint32_t offset, uint32_t value)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_exec_object2 obj[2];
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	uint32_t batch[16];
> +	int i;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	execbuf.flags = 0;
> +	if (gen < 6)
> +		execbuf.flags |= I915_EXEC_SECURE;
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[0].handle = target;
> +	obj[1].handle = gem_create(fd, 4096);
> +
> +	memset(&reloc, 0, sizeof(reloc));
> +	reloc.target_handle = obj[0].handle;
> +	reloc.presumed_offset = 0;
> +	reloc.offset = sizeof(uint32_t);
> +	reloc.delta = offset;
> +	reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +	obj[1].relocs_ptr = to_user_pointer(&reloc);
> +	obj[1].relocation_count = 1;
> +
> +	i = 0;
> +	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +	if (gen >= 8) {
> +		batch[++i] = offset;
> +		batch[++i] = 0;
> +	} else if (gen >= 4) {
> +		batch[++i] = 0;
> +		batch[++i] = offset;
> +		reloc.offset += sizeof(uint32_t);
> +	} else {
> +		batch[i]--;
> +		batch[++i] = offset;
> +	}
> +	batch[++i] = value;
> +	batch[++i] = MI_BATCH_BUFFER_END;
> +	gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
> +	gem_execbuf(fd, &execbuf);
> +	gem_close(fd, obj[1].handle);
> +}
> +
>  static uint32_t
>  create_userptr(int fd, uint32_t val, uint32_t *ptr)
>  {
> @@ -941,6 +996,275 @@ static int test_dmabuf(void)
>  	return 0;
>  }
>  
> +static void test_readonly(int i915)
> +{
> +	unsigned char orig[SHA_DIGEST_LENGTH];
> +	uint64_t aperture_size;
> +	uint32_t whandle, rhandle;
> +	size_t sz, total;
> +	void *pages, *space;
> +	int memfd;
> +
> +	/*
> +	 * A small batch of pages; small enough to cheaply check for stray
> +	 * writes but large enough that we don't create too many VMA pointing
> +	 * back to this set from the large arena. The limit on total number
> +	 * of VMA for a process is 65,536 (at least on this kernel).
> +	 */
> +	sz = 16 << 12;
> +	memfd = memfd_create("pages", 0);
> +	igt_require(memfd != -1);
> +	igt_require(ftruncate(memfd, sz) == 0);
> +
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> +	gem_close(i915, rhandle);
> +
> +	gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> +
> +	total = 2048ull << 20;
> +	aperture_size = gem_aperture_size(i915) / 2;
> +	if (aperture_size < total)
> +		total = aperture_size;
> +	total = total / sz * sz;
> +	igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> +		 total, total >> 12, sz >> 12);
> +
> +	/* Create an arena all pointing to the same set of pages */
> +	space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
> +	igt_require(space != MAP_FAILED);
> +	for (size_t offset = 0; offset < total; offset += sz) {
> +		igt_assert(mmap(space + offset, sz,
> +				PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +				memfd, 0) != MAP_FAILED);
> +		*(uint32_t *)(space + offset) = offset;
> +	}
> +	igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
> +	igt_assert(mlock(space, total) == 0);
> +	close(memfd);
> +
> +	/* Check we can create a normal userptr bo wrapping the wrapper */
> +	gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> +	gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> +	for (size_t offset = 0; offset < total; offset += sz)
> +		store_dword(i915, rhandle, offset + 4, offset / sz);
> +	gem_sync(i915, rhandle);
> +	igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> +	igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
> +	gem_close(i915, rhandle);
> +
> +	/* Now enforce read-only henceforth */
> +	igt_assert(mprotect(space, total, PROT_READ) == 0);
> +
> +	SHA1(pages, sz, orig);
> +	igt_fork(child, 1) {
> +		const int gen = intel_gen(intel_get_drm_devid(i915));
> +		const int nreloc = 1024;
> +		struct drm_i915_gem_relocation_entry *reloc;
> +		struct drm_i915_gem_exec_object2 obj[2];
> +		struct drm_i915_gem_execbuffer2 exec;
> +		unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> +		uint32_t *batch;
> +		int i;
> +
> +		reloc = calloc(sizeof(*reloc), nreloc);
> +		gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> +
> +
> +		memset(obj, 0, sizeof(obj));
> +		obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> +		obj[1].handle = gem_create(i915, 4096*16);
> +		obj[1].relocation_count = nreloc;
> +		obj[1].relocs_ptr = to_user_pointer(reloc);
> +
> +		batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> +
> +		memset(&exec, 0, sizeof(exec));
> +		exec.buffer_count =2;
> +		exec.buffers_ptr = to_user_pointer(obj);
> +
> +		for_each_engine(i915, exec.flags) {
> +			/* First tweak the backing store through the write */
> +			i = 0;
> +			obj[0].handle = whandle;
> +			for (int n = 0; n < nreloc; n++) {
> +				uint64_t offset;
> +
> +				reloc[n].target_handle = obj[0].handle;
> +				reloc[n].delta = 4*(rand() % (sz/4));
> +				reloc[n].offset = (i+1) * sizeof(uint32_t);
> +				reloc[n].presumed_offset = obj[0].offset;
> +				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +
> +				offset = reloc[n].presumed_offset + reloc[n].delta;
> +
> +				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +				if (gen >= 8) {
> +					batch[++i] = offset;
> +					batch[++i] = offset >> 32;
> +				} else if (gen >= 4) {
> +					batch[++i] = 0;
> +					batch[++i] = offset;
> +					reloc[n].offset += sizeof(uint32_t);
> +				} else {
> +					batch[i]--;
> +					batch[++i] = offset;
> +				}
> +				batch[++i] = rand();
> +				i++;
> +			}
> +			batch[i] = MI_BATCH_BUFFER_END;
> +
> +			gem_execbuf(i915, &exec);
> +			gem_sync(i915, obj[0].handle);
> +			SHA1(pages, sz, ref);
> +
> +			igt_assert(memcmp(ref, orig, sizeof(ref)));
> +			memcpy(orig, ref, sizeof(orig));

This memcpy seems obsolete.
-Mika
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
  2018-06-28 14:25   ` [igt-dev] [Intel-gfx] " Mika Kuoppala
@ 2018-06-28 14:37     ` Chris Wilson
  -1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-06-28 14:37 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx; +Cc: igt-dev

Quoting Mika Kuoppala (2018-06-28 15:25:49)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Setup a userptr object that only has a read-only mapping back to a file
> > store (memfd). Then attempt to write into that mapping using the GPU and
> > assert that those writes do not land (while also writing via a writable
> > userptr mapping into the same memfd to verify that the GPU is working!)
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  configure.ac              |   1 +
> >  lib/ioctl_wrappers.c      |   4 +-
> >  lib/ioctl_wrappers.h      |   4 +-
> >  lib/meson.build           |   1 +
> >  meson.build               |   1 +
> >  tests/Makefile.am         |   4 +-
> >  tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
> >  7 files changed, 342 insertions(+), 10 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 1ee4e90e9..195963d4f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
> >  PKG_CHECK_MODULES(KMOD, [libkmod])
> >  PKG_CHECK_MODULES(PROCPS, [libprocps])
> >  PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
> > +PKG_CHECK_MODULES(SSL, [openssl])
> >  PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
> >  
> >  if test x$have_valgrind = xyes; then
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 79db44a8c..d5d2a4e4c 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
> >       return madv.retained;
> >  }
> >  
> > -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> > +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
> >  {
> >       struct drm_i915_gem_userptr userptr;
> >  
> > @@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
> >   *
> >   * Returns userptr handle for the GEM object.
> >   */
> > -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> > +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
> >  {
> >       igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
> >  }
> > diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> > index b966f72c9..8e2cd380b 100644
> > --- a/lib/ioctl_wrappers.h
> > +++ b/lib/ioctl_wrappers.h
> > @@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
> >  #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
> >       uint32_t handle;
> >  };
> > -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> > -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> > +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> > +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> >  
> >  void gem_sw_finish(int fd, uint32_t handle);
> >  
> > diff --git a/lib/meson.build b/lib/meson.build
> > index 1a355414e..939167f91 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -62,6 +62,7 @@ lib_deps = [
> >       pthreads,
> >       math,
> >       realtime,
> > +     ssl,
> >  ]
> >  
> >  if libdrm_intel.found()
> > diff --git a/meson.build b/meson.build
> > index 4d15d6238..638c01066 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
> >  libkmod = dependency('libkmod')
> >  libprocps = dependency('libprocps', required : true)
> >  libunwind = dependency('libunwind', required : true)
> > +ssl = dependency('openssl', required : true)
> >  
> >  valgrind = null_dep
> >  valgrindinfo = 'No'
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index f41ad5096..ba307b220 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> >  gem_tiled_swapping_LDADD = $(LDADD) -lpthread
> >  prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> >  prime_self_import_LDADD = $(LDADD) -lpthread
> > -gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> > -gem_userptr_blits_LDADD = $(LDADD) -lpthread
> > +gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
> > +gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
> >  perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
> >  
> >  gem_eio_LDADD = $(LDADD) -lrt
> > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> > index 7e3b6ef38..30c6bc48c 100644
> > --- a/tests/gem_userptr_blits.c
> > +++ b/tests/gem_userptr_blits.c
> > @@ -43,13 +43,17 @@
> >  #include <fcntl.h>
> >  #include <inttypes.h>
> >  #include <errno.h>
> > +#include <setjmp.h>
> >  #include <sys/stat.h>
> >  #include <sys/time.h>
> >  #include <sys/mman.h>
> > +#include <openssl/sha.h>
> >  #include <signal.h>
> >  #include <pthread.h>
> >  #include <time.h>
> >  
> > +#include <linux/memfd.h>
> > +
> >  #include "drm.h"
> >  #include "i915_drm.h"
> >  
> > @@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
> >       return ret;
> >  }
> >  
> > +static void store_dword(int fd, uint32_t target,
> > +                     uint32_t offset, uint32_t value)
> > +{
> > +     const int gen = intel_gen(intel_get_drm_devid(fd));
> > +     struct drm_i915_gem_exec_object2 obj[2];
> > +     struct drm_i915_gem_relocation_entry reloc;
> > +     struct drm_i915_gem_execbuffer2 execbuf;
> > +     uint32_t batch[16];
> > +     int i;
> > +
> > +     memset(&execbuf, 0, sizeof(execbuf));
> > +     execbuf.buffers_ptr = to_user_pointer(obj);
> > +     execbuf.buffer_count = ARRAY_SIZE(obj);
> > +     execbuf.flags = 0;
> > +     if (gen < 6)
> > +             execbuf.flags |= I915_EXEC_SECURE;
> > +
> > +     memset(obj, 0, sizeof(obj));
> > +     obj[0].handle = target;
> > +     obj[1].handle = gem_create(fd, 4096);
> > +
> > +     memset(&reloc, 0, sizeof(reloc));
> > +     reloc.target_handle = obj[0].handle;
> > +     reloc.presumed_offset = 0;
> > +     reloc.offset = sizeof(uint32_t);
> > +     reloc.delta = offset;
> > +     reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> > +     reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> > +     obj[1].relocs_ptr = to_user_pointer(&reloc);
> > +     obj[1].relocation_count = 1;
> > +
> > +     i = 0;
> > +     batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +     if (gen >= 8) {
> > +             batch[++i] = offset;
> > +             batch[++i] = 0;
> > +     } else if (gen >= 4) {
> > +             batch[++i] = 0;
> > +             batch[++i] = offset;
> > +             reloc.offset += sizeof(uint32_t);
> > +     } else {
> > +             batch[i]--;
> > +             batch[++i] = offset;
> > +     }
> > +     batch[++i] = value;
> > +     batch[++i] = MI_BATCH_BUFFER_END;
> > +     gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
> > +     gem_execbuf(fd, &execbuf);
> > +     gem_close(fd, obj[1].handle);
> > +}
> > +
> >  static uint32_t
> >  create_userptr(int fd, uint32_t val, uint32_t *ptr)
> >  {
> > @@ -941,6 +996,275 @@ static int test_dmabuf(void)
> >       return 0;
> >  }
> >  
> > +static void test_readonly(int i915)
> > +{
> > +     unsigned char orig[SHA_DIGEST_LENGTH];
> > +     uint64_t aperture_size;
> > +     uint32_t whandle, rhandle;
> > +     size_t sz, total;
> > +     void *pages, *space;
> > +     int memfd;
> > +
> > +     /*
> > +      * A small batch of pages; small enough to cheaply check for stray
> > +      * writes but large enough that we don't create too many VMA pointing
> > +      * back to this set from the large arena. The limit on total number
> > +      * of VMA for a process is 65,536 (at least on this kernel).
> > +      */
> > +     sz = 16 << 12;
> > +     memfd = memfd_create("pages", 0);
> > +     igt_require(memfd != -1);
> > +     igt_require(ftruncate(memfd, sz) == 0);
> > +
> > +     pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> > +     igt_assert(pages != MAP_FAILED);
> > +
> > +     igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> > +     gem_close(i915, rhandle);
> > +
> > +     gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> > +
> > +     total = 2048ull << 20;
> > +     aperture_size = gem_aperture_size(i915) / 2;
> > +     if (aperture_size < total)
> > +             total = aperture_size;
> > +     total = total / sz * sz;
> > +     igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> > +              total, total >> 12, sz >> 12);
> > +
> > +     /* Create an arena all pointing to the same set of pages */
> > +     space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
> > +     igt_require(space != MAP_FAILED);
> > +     for (size_t offset = 0; offset < total; offset += sz) {
> > +             igt_assert(mmap(space + offset, sz,
> > +                             PROT_WRITE, MAP_SHARED | MAP_FIXED,
> > +                             memfd, 0) != MAP_FAILED);
> > +             *(uint32_t *)(space + offset) = offset;
> > +     }
> > +     igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
> > +     igt_assert(mlock(space, total) == 0);
> > +     close(memfd);
> > +
> > +     /* Check we can create a normal userptr bo wrapping the wrapper */
> > +     gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> > +     gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> > +     for (size_t offset = 0; offset < total; offset += sz)
> > +             store_dword(i915, rhandle, offset + 4, offset / sz);
> > +     gem_sync(i915, rhandle);
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
> > +     gem_close(i915, rhandle);
> > +
> > +     /* Now enforce read-only henceforth */
> > +     igt_assert(mprotect(space, total, PROT_READ) == 0);
> > +
> > +     SHA1(pages, sz, orig);
> > +     igt_fork(child, 1) {
> > +             const int gen = intel_gen(intel_get_drm_devid(i915));
> > +             const int nreloc = 1024;
> > +             struct drm_i915_gem_relocation_entry *reloc;
> > +             struct drm_i915_gem_exec_object2 obj[2];
> > +             struct drm_i915_gem_execbuffer2 exec;
> > +             unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> > +             uint32_t *batch;
> > +             int i;
> > +
> > +             reloc = calloc(sizeof(*reloc), nreloc);
> > +             gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> > +
> > +
> > +             memset(obj, 0, sizeof(obj));
> > +             obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> > +             obj[1].handle = gem_create(i915, 4096*16);
> > +             obj[1].relocation_count = nreloc;
> > +             obj[1].relocs_ptr = to_user_pointer(reloc);
> > +
> > +             batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> > +
> > +             memset(&exec, 0, sizeof(exec));
> > +             exec.buffer_count =2;
> > +             exec.buffers_ptr = to_user_pointer(obj);
> > +
> > +             for_each_engine(i915, exec.flags) {
> > +                     /* First tweak the backing store through the write */
> > +                     i = 0;
> > +                     obj[0].handle = whandle;
> > +                     for (int n = 0; n < nreloc; n++) {
> > +                             uint64_t offset;
> > +
> > +                             reloc[n].target_handle = obj[0].handle;
> > +                             reloc[n].delta = 4*(rand() % (sz/4));
> > +                             reloc[n].offset = (i+1) * sizeof(uint32_t);
> > +                             reloc[n].presumed_offset = obj[0].offset;
> > +                             reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> > +                             reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +                             offset = reloc[n].presumed_offset + reloc[n].delta;
> > +
> > +                             batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +                             if (gen >= 8) {
> > +                                     batch[++i] = offset;
> > +                                     batch[++i] = offset >> 32;
> > +                             } else if (gen >= 4) {
> > +                                     batch[++i] = 0;
> > +                                     batch[++i] = offset;
> > +                                     reloc[n].offset += sizeof(uint32_t);
> > +                             } else {
> > +                                     batch[i]--;
> > +                                     batch[++i] = offset;
> > +                             }
> > +                             batch[++i] = rand();
> > +                             i++;
> > +                     }
> > +                     batch[i] = MI_BATCH_BUFFER_END;
> > +
> > +                     gem_execbuf(i915, &exec);
> > +                     gem_sync(i915, obj[0].handle);
> > +                     SHA1(pages, sz, ref);
> > +
> > +                     igt_assert(memcmp(ref, orig, sizeof(ref)));
> > +                     memcpy(orig, ref, sizeof(orig));
> 
> This memcpy seems obsolete.

We use the current state for the next engine. After we write to the
buffer, we want to compare against the current, not original, checksum
to be sure that writes are landing.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
@ 2018-06-28 14:37     ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-06-28 14:37 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx; +Cc: igt-dev

Quoting Mika Kuoppala (2018-06-28 15:25:49)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Setup a userptr object that only has a read-only mapping back to a file
> > store (memfd). Then attempt to write into that mapping using the GPU and
> > assert that those writes do not land (while also writing via a writable
> > userptr mapping into the same memfd to verify that the GPU is working!)
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  configure.ac              |   1 +
> >  lib/ioctl_wrappers.c      |   4 +-
> >  lib/ioctl_wrappers.h      |   4 +-
> >  lib/meson.build           |   1 +
> >  meson.build               |   1 +
> >  tests/Makefile.am         |   4 +-
> >  tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
> >  7 files changed, 342 insertions(+), 10 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 1ee4e90e9..195963d4f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
> >  PKG_CHECK_MODULES(KMOD, [libkmod])
> >  PKG_CHECK_MODULES(PROCPS, [libprocps])
> >  PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
> > +PKG_CHECK_MODULES(SSL, [openssl])
> >  PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
> >  
> >  if test x$have_valgrind = xyes; then
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 79db44a8c..d5d2a4e4c 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
> >       return madv.retained;
> >  }
> >  
> > -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> > +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
> >  {
> >       struct drm_i915_gem_userptr userptr;
> >  
> > @@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
> >   *
> >   * Returns userptr handle for the GEM object.
> >   */
> > -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> > +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
> >  {
> >       igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
> >  }
> > diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> > index b966f72c9..8e2cd380b 100644
> > --- a/lib/ioctl_wrappers.h
> > +++ b/lib/ioctl_wrappers.h
> > @@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
> >  #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
> >       uint32_t handle;
> >  };
> > -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> > -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> > +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> > +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> >  
> >  void gem_sw_finish(int fd, uint32_t handle);
> >  
> > diff --git a/lib/meson.build b/lib/meson.build
> > index 1a355414e..939167f91 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -62,6 +62,7 @@ lib_deps = [
> >       pthreads,
> >       math,
> >       realtime,
> > +     ssl,
> >  ]
> >  
> >  if libdrm_intel.found()
> > diff --git a/meson.build b/meson.build
> > index 4d15d6238..638c01066 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
> >  libkmod = dependency('libkmod')
> >  libprocps = dependency('libprocps', required : true)
> >  libunwind = dependency('libunwind', required : true)
> > +ssl = dependency('openssl', required : true)
> >  
> >  valgrind = null_dep
> >  valgrindinfo = 'No'
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index f41ad5096..ba307b220 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> >  gem_tiled_swapping_LDADD = $(LDADD) -lpthread
> >  prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> >  prime_self_import_LDADD = $(LDADD) -lpthread
> > -gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> > -gem_userptr_blits_LDADD = $(LDADD) -lpthread
> > +gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
> > +gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
> >  perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
> >  
> >  gem_eio_LDADD = $(LDADD) -lrt
> > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> > index 7e3b6ef38..30c6bc48c 100644
> > --- a/tests/gem_userptr_blits.c
> > +++ b/tests/gem_userptr_blits.c
> > @@ -43,13 +43,17 @@
> >  #include <fcntl.h>
> >  #include <inttypes.h>
> >  #include <errno.h>
> > +#include <setjmp.h>
> >  #include <sys/stat.h>
> >  #include <sys/time.h>
> >  #include <sys/mman.h>
> > +#include <openssl/sha.h>
> >  #include <signal.h>
> >  #include <pthread.h>
> >  #include <time.h>
> >  
> > +#include <linux/memfd.h>
> > +
> >  #include "drm.h"
> >  #include "i915_drm.h"
> >  
> > @@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
> >       return ret;
> >  }
> >  
> > +static void store_dword(int fd, uint32_t target,
> > +                     uint32_t offset, uint32_t value)
> > +{
> > +     const int gen = intel_gen(intel_get_drm_devid(fd));
> > +     struct drm_i915_gem_exec_object2 obj[2];
> > +     struct drm_i915_gem_relocation_entry reloc;
> > +     struct drm_i915_gem_execbuffer2 execbuf;
> > +     uint32_t batch[16];
> > +     int i;
> > +
> > +     memset(&execbuf, 0, sizeof(execbuf));
> > +     execbuf.buffers_ptr = to_user_pointer(obj);
> > +     execbuf.buffer_count = ARRAY_SIZE(obj);
> > +     execbuf.flags = 0;
> > +     if (gen < 6)
> > +             execbuf.flags |= I915_EXEC_SECURE;
> > +
> > +     memset(obj, 0, sizeof(obj));
> > +     obj[0].handle = target;
> > +     obj[1].handle = gem_create(fd, 4096);
> > +
> > +     memset(&reloc, 0, sizeof(reloc));
> > +     reloc.target_handle = obj[0].handle;
> > +     reloc.presumed_offset = 0;
> > +     reloc.offset = sizeof(uint32_t);
> > +     reloc.delta = offset;
> > +     reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> > +     reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> > +     obj[1].relocs_ptr = to_user_pointer(&reloc);
> > +     obj[1].relocation_count = 1;
> > +
> > +     i = 0;
> > +     batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +     if (gen >= 8) {
> > +             batch[++i] = offset;
> > +             batch[++i] = 0;
> > +     } else if (gen >= 4) {
> > +             batch[++i] = 0;
> > +             batch[++i] = offset;
> > +             reloc.offset += sizeof(uint32_t);
> > +     } else {
> > +             batch[i]--;
> > +             batch[++i] = offset;
> > +     }
> > +     batch[++i] = value;
> > +     batch[++i] = MI_BATCH_BUFFER_END;
> > +     gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
> > +     gem_execbuf(fd, &execbuf);
> > +     gem_close(fd, obj[1].handle);
> > +}
> > +
> >  static uint32_t
> >  create_userptr(int fd, uint32_t val, uint32_t *ptr)
> >  {
> > @@ -941,6 +996,275 @@ static int test_dmabuf(void)
> >       return 0;
> >  }
> >  
> > +static void test_readonly(int i915)
> > +{
> > +     unsigned char orig[SHA_DIGEST_LENGTH];
> > +     uint64_t aperture_size;
> > +     uint32_t whandle, rhandle;
> > +     size_t sz, total;
> > +     void *pages, *space;
> > +     int memfd;
> > +
> > +     /*
> > +      * A small batch of pages; small enough to cheaply check for stray
> > +      * writes but large enough that we don't create too many VMA pointing
> > +      * back to this set from the large arena. The limit on total number
> > +      * of VMA for a process is 65,536 (at least on this kernel).
> > +      */
> > +     sz = 16 << 12;
> > +     memfd = memfd_create("pages", 0);
> > +     igt_require(memfd != -1);
> > +     igt_require(ftruncate(memfd, sz) == 0);
> > +
> > +     pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> > +     igt_assert(pages != MAP_FAILED);
> > +
> > +     igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> > +     gem_close(i915, rhandle);
> > +
> > +     gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> > +
> > +     total = 2048ull << 20;
> > +     aperture_size = gem_aperture_size(i915) / 2;
> > +     if (aperture_size < total)
> > +             total = aperture_size;
> > +     total = total / sz * sz;
> > +     igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> > +              total, total >> 12, sz >> 12);
> > +
> > +     /* Create an arena all pointing to the same set of pages */
> > +     space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
> > +     igt_require(space != MAP_FAILED);
> > +     for (size_t offset = 0; offset < total; offset += sz) {
> > +             igt_assert(mmap(space + offset, sz,
> > +                             PROT_WRITE, MAP_SHARED | MAP_FIXED,
> > +                             memfd, 0) != MAP_FAILED);
> > +             *(uint32_t *)(space + offset) = offset;
> > +     }
> > +     igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
> > +     igt_assert(mlock(space, total) == 0);
> > +     close(memfd);
> > +
> > +     /* Check we can create a normal userptr bo wrapping the wrapper */
> > +     gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> > +     gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> > +     for (size_t offset = 0; offset < total; offset += sz)
> > +             store_dword(i915, rhandle, offset + 4, offset / sz);
> > +     gem_sync(i915, rhandle);
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
> > +     gem_close(i915, rhandle);
> > +
> > +     /* Now enforce read-only henceforth */
> > +     igt_assert(mprotect(space, total, PROT_READ) == 0);
> > +
> > +     SHA1(pages, sz, orig);
> > +     igt_fork(child, 1) {
> > +             const int gen = intel_gen(intel_get_drm_devid(i915));
> > +             const int nreloc = 1024;
> > +             struct drm_i915_gem_relocation_entry *reloc;
> > +             struct drm_i915_gem_exec_object2 obj[2];
> > +             struct drm_i915_gem_execbuffer2 exec;
> > +             unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> > +             uint32_t *batch;
> > +             int i;
> > +
> > +             reloc = calloc(sizeof(*reloc), nreloc);
> > +             gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> > +
> > +
> > +             memset(obj, 0, sizeof(obj));
> > +             obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> > +             obj[1].handle = gem_create(i915, 4096*16);
> > +             obj[1].relocation_count = nreloc;
> > +             obj[1].relocs_ptr = to_user_pointer(reloc);
> > +
> > +             batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> > +
> > +             memset(&exec, 0, sizeof(exec));
> > +             exec.buffer_count =2;
> > +             exec.buffers_ptr = to_user_pointer(obj);
> > +
> > +             for_each_engine(i915, exec.flags) {
> > +                     /* First tweak the backing store through the write */
> > +                     i = 0;
> > +                     obj[0].handle = whandle;
> > +                     for (int n = 0; n < nreloc; n++) {
> > +                             uint64_t offset;
> > +
> > +                             reloc[n].target_handle = obj[0].handle;
> > +                             reloc[n].delta = 4*(rand() % (sz/4));
> > +                             reloc[n].offset = (i+1) * sizeof(uint32_t);
> > +                             reloc[n].presumed_offset = obj[0].offset;
> > +                             reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> > +                             reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +                             offset = reloc[n].presumed_offset + reloc[n].delta;
> > +
> > +                             batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +                             if (gen >= 8) {
> > +                                     batch[++i] = offset;
> > +                                     batch[++i] = offset >> 32;
> > +                             } else if (gen >= 4) {
> > +                                     batch[++i] = 0;
> > +                                     batch[++i] = offset;
> > +                                     reloc[n].offset += sizeof(uint32_t);
> > +                             } else {
> > +                                     batch[i]--;
> > +                                     batch[++i] = offset;
> > +                             }
> > +                             batch[++i] = rand();
> > +                             i++;
> > +                     }
> > +                     batch[i] = MI_BATCH_BUFFER_END;
> > +
> > +                     gem_execbuf(i915, &exec);
> > +                     gem_sync(i915, obj[0].handle);
> > +                     SHA1(pages, sz, ref);
> > +
> > +                     igt_assert(memcmp(ref, orig, sizeof(ref)));
> > +                     memcpy(orig, ref, sizeof(orig));
> 
> This memcpy seems obsolete.

We use the current state for the next engine. After we write to the
buffer, we want to compare against the current, not original, checksum
to be sure that writes are landing.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
  2018-06-27 19:44 ` [igt-dev] " Chris Wilson
@ 2018-06-28 16:56   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-06-28 16:56 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 27/06/2018 20:44, Chris Wilson wrote:
> Setup a userptr object that only has a read-only mapping back to a file
> store (memfd). Then attempt to write into that mapping using the GPU and
> assert that those writes do not land (while also writing via a writable
> userptr mapping into the same memfd to verify that the GPU is working!)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   configure.ac              |   1 +
>   lib/ioctl_wrappers.c      |   4 +-
>   lib/ioctl_wrappers.h      |   4 +-
>   lib/meson.build           |   1 +
>   meson.build               |   1 +
>   tests/Makefile.am         |   4 +-
>   tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
>   7 files changed, 342 insertions(+), 10 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 1ee4e90e9..195963d4f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
>   PKG_CHECK_MODULES(KMOD, [libkmod])
>   PKG_CHECK_MODULES(PROCPS, [libprocps])
>   PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
> +PKG_CHECK_MODULES(SSL, [openssl])
>   PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
>   
>   if test x$have_valgrind = xyes; then
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 79db44a8c..d5d2a4e4c 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
>   	return madv.retained;
>   }
>   
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>   {
>   	struct drm_i915_gem_userptr userptr;
>   
> @@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
>    *
>    * Returns userptr handle for the GEM object.
>    */
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>   {
>   	igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
>   }
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b966f72c9..8e2cd380b 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
>   #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
>   	uint32_t handle;
>   };
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
>   
>   void gem_sw_finish(int fd, uint32_t handle);
>   
> diff --git a/lib/meson.build b/lib/meson.build
> index 1a355414e..939167f91 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -62,6 +62,7 @@ lib_deps = [
>   	pthreads,
>   	math,
>   	realtime,
> +	ssl,
>   ]
>   
>   if libdrm_intel.found()
> diff --git a/meson.build b/meson.build
> index 4d15d6238..638c01066 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
>   libkmod = dependency('libkmod')
>   libprocps = dependency('libprocps', required : true)
>   libunwind = dependency('libunwind', required : true)
> +ssl = dependency('openssl', required : true)
>   
>   valgrind = null_dep
>   valgrindinfo = 'No'
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index f41ad5096..ba307b220 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>   gem_tiled_swapping_LDADD = $(LDADD) -lpthread
>   prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>   prime_self_import_LDADD = $(LDADD) -lpthread
> -gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> -gem_userptr_blits_LDADD = $(LDADD) -lpthread
> +gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
> +gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
>   perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
>   
>   gem_eio_LDADD = $(LDADD) -lrt
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 7e3b6ef38..30c6bc48c 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -43,13 +43,17 @@
>   #include <fcntl.h>
>   #include <inttypes.h>
>   #include <errno.h>
> +#include <setjmp.h>
>   #include <sys/stat.h>
>   #include <sys/time.h>
>   #include <sys/mman.h>
> +#include <openssl/sha.h>
>   #include <signal.h>
>   #include <pthread.h>
>   #include <time.h>
>   
> +#include <linux/memfd.h>
> +
>   #include "drm.h"
>   #include "i915_drm.h"
>   
> @@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
>   	return ret;
>   }
>   
> +static void store_dword(int fd, uint32_t target,
> +			uint32_t offset, uint32_t value)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_exec_object2 obj[2];
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	uint32_t batch[16];
> +	int i;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	execbuf.flags = 0;
> +	if (gen < 6)
> +		execbuf.flags |= I915_EXEC_SECURE;
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[0].handle = target;
> +	obj[1].handle = gem_create(fd, 4096);
> +
> +	memset(&reloc, 0, sizeof(reloc));
> +	reloc.target_handle = obj[0].handle;
> +	reloc.presumed_offset = 0;
> +	reloc.offset = sizeof(uint32_t);
> +	reloc.delta = offset;
> +	reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +	obj[1].relocs_ptr = to_user_pointer(&reloc);
> +	obj[1].relocation_count = 1;
> +
> +	i = 0;
> +	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +	if (gen >= 8) {
> +		batch[++i] = offset;
> +		batch[++i] = 0;
> +	} else if (gen >= 4) {
> +		batch[++i] = 0;
> +		batch[++i] = offset;
> +		reloc.offset += sizeof(uint32_t);
> +	} else {
> +		batch[i]--;
> +		batch[++i] = offset;
> +	}
> +	batch[++i] = value;
> +	batch[++i] = MI_BATCH_BUFFER_END;
> +	gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
> +	gem_execbuf(fd, &execbuf);
> +	gem_close(fd, obj[1].handle);
> +}
> +
>   static uint32_t
>   create_userptr(int fd, uint32_t val, uint32_t *ptr)
>   {
> @@ -941,6 +996,275 @@ static int test_dmabuf(void)
>   	return 0;
>   }
>   
> +static void test_readonly(int i915)

Hm.. nice interesting and novel fd naming I think. fd, gem_fd I know we 
have. I wonder if we have drm_fd as well somewhere. Just thinking out 
loud...

> +{
> +	unsigned char orig[SHA_DIGEST_LENGTH];
> +	uint64_t aperture_size;
> +	uint32_t whandle, rhandle;
> +	size_t sz, total;
> +	void *pages, *space;
> +	int memfd;
> +
> +	/*
> +	 * A small batch of pages; small enough to cheaply check for stray
> +	 * writes but large enough that we don't create too many VMA pointing
> +	 * back to this set from the large arena. The limit on total number
> +	 * of VMA for a process is 65,536 (at least on this kernel).
> +	 */
> +	sz = 16 << 12;

12 for page size, so 16 pages? How it is related to VMA limit from the 
comment?

> +	memfd = memfd_create("pages", 0);
> +	igt_require(memfd != -1);

igt_require_fd is available if you care for it...

> +	igt_require(ftruncate(memfd, sz) == 0);

..and igt_require_eq, the double sided sword of API growing rich, huh? :)

> +
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> +	gem_close(i915, rhandle);
> +
> +	gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> +
> +	total = 2048ull << 20;

Why 2GiB? Express with the VMA limit and sz or just accidentally half of 
the VMA limit?

> +	aperture_size = gem_aperture_size(i915) / 2;
> +	if (aperture_size < total)
> +		total = aperture_size;
> +	total = total / sz * sz;

There is round_down in lib/igt_primes but it would need exporting.

> +	igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> +		 total, total >> 12, sz >> 12);
> +
> +	/* Create an arena all pointing to the same set of pages */
> +	space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);

Allocating address space only?

> +	igt_require(space != MAP_FAILED);
> +	for (size_t offset = 0; offset < total; offset += sz) {
> +		igt_assert(mmap(space + offset, sz,
> +				PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +				memfd, 0) != MAP_FAILED);
> +		*(uint32_t *)(space + offset) = offset;
> +	}
> +	igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));

Checking that "arena" somewhat works, ok..

> +	igt_assert(mlock(space, total) == 0);
> +	close(memfd);
> +
> +	/* Check we can create a normal userptr bo wrapping the wrapper */
> +	gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> +	gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> +	for (size_t offset = 0; offset < total; offset += sz)
> +		store_dword(i915, rhandle, offset + 4, offset / sz);
> +	gem_sync(i915, rhandle);

Do you need to move it back to CPU domain before the asserts?

> +	igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> +	igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));

Please add a comment somewhere higher up explaining the layout - I got 
lost what is in the first dword and what in the second of each page, and 
who writes each.

> +	gem_close(i915, rhandle);
> +
> +	/* Now enforce read-only henceforth */
> +	igt_assert(mprotect(space, total, PROT_READ) == 0);

No writes from the CPU, ok, I suppose to guarantee if there is a write 
where it came from.

Please add a high level comment what the following block will test and how.

> +
> +	SHA1(pages, sz, orig);
> +	igt_fork(child, 1) {
> +		const int gen = intel_gen(intel_get_drm_devid(i915));
> +		const int nreloc = 1024;

This has a relationship to the size of the batch buffer created lower below?

> +		struct drm_i915_gem_relocation_entry *reloc;
> +		struct drm_i915_gem_exec_object2 obj[2];
> +		struct drm_i915_gem_execbuffer2 exec;
> +		unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> +		uint32_t *batch;
> +		int i;
> +
> +		reloc = calloc(sizeof(*reloc), nreloc);
> +		gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> +
> +

Extra newline.

> +		memset(obj, 0, sizeof(obj));
> +		obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> +		obj[1].handle = gem_create(i915, 4096*16);

This is the size of store dw times times nreloc? Relationships need to 
be clearer and expressed in one place.

> +		obj[1].relocation_count = nreloc;
> +		obj[1].relocs_ptr = to_user_pointer(reloc);
> +
> +		batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> +
> +		memset(&exec, 0, sizeof(exec));
> +		exec.buffer_count =2;
> +		exec.buffers_ptr = to_user_pointer(obj);
> +
> +		for_each_engine(i915, exec.flags) {
> +			/* First tweak the backing store through the write */
> +			i = 0;
> +			obj[0].handle = whandle;
> +			for (int n = 0; n < nreloc; n++) {
> +				uint64_t offset;
> +
> +				reloc[n].target_handle = obj[0].handle;
> +				reloc[n].delta = 4*(rand() % (sz/4));
> +				reloc[n].offset = (i+1) * sizeof(uint32_t);

You can add spaces around operators to follow our coding style since 
space is not constrained here.

> +				reloc[n].presumed_offset = obj[0].offset;
> +				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +
> +				offset = reloc[n].presumed_offset + reloc[n].delta;
> +
> +				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +				if (gen >= 8) {
> +					batch[++i] = offset;
> +					batch[++i] = offset >> 32;
> +				} else if (gen >= 4) {
> +					batch[++i] = 0;
> +					batch[++i] = offset;
> +					reloc[n].offset += sizeof(uint32_t);
> +				} else {
> +					batch[i]--;
> +					batch[++i] = offset;
> +				}
> +				batch[++i] = rand();
> +				i++;
> +			}
> +			batch[i] = MI_BATCH_BUFFER_END;

Somehow make this possible via previously added store_dword helper 
instead of duplicating?

> +
> +			gem_execbuf(i915, &exec);
> +			gem_sync(i915, obj[0].handle);
> +			SHA1(pages, sz, ref);
> +
> +			igt_assert(memcmp(ref, orig, sizeof(ref)));
> +			memcpy(orig, ref, sizeof(orig));
> +
> +			/* Now try the same through the read-only handle */
> +			i = 0;
> +			obj[0].handle = rhandle;
> +			for (int n = 0; n < nreloc; n++) {
> +				uint64_t offset;
> +
> +				reloc[n].target_handle = obj[0].handle;
> +				reloc[n].delta = 4*(rand() % (total/4));
> +				reloc[n].offset = (i+1) * sizeof(uint32_t);
> +				reloc[n].presumed_offset = obj[0].offset;
> +				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +
> +				offset = reloc[n].presumed_offset + reloc[n].delta;
> +
> +				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +				if (gen >= 8) {
> +					batch[++i] = offset;
> +					batch[++i] = offset >> 32;
> +				} else if (gen >= 4) {
> +					batch[++i] = 0;
> +					batch[++i] = offset;
> +					reloc[n].offset += sizeof(uint32_t);
> +				} else {
> +					batch[i]--;
> +					batch[++i] = offset;
> +				}
> +				batch[++i] = rand();
> +				i++;
> +			}
> +			batch[i] = MI_BATCH_BUFFER_END;

Am I seeing a copy-pasted loop? You know what's next! :D

> +
> +			gem_execbuf(i915, &exec);
> +			gem_sync(i915, obj[0].handle);
> +			SHA1(pages, sz, result);
> +
> +			/*
> +			 * As the writes into the read-only GPU bo should fail,
> +			 * the SHA1 hash of the backing store should be
> +			 * unaffected.
> +			 */
> +			igt_assert(memcmp(ref, result, SHA_DIGEST_LENGTH) == 0);
> +		}
> +
> +		munmap(batch, 16*4096);
> +		gem_close(i915, obj[1].handle);
> +		gem_close(i915, rhandle);
> +	}
> +	igt_waitchildren();
> +
> +	munmap(space, total);
> +	munmap(pages, sz);
> +}

Okay more or less. Just want some tweaks and high level description 
since I (or anyone in the future) don't need/want to reverse engineer 
the patterns.

> +
> +static jmp_buf sigjmp;
> +static void sigjmp_handler(int sig)
> +{
> +	siglongjmp(sigjmp, sig);
> +}
> +
> +static void test_readonly_mmap(int i915)
> +{

Please add high level test description since there is some trickery below.

> +	unsigned char original[SHA_DIGEST_LENGTH];
> +	unsigned char result[SHA_DIGEST_LENGTH];
> +	uint32_t handle;
> +	uint32_t sz;
> +	void *pages;
> +	void *ptr;
> +	int sig;
> +
> +	igt_require(igt_setup_clflush());
> +
> +	sz = 16 << 12;
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
> +	gem_set_caching(i915, handle, 0);
> +
> +	memset(pages, 0xa5, sz);
> +	igt_clflush_range(pages, sz);

Why are cache flushed needed in this test? Because they cannot be done 
via domain management?

> +	SHA1(pages, sz, original);
> +
> +	ptr = __gem_mmap__gtt(i915, handle, sz, PROT_WRITE);
> +	igt_assert(ptr == NULL);
> +
> +	ptr = gem_mmap__gtt(i915, handle, sz, PROT_READ);
> +	gem_close(i915, handle);
> +
> +	if (!(sig = sigsetjmp(sigjmp, 1))) {

What does this do? Comment?

> +		signal(SIGBUS, sigjmp_handler);
> +		signal(SIGSEGV, sigjmp_handler);
> +		memset(ptr, 0x5a, sz);
> +		igt_assert(0);
> +	}
> +	igt_assert_eq(sig, SIGSEGV);
> +
> +	igt_assert(mprotect(ptr, sz, PROT_WRITE));

Why is this needed?

> +	munmap(ptr, sz);
> +
> +	igt_clflush_range(pages, sz);
> +	SHA1(pages, sz, result);
> +	igt_assert(!memcmp(original, result, sizeof(original)));
> +
> +	munmap(pages, sz);
> +}
> +
> +static void test_readonly_pwrite(int i915)
> +{
> +	unsigned char original[SHA_DIGEST_LENGTH];
> +	unsigned char result[SHA_DIGEST_LENGTH];
> +	uint32_t handle;
> +	uint32_t sz;
> +	void *pages;
> +
> +	igt_require(igt_setup_clflush());
> +
> +	sz = 16 << 12;
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
> +	memset(pages, 0xa5, sz);
> +	SHA1(pages, sz, original);
> +
> +	for (int page = 0; page < 16; page++) {
> +		char data[4096];
> +
> +		memset(data, page, sizeof(data));
> +		igt_assert_eq(__gem_write(i915, handle, page << 12, data, sizeof(data)), -EINVAL);
> +	}
> +
> +	gem_close(i915, handle);
> +
> +	SHA1(pages, sz, result);
> +	igt_assert(!memcmp(original, result, sizeof(original)));
> +
> +	munmap(pages, sz);
> +}
> +
>   static int test_usage_restrictions(int fd)
>   {
>   	void *ptr;
> @@ -961,10 +1285,6 @@ static int test_usage_restrictions(int fd)
>   	ret = __gem_userptr(fd, (char *)ptr + 1, PAGE_SIZE - 1, 0, userptr_flags, &handle);
>   	igt_assert_neq(ret, 0);
>   
> -	/* Read-only not supported. */
> -	ret = __gem_userptr(fd, (char *)ptr, PAGE_SIZE, 1, userptr_flags, &handle);
> -	igt_assert_neq(ret, 0);
> -
>   	free(ptr);
>   
>   	return 0;
> @@ -1502,6 +1822,15 @@ int main(int argc, char **argv)
>   		igt_subtest("dmabuf-unsync")
>   			test_dmabuf();
>   
> +		igt_subtest("readonly-unsync")
> +			test_readonly(fd);
> +
> +		igt_subtest("readonly-mmap-unsync")
> +			test_readonly_mmap(fd);
> +
> +		igt_subtest("readonly-pwrite-unsync")
> +			test_readonly_pwrite(fd);
> +
>   		for (unsigned flags = 0; flags < ALL_FORKING_EVICTIONS + 1; flags++) {
>   			igt_subtest_f("forked-unsync%s%s%s-%s",
>   					flags & FORKING_EVICTIONS_SWAPPING ? "-swapping" : "",
> 

Looks like thorough coverage, just some final polish needed.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
@ 2018-06-28 16:56   ` Tvrtko Ursulin
  0 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-06-28 16:56 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 27/06/2018 20:44, Chris Wilson wrote:
> Setup a userptr object that only has a read-only mapping back to a file
> store (memfd). Then attempt to write into that mapping using the GPU and
> assert that those writes do not land (while also writing via a writable
> userptr mapping into the same memfd to verify that the GPU is working!)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   configure.ac              |   1 +
>   lib/ioctl_wrappers.c      |   4 +-
>   lib/ioctl_wrappers.h      |   4 +-
>   lib/meson.build           |   1 +
>   meson.build               |   1 +
>   tests/Makefile.am         |   4 +-
>   tests/gem_userptr_blits.c | 337 +++++++++++++++++++++++++++++++++++++-
>   7 files changed, 342 insertions(+), 10 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 1ee4e90e9..195963d4f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,6 +125,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
>   PKG_CHECK_MODULES(KMOD, [libkmod])
>   PKG_CHECK_MODULES(PROCPS, [libprocps])
>   PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
> +PKG_CHECK_MODULES(SSL, [openssl])
>   PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
>   
>   if test x$have_valgrind = xyes; then
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 79db44a8c..d5d2a4e4c 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -869,7 +869,7 @@ int gem_madvise(int fd, uint32_t handle, int state)
>   	return madv.retained;
>   }
>   
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>   {
>   	struct drm_i915_gem_userptr userptr;
>   
> @@ -898,7 +898,7 @@ int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, ui
>    *
>    * Returns userptr handle for the GEM object.
>    */
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle)
>   {
>   	igt_assert_eq(__gem_userptr(fd, ptr, size, read_only, flags, handle), 0);
>   }
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b966f72c9..8e2cd380b 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -133,8 +133,8 @@ struct local_i915_gem_userptr {
>   #define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31)
>   	uint32_t handle;
>   };
> -void gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> -int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle);
> +void gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
> +int __gem_userptr(int fd, void *ptr, uint64_t size, int read_only, uint32_t flags, uint32_t *handle);
>   
>   void gem_sw_finish(int fd, uint32_t handle);
>   
> diff --git a/lib/meson.build b/lib/meson.build
> index 1a355414e..939167f91 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -62,6 +62,7 @@ lib_deps = [
>   	pthreads,
>   	math,
>   	realtime,
> +	ssl,
>   ]
>   
>   if libdrm_intel.found()
> diff --git a/meson.build b/meson.build
> index 4d15d6238..638c01066 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -98,6 +98,7 @@ pciaccess = dependency('pciaccess', version : '>=0.10')
>   libkmod = dependency('libkmod')
>   libprocps = dependency('libprocps', required : true)
>   libunwind = dependency('libunwind', required : true)
> +ssl = dependency('openssl', required : true)
>   
>   valgrind = null_dep
>   valgrindinfo = 'No'
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index f41ad5096..ba307b220 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -126,8 +126,8 @@ gem_tiled_swapping_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>   gem_tiled_swapping_LDADD = $(LDADD) -lpthread
>   prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
>   prime_self_import_LDADD = $(LDADD) -lpthread
> -gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> -gem_userptr_blits_LDADD = $(LDADD) -lpthread
> +gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) $(SSL_CFLAGS)
> +gem_userptr_blits_LDADD = $(LDADD) $(SSL_LIBS) -lpthread
>   perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
>   
>   gem_eio_LDADD = $(LDADD) -lrt
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 7e3b6ef38..30c6bc48c 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -43,13 +43,17 @@
>   #include <fcntl.h>
>   #include <inttypes.h>
>   #include <errno.h>
> +#include <setjmp.h>
>   #include <sys/stat.h>
>   #include <sys/time.h>
>   #include <sys/mman.h>
> +#include <openssl/sha.h>
>   #include <signal.h>
>   #include <pthread.h>
>   #include <time.h>
>   
> +#include <linux/memfd.h>
> +
>   #include "drm.h"
>   #include "i915_drm.h"
>   
> @@ -238,6 +242,57 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo)
>   	return ret;
>   }
>   
> +static void store_dword(int fd, uint32_t target,
> +			uint32_t offset, uint32_t value)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_exec_object2 obj[2];
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	uint32_t batch[16];
> +	int i;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = ARRAY_SIZE(obj);
> +	execbuf.flags = 0;
> +	if (gen < 6)
> +		execbuf.flags |= I915_EXEC_SECURE;
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[0].handle = target;
> +	obj[1].handle = gem_create(fd, 4096);
> +
> +	memset(&reloc, 0, sizeof(reloc));
> +	reloc.target_handle = obj[0].handle;
> +	reloc.presumed_offset = 0;
> +	reloc.offset = sizeof(uint32_t);
> +	reloc.delta = offset;
> +	reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> +	reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> +	obj[1].relocs_ptr = to_user_pointer(&reloc);
> +	obj[1].relocation_count = 1;
> +
> +	i = 0;
> +	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +	if (gen >= 8) {
> +		batch[++i] = offset;
> +		batch[++i] = 0;
> +	} else if (gen >= 4) {
> +		batch[++i] = 0;
> +		batch[++i] = offset;
> +		reloc.offset += sizeof(uint32_t);
> +	} else {
> +		batch[i]--;
> +		batch[++i] = offset;
> +	}
> +	batch[++i] = value;
> +	batch[++i] = MI_BATCH_BUFFER_END;
> +	gem_write(fd, obj[1].handle, 0, batch, sizeof(batch));
> +	gem_execbuf(fd, &execbuf);
> +	gem_close(fd, obj[1].handle);
> +}
> +
>   static uint32_t
>   create_userptr(int fd, uint32_t val, uint32_t *ptr)
>   {
> @@ -941,6 +996,275 @@ static int test_dmabuf(void)
>   	return 0;
>   }
>   
> +static void test_readonly(int i915)

Hm.. nice interesting and novel fd naming I think. fd, gem_fd I know we 
have. I wonder if we have drm_fd as well somewhere. Just thinking out 
loud...

> +{
> +	unsigned char orig[SHA_DIGEST_LENGTH];
> +	uint64_t aperture_size;
> +	uint32_t whandle, rhandle;
> +	size_t sz, total;
> +	void *pages, *space;
> +	int memfd;
> +
> +	/*
> +	 * A small batch of pages; small enough to cheaply check for stray
> +	 * writes but large enough that we don't create too many VMA pointing
> +	 * back to this set from the large arena. The limit on total number
> +	 * of VMA for a process is 65,536 (at least on this kernel).
> +	 */
> +	sz = 16 << 12;

12 for page size, so 16 pages? How it is related to VMA limit from the 
comment?

> +	memfd = memfd_create("pages", 0);
> +	igt_require(memfd != -1);

igt_require_fd is available if you care for it...

> +	igt_require(ftruncate(memfd, sz) == 0);

..and igt_require_eq, the double sided sword of API growing rich, huh? :)

> +
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> +	gem_close(i915, rhandle);
> +
> +	gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> +
> +	total = 2048ull << 20;

Why 2GiB? Express with the VMA limit and sz or just accidentally half of 
the VMA limit?

> +	aperture_size = gem_aperture_size(i915) / 2;
> +	if (aperture_size < total)
> +		total = aperture_size;
> +	total = total / sz * sz;

There is round_down in lib/igt_primes but it would need exporting.

> +	igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> +		 total, total >> 12, sz >> 12);
> +
> +	/* Create an arena all pointing to the same set of pages */
> +	space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);

Allocating address space only?

> +	igt_require(space != MAP_FAILED);
> +	for (size_t offset = 0; offset < total; offset += sz) {
> +		igt_assert(mmap(space + offset, sz,
> +				PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +				memfd, 0) != MAP_FAILED);
> +		*(uint32_t *)(space + offset) = offset;
> +	}
> +	igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));

Checking that "arena" somewhat works, ok..

> +	igt_assert(mlock(space, total) == 0);
> +	close(memfd);
> +
> +	/* Check we can create a normal userptr bo wrapping the wrapper */
> +	gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> +	gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> +	for (size_t offset = 0; offset < total; offset += sz)
> +		store_dword(i915, rhandle, offset + 4, offset / sz);
> +	gem_sync(i915, rhandle);

Do you need to move it back to CPU domain before the asserts?

> +	igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> +	igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));

Please add a comment somewhere higher up explaining the layout - I got 
lost what is in the first dword and what in the second of each page, and 
who writes each.

> +	gem_close(i915, rhandle);
> +
> +	/* Now enforce read-only henceforth */
> +	igt_assert(mprotect(space, total, PROT_READ) == 0);

No writes from the CPU, ok, I suppose to guarantee if there is a write 
where it came from.

Please add a high level comment what the following block will test and how.

> +
> +	SHA1(pages, sz, orig);
> +	igt_fork(child, 1) {
> +		const int gen = intel_gen(intel_get_drm_devid(i915));
> +		const int nreloc = 1024;

This has a relationship to the size of the batch buffer created lower below?

> +		struct drm_i915_gem_relocation_entry *reloc;
> +		struct drm_i915_gem_exec_object2 obj[2];
> +		struct drm_i915_gem_execbuffer2 exec;
> +		unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> +		uint32_t *batch;
> +		int i;
> +
> +		reloc = calloc(sizeof(*reloc), nreloc);
> +		gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> +
> +

Extra newline.

> +		memset(obj, 0, sizeof(obj));
> +		obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> +		obj[1].handle = gem_create(i915, 4096*16);

This is the size of store dw times times nreloc? Relationships need to 
be clearer and expressed in one place.

> +		obj[1].relocation_count = nreloc;
> +		obj[1].relocs_ptr = to_user_pointer(reloc);
> +
> +		batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> +
> +		memset(&exec, 0, sizeof(exec));
> +		exec.buffer_count =2;
> +		exec.buffers_ptr = to_user_pointer(obj);
> +
> +		for_each_engine(i915, exec.flags) {
> +			/* First tweak the backing store through the write */
> +			i = 0;
> +			obj[0].handle = whandle;
> +			for (int n = 0; n < nreloc; n++) {
> +				uint64_t offset;
> +
> +				reloc[n].target_handle = obj[0].handle;
> +				reloc[n].delta = 4*(rand() % (sz/4));
> +				reloc[n].offset = (i+1) * sizeof(uint32_t);

You can add spaces around operators to follow our coding style since 
space is not constrained here.

> +				reloc[n].presumed_offset = obj[0].offset;
> +				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +
> +				offset = reloc[n].presumed_offset + reloc[n].delta;
> +
> +				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +				if (gen >= 8) {
> +					batch[++i] = offset;
> +					batch[++i] = offset >> 32;
> +				} else if (gen >= 4) {
> +					batch[++i] = 0;
> +					batch[++i] = offset;
> +					reloc[n].offset += sizeof(uint32_t);
> +				} else {
> +					batch[i]--;
> +					batch[++i] = offset;
> +				}
> +				batch[++i] = rand();
> +				i++;
> +			}
> +			batch[i] = MI_BATCH_BUFFER_END;

Somehow make this possible via previously added store_dword helper 
instead of duplicating?

> +
> +			gem_execbuf(i915, &exec);
> +			gem_sync(i915, obj[0].handle);
> +			SHA1(pages, sz, ref);
> +
> +			igt_assert(memcmp(ref, orig, sizeof(ref)));
> +			memcpy(orig, ref, sizeof(orig));
> +
> +			/* Now try the same through the read-only handle */
> +			i = 0;
> +			obj[0].handle = rhandle;
> +			for (int n = 0; n < nreloc; n++) {
> +				uint64_t offset;
> +
> +				reloc[n].target_handle = obj[0].handle;
> +				reloc[n].delta = 4*(rand() % (total/4));
> +				reloc[n].offset = (i+1) * sizeof(uint32_t);
> +				reloc[n].presumed_offset = obj[0].offset;
> +				reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +				reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +
> +				offset = reloc[n].presumed_offset + reloc[n].delta;
> +
> +				batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +				if (gen >= 8) {
> +					batch[++i] = offset;
> +					batch[++i] = offset >> 32;
> +				} else if (gen >= 4) {
> +					batch[++i] = 0;
> +					batch[++i] = offset;
> +					reloc[n].offset += sizeof(uint32_t);
> +				} else {
> +					batch[i]--;
> +					batch[++i] = offset;
> +				}
> +				batch[++i] = rand();
> +				i++;
> +			}
> +			batch[i] = MI_BATCH_BUFFER_END;

Am I seeing a copy-pasted loop? You know what's next! :D

> +
> +			gem_execbuf(i915, &exec);
> +			gem_sync(i915, obj[0].handle);
> +			SHA1(pages, sz, result);
> +
> +			/*
> +			 * As the writes into the read-only GPU bo should fail,
> +			 * the SHA1 hash of the backing store should be
> +			 * unaffected.
> +			 */
> +			igt_assert(memcmp(ref, result, SHA_DIGEST_LENGTH) == 0);
> +		}
> +
> +		munmap(batch, 16*4096);
> +		gem_close(i915, obj[1].handle);
> +		gem_close(i915, rhandle);
> +	}
> +	igt_waitchildren();
> +
> +	munmap(space, total);
> +	munmap(pages, sz);
> +}

Okay more or less. Just want some tweaks and high level description 
since I (or anyone in the future) don't need/want to reverse engineer 
the patterns.

> +
> +static jmp_buf sigjmp;
> +static void sigjmp_handler(int sig)
> +{
> +	siglongjmp(sigjmp, sig);
> +}
> +
> +static void test_readonly_mmap(int i915)
> +{

Please add high level test description since there is some trickery below.

> +	unsigned char original[SHA_DIGEST_LENGTH];
> +	unsigned char result[SHA_DIGEST_LENGTH];
> +	uint32_t handle;
> +	uint32_t sz;
> +	void *pages;
> +	void *ptr;
> +	int sig;
> +
> +	igt_require(igt_setup_clflush());
> +
> +	sz = 16 << 12;
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
> +	gem_set_caching(i915, handle, 0);
> +
> +	memset(pages, 0xa5, sz);
> +	igt_clflush_range(pages, sz);

Why are cache flushed needed in this test? Because they cannot be done 
via domain management?

> +	SHA1(pages, sz, original);
> +
> +	ptr = __gem_mmap__gtt(i915, handle, sz, PROT_WRITE);
> +	igt_assert(ptr == NULL);
> +
> +	ptr = gem_mmap__gtt(i915, handle, sz, PROT_READ);
> +	gem_close(i915, handle);
> +
> +	if (!(sig = sigsetjmp(sigjmp, 1))) {

What does this do? Comment?

> +		signal(SIGBUS, sigjmp_handler);
> +		signal(SIGSEGV, sigjmp_handler);
> +		memset(ptr, 0x5a, sz);
> +		igt_assert(0);
> +	}
> +	igt_assert_eq(sig, SIGSEGV);
> +
> +	igt_assert(mprotect(ptr, sz, PROT_WRITE));

Why is this needed?

> +	munmap(ptr, sz);
> +
> +	igt_clflush_range(pages, sz);
> +	SHA1(pages, sz, result);
> +	igt_assert(!memcmp(original, result, sizeof(original)));
> +
> +	munmap(pages, sz);
> +}
> +
> +static void test_readonly_pwrite(int i915)
> +{
> +	unsigned char original[SHA_DIGEST_LENGTH];
> +	unsigned char result[SHA_DIGEST_LENGTH];
> +	uint32_t handle;
> +	uint32_t sz;
> +	void *pages;
> +
> +	igt_require(igt_setup_clflush());
> +
> +	sz = 16 << 12;
> +	pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> +	igt_assert(pages != MAP_FAILED);
> +
> +	igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
> +	memset(pages, 0xa5, sz);
> +	SHA1(pages, sz, original);
> +
> +	for (int page = 0; page < 16; page++) {
> +		char data[4096];
> +
> +		memset(data, page, sizeof(data));
> +		igt_assert_eq(__gem_write(i915, handle, page << 12, data, sizeof(data)), -EINVAL);
> +	}
> +
> +	gem_close(i915, handle);
> +
> +	SHA1(pages, sz, result);
> +	igt_assert(!memcmp(original, result, sizeof(original)));
> +
> +	munmap(pages, sz);
> +}
> +
>   static int test_usage_restrictions(int fd)
>   {
>   	void *ptr;
> @@ -961,10 +1285,6 @@ static int test_usage_restrictions(int fd)
>   	ret = __gem_userptr(fd, (char *)ptr + 1, PAGE_SIZE - 1, 0, userptr_flags, &handle);
>   	igt_assert_neq(ret, 0);
>   
> -	/* Read-only not supported. */
> -	ret = __gem_userptr(fd, (char *)ptr, PAGE_SIZE, 1, userptr_flags, &handle);
> -	igt_assert_neq(ret, 0);
> -
>   	free(ptr);
>   
>   	return 0;
> @@ -1502,6 +1822,15 @@ int main(int argc, char **argv)
>   		igt_subtest("dmabuf-unsync")
>   			test_dmabuf();
>   
> +		igt_subtest("readonly-unsync")
> +			test_readonly(fd);
> +
> +		igt_subtest("readonly-mmap-unsync")
> +			test_readonly_mmap(fd);
> +
> +		igt_subtest("readonly-pwrite-unsync")
> +			test_readonly_pwrite(fd);
> +
>   		for (unsigned flags = 0; flags < ALL_FORKING_EVICTIONS + 1; flags++) {
>   			igt_subtest_f("forked-unsync%s%s%s-%s",
>   					flags & FORKING_EVICTIONS_SWAPPING ? "-swapping" : "",
> 

Looks like thorough coverage, just some final polish needed.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
  2018-06-28 16:56   ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
@ 2018-06-28 18:09     ` Chris Wilson
  -1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-06-28 18:09 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2018-06-28 17:56:24)
> 
> On 27/06/2018 20:44, Chris Wilson wrote:
> > +static void test_readonly(int i915)
> 
> Hm.. nice interesting and novel fd naming I think. fd, gem_fd I know we 
> have. I wonder if we have drm_fd as well somewhere. Just thinking out 
> loud...

Not that novel.

> > +{
> > +     unsigned char orig[SHA_DIGEST_LENGTH];
> > +     uint64_t aperture_size;
> > +     uint32_t whandle, rhandle;
> > +     size_t sz, total;
> > +     void *pages, *space;
> > +     int memfd;
> > +
> > +     /*
> > +      * A small batch of pages; small enough to cheaply check for stray
> > +      * writes but large enough that we don't create too many VMA pointing
> > +      * back to this set from the large arena. The limit on total number
> > +      * of VMA for a process is 65,536 (at least on this kernel).
> > +      */
> > +     sz = 16 << 12;
> 
> 12 for page size, so 16 pages? How it is related to VMA limit from the 
> comment?

A few lines later in this block.

> > +     memfd = memfd_create("pages", 0);
> > +     igt_require(memfd != -1);
> 
> igt_require_fd is available if you care for it...

Not fond of it.

> > +     igt_require(ftruncate(memfd, sz) == 0);
> 
> ..and igt_require_eq, the double sided sword of API growing rich, huh? :)

Nope, return code is not interesting, thanks glibc.

> > +
> > +     pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> > +     igt_assert(pages != MAP_FAILED);
> > +
> > +     igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> > +     gem_close(i915, rhandle);
> > +
> > +     gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> > +
> > +     total = 2048ull << 20;
> 
> Why 2GiB? Express with the VMA limit and sz or just accidentally half of 
> the VMA limit?

Nah, the largest offset we can use is 4G, and we can't use the full range
as we need some extra room for batches, and we can't use the full VMA
limit without serious slow down and risk of exhaustion.
Then sticking to a pot.

> > +     aperture_size = gem_aperture_size(i915) / 2;
> > +     if (aperture_size < total)
> > +             total = aperture_size;
> > +     total = total / sz * sz;
> 
> There is round_down in lib/igt_primes but it would need exporting.
> 
> > +     igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> > +              total, total >> 12, sz >> 12);
> > +
> > +     /* Create an arena all pointing to the same set of pages */
> > +     space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
> 
> Allocating address space only?

Repeating set of PTEs.

> > +     igt_require(space != MAP_FAILED);
> > +     for (size_t offset = 0; offset < total; offset += sz) {
> > +             igt_assert(mmap(space + offset, sz,
> > +                             PROT_WRITE, MAP_SHARED | MAP_FIXED,
> > +                             memfd, 0) != MAP_FAILED);
> > +             *(uint32_t *)(space + offset) = offset;
> > +     }
> > +     igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
> 
> Checking that "arena" somewhat works, ok..

Checking we can allocate.

> > +     igt_assert(mlock(space, total) == 0);
> > +     close(memfd);
> > +
> > +     /* Check we can create a normal userptr bo wrapping the wrapper */
> > +     gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> > +     gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> > +     for (size_t offset = 0; offset < total; offset += sz)
> > +             store_dword(i915, rhandle, offset + 4, offset / sz);
> > +     gem_sync(i915, rhandle);
> 
> Do you need to move it back to CPU domain before the asserts?

The set-domain checks we can populate the userptr.
 
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
> 
> Please add a comment somewhere higher up explaining the layout - I got 
> lost what is in the first dword and what in the second of each page, and 
> who writes each.

First dword written by CPU of the page address. Second dword written by
GPU of the overlap. Just checking the setup of the arena.

It's irrelevant to the rest of the test, so not sure it's worth
repeating the code.
 
> > +     gem_close(i915, rhandle);
> > +
> > +     /* Now enforce read-only henceforth */
> > +     igt_assert(mprotect(space, total, PROT_READ) == 0);
> 
> No writes from the CPU, ok, I suppose to guarantee if there is a write 
> where it came from.

To check the read-only part; to import a PROT_READ set of pages, you
must use I915_USERPTR_READ_ONLY.
 
> Please add a high level comment what the following block will test and how.

Oh, it's just the same old test as in the kernel with the stages
explained.
 
> > +     SHA1(pages, sz, orig);
> > +     igt_fork(child, 1) {
> > +             const int gen = intel_gen(intel_get_drm_devid(i915));
> > +             const int nreloc = 1024;
> 
> This has a relationship to the size of the batch buffer created lower below?

We want to use this number of nrelocs? It only has to be less. 64k is
interesting for something else.
 
> > +             struct drm_i915_gem_execbuffer2 exec;
> > +             unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> > +             uint32_t *batch;
> > +             int i;
> > +
> > +             reloc = calloc(sizeof(*reloc), nreloc);
> > +             gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> > +
> > +
> 
> Extra newline.
> 
> > +             memset(obj, 0, sizeof(obj));
> > +             obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> > +             obj[1].handle = gem_create(i915, 4096*16);
> 
> This is the size of store dw times times nreloc? Relationships need to 
> be clearer and expressed in one place.

Nope. It's 16 pages.

> > +             obj[1].relocation_count = nreloc;
> > +             obj[1].relocs_ptr = to_user_pointer(reloc);
> > +
> > +             batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> > +
> > +             memset(&exec, 0, sizeof(exec));
> > +             exec.buffer_count =2;
> > +             exec.buffers_ptr = to_user_pointer(obj);
> > +
> > +             for_each_engine(i915, exec.flags) {
> > +                     /* First tweak the backing store through the write */
> > +                     i = 0;
> > +                     obj[0].handle = whandle;
> > +                     for (int n = 0; n < nreloc; n++) {
> > +                             uint64_t offset;
> > +
> > +                             reloc[n].target_handle = obj[0].handle;
> > +                             reloc[n].delta = 4*(rand() % (sz/4));
> > +                             reloc[n].offset = (i+1) * sizeof(uint32_t);
> 
> You can add spaces around operators to follow our coding style since 
> space is not constrained here.
> 
> > +                             reloc[n].presumed_offset = obj[0].offset;
> > +                             reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> > +                             reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +                             offset = reloc[n].presumed_offset + reloc[n].delta;
> > +
> > +                             batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +                             if (gen >= 8) {
> > +                                     batch[++i] = offset;
> > +                                     batch[++i] = offset >> 32;
> > +                             } else if (gen >= 4) {
> > +                                     batch[++i] = 0;
> > +                                     batch[++i] = offset;
> > +                                     reloc[n].offset += sizeof(uint32_t);
> > +                             } else {
> > +                                     batch[i]--;
> > +                                     batch[++i] = offset;
> > +                             }
> > +                             batch[++i] = rand();
> > +                             i++;
> > +                     }
> > +                     batch[i] = MI_BATCH_BUFFER_END;
> 
> Somehow make this possible via previously added store_dword helper 
> instead of duplicating?

There's no point making either more complicated, as I think this is very
straightforward.

> > +
> > +                     gem_execbuf(i915, &exec);
> > +                     gem_sync(i915, obj[0].handle);
> > +                     SHA1(pages, sz, ref);
> > +
> > +                     igt_assert(memcmp(ref, orig, sizeof(ref)));
> > +                     memcpy(orig, ref, sizeof(orig));
> > +
> > +                     /* Now try the same through the read-only handle */
> > +                     i = 0;
> > +                     obj[0].handle = rhandle;
> > +                     for (int n = 0; n < nreloc; n++) {
> > +                             uint64_t offset;
> > +
> > +                             reloc[n].target_handle = obj[0].handle;
> > +                             reloc[n].delta = 4*(rand() % (total/4));
> > +                             reloc[n].offset = (i+1) * sizeof(uint32_t);
> > +                             reloc[n].presumed_offset = obj[0].offset;
> > +                             reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> > +                             reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +                             offset = reloc[n].presumed_offset + reloc[n].delta;
> > +
> > +                             batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +                             if (gen >= 8) {
> > +                                     batch[++i] = offset;
> > +                                     batch[++i] = offset >> 32;
> > +                             } else if (gen >= 4) {
> > +                                     batch[++i] = 0;
> > +                                     batch[++i] = offset;
> > +                                     reloc[n].offset += sizeof(uint32_t);
> > +                             } else {
> > +                                     batch[i]--;
> > +                                     batch[++i] = offset;
> > +                             }
> > +                             batch[++i] = rand();
> > +                             i++;
> > +                     }
> > +                     batch[i] = MI_BATCH_BUFFER_END;
> 
> Am I seeing a copy-pasted loop? You know what's next! :D
Not worth it surely.
 
> > +
> > +                     gem_execbuf(i915, &exec);
> > +                     gem_sync(i915, obj[0].handle);
> > +                     SHA1(pages, sz, result);
> > +
> > +                     /*
> > +                      * As the writes into the read-only GPU bo should fail,
> > +                      * the SHA1 hash of the backing store should be
> > +                      * unaffected.
> > +                      */
> > +                     igt_assert(memcmp(ref, result, SHA_DIGEST_LENGTH) == 0);
> > +             }
> > +
> > +             munmap(batch, 16*4096);
> > +             gem_close(i915, obj[1].handle);
> > +             gem_close(i915, rhandle);
> > +     }
> > +     igt_waitchildren();
> > +
> > +     munmap(space, total);
> > +     munmap(pages, sz);
> > +}
> 
> Okay more or less. Just want some tweaks and high level description 
> since I (or anyone in the future) don't need/want to reverse engineer 
> the patterns.
> 
> > +
> > +static jmp_buf sigjmp;
> > +static void sigjmp_handler(int sig)
> > +{
> > +     siglongjmp(sigjmp, sig);
> > +}
> > +
> > +static void test_readonly_mmap(int i915)
> > +{
> 
> Please add high level test description since there is some trickery below.

Tests handling of readonly userptr vs mmap.

> > +     unsigned char original[SHA_DIGEST_LENGTH];
> > +     unsigned char result[SHA_DIGEST_LENGTH];
> > +     uint32_t handle;
> > +     uint32_t sz;
> > +     void *pages;
> > +     void *ptr;
> > +     int sig;
> > +
> > +     igt_require(igt_setup_clflush());
> > +
> > +     sz = 16 << 12;
> > +     pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> > +     igt_assert(pages != MAP_FAILED);
> > +
> > +     igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
> > +     gem_set_caching(i915, handle, 0);
> > +
> > +     memset(pages, 0xa5, sz);
> > +     igt_clflush_range(pages, sz);
> 
> Why are cache flushed needed in this test? Because they cannot be done 
> via domain management?

Because we are playing tricks here, doing things that are advised
against but not outright forbidden and want to catch out if the
kernel/hw, beyond the control of the bo, so only via pages.

> > +     SHA1(pages, sz, original);
> > +
> > +     ptr = __gem_mmap__gtt(i915, handle, sz, PROT_WRITE);
> > +     igt_assert(ptr == NULL);
> > +
> > +     ptr = gem_mmap__gtt(i915, handle, sz, PROT_READ);
> > +     gem_close(i915, handle);
> > +
> > +     if (!(sig = sigsetjmp(sigjmp, 1))) {
> 
> What does this do? Comment?

It's a sigsetjmp. What's unusual?

> > +             signal(SIGBUS, sigjmp_handler);
> > +             signal(SIGSEGV, sigjmp_handler);
> > +             memset(ptr, 0x5a, sz);
> > +             igt_assert(0);
> > +     }
> > +     igt_assert_eq(sig, SIGSEGV);
> > +
> > +     igt_assert(mprotect(ptr, sz, PROT_WRITE));
> 
> Why is this needed?

? It's a test that we can't change the CPU protection from read-only to
read-write.

> > +     munmap(ptr, sz);
> > +
> > +     igt_clflush_range(pages, sz);
> > +     SHA1(pages, sz, result);
> > +     igt_assert(!memcmp(original, result, sizeof(original)));
> > +
> > +     munmap(pages, sz);
> > +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] igt/gem_userptr: Check read-only mappings
@ 2018-06-28 18:09     ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-06-28 18:09 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2018-06-28 17:56:24)
> 
> On 27/06/2018 20:44, Chris Wilson wrote:
> > +static void test_readonly(int i915)
> 
> Hm.. nice interesting and novel fd naming I think. fd, gem_fd I know we 
> have. I wonder if we have drm_fd as well somewhere. Just thinking out 
> loud...

Not that novel.

> > +{
> > +     unsigned char orig[SHA_DIGEST_LENGTH];
> > +     uint64_t aperture_size;
> > +     uint32_t whandle, rhandle;
> > +     size_t sz, total;
> > +     void *pages, *space;
> > +     int memfd;
> > +
> > +     /*
> > +      * A small batch of pages; small enough to cheaply check for stray
> > +      * writes but large enough that we don't create too many VMA pointing
> > +      * back to this set from the large arena. The limit on total number
> > +      * of VMA for a process is 65,536 (at least on this kernel).
> > +      */
> > +     sz = 16 << 12;
> 
> 12 for page size, so 16 pages? How it is related to VMA limit from the 
> comment?

A few lines later in this block.

> > +     memfd = memfd_create("pages", 0);
> > +     igt_require(memfd != -1);
> 
> igt_require_fd is available if you care for it...

Not fond of it.

> > +     igt_require(ftruncate(memfd, sz) == 0);
> 
> ..and igt_require_eq, the double sided sword of API growing rich, huh? :)

Nope, return code is not interesting, thanks glibc.

> > +
> > +     pages = mmap(NULL, sz, PROT_WRITE, MAP_SHARED, memfd, 0);
> > +     igt_assert(pages != MAP_FAILED);
> > +
> > +     igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &rhandle) == 0);
> > +     gem_close(i915, rhandle);
> > +
> > +     gem_userptr(i915, pages, sz, false, userptr_flags, &whandle);
> > +
> > +     total = 2048ull << 20;
> 
> Why 2GiB? Express with the VMA limit and sz or just accidentally half of 
> the VMA limit?

Nah, the largest offset we can use is 4G, and we can't use the full range
as we need some extra room for batches, and we can't use the full VMA
limit without serious slow down and risk of exhaustion.
Then sticking to a pot.

> > +     aperture_size = gem_aperture_size(i915) / 2;
> > +     if (aperture_size < total)
> > +             total = aperture_size;
> > +     total = total / sz * sz;
> 
> There is round_down in lib/igt_primes but it would need exporting.
> 
> > +     igt_info("Using a %'zuB (%'zu pages) arena onto %zu pages\n",
> > +              total, total >> 12, sz >> 12);
> > +
> > +     /* Create an arena all pointing to the same set of pages */
> > +     space = mmap(NULL, total, PROT_READ, MAP_ANON | MAP_SHARED, -1, 0);
> 
> Allocating address space only?

Repeating set of PTEs.

> > +     igt_require(space != MAP_FAILED);
> > +     for (size_t offset = 0; offset < total; offset += sz) {
> > +             igt_assert(mmap(space + offset, sz,
> > +                             PROT_WRITE, MAP_SHARED | MAP_FIXED,
> > +                             memfd, 0) != MAP_FAILED);
> > +             *(uint32_t *)(space + offset) = offset;
> > +     }
> > +     igt_assert_eq_u32(*(uint32_t *)pages, (uint32_t)(total - sz));
> 
> Checking that "arena" somewhat works, ok..

Checking we can allocate.

> > +     igt_assert(mlock(space, total) == 0);
> > +     close(memfd);
> > +
> > +     /* Check we can create a normal userptr bo wrapping the wrapper */
> > +     gem_userptr(i915, space, total, false, userptr_flags, &rhandle);
> > +     gem_set_domain(i915, rhandle, I915_GEM_DOMAIN_CPU, 0);
> > +     for (size_t offset = 0; offset < total; offset += sz)
> > +             store_dword(i915, rhandle, offset + 4, offset / sz);
> > +     gem_sync(i915, rhandle);
> 
> Do you need to move it back to CPU domain before the asserts?

The set-domain checks we can populate the userptr.
 
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 0), (uint32_t)(total - sz));
> > +     igt_assert_eq_u32(*(uint32_t *)(pages + 4), (uint32_t)(total / sz - 1));
> 
> Please add a comment somewhere higher up explaining the layout - I got 
> lost what is in the first dword and what in the second of each page, and 
> who writes each.

First dword written by CPU of the page address. Second dword written by
GPU of the overlap. Just checking the setup of the arena.

It's irrelevant to the rest of the test, so not sure it's worth
repeating the code.
 
> > +     gem_close(i915, rhandle);
> > +
> > +     /* Now enforce read-only henceforth */
> > +     igt_assert(mprotect(space, total, PROT_READ) == 0);
> 
> No writes from the CPU, ok, I suppose to guarantee if there is a write 
> where it came from.

To check the read-only part; to import a PROT_READ set of pages, you
must use I915_USERPTR_READ_ONLY.
 
> Please add a high level comment what the following block will test and how.

Oh, it's just the same old test as in the kernel with the stages
explained.
 
> > +     SHA1(pages, sz, orig);
> > +     igt_fork(child, 1) {
> > +             const int gen = intel_gen(intel_get_drm_devid(i915));
> > +             const int nreloc = 1024;
> 
> This has a relationship to the size of the batch buffer created lower below?

We want to use this number of nrelocs? It only has to be less. 64k is
interesting for something else.
 
> > +             struct drm_i915_gem_execbuffer2 exec;
> > +             unsigned char ref[SHA_DIGEST_LENGTH], result[SHA_DIGEST_LENGTH];
> > +             uint32_t *batch;
> > +             int i;
> > +
> > +             reloc = calloc(sizeof(*reloc), nreloc);
> > +             gem_userptr(i915, space, total, true, userptr_flags, &rhandle);
> > +
> > +
> 
> Extra newline.
> 
> > +             memset(obj, 0, sizeof(obj));
> > +             obj[0].flags = LOCAL_EXEC_OBJECT_SUPPORTS_48B;
> > +             obj[1].handle = gem_create(i915, 4096*16);
> 
> This is the size of store dw times times nreloc? Relationships need to 
> be clearer and expressed in one place.

Nope. It's 16 pages.

> > +             obj[1].relocation_count = nreloc;
> > +             obj[1].relocs_ptr = to_user_pointer(reloc);
> > +
> > +             batch = gem_mmap__wc(i915, obj[1].handle, 0, 4096*16, PROT_WRITE);
> > +
> > +             memset(&exec, 0, sizeof(exec));
> > +             exec.buffer_count =2;
> > +             exec.buffers_ptr = to_user_pointer(obj);
> > +
> > +             for_each_engine(i915, exec.flags) {
> > +                     /* First tweak the backing store through the write */
> > +                     i = 0;
> > +                     obj[0].handle = whandle;
> > +                     for (int n = 0; n < nreloc; n++) {
> > +                             uint64_t offset;
> > +
> > +                             reloc[n].target_handle = obj[0].handle;
> > +                             reloc[n].delta = 4*(rand() % (sz/4));
> > +                             reloc[n].offset = (i+1) * sizeof(uint32_t);
> 
> You can add spaces around operators to follow our coding style since 
> space is not constrained here.
> 
> > +                             reloc[n].presumed_offset = obj[0].offset;
> > +                             reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> > +                             reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +                             offset = reloc[n].presumed_offset + reloc[n].delta;
> > +
> > +                             batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +                             if (gen >= 8) {
> > +                                     batch[++i] = offset;
> > +                                     batch[++i] = offset >> 32;
> > +                             } else if (gen >= 4) {
> > +                                     batch[++i] = 0;
> > +                                     batch[++i] = offset;
> > +                                     reloc[n].offset += sizeof(uint32_t);
> > +                             } else {
> > +                                     batch[i]--;
> > +                                     batch[++i] = offset;
> > +                             }
> > +                             batch[++i] = rand();
> > +                             i++;
> > +                     }
> > +                     batch[i] = MI_BATCH_BUFFER_END;
> 
> Somehow make this possible via previously added store_dword helper 
> instead of duplicating?

There's no point making either more complicated, as I think this is very
straightforward.

> > +
> > +                     gem_execbuf(i915, &exec);
> > +                     gem_sync(i915, obj[0].handle);
> > +                     SHA1(pages, sz, ref);
> > +
> > +                     igt_assert(memcmp(ref, orig, sizeof(ref)));
> > +                     memcpy(orig, ref, sizeof(orig));
> > +
> > +                     /* Now try the same through the read-only handle */
> > +                     i = 0;
> > +                     obj[0].handle = rhandle;
> > +                     for (int n = 0; n < nreloc; n++) {
> > +                             uint64_t offset;
> > +
> > +                             reloc[n].target_handle = obj[0].handle;
> > +                             reloc[n].delta = 4*(rand() % (total/4));
> > +                             reloc[n].offset = (i+1) * sizeof(uint32_t);
> > +                             reloc[n].presumed_offset = obj[0].offset;
> > +                             reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> > +                             reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> > +
> > +                             offset = reloc[n].presumed_offset + reloc[n].delta;
> > +
> > +                             batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> > +                             if (gen >= 8) {
> > +                                     batch[++i] = offset;
> > +                                     batch[++i] = offset >> 32;
> > +                             } else if (gen >= 4) {
> > +                                     batch[++i] = 0;
> > +                                     batch[++i] = offset;
> > +                                     reloc[n].offset += sizeof(uint32_t);
> > +                             } else {
> > +                                     batch[i]--;
> > +                                     batch[++i] = offset;
> > +                             }
> > +                             batch[++i] = rand();
> > +                             i++;
> > +                     }
> > +                     batch[i] = MI_BATCH_BUFFER_END;
> 
> Am I seeing a copy-pasted loop? You know what's next! :D
Not worth it surely.
 
> > +
> > +                     gem_execbuf(i915, &exec);
> > +                     gem_sync(i915, obj[0].handle);
> > +                     SHA1(pages, sz, result);
> > +
> > +                     /*
> > +                      * As the writes into the read-only GPU bo should fail,
> > +                      * the SHA1 hash of the backing store should be
> > +                      * unaffected.
> > +                      */
> > +                     igt_assert(memcmp(ref, result, SHA_DIGEST_LENGTH) == 0);
> > +             }
> > +
> > +             munmap(batch, 16*4096);
> > +             gem_close(i915, obj[1].handle);
> > +             gem_close(i915, rhandle);
> > +     }
> > +     igt_waitchildren();
> > +
> > +     munmap(space, total);
> > +     munmap(pages, sz);
> > +}
> 
> Okay more or less. Just want some tweaks and high level description 
> since I (or anyone in the future) don't need/want to reverse engineer 
> the patterns.
> 
> > +
> > +static jmp_buf sigjmp;
> > +static void sigjmp_handler(int sig)
> > +{
> > +     siglongjmp(sigjmp, sig);
> > +}
> > +
> > +static void test_readonly_mmap(int i915)
> > +{
> 
> Please add high level test description since there is some trickery below.

Tests handling of readonly userptr vs mmap.

> > +     unsigned char original[SHA_DIGEST_LENGTH];
> > +     unsigned char result[SHA_DIGEST_LENGTH];
> > +     uint32_t handle;
> > +     uint32_t sz;
> > +     void *pages;
> > +     void *ptr;
> > +     int sig;
> > +
> > +     igt_require(igt_setup_clflush());
> > +
> > +     sz = 16 << 12;
> > +     pages = mmap(NULL, sz, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> > +     igt_assert(pages != MAP_FAILED);
> > +
> > +     igt_require(__gem_userptr(i915, pages, sz, true, userptr_flags, &handle) == 0);
> > +     gem_set_caching(i915, handle, 0);
> > +
> > +     memset(pages, 0xa5, sz);
> > +     igt_clflush_range(pages, sz);
> 
> Why are cache flushed needed in this test? Because they cannot be done 
> via domain management?

Because we are playing tricks here, doing things that are advised
against but not outright forbidden and want to catch out if the
kernel/hw, beyond the control of the bo, so only via pages.

> > +     SHA1(pages, sz, original);
> > +
> > +     ptr = __gem_mmap__gtt(i915, handle, sz, PROT_WRITE);
> > +     igt_assert(ptr == NULL);
> > +
> > +     ptr = gem_mmap__gtt(i915, handle, sz, PROT_READ);
> > +     gem_close(i915, handle);
> > +
> > +     if (!(sig = sigsetjmp(sigjmp, 1))) {
> 
> What does this do? Comment?

It's a sigsetjmp. What's unusual?

> > +             signal(SIGBUS, sigjmp_handler);
> > +             signal(SIGSEGV, sigjmp_handler);
> > +             memset(ptr, 0x5a, sz);
> > +             igt_assert(0);
> > +     }
> > +     igt_assert_eq(sig, SIGSEGV);
> > +
> > +     igt_assert(mprotect(ptr, sz, PROT_WRITE));
> 
> Why is this needed?

? It's a test that we can't change the CPU protection from read-only to
read-write.

> > +     munmap(ptr, sz);
> > +
> > +     igt_clflush_range(pages, sz);
> > +     SHA1(pages, sz, result);
> > +     igt_assert(!memcmp(original, result, sizeof(original)));
> > +
> > +     munmap(pages, sz);
> > +}
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-06-28 18:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-27 19:44 [PATCH i-g-t] igt/gem_userptr: Check read-only mappings Chris Wilson
2018-06-27 19:44 ` [igt-dev] " Chris Wilson
2018-06-27 20:10 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2018-06-28  0:17 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-06-28 14:25 ` [PATCH i-g-t] " Mika Kuoppala
2018-06-28 14:25   ` [igt-dev] [Intel-gfx] " Mika Kuoppala
2018-06-28 14:37   ` Chris Wilson
2018-06-28 14:37     ` [igt-dev] [Intel-gfx] " Chris Wilson
2018-06-28 16:56 ` Tvrtko Ursulin
2018-06-28 16:56   ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
2018-06-28 18:09   ` Chris Wilson
2018-06-28 18:09     ` [igt-dev] [Intel-gfx] " Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.