On Thu, Mar 27, 2014 at 02:58:01PM -0700, Kenneth Graunke wrote:
> On 03/27/2014 11:43 AM, bradley.d.vol...@intel.com wrote:
> > From: Brad Volkin <bradley.d.vol...@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 <kenn...@whitecape.org>
> > Signed-off-by: Brad Volkin <bradley.d.vol...@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;
> >
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
