[igt-dev] [Intel-gfx] [PATH i-g-t 2/2] tests: add slice power programming test

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-09-06 10:31:21)
> 
> On 06/09/2018 08:00, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-09-05 15:25:44)
> >> From: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> >>
> >> Verifies that the kernel programs slices correctly based by reading
> >> the value of PWR_CLK_STATE register or MI_SET_PREDICATE on platforms
> >> before Cannonlake.
> >>
> >> v2: Add subslice tests (Lionel)
> >>      Use MI_SET_PREDICATE for further verification when available (Lionel)
> >>
> >> v3: Rename to gem_ctx_rpcs (Lionel)
> >>
> >> v4: Update kernel API (Lionel)
> >>      Add 0 value test (Lionel)
> >>      Exercise invalid values (Lionel)
> >>
> >> v5: Add perf tests (Lionel)
> >>
> >> v6: Add new sysfs entry tests (Lionel)
> >>
> >> v7: Test rsvd fields
> >>      Update for kernel series changes
> >>
> >> v8: Drop test_no_sseu_support() test (Kelvin)
> >>      Drop drm_intel_*() apis (Chris)
> >>
> >> v9: by Chris:
> >>      Drop all do_ioctl/do_ioctl_err()
> >>      Use gem_context_[gs]et_param()
> >>      Use gem_read() instead of mapping memory
> >>      by Lionel:
> >>      Test dynamic sseu on/off more
> >>
> >> Tvrtko Ursulin:
> >>
> >> v10:
> >>   * Various style tweaks and refactorings.
> >>   * New test coverage.
> > 
> > I didn't notice any testing of:
> >   - param->size
> 
> It exists in test_invalid_args.
> 
> >   - feeding garbage into param->value user pointer (always cleared before
> >     use, perhaps just poison instead), along with abusive pointers.
> 
> Also in test_invalid_args - but only the null pointer. I can add an 
> unmapped or read-only one.

I think passing a pointer that starts valid and crosses into an unmapped
page is essential. That makes sure we are using copy_from_user
correctly. Another trivial one is param->value = -param->size + 1.

As for the garbage, I wasn't sure (because I'm fully cogniscent of the
sseu responses) if we would not interpret 0 as a valid response. If you
are happy that we can differentiate an output 0 after passing in 0,
that's all fine (as opposed to us forgetting to fill a value along some
path).

> >     E.g., how does the code fare if we pass in an unfaulted GGTT mmap?
> 
> Would not fare well. :I It would be best to be able to reject them but 
> how? We'll hit the same problem in future other patches so to support 
> this, I think we need to refactor

Do you want my patch to drop struct_mutex entirely here :) So then you
have to add it where you need it, which is after the copy_from_user() so
recursive faults are not a problem.
-Chris