Re: [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.

["Volkin, Bradley D" <bradley.d.volkin@intel.com>]

On Wed, Mar 26, 2014 at 10:37:44AM -0700, Kenneth Graunke wrote:
> On 03/26/2014 09:38 AM, Daniel Vetter wrote:
> > On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote:
> >> On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote:
> >>> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote:
> >>>> Mesa needs to be able to write OACONTROL in order to expose the
> >>>> Observability Architecture's performance counters via OpenGL.
> >>>>
> >>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> >>>
> >>> Thanks a lot for quickly tracking this down. Now when we've talked about
> >>> OA a little while ago we concluded that mesa should clear OACONTROL again
> >>> before the batch ends to make sure that userspace can't unduly observe
> >>> other processes. So I think it'd be worth to keep track of this with a
> >>> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also
> >>> we need to make sure that userspace sets the right OACONTROL modes (not
> >>> the one which streams into a global gtt buffer essentially). So some
> >>> additional work required.
> >>
> >> Ok, I'll look into this. And apologies for not catching it myself.
> >>
> >> If we have to do additional checks on fields within the registers then I
> >> suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That
> >> might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end
> >> of the world, but certainly some additional complexity.
> >>
> >> For the resetting check, are there other registers in the current list that
> >> should have this tracking? If so, is 0 the reset value in all cases?
> >>
> >> Let me know if there is anything in the works that would require additional
> >> registers or different uses of any registers.
> > 
> > Afaik there's no other register we want to reset again. I think all other
> > register we might want to clear are already part of hw contexts, so no
> > chance to leak stuff (e.g. the streamout registers and a end-of-pipe
> > counters). Ken might know of something I've missed.
> > -Daniel
> 
> Right, I think it's just OACONTROL.  I don't think there's a need to
> filter particular values.

Sorry, I don't follow you here. Can you clarify what you mean r.e. filtering?

> 
> I don't really buy the snooping problem, though...just because I leave
> OACONTROL set doesn't mean I'll get useful data.  Another context might
> clobber it, and empirically the numbers seem to reset across RC6 anyway.
>  So in actuality, they're likely to get bogus data.
> 
> Even if they did somehow miraculously get decent values, it basically
> gives information akin to 'top', which is unprivileged on every system
> I've ever used.

I tend to agree with this assesment. The data doesn't seem particularly
sensitive. Also, I assume this data is similar in nature to what you can
already get from graphics perf tools, right?

Thanks,
Brad

> 
> The other alternative is to have the kernel write OACONTROL to 0 after
> any batch that alters it.  Then the kernel wouldn't need to care what
> userspace does with it.  (I think Daniel preferred having userspace
> reset it, though.)
> 
> --Ken
>