Re: [PATCH 3/3] drm/i915: Track OACONTROL register enable/disable during parsing

[Kenneth Graunke <kenneth@whitecape.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4664 bytes --]

On 03/27/2014 11:43 AM, bradley.d.volkin@intel.com wrote:
> From: Brad Volkin <bradley.d.volkin@intel.com>
> 
> There is some thought that the data from the performance counters enabled
> via OACONTROL should only be available to the process that enabled counting.
> To limit snooping, require that any batch buffer which sets OACONTROL to a
> non-zero value also sets it back to 0 before the end of the batch.
> 
> This requires limiting OACONTROL writes to happen via MI_LOAD_REGISTER_IMM
> so that we can access the value being written. This should be in line with
> the expected use case for writing OACONTROL.
> 
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 35 ++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 2eb2aca..779e14c 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -407,12 +407,7 @@ static const u32 gen7_render_regs[] = {
>  	REG64(CL_PRIMITIVES_COUNT),
>  	REG64(PS_INVOCATION_COUNT),
>  	REG64(PS_DEPTH_COUNT),
> -	/*
> -	 * FIXME: This is just to keep mesa working for now, we need to check
> -	 * that mesa resets this again and that it doesn't use any of the
> -	 * special modes which write into the gtt.
> -	 */
> -	OACONTROL,
> +	OACONTROL, /* Only allowed for LRI and SRM. See below. */
>  	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
>  	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
>  	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
> @@ -761,7 +756,8 @@ bool i915_needs_cmd_parser(struct intel_ring_buffer *ring)
>  static bool check_cmd(const struct intel_ring_buffer *ring,
>  		      const struct drm_i915_cmd_descriptor *desc,
>  		      const u32 *cmd,
> -		      const bool is_master)
> +		      const bool is_master,
> +		      bool *oacontrol_set)
>  {
>  	if (desc->flags & CMD_DESC_REJECT) {
>  		DRM_DEBUG_DRIVER("CMD: Rejected command: 0x%08X\n", *cmd);
> @@ -777,6 +773,23 @@ static bool check_cmd(const struct intel_ring_buffer *ring,
>  	if (desc->flags & CMD_DESC_REGISTER) {
>  		u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
>  
> +		/*
> +		 * OACONTROL requires some special handling for writes. We
> +		 * want to make sure that any batch which enables OA also
> +		 * disables it before the end of the batch. The goal is to
> +		 * prevent one process from snooping on the perf data from
> +		 * another process. To do that, we need to check the value
> +		 * that will be written to the register. Hence, limit
> +		 * OACONTROL writes to only MI_LOAD_REGISTER_IMM commands.
> +		 */
> +		if (reg_addr == OACONTROL) {
> +			if (desc->cmd.value == MI_LOAD_REGISTER_MEM)
> +				return false;

While I don't need the ability to use LRM on OACONTROL, I don't see any
specific reason to prohibit it, as long as there's a later LRI of 0.

I think you could just do:

if (desc->cmd.value == MI_LOAD_REGISTER_MEM)
   oacontrol_set = true;

which will later get cleared if it sees a LRI of 0, and if not, you
reject the batch.

> +
> +			if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
> +				*oacontrol_set = (cmd[2] != 0) ? true : false;

You shouldn't need to do both != 0 and ? true : false...

*oacontrol_set = cmd[2] != 0;

Thanks for implementing this!  That was surprisingly less code than I
thought.

> +		}
> +
>  		if (!valid_reg(ring->reg_table,
>  			       ring->reg_count, reg_addr)) {
>  			if (!is_master ||
> @@ -851,6 +864,7 @@ int i915_parse_cmds(struct intel_ring_buffer *ring,
>  	u32 *cmd, *batch_base, *batch_end;
>  	struct drm_i915_cmd_descriptor default_desc = { 0 };
>  	int needs_clflush = 0;
> +	bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */
>  
>  	ret = i915_gem_obj_prepare_shmem_read(batch_obj, &needs_clflush);
>  	if (ret) {
> @@ -900,7 +914,7 @@ int i915_parse_cmds(struct intel_ring_buffer *ring,
>  			break;
>  		}
>  
> -		if (!check_cmd(ring, desc, cmd, is_master)) {
> +		if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) {
>  			ret = -EINVAL;
>  			break;
>  		}
> @@ -908,6 +922,11 @@ int i915_parse_cmds(struct intel_ring_buffer *ring,
>  		cmd += length;
>  	}
>  
> +	if (oacontrol_set) {
> +		DRM_DEBUG_DRIVER("CMD: batch set OACONTROL but did not clear it\n");
> +		ret = -EINVAL;
> +	}
> +
>  	if (cmd >= batch_end) {
>  		DRM_DEBUG_DRIVER("CMD: Got to the end of the buffer w/o a BBE cmd!\n");
>  		ret = -EINVAL;
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx