Re: [PATCH] drm/i915: Abort command parsing for chained batches

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 16, 2014 at 12:24:42PM -0700, bradley.d.volkin@intel.com wrote:
> From: Brad Volkin <bradley.d.volkin@intel.com>
> 
> libva uses chained batch buffers in a way that the command parser
> can't generally handle. Fortunately, libva doesn't need to write
> registers from batch buffers in the way that mesa does, so this
> patch causes the driver to fall back to non-secure dispatch if
> the parser detects a chained batch buffer.
> 
> Testcase: igt/gem_exec_parse/chained-batch
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c     | 18 +++++++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 24 +++++++++++++-----------
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 86b3ae0..ef38915 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -138,6 +138,11 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
>  			.mask = MI_GLOBAL_GTT,
>  			.expected = 0,
>  	      }},						       ),
> +	/*
> +	 * MI_BATCH_BUFFER_START requires some special handling. It's not
> +	 * really a 'skip' action but it doesn't seem like it's worth adding
> +	 * a new action. See i915_parse_cmds().
> +	 */
>  	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
>  };
>  
> @@ -955,7 +960,8 @@ static bool check_cmd(const struct intel_engine_cs *ring,
>   * Parses the specified batch buffer looking for privilege violations as
>   * described in the overview.
>   *
> - * Return: non-zero if the parser finds violations or otherwise fails
> + * Return: non-zero if the parser finds violations or otherwise fails; -EACCES
> + * if the batch appears legal but should use hardware parsing
>   */
>  int i915_parse_cmds(struct intel_engine_cs *ring,
>  		    struct drm_i915_gem_object *batch_obj,
> @@ -1002,6 +1008,16 @@ int i915_parse_cmds(struct intel_engine_cs *ring,
>  			break;
>  		}
>  
> +		/*
> +		 * If the batch buffer contains a chained batch, return an
> +		 * error that tells the caller to abort and dispatch the
> +		 * workload as a non-secure batch.
> +		 */
> +		if (desc->cmd.value == MI_BATCH_BUFFER_START) {
> +			ret = -EACCES;
> +			break;
> +		}
> +
>  		if (desc->flags & CMD_DESC_FIXED)
>  			length = desc->length.fixed;
>  		else
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 1a0611b..1ed5702 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1368,17 +1368,19 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  				      batch_obj,
>  				      args->batch_start_offset,
>  				      file->is_master);
> -		if (ret)
> -			goto err;
> -
> -		/*
> -		 * XXX: Actually do this when enabling batch copy...
> -		 *
> -		 * Set the DISPATCH_SECURE bit to remove the NON_SECURE bit
> -		 * from MI_BATCH_BUFFER_START commands issued in the
> -		 * dispatch_execbuffer implementations. We specifically don't
> -		 * want that set when the command parser is enabled.
> -		 */
> +		if (ret) {
> +			if (ret != -EACCES)
> +				goto err;
> +		} else {
> +			/*
> +			 * XXX: Actually do this when enabling batch copy...
> +			 *
> +			 * Set the DISPATCH_SECURE bit to remove the NON_SECURE bit
> +			 * from MI_BATCH_BUFFER_START commands issued in the
> +			 * dispatch_execbuffer implementations. We specifically don't
> +			 * want that set when the command parser is enabled.
> +			 */
> +		}

Tbh this hunk here confuses me ... Why do we need to change anything here?

And since we we currently scan batches unconditionally: Shouldn't we
filter out the -EACCESS at a higher level?

In the end I imagine the logic will be:

a) Userspace asks for secure batch
 -> Scan and reject or copy and run.
b) Userspace asks for normal batch
 -> Don't scan, but run without additional hw privs.

Or am I again completely missing the point?

Thanks, Daniel

>  	}
>  
>  	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch