[PATCH i-g-t 0/4] igt/gem_create cleanup

[PATCH i-g-t 0/4] igt/gem_create cleanup

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Just a quick cleanup of some tests and marking them as basic because they
really should be.

Tvrtko Ursulin (4):
  lib/ioctl_wrappers: __gem_create needs u64 for size
  gem_create: Test huge object creation as a basic test
  gem_create: Tidy object creation tests
  gem_create: Convert stolen test to uABI checking

 lib/ioctl_wrappers.c |  2 +-
 lib/ioctl_wrappers.h |  2 +-
 tests/gem_create.c   | 90 ++++++++++++++++------------------------------------
 3 files changed, 29 insertions(+), 65 deletions(-)

-- 
2.9.3

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

[PATCH i-g-t 1/4] lib/ioctl_wrappers: __gem_create needs u64 for size

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Otherwise we cannot test the ioctl properly.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/ioctl_wrappers.c | 2 +-
 lib/ioctl_wrappers.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 72fb0ebc2dba..3c7fbe1ebd83 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -554,7 +554,7 @@ uint32_t gem_create_stolen(int fd, uint64_t size)
 }
 
 
-uint32_t __gem_create(int fd, int size)
+uint32_t __gem_create(int fd, uint64_t size)
 {
 	struct drm_i915_gem_create create;
 	int ret;
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index 56c5d14ced8e..74b53e66d516 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -72,7 +72,7 @@ void gem_sync(int fd, uint32_t handle);
 bool gem_create__has_stolen_support(int fd);
 uint32_t __gem_create_stolen(int fd, uint64_t size);
 uint32_t gem_create_stolen(int fd, uint64_t size);
-uint32_t __gem_create(int fd, int size);
+uint32_t __gem_create(int fd, uint64_t size);
 uint32_t gem_create(int fd, uint64_t size);
 void gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
 int __gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
-- 
2.9.3

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

[PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It is hard to imagine a more basic test than this one.

Also removed the skip on simulation since I don't know why
would that be needed here.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_create.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/gem_create.c b/tests/gem_create.c
index de7b82094545..f687b7b40be4 100644
--- a/tests/gem_create.c
+++ b/tests/gem_create.c
@@ -44,6 +44,7 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <getopt.h>
+#include <limits.h>
 
 #include <drm.h>
 
@@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
 
 static void invalid_size_test(int fd)
 {
-	int handle;
+	uint32_t handle;
 
 	handle = __gem_create(fd, 0);
 	igt_assert(!handle);
+
+	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
+	igt_assert(!handle);
 }
 
 /*
@@ -146,8 +150,6 @@ igt_main
 {
 	int fd = -1;
 
-	igt_skip_on_simulation();
-
 	igt_fixture {
 		fd = drm_open_driver(DRIVER_INTEL);
 	}
@@ -155,7 +157,7 @@ igt_main
 	igt_subtest("stolen-invalid-flag")
 		invalid_flag_test(fd);
 
-	igt_subtest("create-invalid-size")
+	igt_subtest("basic-create-invalid-size")
 		invalid_size_test(fd);
 
 	igt_subtest("create-valid-nonaligned")
-- 
2.9.3

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

[PATCH i-g-t 3/4] gem_create: Tidy object creation tests

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Copnsolidate and simplify tests that small objects are rounded
up to PAGE_SIZE and no more.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_create.c | 43 ++++++++++---------------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/tests/gem_create.c b/tests/gem_create.c
index f687b7b40be4..21df13f7b655 100644
--- a/tests/gem_create.c
+++ b/tests/gem_create.c
@@ -106,41 +106,21 @@ static void invalid_size_test(int fd)
 }
 
 /*
- * Creating an object with non-aligned size and trying to access it with an
- * offset, which is greater than the requested size but smaller than the
- * object's last page boundary. pwrite here must be successful.
+ * Verify that the kernel rounds up object creation to PAGE_SIZE and no more.
  */
-static void valid_nonaligned_size(int fd)
+static void test_size_alignment(int fd)
 {
-	int handle;
-	char buf[PAGE_SIZE];
-
-	handle = gem_create(fd, PAGE_SIZE / 2);
-
-	gem_write(fd, handle, PAGE_SIZE / 2, buf, PAGE_SIZE / 2);
-
-	gem_close(fd, handle);
-}
-
-/*
- * Creating an object with non-aligned size and trying to access it with an
- * offset, which is greater than the requested size and larger than the
- * object's last page boundary. pwrite here must fail.
- */
-static void invalid_nonaligned_size(int fd)
-{
-	int handle;
-	char buf[PAGE_SIZE];
-	struct drm_i915_gem_pwrite gem_pwrite;
+	uint32_t handle;
+	char buf[PAGE_SIZE + 1];
+	struct drm_i915_gem_pwrite gem_pwrite = { };
 
 	handle = gem_create(fd, PAGE_SIZE / 2);
+	gem_write(fd, handle, 0, buf, PAGE_SIZE);
 
-	CLEAR(gem_pwrite);
 	gem_pwrite.handle = handle;
-	gem_pwrite.offset = PAGE_SIZE / 2;
-	gem_pwrite.size = PAGE_SIZE;
+	gem_pwrite.offset = 0;
+	gem_pwrite.size = PAGE_SIZE + 1;
 	gem_pwrite.data_ptr = to_user_pointer(buf);
-	/* This should fail. Hence cannot use gem_write. */
 	igt_assert(drmIoctl(fd, DRM_IOCTL_I915_GEM_PWRITE, &gem_pwrite));
 
 	gem_close(fd, handle);
@@ -160,9 +140,6 @@ igt_main
 	igt_subtest("basic-create-invalid-size")
 		invalid_size_test(fd);
 
-	igt_subtest("create-valid-nonaligned")
-		valid_nonaligned_size(fd);
-
-	igt_subtest("create-invalid-nonaligned")
-		invalid_nonaligned_size(fd);
+	igt_subtest("basic-size-alignment")
+		test_size_alignment(fd);
 }
-- 
2.9.3

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

[PATCH i-g-t 4/4] gem_create: Convert stolen test to uABI checking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Stolen never materialized so convert this test into checking
that the garbage in padding remains legal.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_create.c | 39 ++++++++++++---------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

diff --git a/tests/gem_create.c b/tests/gem_create.c
index 21df13f7b655..0dad06c784c5 100644
--- a/tests/gem_create.c
+++ b/tests/gem_create.c
@@ -27,8 +27,7 @@
 
 /** @file gem_create.c
  *
- * This is a test for the extended and old gem_create ioctl, that
- * includes allocation of object from stolen memory and shmem.
+ * This is a test for the gem_create ioctl.
  *
  * The goal is to simply ensure that basics work and invalid input
  * combinations are rejected.
@@ -62,36 +61,22 @@ IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
 		     " that includes allocation of object from stolen memory"
 		     " and shmem.");
 
-#define CLEAR(s) memset(&s, 0, sizeof(s))
 #define PAGE_SIZE 4096
 
-struct local_i915_gem_create_v2 {
-	uint64_t size;
-	uint32_t handle;
-	uint32_t pad;
-#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
-	uint32_t flags;
-} create;
-
-#define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
-
-static void invalid_flag_test(int fd)
+/*
+ * Verify that historical omission of checking for garbage in the padding
+ * field still holds.
+ */
+static void test_pad_garbage(int fd)
 {
+	struct drm_i915_gem_create create = { };
 	int ret;
 
-	gem_require_stolen_support(fd);
-
 	create.handle = 0;
 	create.size = PAGE_SIZE;
-	create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
-	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
-
-	igt_assert(ret <= 0);
-
-	create.flags = ~0;
-	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
-
-	igt_assert(ret <= 0);
+	create.pad = 1;
+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
+	igt_assert_eq(ret, 0);
 }
 
 static void invalid_size_test(int fd)
@@ -134,8 +119,8 @@ igt_main
 		fd = drm_open_driver(DRIVER_INTEL);
 	}
 
-	igt_subtest("stolen-invalid-flag")
-		invalid_flag_test(fd);
+	igt_subtest("basic-pad-garbage")
+		test_pad_garbage(fd);
 
 	igt_subtest("basic-create-invalid-size")
 		invalid_size_test(fd);
-- 
2.9.3

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

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Chris Wilson]

On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> It is hard to imagine a more basic test than this one.
> 
> Also removed the skip on simulation since I don't know why
> would that be needed here.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tests/gem_create.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/gem_create.c b/tests/gem_create.c
> index de7b82094545..f687b7b40be4 100644
> --- a/tests/gem_create.c
> +++ b/tests/gem_create.c
> @@ -44,6 +44,7 @@
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <getopt.h>
> +#include <limits.h>
>  
>  #include <drm.h>
>  
> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
>  
>  static void invalid_size_test(int fd)
>  {
> -	int handle;
> +	uint32_t handle;
>  
>  	handle = __gem_create(fd, 0);
>  	igt_assert(!handle);
> +
> +	handle = __gem_create(fd, INT_MAX * 4096UL + 1);

Why is that an invalid size? Invalid huge in terms of API might arguably
be 1<<virtual_bits + 1, but otherwise our only limitation is that it
has to be >0 and page aligned.

/* Only multiples of page size (4096) are allowed. Check all likely
 * misalignments from pot boundaries to check validity and possibility
 * of incorrect overflow.
 */
 for (int order = 0; order < 64; order++) {
 	uint64_t size = (uint64_t)1 << order;
	igt_assert(!__gem_create(fd, size - 1));
	igt_assert(!__gem_create(fd, size + 1));
	if (order < 12)
		igt_assert(!__gem_create(fd, size + 1));
}

Still enshrines knowlege of PAGE_SIZE into the ABI. Meh.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/4] gem_create: Convert stolen test to uABI checking

[Chris Wilson]

On Thu, Mar 30, 2017 at 05:58:09PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Stolen never materialized so convert this test into checking
> that the garbage in padding remains legal.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tests/gem_create.c | 39 ++++++++++++---------------------------
>  1 file changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/gem_create.c b/tests/gem_create.c
> index 21df13f7b655..0dad06c784c5 100644
> --- a/tests/gem_create.c
> +++ b/tests/gem_create.c
> @@ -27,8 +27,7 @@
>  
>  /** @file gem_create.c
>   *
> - * This is a test for the extended and old gem_create ioctl, that
> - * includes allocation of object from stolen memory and shmem.
> + * This is a test for the gem_create ioctl.
>   *
>   * The goal is to simply ensure that basics work and invalid input
>   * combinations are rejected.
> @@ -62,36 +61,22 @@ IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
>  		     " that includes allocation of object from stolen memory"
>  		     " and shmem.");
>  
> -#define CLEAR(s) memset(&s, 0, sizeof(s))
>  #define PAGE_SIZE 4096
>  
> -struct local_i915_gem_create_v2 {
> -	uint64_t size;
> -	uint32_t handle;
> -	uint32_t pad;
> -#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
> -	uint32_t flags;
> -} create;
> -
> -#define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
> -
> -static void invalid_flag_test(int fd)
> +/*
> + * Verify that historical omission of checking for garbage in the padding
> + * field still holds.
> + */
> +static void test_pad_garbage(int fd)
>  {
> +	struct drm_i915_gem_create create = { };
>  	int ret;
>  
> -	gem_require_stolen_support(fd);
> -
>  	create.handle = 0;
>  	create.size = PAGE_SIZE;
> -	create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
> -	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> -
> -	igt_assert(ret <= 0);
> -
> -	create.flags = ~0;
> -	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> -
> -	igt_assert(ret <= 0);
> +	create.pad = 1;
> +	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> +	igt_assert_eq(ret, 0);
>  }
>  
>  static void invalid_size_test(int fd)
> @@ -134,8 +119,8 @@ igt_main
>  		fd = drm_open_driver(DRIVER_INTEL);
>  	}
>  
> -	igt_subtest("stolen-invalid-flag")
> -		invalid_flag_test(fd);
> +	igt_subtest("basic-pad-garbage")
> +		test_pad_garbage(fd);

Not basic though. I just dislike having negative tests that we intend to
break be part of basic. (I dislike negative tests in general as they are
restrictive and limit creativity, a false limitation in terms of ABI.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Tvrtko Ursulin]

On 30/03/2017 18:22, Chris Wilson wrote:
> On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> It is hard to imagine a more basic test than this one.
>>
>> Also removed the skip on simulation since I don't know why
>> would that be needed here.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  tests/gem_create.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/gem_create.c b/tests/gem_create.c
>> index de7b82094545..f687b7b40be4 100644
>> --- a/tests/gem_create.c
>> +++ b/tests/gem_create.c
>> @@ -44,6 +44,7 @@
>>  #include <sys/stat.h>
>>  #include <sys/time.h>
>>  #include <getopt.h>
>> +#include <limits.h>
>>
>>  #include <drm.h>
>>
>> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
>>
>>  static void invalid_size_test(int fd)
>>  {
>> -	int handle;
>> +	uint32_t handle;
>>
>>  	handle = __gem_create(fd, 0);
>>  	igt_assert(!handle);
>> +
>> +	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
>
> Why is that an invalid size? Invalid huge in terms of API might arguably
> be 1<<virtual_bits + 1, but otherwise our only limitation is that it
> has to be >0 and page aligned.

Because of the comment above the WARN I am removing in "drm/i915: Remove 
user triggerable WARN from i915_gem_object_create". We cannot support 
larger ones with the combination of sg_table data types and how we use 
them (unsigned int nents).

> /* Only multiples of page size (4096) are allowed. Check all likely
>  * misalignments from pot boundaries to check validity and possibility
>  * of incorrect overflow.
>  */
>  for (int order = 0; order < 64; order++) {
>  	uint64_t size = (uint64_t)1 << order;
> 	igt_assert(!__gem_create(fd, size - 1));
> 	igt_assert(!__gem_create(fd, size + 1));
> 	if (order < 12)
> 		igt_assert(!__gem_create(fd, size + 1));
> }
>
> Still enshrines knowlege of PAGE_SIZE into the ABI. Meh.

We round up for the user transparently which I am actually making the 
other subtest in this file test in a later patch.

Regards,

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

Re: [PATCH i-g-t 4/4] gem_create: Convert stolen test to uABI checking

[Tvrtko Ursulin]

On 30/03/2017 18:25, Chris Wilson wrote:
> On Thu, Mar 30, 2017 at 05:58:09PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Stolen never materialized so convert this test into checking
>> that the garbage in padding remains legal.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  tests/gem_create.c | 39 ++++++++++++---------------------------
>>  1 file changed, 12 insertions(+), 27 deletions(-)
>>
>> diff --git a/tests/gem_create.c b/tests/gem_create.c
>> index 21df13f7b655..0dad06c784c5 100644
>> --- a/tests/gem_create.c
>> +++ b/tests/gem_create.c
>> @@ -27,8 +27,7 @@
>>
>>  /** @file gem_create.c
>>   *
>> - * This is a test for the extended and old gem_create ioctl, that
>> - * includes allocation of object from stolen memory and shmem.
>> + * This is a test for the gem_create ioctl.
>>   *
>>   * The goal is to simply ensure that basics work and invalid input
>>   * combinations are rejected.
>> @@ -62,36 +61,22 @@ IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
>>  		     " that includes allocation of object from stolen memory"
>>  		     " and shmem.");
>>
>> -#define CLEAR(s) memset(&s, 0, sizeof(s))
>>  #define PAGE_SIZE 4096
>>
>> -struct local_i915_gem_create_v2 {
>> -	uint64_t size;
>> -	uint32_t handle;
>> -	uint32_t pad;
>> -#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
>> -	uint32_t flags;
>> -} create;
>> -
>> -#define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
>> -
>> -static void invalid_flag_test(int fd)
>> +/*
>> + * Verify that historical omission of checking for garbage in the padding
>> + * field still holds.
>> + */
>> +static void test_pad_garbage(int fd)
>>  {
>> +	struct drm_i915_gem_create create = { };
>>  	int ret;
>>
>> -	gem_require_stolen_support(fd);
>> -
>>  	create.handle = 0;
>>  	create.size = PAGE_SIZE;
>> -	create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
>> -	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
>> -
>> -	igt_assert(ret <= 0);
>> -
>> -	create.flags = ~0;
>> -	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
>> -
>> -	igt_assert(ret <= 0);
>> +	create.pad = 1;
>> +	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
>> +	igt_assert_eq(ret, 0);
>>  }
>>
>>  static void invalid_size_test(int fd)
>> @@ -134,8 +119,8 @@ igt_main
>>  		fd = drm_open_driver(DRIVER_INTEL);
>>  	}
>>
>> -	igt_subtest("stolen-invalid-flag")
>> -		invalid_flag_test(fd);
>> +	igt_subtest("basic-pad-garbage")
>> +		test_pad_garbage(fd);
>
> Not basic though. I just dislike having negative tests that we intend to
> break be part of basic. (I dislike negative tests in general as they are
> restrictive and limit creativity, a false limitation in terms of ABI.)

My understanding is that we can never break this now. I mean that we 
have to allow userspace putting garbage in the padding field forever now.

I don't fully understand right now then how createv2 implementation was 
suggesting to re-purpose half of the padding for flags at the moment as 
well.

Regards,

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

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Chris Wilson]

On Fri, Mar 31, 2017 at 08:08:28AM +0100, Tvrtko Ursulin wrote:
> 
> On 30/03/2017 18:22, Chris Wilson wrote:
> >On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>It is hard to imagine a more basic test than this one.
> >>
> >>Also removed the skip on simulation since I don't know why
> >>would that be needed here.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>---
> >> tests/gem_create.c | 10 ++++++----
> >> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/tests/gem_create.c b/tests/gem_create.c
> >>index de7b82094545..f687b7b40be4 100644
> >>--- a/tests/gem_create.c
> >>+++ b/tests/gem_create.c
> >>@@ -44,6 +44,7 @@
> >> #include <sys/stat.h>
> >> #include <sys/time.h>
> >> #include <getopt.h>
> >>+#include <limits.h>
> >>
> >> #include <drm.h>
> >>
> >>@@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
> >>
> >> static void invalid_size_test(int fd)
> >> {
> >>-	int handle;
> >>+	uint32_t handle;
> >>
> >> 	handle = __gem_create(fd, 0);
> >> 	igt_assert(!handle);
> >>+
> >>+	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
> >
> >Why is that an invalid size? Invalid huge in terms of API might arguably
> >be 1<<virtual_bits + 1, but otherwise our only limitation is that it
> >has to be >0 and page aligned.
> 
> Because of the comment above the WARN I am removing in "drm/i915:
> Remove user triggerable WARN from i915_gem_object_create". We cannot
> support larger ones with the combination of sg_table data types and
> how we use them (unsigned int nents).

That's an implementation limitation, not an abi one. It is really
important that we do not enshrine kernel internals as expectations,
especially not as a basic test - the expectation is that we will support
massive objects. Having a reality check test to see how far we can get
is useful.

For example, lazy population of pages is an abi feature (I think so, at
least all userspace takes that into account, but I don't know if anyone
is really depending on it -- certainly due to the limitations we have in
place lazy is good, and userspace does try to take advantage of that,
and compensates for it at others, but whether anyone would break because
of a change to population semantics, not sure), so we could reasonably
allocate all pot of sizes and check the kernel doesn't reject them, until
the overflow check at -4095.
 
> >/* Only multiples of page size (4096) are allowed. Check all likely
> > * misalignments from pot boundaries to check validity and possibility
> > * of incorrect overflow.
> > */
> > for (int order = 0; order < 64; order++) {
> > 	uint64_t size = (uint64_t)1 << order;
> >	igt_assert(!__gem_create(fd, size - 1));
> >	igt_assert(!__gem_create(fd, size + 1));
> >	if (order < 12)
> >		igt_assert(!__gem_create(fd, size + 1));
> >}
> >
> >Still enshrines knowlege of PAGE_SIZE into the ABI. Meh.
> 
> We round up for the user transparently which I am actually making
> the other subtest in this file test in a later patch.

Bah, I keep creating objects at too low a level where that rounding
doesn't occur automatically :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/4] gem_create: Convert stolen test to uABI checking

[Chris Wilson]

On Fri, Mar 31, 2017 at 08:11:10AM +0100, Tvrtko Ursulin wrote:
> 
> On 30/03/2017 18:25, Chris Wilson wrote:
> >On Thu, Mar 30, 2017 at 05:58:09PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Stolen never materialized so convert this test into checking
> >>that the garbage in padding remains legal.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>---
> >> tests/gem_create.c | 39 ++++++++++++---------------------------
> >> 1 file changed, 12 insertions(+), 27 deletions(-)
> >>
> >>diff --git a/tests/gem_create.c b/tests/gem_create.c
> >>index 21df13f7b655..0dad06c784c5 100644
> >>--- a/tests/gem_create.c
> >>+++ b/tests/gem_create.c
> >>@@ -27,8 +27,7 @@
> >>
> >> /** @file gem_create.c
> >>  *
> >>- * This is a test for the extended and old gem_create ioctl, that
> >>- * includes allocation of object from stolen memory and shmem.
> >>+ * This is a test for the gem_create ioctl.
> >>  *
> >>  * The goal is to simply ensure that basics work and invalid input
> >>  * combinations are rejected.
> >>@@ -62,36 +61,22 @@ IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
> >> 		     " that includes allocation of object from stolen memory"
> >> 		     " and shmem.");
> >>
> >>-#define CLEAR(s) memset(&s, 0, sizeof(s))
> >> #define PAGE_SIZE 4096
> >>
> >>-struct local_i915_gem_create_v2 {
> >>-	uint64_t size;
> >>-	uint32_t handle;
> >>-	uint32_t pad;
> >>-#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
> >>-	uint32_t flags;
> >>-} create;
> >>-
> >>-#define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
> >>-
> >>-static void invalid_flag_test(int fd)
> >>+/*
> >>+ * Verify that historical omission of checking for garbage in the padding
> >>+ * field still holds.
> >>+ */
> >>+static void test_pad_garbage(int fd)
> >> {
> >>+	struct drm_i915_gem_create create = { };
> >> 	int ret;
> >>
> >>-	gem_require_stolen_support(fd);
> >>-
> >> 	create.handle = 0;
> >> 	create.size = PAGE_SIZE;
> >>-	create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
> >>-	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> >>-
> >>-	igt_assert(ret <= 0);
> >>-
> >>-	create.flags = ~0;
> >>-	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> >>-
> >>-	igt_assert(ret <= 0);
> >>+	create.pad = 1;
> >>+	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> >>+	igt_assert_eq(ret, 0);
> >> }
> >>
> >> static void invalid_size_test(int fd)
> >>@@ -134,8 +119,8 @@ igt_main
> >> 		fd = drm_open_driver(DRIVER_INTEL);
> >> 	}
> >>
> >>-	igt_subtest("stolen-invalid-flag")
> >>-		invalid_flag_test(fd);
> >>+	igt_subtest("basic-pad-garbage")
> >>+		test_pad_garbage(fd);
> >
> >Not basic though. I just dislike having negative tests that we intend to
> >break be part of basic. (I dislike negative tests in general as they are
> >restrictive and limit creativity, a false limitation in terms of ABI.)
> 
> My understanding is that we can never break this now. I mean that we
> have to allow userspace putting garbage in the padding field forever
> now.
> 
> I don't fully understand right now then how createv2 implementation
> was suggesting to re-purpose half of the padding for flags at the
> moment as well.

It's simple, the code here doesn't reflect the kernel interface :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Tvrtko Ursulin]

On 31/03/2017 11:48, Chris Wilson wrote:
> On Fri, Mar 31, 2017 at 08:08:28AM +0100, Tvrtko Ursulin wrote:
>>
>> On 30/03/2017 18:22, Chris Wilson wrote:
>>> On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> It is hard to imagine a more basic test than this one.
>>>>
>>>> Also removed the skip on simulation since I don't know why
>>>> would that be needed here.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>> tests/gem_create.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/gem_create.c b/tests/gem_create.c
>>>> index de7b82094545..f687b7b40be4 100644
>>>> --- a/tests/gem_create.c
>>>> +++ b/tests/gem_create.c
>>>> @@ -44,6 +44,7 @@
>>>> #include <sys/stat.h>
>>>> #include <sys/time.h>
>>>> #include <getopt.h>
>>>> +#include <limits.h>
>>>>
>>>> #include <drm.h>
>>>>
>>>> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
>>>>
>>>> static void invalid_size_test(int fd)
>>>> {
>>>> -	int handle;
>>>> +	uint32_t handle;
>>>>
>>>> 	handle = __gem_create(fd, 0);
>>>> 	igt_assert(!handle);
>>>> +
>>>> +	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
>>>
>>> Why is that an invalid size? Invalid huge in terms of API might arguably
>>> be 1<<virtual_bits + 1, but otherwise our only limitation is that it
>>> has to be >0 and page aligned.
>>
>> Because of the comment above the WARN I am removing in "drm/i915:
>> Remove user triggerable WARN from i915_gem_object_create". We cannot
>> support larger ones with the combination of sg_table data types and
>> how we use them (unsigned int nents).
>
> That's an implementation limitation, not an abi one. It is really
> important that we do not enshrine kernel internals as expectations,
> especially not as a basic test - the expectation is that we will support
> massive objects. Having a reality check test to see how far we can get
> is useful.

We added code to the driver to prevent userspace from attempting 
something. It makes sense to have a test for that, so that one day if it 
gets removed in error the test fails, rather than memory corruption in 
the kernel happens.

We can talk about basic or not basic, but I don't see how the existence 
of such test can be argued against in principle.

Regards,

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

Re: [PATCH i-g-t 4/4] gem_create: Convert stolen test to uABI checking

[Tvrtko Ursulin]

On 31/03/2017 11:50, Chris Wilson wrote:
> On Fri, Mar 31, 2017 at 08:11:10AM +0100, Tvrtko Ursulin wrote:
>>
>> On 30/03/2017 18:25, Chris Wilson wrote:
>>> On Thu, Mar 30, 2017 at 05:58:09PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Stolen never materialized so convert this test into checking
>>>> that the garbage in padding remains legal.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>> tests/gem_create.c | 39 ++++++++++++---------------------------
>>>> 1 file changed, 12 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/tests/gem_create.c b/tests/gem_create.c
>>>> index 21df13f7b655..0dad06c784c5 100644
>>>> --- a/tests/gem_create.c
>>>> +++ b/tests/gem_create.c
>>>> @@ -27,8 +27,7 @@
>>>>
>>>> /** @file gem_create.c
>>>>  *
>>>> - * This is a test for the extended and old gem_create ioctl, that
>>>> - * includes allocation of object from stolen memory and shmem.
>>>> + * This is a test for the gem_create ioctl.
>>>>  *
>>>>  * The goal is to simply ensure that basics work and invalid input
>>>>  * combinations are rejected.
>>>> @@ -62,36 +61,22 @@ IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
>>>> 		     " that includes allocation of object from stolen memory"
>>>> 		     " and shmem.");
>>>>
>>>> -#define CLEAR(s) memset(&s, 0, sizeof(s))
>>>> #define PAGE_SIZE 4096
>>>>
>>>> -struct local_i915_gem_create_v2 {
>>>> -	uint64_t size;
>>>> -	uint32_t handle;
>>>> -	uint32_t pad;
>>>> -#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
>>>> -	uint32_t flags;
>>>> -} create;
>>>> -
>>>> -#define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
>>>> -
>>>> -static void invalid_flag_test(int fd)
>>>> +/*
>>>> + * Verify that historical omission of checking for garbage in the padding
>>>> + * field still holds.
>>>> + */
>>>> +static void test_pad_garbage(int fd)
>>>> {
>>>> +	struct drm_i915_gem_create create = { };
>>>> 	int ret;
>>>>
>>>> -	gem_require_stolen_support(fd);
>>>> -
>>>> 	create.handle = 0;
>>>> 	create.size = PAGE_SIZE;
>>>> -	create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
>>>> -	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
>>>> -
>>>> -	igt_assert(ret <= 0);
>>>> -
>>>> -	create.flags = ~0;
>>>> -	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
>>>> -
>>>> -	igt_assert(ret <= 0);
>>>> +	create.pad = 1;
>>>> +	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
>>>> +	igt_assert_eq(ret, 0);
>>>> }
>>>>
>>>> static void invalid_size_test(int fd)
>>>> @@ -134,8 +119,8 @@ igt_main
>>>> 		fd = drm_open_driver(DRIVER_INTEL);
>>>> 	}
>>>>
>>>> -	igt_subtest("stolen-invalid-flag")
>>>> -		invalid_flag_test(fd);
>>>> +	igt_subtest("basic-pad-garbage")
>>>> +		test_pad_garbage(fd);
>>>
>>> Not basic though. I just dislike having negative tests that we intend to
>>> break be part of basic. (I dislike negative tests in general as they are
>>> restrictive and limit creativity, a false limitation in terms of ABI.)
>>
>> My understanding is that we can never break this now. I mean that we
>> have to allow userspace putting garbage in the padding field forever
>> now.
>>
>> I don't fully understand right now then how createv2 implementation
>> was suggesting to re-purpose half of the padding for flags at the
>> moment as well.
>
> It's simple, the code here doesn't reflect the kernel interface :)

Okay I was mistaken that createv2 split the u64 pad into u32 pad and u32 
flags. In fact pad is u32 today and createv2 was extending the structure.

Kernel interface for gem_create today accepts garbage in the pad field. 
Hence we can never use it for something else.

So please explain in more detail why testing that garbage in pad is not 
rejected is wrong. Maybe it's a slow day for me, but I don't get it.

Regards,

Tvrtko


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

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Chris Wilson]

On Fri, Mar 31, 2017 at 12:07:29PM +0100, Tvrtko Ursulin wrote:
> 
> On 31/03/2017 11:48, Chris Wilson wrote:
> >On Fri, Mar 31, 2017 at 08:08:28AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 30/03/2017 18:22, Chris Wilson wrote:
> >>>On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
> >>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>>It is hard to imagine a more basic test than this one.
> >>>>
> >>>>Also removed the skip on simulation since I don't know why
> >>>>would that be needed here.
> >>>>
> >>>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>---
> >>>>tests/gem_create.c | 10 ++++++----
> >>>>1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/tests/gem_create.c b/tests/gem_create.c
> >>>>index de7b82094545..f687b7b40be4 100644
> >>>>--- a/tests/gem_create.c
> >>>>+++ b/tests/gem_create.c
> >>>>@@ -44,6 +44,7 @@
> >>>>#include <sys/stat.h>
> >>>>#include <sys/time.h>
> >>>>#include <getopt.h>
> >>>>+#include <limits.h>
> >>>>
> >>>>#include <drm.h>
> >>>>
> >>>>@@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
> >>>>
> >>>>static void invalid_size_test(int fd)
> >>>>{
> >>>>-	int handle;
> >>>>+	uint32_t handle;
> >>>>
> >>>>	handle = __gem_create(fd, 0);
> >>>>	igt_assert(!handle);
> >>>>+
> >>>>+	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
> >>>
> >>>Why is that an invalid size? Invalid huge in terms of API might arguably
> >>>be 1<<virtual_bits + 1, but otherwise our only limitation is that it
> >>>has to be >0 and page aligned.
> >>
> >>Because of the comment above the WARN I am removing in "drm/i915:
> >>Remove user triggerable WARN from i915_gem_object_create". We cannot
> >>support larger ones with the combination of sg_table data types and
> >>how we use them (unsigned int nents).
> >
> >That's an implementation limitation, not an abi one. It is really
> >important that we do not enshrine kernel internals as expectations,
> >especially not as a basic test - the expectation is that we will support
> >massive objects. Having a reality check test to see how far we can get
> >is useful.
> 
> We added code to the driver to prevent userspace from attempting
> something. It makes sense to have a test for that, so that one day
> if it gets removed in error the test fails, rather than memory
> corruption in the kernel happens.

We added code to detect if we would overflow the internal types. We
don't want that to be an artificial ABI restriction.
 
> We can talk about basic or not basic, but I don't see how the
> existence of such test can be argued against in principle.

My argument is that userspace should be expecting it to succeed and the
test should fail on current kernels because we are not able to live up
to those expectations. It's enshrining the failure as part of the ABI
that I am wary of.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Tvrtko Ursulin]

On 31/03/2017 12:16, Chris Wilson wrote:
> On Fri, Mar 31, 2017 at 12:07:29PM +0100, Tvrtko Ursulin wrote:
>>
>> On 31/03/2017 11:48, Chris Wilson wrote:
>>> On Fri, Mar 31, 2017 at 08:08:28AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 30/03/2017 18:22, Chris Wilson wrote:
>>>>> On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> It is hard to imagine a more basic test than this one.
>>>>>>
>>>>>> Also removed the skip on simulation since I don't know why
>>>>>> would that be needed here.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>> ---
>>>>>> tests/gem_create.c | 10 ++++++----
>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/gem_create.c b/tests/gem_create.c
>>>>>> index de7b82094545..f687b7b40be4 100644
>>>>>> --- a/tests/gem_create.c
>>>>>> +++ b/tests/gem_create.c
>>>>>> @@ -44,6 +44,7 @@
>>>>>> #include <sys/stat.h>
>>>>>> #include <sys/time.h>
>>>>>> #include <getopt.h>
>>>>>> +#include <limits.h>
>>>>>>
>>>>>> #include <drm.h>
>>>>>>
>>>>>> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
>>>>>>
>>>>>> static void invalid_size_test(int fd)
>>>>>> {
>>>>>> -	int handle;
>>>>>> +	uint32_t handle;
>>>>>>
>>>>>> 	handle = __gem_create(fd, 0);
>>>>>> 	igt_assert(!handle);
>>>>>> +
>>>>>> +	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
>>>>>
>>>>> Why is that an invalid size? Invalid huge in terms of API might arguably
>>>>> be 1<<virtual_bits + 1, but otherwise our only limitation is that it
>>>>> has to be >0 and page aligned.
>>>>
>>>> Because of the comment above the WARN I am removing in "drm/i915:
>>>> Remove user triggerable WARN from i915_gem_object_create". We cannot
>>>> support larger ones with the combination of sg_table data types and
>>>> how we use them (unsigned int nents).
>>>
>>> That's an implementation limitation, not an abi one. It is really
>>> important that we do not enshrine kernel internals as expectations,
>>> especially not as a basic test - the expectation is that we will support
>>> massive objects. Having a reality check test to see how far we can get
>>> is useful.
>>
>> We added code to the driver to prevent userspace from attempting
>> something. It makes sense to have a test for that, so that one day
>> if it gets removed in error the test fails, rather than memory
>> corruption in the kernel happens.
>
> We added code to detect if we would overflow the internal types. We
> don't want that to be an artificial ABI restriction.
>
>> We can talk about basic or not basic, but I don't see how the
>> existence of such test can be argued against in principle.
>
> My argument is that userspace should be expecting it to succeed and the
> test should fail on current kernels because we are not able to live up
> to those expectations. It's enshrining the failure as part of the ABI
> that I am wary of.

IGT tests many things apart from the ABI so I wouldn't be worried about 
that so much. Basic keyword is not the same as ABI to me.

Failing test has a problem that it is not very useful since it doesn't 
get run anywhere. Or we could start tagging tests with a test driven 
development tag. Those would then represent tests for features which we 
won't but don't have, or are simply broken.

Still the two are not mutually exclusive. We could have a test with 
knowledge of implementation details to ensure no memory 
corruptions/exploits. And we could have a tdd test like I described 
above. Former would pass today and the latter would fail.

Compromise as having the succeeding test as not basic?

Regards,

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

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Chris Wilson]

On Fri, Mar 31, 2017 at 12:38:51PM +0100, Tvrtko Ursulin wrote:
> 
> On 31/03/2017 12:16, Chris Wilson wrote:
> >On Fri, Mar 31, 2017 at 12:07:29PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 31/03/2017 11:48, Chris Wilson wrote:
> >>>On Fri, Mar 31, 2017 at 08:08:28AM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 30/03/2017 18:22, Chris Wilson wrote:
> >>>>>On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
> >>>>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>
> >>>>>>It is hard to imagine a more basic test than this one.
> >>>>>>
> >>>>>>Also removed the skip on simulation since I don't know why
> >>>>>>would that be needed here.
> >>>>>>
> >>>>>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>---
> >>>>>>tests/gem_create.c | 10 ++++++----
> >>>>>>1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>>diff --git a/tests/gem_create.c b/tests/gem_create.c
> >>>>>>index de7b82094545..f687b7b40be4 100644
> >>>>>>--- a/tests/gem_create.c
> >>>>>>+++ b/tests/gem_create.c
> >>>>>>@@ -44,6 +44,7 @@
> >>>>>>#include <sys/stat.h>
> >>>>>>#include <sys/time.h>
> >>>>>>#include <getopt.h>
> >>>>>>+#include <limits.h>
> >>>>>>
> >>>>>>#include <drm.h>
> >>>>>>
> >>>>>>@@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
> >>>>>>
> >>>>>>static void invalid_size_test(int fd)
> >>>>>>{
> >>>>>>-	int handle;
> >>>>>>+	uint32_t handle;
> >>>>>>
> >>>>>>	handle = __gem_create(fd, 0);
> >>>>>>	igt_assert(!handle);
> >>>>>>+
> >>>>>>+	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
> >>>>>
> >>>>>Why is that an invalid size? Invalid huge in terms of API might arguably
> >>>>>be 1<<virtual_bits + 1, but otherwise our only limitation is that it
> >>>>>has to be >0 and page aligned.
> >>>>
> >>>>Because of the comment above the WARN I am removing in "drm/i915:
> >>>>Remove user triggerable WARN from i915_gem_object_create". We cannot
> >>>>support larger ones with the combination of sg_table data types and
> >>>>how we use them (unsigned int nents).
> >>>
> >>>That's an implementation limitation, not an abi one. It is really
> >>>important that we do not enshrine kernel internals as expectations,
> >>>especially not as a basic test - the expectation is that we will support
> >>>massive objects. Having a reality check test to see how far we can get
> >>>is useful.
> >>
> >>We added code to the driver to prevent userspace from attempting
> >>something. It makes sense to have a test for that, so that one day
> >>if it gets removed in error the test fails, rather than memory
> >>corruption in the kernel happens.
> >
> >We added code to detect if we would overflow the internal types. We
> >don't want that to be an artificial ABI restriction.
> >
> >>We can talk about basic or not basic, but I don't see how the
> >>existence of such test can be argued against in principle.
> >
> >My argument is that userspace should be expecting it to succeed and the
> >test should fail on current kernels because we are not able to live up
> >to those expectations. It's enshrining the failure as part of the ABI
> >that I am wary of.
> 
> IGT tests many things apart from the ABI so I wouldn't be worried
> about that so much. Basic keyword is not the same as ABI to me.
> 
> Failing test has a problem that it is not very useful since it
> doesn't get run anywhere. Or we could start tagging tests with a
> test driven development tag. Those would then represent tests for
> features which we won't but don't have, or are simply broken.
> 
> Still the two are not mutually exclusive. We could have a test with
> knowledge of implementation details to ensure no memory
> corruptions/exploits. And we could have a tdd test like I described
> above. Former would pass today and the latter would fail.
> 
> Compromise as having the succeeding test as not basic?

Can we just probe what the maximum size create will take. Report it and
fail if less than 4GiB. If your ulterior motive is to ensure that we
don't WARN from userspace paths, that suits you, and it suits my
ulterior motive that we do move to support full 64bit allocations.
(And when we do, start planning for 128bit allocations.... :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Tvrtko Ursulin]

On 31/03/2017 12:46, Chris Wilson wrote:
> On Fri, Mar 31, 2017 at 12:38:51PM +0100, Tvrtko Ursulin wrote:
>>
>> On 31/03/2017 12:16, Chris Wilson wrote:
>>> On Fri, Mar 31, 2017 at 12:07:29PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 31/03/2017 11:48, Chris Wilson wrote:
>>>>> On Fri, Mar 31, 2017 at 08:08:28AM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 30/03/2017 18:22, Chris Wilson wrote:
>>>>>>> On Thu, Mar 30, 2017 at 05:58:07PM +0100, Tvrtko Ursulin wrote:
>>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>>
>>>>>>>> It is hard to imagine a more basic test than this one.
>>>>>>>>
>>>>>>>> Also removed the skip on simulation since I don't know why
>>>>>>>> would that be needed here.
>>>>>>>>
>>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>> ---
>>>>>>>> tests/gem_create.c | 10 ++++++----
>>>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/tests/gem_create.c b/tests/gem_create.c
>>>>>>>> index de7b82094545..f687b7b40be4 100644
>>>>>>>> --- a/tests/gem_create.c
>>>>>>>> +++ b/tests/gem_create.c
>>>>>>>> @@ -44,6 +44,7 @@
>>>>>>>> #include <sys/stat.h>
>>>>>>>> #include <sys/time.h>
>>>>>>>> #include <getopt.h>
>>>>>>>> +#include <limits.h>
>>>>>>>>
>>>>>>>> #include <drm.h>
>>>>>>>>
>>>>>>>> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
>>>>>>>>
>>>>>>>> static void invalid_size_test(int fd)
>>>>>>>> {
>>>>>>>> -	int handle;
>>>>>>>> +	uint32_t handle;
>>>>>>>>
>>>>>>>> 	handle = __gem_create(fd, 0);
>>>>>>>> 	igt_assert(!handle);
>>>>>>>> +
>>>>>>>> +	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
>>>>>>>
>>>>>>> Why is that an invalid size? Invalid huge in terms of API might arguably
>>>>>>> be 1<<virtual_bits + 1, but otherwise our only limitation is that it
>>>>>>> has to be >0 and page aligned.
>>>>>>
>>>>>> Because of the comment above the WARN I am removing in "drm/i915:
>>>>>> Remove user triggerable WARN from i915_gem_object_create". We cannot
>>>>>> support larger ones with the combination of sg_table data types and
>>>>>> how we use them (unsigned int nents).
>>>>>
>>>>> That's an implementation limitation, not an abi one. It is really
>>>>> important that we do not enshrine kernel internals as expectations,
>>>>> especially not as a basic test - the expectation is that we will support
>>>>> massive objects. Having a reality check test to see how far we can get
>>>>> is useful.
>>>>
>>>> We added code to the driver to prevent userspace from attempting
>>>> something. It makes sense to have a test for that, so that one day
>>>> if it gets removed in error the test fails, rather than memory
>>>> corruption in the kernel happens.
>>>
>>> We added code to detect if we would overflow the internal types. We
>>> don't want that to be an artificial ABI restriction.
>>>
>>>> We can talk about basic or not basic, but I don't see how the
>>>> existence of such test can be argued against in principle.
>>>
>>> My argument is that userspace should be expecting it to succeed and the
>>> test should fail on current kernels because we are not able to live up
>>> to those expectations. It's enshrining the failure as part of the ABI
>>> that I am wary of.
>>
>> IGT tests many things apart from the ABI so I wouldn't be worried
>> about that so much. Basic keyword is not the same as ABI to me.
>>
>> Failing test has a problem that it is not very useful since it
>> doesn't get run anywhere. Or we could start tagging tests with a
>> test driven development tag. Those would then represent tests for
>> features which we won't but don't have, or are simply broken.
>>
>> Still the two are not mutually exclusive. We could have a test with
>> knowledge of implementation details to ensure no memory
>> corruptions/exploits. And we could have a tdd test like I described
>> above. Former would pass today and the latter would fail.
>>
>> Compromise as having the succeeding test as not basic?
>
> Can we just probe what the maximum size create will take. Report it and
> fail if less than 4GiB. If your ulterior motive is to ensure that we
> don't WARN from userspace paths, that suits you, and it suits my
> ulterior motive that we do move to support full 64bit allocations.
> (And when we do, start planning for 128bit allocations.... :)

Userspace WARN was the trigger, but real motive is making sure page 
count overflow protection remains in place. Ensuring obj->base.size 
matches the backing store and there is no possibility of having an VMA 
larger than the object and so either corruption or reading foreign data. 
Or maybe some other fails along the way as well.

Regards,

Tvrtko




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

Re: [PATCH i-g-t 2/4] gem_create: Test huge object creation as a basic test

[Chris Wilson]

On Fri, Mar 31, 2017 at 12:56:40PM +0100, Tvrtko Ursulin wrote:
> Userspace WARN was the trigger, but real motive is making sure page
> count overflow protection remains in place. Ensuring obj->base.size
> matches the backing store and there is no possibility of having an
> VMA larger than the object and so either corruption or reading
> foreign data. Or maybe some other fails along the way as well.

Just to say: we do have VMA both larger and smaller than the object,
even ones the same size! I regard the kselftests as better means to
really probe internal corner cases, but will never disregard any test
for what it tells us about the system and what it may find that no other
test might.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 2/4] gem_create: Test huge object creation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Verify that we reject attempts to create object larger than
INT_MAX * PAGE_SIZE since i915 currently cannot support that.

Also removed the skip on simulation since I don't know why
would that be needed here.

v2: Removed basic tag and adjusted commit msg.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/gem_create.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/gem_create.c b/tests/gem_create.c
index de7b82094545..844ae8ce86cb 100644
--- a/tests/gem_create.c
+++ b/tests/gem_create.c
@@ -44,6 +44,7 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <getopt.h>
+#include <limits.h>
 
 #include <drm.h>
 
@@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
 
 static void invalid_size_test(int fd)
 {
-	int handle;
+	uint32_t handle;
 
 	handle = __gem_create(fd, 0);
 	igt_assert(!handle);
+
+	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
+	igt_assert(!handle);
 }
 
 /*
@@ -146,8 +150,6 @@ igt_main
 {
 	int fd = -1;
 
-	igt_skip_on_simulation();
-
 	igt_fixture {
 		fd = drm_open_driver(DRIVER_INTEL);
 	}
-- 
2.9.3

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

Re: [PATCH i-g-t v2 2/4] gem_create: Test huge object creation

[Chris Wilson]

On Tue, Apr 18, 2017 at 11:29:34AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Verify that we reject attempts to create object larger than
> INT_MAX * PAGE_SIZE since i915 currently cannot support that.
> 
> Also removed the skip on simulation since I don't know why
> would that be needed here.
> 
> v2: Removed basic tag and adjusted commit msg.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/gem_create.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/gem_create.c b/tests/gem_create.c
> index de7b82094545..844ae8ce86cb 100644
> --- a/tests/gem_create.c
> +++ b/tests/gem_create.c
> @@ -44,6 +44,7 @@
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <getopt.h>
> +#include <limits.h>
>  
>  #include <drm.h>
>  
> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
>  
>  static void invalid_size_test(int fd)
>  {
> -	int handle;
> +	uint32_t handle;
>  
>  	handle = __gem_create(fd, 0);
>  	igt_assert(!handle);
> +
> +	handle = __gem_create(fd, INT_MAX * 4096UL + 1);
> +	igt_assert(!handle);

I'm still not going to agree this is the correct behaviour. That it no
longer triggers the warn will be sufficient for you and Daniel without
enshrining a known bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx