| Message ID | 20170330165809.24838-3-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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")