[Intel-gfx] [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 at 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 at 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