Re: [igt-dev] [PATCH i-g-t 1/1] tests/i915/gem_ctx_persistence: Set context with supported engines

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/01/2020 11:03, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-10 10:56:59)
>>
>> These were my suggestions so I'll reply. :)
>>
>> On 09/01/2020 09:18, Chris Wilson wrote:
>>> Quoting Bommu Krishnaiah (2020-01-09 08:50:23)
>>>> Update the context with supported engines on the platform with set_property
>>>> I915_CONTEXT_PARAM_ENGINES to make sure the work load is submitted to
>>>> the available engines only.
>>>>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
>>>> ---
>>>>    tests/i915/gem_ctx_persistence.c | 34 ++++++++++++++++++--------------
>>>>    1 file changed, 19 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
>>>> index d68431ae..e410e85f 100644
>>>> --- a/tests/i915/gem_ctx_persistence.c
>>>> +++ b/tests/i915/gem_ctx_persistence.c
>>>> @@ -145,6 +145,15 @@ static void test_clone(int i915)
>>>>           gem_context_destroy(i915, clone);
>>>>    }
>>>>    
>>>> +static uint32_t create_context(int i915, bool persistent)
>>>> +{
>>>> +       uint32_t ctx;
>>>> +
>>>> +       ctx = gem_context_create(i915);
>>>> +       gem_context_set_persistence(i915, ctx, persistent);
>>>> +       gem_context_set_all_engines(i915, ctx);
>>>
>>> Really? We only need a single engine, could you not be a little more
>>> precise.
>>
>> We have a general situation where as soon as we call
>> __for_each_physical_engine, all code/subtests that follow and which
>> create new contexts, which are then submitted mixing the flags from the
>> iterated engines, needs to have a matching engine map. This stateful
>> behaviour causes problems but it's what we have. I don't have any better
>> ideas on how to universally go about this.
>>
>>>
>>>> +       return ctx;
>>>> +}
>>>>    static void test_persistence(int i915, unsigned int engine)
>>>>    {
>>>>           igt_spin_t *spin;
>>>> @@ -156,8 +165,7 @@ static void test_persistence(int i915, unsigned int engine)
>>>>            * request is retired -- no early termination.
>>>>            */
>>>>    
>>>> -       ctx = gem_context_create(i915);
>>>> -       gem_context_set_persistence(i915, ctx, true);
>>>> +       ctx = create_context(i915, true);
>>>>    
>>>>           spin = igt_spin_new(i915, ctx,
>>>>                               .engine = engine,
>>>> @@ -188,8 +196,7 @@ static void test_nonpersistent_cleanup(int i915, unsigned int engine)
>>>>            * any inflight request is cancelled.
>>>>            */
>>>>    
>>>> -       ctx = gem_context_create(i915);
>>>> -       gem_context_set_persistence(i915, ctx, false);
>>>> +       ctx = create_context(i915, false);
>>>>    
>>>>           spin = igt_spin_new(i915, ctx,
>>>>                               .engine = engine,
>>>> @@ -217,8 +224,7 @@ static void test_nonpersistent_mixed(int i915, unsigned int engine)
>>>>                   igt_spin_t *spin;
>>>>                   uint32_t ctx;
>>>>    
>>>> -               ctx = gem_context_create(i915);
>>>> -               gem_context_set_persistence(i915, ctx, i & 1);
>>>> +               ctx = create_context(i915, i & 1);
>>>>    
>>>>                   spin = igt_spin_new(i915, ctx,
>>>>                                       .engine = engine,
>>>> @@ -253,8 +259,7 @@ static void test_nonpersistent_hostile(int i915, unsigned int engine)
>>>>            * the requests and cleanup the context.
>>>>            */
>>>>    
>>>> -       ctx = gem_context_create(i915);
>>>> -       gem_context_set_persistence(i915, ctx, false);
>>>> +       ctx = create_context(i915, false);
>>>>    
>>>>           spin = igt_spin_new(i915, ctx,
>>>>                               .engine = engine,
>>>> @@ -284,8 +289,8 @@ static void test_nonpersistent_hostile_preempt(int i915, unsigned int engine)
>>>>    
>>>>           igt_require(gem_scheduler_has_preemption(i915));
>>>>    
>>>> -       ctx = gem_context_create(i915);
>>>> -       gem_context_set_persistence(i915, ctx, true);
>>>> +       ctx = create_context(i915, true);
>>>> +
>>>>           gem_context_set_priority(i915, ctx, 0);
>>>>           spin[0] = igt_spin_new(i915, ctx,
>>>>                                  .engine = engine,
>>>> @@ -385,8 +390,8 @@ static void test_nonpersistent_queued(int i915, unsigned int engine)
>>>>    
>>>>           gem_quiescent_gpu(i915);
>>>>    
>>>> -       ctx = gem_context_create(i915);
>>>> -       gem_context_set_persistence(i915, ctx, false);
>>>> +       ctx = create_context(i915, false);
>>>> +
>>>>           spin = igt_spin_new(i915, ctx,
>>>>                               .engine = engine,
>>>>                               .flags = IGT_SPIN_FENCE_OUT);
>>>> @@ -512,8 +517,7 @@ static void test_process_mixed(int i915, unsigned int engine)
>>>>                           igt_spin_t *spin;
>>>>                           uint32_t ctx;
>>>>    
>>>> -                       ctx = gem_context_create(i915);
>>>> -                       gem_context_set_persistence(i915, ctx, persists);
>>>> +                       ctx = create_context(i915, persists);
>>>>    
>>>>                           spin = igt_spin_new(i915, ctx,
>>>>                                               .engine = engine,
>>>> @@ -727,7 +731,7 @@ igt_main
>>>>           igt_subtest("hangcheck")
>>>>                   test_nohangcheck_hostile(i915);
>>>>    
>>>> -       __for_each_static_engine(e) {
>>>> +       __for_each_physical_engine(i915, e) {
>>>
>>> Please note this has to check ABI. We have to confirm that no matter how
>>> the user tries to fool us, they cannot. So the gem_context_set_engines()
>>> should be considered a separate step, or a very, very persuasive
>>> argument made as to why we consider the multiple ABI paths equivalent,
>>> bearing in mind the security implications of getting it wrong.
>>
>> So duplicate the subtest loop, first do ABI, then physical?
>>
>> for_each_engine(..) {
>>          ...
>>
>>          igt_subtest_f("legacy-%s-persistence")
>>                  test_persistence(..)
>>          ...
>> }
>>
>> __for_each_physical_engine(..) {
>>          ...
>>
>>          igt_subtest_f("%s-persistence")
>>                  test_persistence(..)
>>          ...
>> }
>>
>> Then in create_context helper we need to query the default context (
>> gem_context_has_engine_map) to establish if we need to call
>> gem_context_set_all_engines or not.
>>
>> Acceptable?
> 
> Along the lines (I don't like gem_context_set_all_engines very much, and
> so will keep nagging.). However, talking about engines[] vs persistence
> opened up a can of worms, in that we need to keep silly/abusive
> engines[] to make sure a hostile user can not circumvent the DoS
> prevention. So the task got a bit bigger :-p

Let Bommu finish a simple task first if you can live with the above 
plan. And even this simple task is not so simple as demonstrated! :) 
More evil testing can be then added later.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev