Message ID | 1395945820-20376-4-git-send-email-bradley.d.volkin@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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; >
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; > > > >
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 --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;