diff mbox

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

Message ID 1395945820-20376-4-git-send-email-bradley.d.volkin@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

bradley.d.volkin@intel.com March 27, 2014, 6:43 p.m. UTC
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(-)

Comments

Kenneth Graunke March 27, 2014, 9:58 p.m. UTC | #1
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;
>
bradley.d.volkin@intel.com March 28, 2014, 12:06 a.m. UTC | #2
On Thu, Mar 27, 2014 at 02:58:01PM -0700, Kenneth Graunke wrote:
> 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.

Fair enough. Rejecting LRM was as much a carryover from thinking we needed to
check bits within the value. I'll make this change if there are no objections.

> 
> > +
> > +			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;

Will fix.

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

This is the "you told me there's only one register that needs this" implementation :)
A more general solution would probably be uglier.

Thanks,
Brad

> 
> > +		}
> > +
> >  		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;
> > 
> 
>
Daniel Vetter March 28, 2014, 7:51 a.m. UTC | #3
On Thu, Mar 27, 2014 at 05:06:33PM -0700, Volkin, Bradley D wrote:
> On Thu, Mar 27, 2014 at 02:58:01PM -0700, Kenneth Graunke wrote:
> > 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.
> 
> Fair enough. Rejecting LRM was as much a carryover from thinking we needed to
> check bits within the value. I'll make this change if there are no objections.

Imo if there's no user there's no need for that.
-Daniel
diff mbox

Patch

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;
+
+			if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
+				*oacontrol_set = (cmd[2] != 0) ? true : false;
+		}
+
 		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;