[Intel-gfx] [PATCH i-g-t] tests/gem_ctx_param: Update invalid param

[Chris Wilson]

Quoting Antonio Argenziano (2017-12-18 18:15:35)
> 
> 
> On 15/12/17 16:14, Chris Wilson wrote:
> > Quoting Antonio Argenziano (2017-12-15 19:01:11)
> >> +       arg.param = I915_CONTEXT_PARAM_PRIORITY;
> >> +
> >> +       igt_subtest("root-set-priority") {
> >> +               arg.ctx_id = ctx;
> >> +               arg.size = 0;
> >> +
> > 
> > Bonus points for CAP_SYS_NICE checking.
> > 
> > arg.size validation checking. > arg.value > u32 (checking for overflows)
> 
> What is the expectation for overflows? It looks like we only cast value 
> to int.

That was my point. We could create 1<<32 + prio, so we really should be
failing with -EINVAL rather than setting the parameter to prio. Trust
nothing, least of all the kernel.

> > gem_context_get_param() of a new context should return DEF_PRIO
> > 
> > set_param on older machines returns -ENODEV
> > 
> > And that's not even trying to fuzz bad values beyond the sanity checks
> > of I915_CONTEXT_PARAM_PRIORITY.
> > 
> >> +               for (int i = MIN_PRIO; i <= MAX_PRIO; i += 1023) {
> > 
> > It's a couple of ioctls, do all 1024. Do them in a random order, trust
> > no one.
> 
> Do we have a lib for doing that (generate a random permutation) already?

See igt_permute_array. Maybe igt_random_array(count, prng_state).

> >> +                       arg.value = prio_levels[i];
> >> +                       igt_assert_eq(__gem_context_set_param(fd, &arg), -EINVAL);
> >> +
> >> +                       gem_context_get_param(fd, &arg);
> >> +                       igt_assert(arg.value == old_value); /* Verify prio was not set */
> >> +               }
> >> +       }
> >> +
> >> +       igt_subtest("user-set-priority") {
> >> +               arg.size = 0;
> >> +
> >> +               igt_fork(child, 1) {
> >> +                       igt_drop_root();
> >> +                       for (int i = MIN_PRIO; i <= DEF_PRIO; i += 1023) {
> >> +                               arg.value = i;
> >> +                               gem_context_set_param(fd, &arg);
> >> +
> >> +                               gem_context_get_param(fd, &arg);
> >> +                               igt_assert(arg.value == i); /* Verify prio was set */
> > 
> > I wonder if the CAP_SYS_NICE limit might be adjusted in the future.
> > Certainly preemption rules are flexible, as we haven't told anyone about
> > them yet.
> 
> I guess when the change comes we will have a new constant we could swap 
> DEF_PRIO with.

The question is whether it is ABI, as it may be changed. If its not,
don't enshrine it into law.
-Chris