Re: [igt-dev] [PATCH i-g-t 13/17] Verify execbuf fails with stale PXP buffer after teardown

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, May 18, 2021 at 03:33:40AM -0700, Alan Previn wrote:
> Add a check to verify that reusing a stale protected buffer
> in a gem_execbuff (with a protected context) after a teardown
> (triggered by suspend-resume cycle) shall fail with -ENOEXEC error.
> Reuse the existing subtest function to with separate assets
> to minimize the number of suspend-resume cycles we take.
> 
> NOTE: The end-to-end architecture requirement includes that
> any break in the links of the PXP sessions needs to trigger a
> full teardown and the application needs to be made aware of that
> allowing it to re-establish the end-to-end pipeline of buffers,
> contexts and renders again if it chooses to. This stricter
> behavior targets only contexts created with PXP enabled.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  tests/i915/gem_pxp.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/i915/gem_pxp.c b/tests/i915/gem_pxp.c
> index f1ef0a24..b4f369ae 100644
> --- a/tests/i915/gem_pxp.c
> +++ b/tests/i915/gem_pxp.c
> @@ -760,23 +760,45 @@ static void free_exec_assets(int i915, struct simple_exec_assets *data)
>  static void test_pxp_pwrcycle_staleasset_execution(int i915, struct powermgt_data *pm)
>  {
>  	int ret;
> -	struct simple_exec_assets data = {0};
> +	struct simple_exec_assets data[2] = {{0}, {0}};
> +	uint32_t ctx2;
> +	struct intel_bb *ibb2;
>  
>  	/*
> -	 * Use normal buffers for testing for invalidation
> +	 * For data[0]: Use normal buffers for testing for invalidation
>  	 * of protected contexts to ensure kernel is catching
>  	 * the invalidated context (not buffer)
>  	 */
> -	prepare_exec_assets(i915, &data, true, false);
> -	ret = gem_execbuf_flush_store_dw(i915, data.ibb, data.ctx, data.fencebuf);
> +	prepare_exec_assets(i915, &data[0], true, false);
> +	ret = gem_execbuf_flush_store_dw(i915, data[0].ibb, data[0].ctx, data[0].fencebuf);
> +	igt_assert(ret == 0);
> +
> +	prepare_exec_assets(i915, &data[1], true, true);
> +	ret = gem_execbuf_flush_store_dw(i915, data[1].ibb, data[1].ctx, data[1].fencebuf);
>  	igt_assert(ret == 0);
>  
>  	trigger_powermgt_suspend_cycle(i915, pm);
>  
> -	ret = gem_execbuf_flush_store_dw(i915, data.ibb, data.ctx, data.fencebuf);
> +	ret = gem_execbuf_flush_store_dw(i915, data[0].ibb, data[0].ctx, data[0].fencebuf);
>  	igt_assert_f((ret == -EACCES), "Executing stale pxp context didn't fail with -EACCES\n");
>  
> -	free_exec_assets(i915, &data);
> +	/*
> +	 * For data[1]: after teardown, alloc new assets for context but
> +	 * reuse the bo to ensure the kernel is catching the
> +	 * invalidated bo (not context)
> +	 */
> +	ret = create_ctx_with_params(i915, true, true, true, false, &ctx2);
> +	igt_assert_eq(ret, 0);
> +	igt_assert_eq(get_ctx_protected_param(i915, ctx2), 1);
> +	ibb2 = intel_bb_create_with_context(i915, ctx2, 4096);
> +	igt_assert(ibb2);
> +	intel_bb_detach_intel_buf(data[1].ibb, data[1].fencebuf);
> +	intel_bb_add_intel_buf(ibb2, data[1].fencebuf, true);
> +	ret = gem_execbuf_flush_store_dw(i915, ibb2, ctx2, data[1].fencebuf);
> +	igt_assert_f((ret == -ENOEXEC), "Executing stale pxp buffer didn't fail with -ENOEXEC\n");
> +
> +	free_exec_assets(i915, &data[0]);
> +	free_exec_assets(i915, &data[1]);
>  }
>  
>  igt_main
> -- 
> 2.25.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev