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

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

[Kenneth Graunke]

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>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 1 +
 drivers/gpu/drm/i915/i915_reg.h        | 2 ++
 2 files changed, 3 insertions(+)

This patch needs to go before

   commit 6d42f94084b8c69813d7ecd0466c33fe561bf127
   Author: Brad Volkin <bradley.d.volkin@intel.com>
   Date:   Tue Feb 18 10:15:57 2014 -0800

       drm/i915: Enable command parsing by default

in whatever branch gets submitted to Dave Airlie.  Or, that commit needs
to be reverted.  Otherwise, every OpenGL program will abort.  Examples
of programs that abort include GNOME, KDE, Firefox, and glxgears.

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index bae7c2f..d4a50b9 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = {
 	GEN7_SO_WRITE_OFFSET(1),
 	GEN7_SO_WRITE_OFFSET(2),
 	GEN7_SO_WRITE_OFFSET(3),
+	OACONTROL,
 };
 
 static const u32 gen7_blt_regs[] = {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9f9e2b7..0ebc20d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -427,6 +427,8 @@
 /* There are the 4 64-bit counter registers, one for each stream output */
 #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
 
+#define OACONTROL 0x2360
+
 #define _GEN7_PIPEA_DE_LOAD_SL	0x70068
 #define _GEN7_PIPEB_DE_LOAD_SL	0x71068
 #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
-- 
1.9.0

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

[Daniel Vetter]

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.

I've added a FIXME comment to the code.

Brad, can you please look into this, including the corresponding i-g-t
which tries to enable OA but doesn't disable it and one which tries to
enable it in the continuous mode (which writes to gtt)? Ken's previous
mails has the cmds mesa emits.

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 1 +
>  drivers/gpu/drm/i915/i915_reg.h        | 2 ++
>  2 files changed, 3 insertions(+)
> 
> This patch needs to go before
> 
>    commit 6d42f94084b8c69813d7ecd0466c33fe561bf127
>    Author: Brad Volkin <bradley.d.volkin@intel.com>
>    Date:   Tue Feb 18 10:15:57 2014 -0800
> 
>        drm/i915: Enable command parsing by default
> 
> in whatever branch gets submitted to Dave Airlie.  Or, that commit needs
> to be reverted.  Otherwise, every OpenGL program will abort.  Examples
> of programs that abort include GNOME, KDE, Firefox, and glxgears.
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index bae7c2f..d4a50b9 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = {
>  	GEN7_SO_WRITE_OFFSET(1),
>  	GEN7_SO_WRITE_OFFSET(2),
>  	GEN7_SO_WRITE_OFFSET(3),
> +	OACONTROL,
>  };
>  
>  static const u32 gen7_blt_regs[] = {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9f9e2b7..0ebc20d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -427,6 +427,8 @@
>  /* There are the 4 64-bit counter registers, one for each stream output */
>  #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
>  
> +#define OACONTROL 0x2360
> +
>  #define _GEN7_PIPEA_DE_LOAD_SL	0x70068
>  #define _GEN7_PIPEB_DE_LOAD_SL	0x71068
>  #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Volkin, Bradley D]

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.

Thanks,
Brad

> 
> I've added a FIXME comment to the code.
> 
> Brad, can you please look into this, including the corresponding i-g-t
> which tries to enable OA but doesn't disable it and one which tries to
> enable it in the continuous mode (which writes to gtt)? Ken's previous
> mails has the cmds mesa emits.
> 
> Thanks, Daniel
> > ---
> >  drivers/gpu/drm/i915/i915_cmd_parser.c | 1 +
> >  drivers/gpu/drm/i915/i915_reg.h        | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > This patch needs to go before
> > 
> >    commit 6d42f94084b8c69813d7ecd0466c33fe561bf127
> >    Author: Brad Volkin <bradley.d.volkin@intel.com>
> >    Date:   Tue Feb 18 10:15:57 2014 -0800
> > 
> >        drm/i915: Enable command parsing by default
> > 
> > in whatever branch gets submitted to Dave Airlie.  Or, that commit needs
> > to be reverted.  Otherwise, every OpenGL program will abort.  Examples
> > of programs that abort include GNOME, KDE, Firefox, and glxgears.
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index bae7c2f..d4a50b9 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = {
> >  	GEN7_SO_WRITE_OFFSET(1),
> >  	GEN7_SO_WRITE_OFFSET(2),
> >  	GEN7_SO_WRITE_OFFSET(3),
> > +	OACONTROL,
> >  };
> >  
> >  static const u32 gen7_blt_regs[] = {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 9f9e2b7..0ebc20d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -427,6 +427,8 @@
> >  /* There are the 4 64-bit counter registers, one for each stream output */
> >  #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
> >  
> > +#define OACONTROL 0x2360
> > +
> >  #define _GEN7_PIPEA_DE_LOAD_SL	0x70068
> >  #define _GEN7_PIPEB_DE_LOAD_SL	0x71068
> >  #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
> > -- 
> > 1.9.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Kenneth Graunke]

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

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.

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.

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


[-- 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

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

[Volkin, Bradley D]

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
> 

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

[Daniel Vetter]

On Wed, Mar 26, 2014 at 11:26:05AM -0700, Volkin, Bradley D wrote:
> 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?

I've thought perf is priviledged ... And I've looked at the trigger based
stuff again in Bspec and it looks like by default that's all disabled, so
even when batches enable the timer nothing should happen.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Kenneth Graunke]

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

On 03/26/2014 11:26 AM, Volkin, Bradley D wrote:
> 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?

Oh, I meant I don't think we need to do additional checks on "fields
within the registers" (to quote your email) or restrict the values we
can write to the registers.  Just the ability to write the whole
register to any arbitrary value should be sufficient.

Thanks Brad!

--Ken


[-- 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

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

[Daniel Vetter]

On Wed, Mar 26, 2014 at 11:26:05AM -0700, Volkin, Bradley D wrote:
> 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?

Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's
an issue with the test or the checker:

https://bugs.freedesktop.org/show_bug.cgi?id=76670

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Volkin, Bradley D]

[snip]

On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote:
> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's
> an issue with the test or the checker:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=76670

For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit
set. We don't currently allow that, even from master. It sounds like there
might be released versions of the ddx that do this as well. If that's the
case, or if there are other situations where tests, etc rely on being able
to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I
think we might as well stop parsing secure batches and let them go through
as before.

Thanks,
Brad

> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D
<bradley.d.volkin@intel.com> wrote:
> On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote:
>> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's
>> an issue with the test or the checker:
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=76670
>
> For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit
> set. We don't currently allow that, even from master. It sounds like there
> might be released versions of the ddx that do this as well. If that's the
> case, or if there are other situations where tests, etc rely on being able
> to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I
> think we might as well stop parsing secure batches and let them go through
> as before.

Well for the testcase I think we can just add the missing flag. If
there's indeed shipping userspace out there which is getting these
flags wrong then I think we need to silently upgrade them when copying
the cmds over to the 2nd batch. But I guess until that need is really
established we can hope we don't need this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Kenneth Graunke]

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

On 03/27/2014 01:16 PM, Daniel Vetter wrote:
> On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D
> <bradley.d.volkin@intel.com> wrote:
>> On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote:
>>> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's
>>> an issue with the test or the checker:
>>>
>>> https://bugs.freedesktop.org/show_bug.cgi?id=76670
>>
>> For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit
>> set. We don't currently allow that, even from master. It sounds like there
>> might be released versions of the ddx that do this as well. If that's the
>> case, or if there are other situations where tests, etc rely on being able
>> to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I
>> think we might as well stop parsing secure batches and let them go through
>> as before.
> 
> Well for the testcase I think we can just add the missing flag. If
> there's indeed shipping userspace out there which is getting these
> flags wrong then I think we need to silently upgrade them when copying
> the cmds over to the 2nd batch. But I guess until that need is really
> established we can hope we don't need this.
> -Daniel

Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
are only issued from trusted code which is guaranteed to be running as
root.  I don't see any benefit to scanning those batches, and there's
definitely overhead.

I mean, sure, it may be reasonable in the short term as a way to test
the command parser, but I certainly hope we don't *ship* that.

--Ken


[-- 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

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

[Daniel Vetter]

On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
> are only issued from trusted code which is guaranteed to be running as
> root.  I don't see any benefit to scanning those batches, and there's
> definitely overhead.
>
> I mean, sure, it may be reasonable in the short term as a way to test
> the command parser, but I certainly hope we don't *ship* that.

Everyone runs X as root, but I kinda want X to also be able to run as
non-root. The cmd parser has a special list of drm master register
lists which should allow this, but if we just bypass the cmd parser
for all normal X installs we'll have 0 test coverage on this. Which
means broken like hell.

Hence I actually intend to ship this, yes. Chris doesn't like it either really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Kenneth Graunke]

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

On 03/27/2014 03:44 PM, Daniel Vetter wrote:
> On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
>> Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
>> are only issued from trusted code which is guaranteed to be running as
>> root.  I don't see any benefit to scanning those batches, and there's
>> definitely overhead.
>>
>> I mean, sure, it may be reasonable in the short term as a way to test
>> the command parser, but I certainly hope we don't *ship* that.
> 
> Everyone runs X as root, but I kinda want X to also be able to run as
> non-root. The cmd parser has a special list of drm master register
> lists which should allow this, but if we just bypass the cmd parser
> for all normal X installs we'll have 0 test coverage on this. Which
> means broken like hell.
> 
> Hence I actually intend to ship this, yes. Chris doesn't like it either really.
> -Daniel

Seriously?  Hurt performance on every user's system just so you can test
things?  That a classic case of the tail wagging the dog.

Why not make a i915.enable_cmd_parser=2 value which enables it all the
time and use that when running igt?  Clearly being able to test this is
valuable, but enabling it universally is *not* OK.


[-- 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

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

[Jesse Barnes]

On Thu, 27 Mar 2014 16:22:44 -0700
Kenneth Graunke <kenneth@whitecape.org> wrote:

> On 03/27/2014 03:44 PM, Daniel Vetter wrote:
> > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> >> Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
> >> are only issued from trusted code which is guaranteed to be running as
> >> root.  I don't see any benefit to scanning those batches, and there's
> >> definitely overhead.
> >>
> >> I mean, sure, it may be reasonable in the short term as a way to test
> >> the command parser, but I certainly hope we don't *ship* that.
> > 
> > Everyone runs X as root, but I kinda want X to also be able to run as
> > non-root. The cmd parser has a special list of drm master register
> > lists which should allow this, but if we just bypass the cmd parser
> > for all normal X installs we'll have 0 test coverage on this. Which
> > means broken like hell.
> > 
> > Hence I actually intend to ship this, yes. Chris doesn't like it either really.
> > -Daniel
> 
> Seriously?  Hurt performance on every user's system just so you can test
> things?  That a classic case of the tail wagging the dog.
> 
> Why not make a i915.enable_cmd_parser=2 value which enables it all the
> time and use that when running igt?  Clearly being able to test this is
> valuable, but enabling it universally is *not* OK.

Daniel, I'm not sure what you mean by 0 coverage.  Surely DRI clients
count for something?  And X shouldn't be submitting all its batches
with the secure bit set, right?  If so, we ought to fix that and only
use it for ones where it's necessary (e.g. wait events or similar).  I
agree with Ken and Chris here.

Chris?

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Chris Wilson]

On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote:
> On Thu, 27 Mar 2014 16:22:44 -0700
> Kenneth Graunke <kenneth@whitecape.org> wrote:
> 
> > On 03/27/2014 03:44 PM, Daniel Vetter wrote:
> > > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> > >> Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
> > >> are only issued from trusted code which is guaranteed to be running as
> > >> root.  I don't see any benefit to scanning those batches, and there's
> > >> definitely overhead.
> > >>
> > >> I mean, sure, it may be reasonable in the short term as a way to test
> > >> the command parser, but I certainly hope we don't *ship* that.
> > > 
> > > Everyone runs X as root, but I kinda want X to also be able to run as
> > > non-root. The cmd parser has a special list of drm master register
> > > lists which should allow this, but if we just bypass the cmd parser
> > > for all normal X installs we'll have 0 test coverage on this. Which
> > > means broken like hell.
> > > 
> > > Hence I actually intend to ship this, yes. Chris doesn't like it either really.
> > > -Daniel
> > 
> > Seriously?  Hurt performance on every user's system just so you can test
> > things?  That a classic case of the tail wagging the dog.
> > 
> > Why not make a i915.enable_cmd_parser=2 value which enables it all the
> > time and use that when running igt?  Clearly being able to test this is
> > valuable, but enabling it universally is *not* OK.
> 
> Daniel, I'm not sure what you mean by 0 coverage.  Surely DRI clients
> count for something?  And X shouldn't be submitting all its batches
> with the secure bit set, right?  If so, we ought to fix that and only
> use it for ones where it's necessary (e.g. wait events or similar).  I
> agree with Ken and Chris here.
> 
> Chris?

We haven't even fixed the major regression from enabling FBC. What's
another massive slowdown?

Yes, X only sets the secure bit when it pokes the display registers, and
those registers should be privileged even with a cmd parser in place
(which they are).

Daniel's argument presumes that we haven't been patching out the
cmd parser all this time anyway.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Fri, 16 May 2014 20:20:50 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote:
> > On Thu, 27 Mar 2014 16:22:44 -0700
> > Kenneth Graunke <kenneth@whitecape.org> wrote:
> > 
> > > On 03/27/2014 03:44 PM, Daniel Vetter wrote:
> > > > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> > > >> Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
> > > >> are only issued from trusted code which is guaranteed to be running as
> > > >> root.  I don't see any benefit to scanning those batches, and there's
> > > >> definitely overhead.
> > > >>
> > > >> I mean, sure, it may be reasonable in the short term as a way to test
> > > >> the command parser, but I certainly hope we don't *ship* that.
> > > > 
> > > > Everyone runs X as root, but I kinda want X to also be able to run as
> > > > non-root. The cmd parser has a special list of drm master register
> > > > lists which should allow this, but if we just bypass the cmd parser
> > > > for all normal X installs we'll have 0 test coverage on this. Which
> > > > means broken like hell.
> > > > 
> > > > Hence I actually intend to ship this, yes. Chris doesn't like it either really.
> > > > -Daniel
> > > 
> > > Seriously?  Hurt performance on every user's system just so you can test
> > > things?  That a classic case of the tail wagging the dog.
> > > 
> > > Why not make a i915.enable_cmd_parser=2 value which enables it all the
> > > time and use that when running igt?  Clearly being able to test this is
> > > valuable, but enabling it universally is *not* OK.
> > 
> > Daniel, I'm not sure what you mean by 0 coverage.  Surely DRI clients
> > count for something?  And X shouldn't be submitting all its batches
> > with the secure bit set, right?  If so, we ought to fix that and only
> > use it for ones where it's necessary (e.g. wait events or similar).  I
> > agree with Ken and Chris here.
> > 
> > Chris?
> 
> We haven't even fixed the major regression from enabling FBC. What's
> another massive slowdown?

I thought you had that fixed in the X driver by avoiding front buffer
rendering altogether.  If that's the case we just need an ioctl to opt
out of front buffer tracking, right?  Presumably that flag would follow
the current DRM_MASTER process...

> Yes, X only sets the secure bit when it pokes the display registers, and
> those registers should be privileged even with a cmd parser in place
> (which they are).
> 
> Daniel's argument presumes that we haven't been patching out the
> cmd parser all this time anyway.

Yeah I know we have some perf issues as it is; it would be nice if the
overhead were so minimal that it didn't matter.  But just on principle,
scanning secure buffers seems wrong, and I'm trying to understand why
Daniel would want it.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Chris Wilson]

On Fri, May 16, 2014 at 12:34:08PM -0700, Jesse Barnes wrote:
> On Fri, 16 May 2014 20:20:50 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > We haven't even fixed the major regression from enabling FBC. What's
> > another massive slowdown?
> 
> I thought you had that fixed in the X driver by avoiding front buffer
> rendering altogether.  If that's the case we just need an ioctl to opt
> out of front buffer tracking, right?  Presumably that flag would follow
> the current DRM_MASTER process...

No. All rendering triggers FBC updates. Ville has patches to fix the
majority of that with only nuking FBC when front buffer rendering. (Note
that games aren't usually affected by this because FBC is disabled by
pageflipping.) The overhead could probably be reduced further by periodic
nuking the FBC like (ideally) PSR. And yes X can avoid front buffer
rendering altogether. The remaining challenge is to know when to enable it
(the kernel doesn't give us any information about FBC or PSR after setting a
mode) and when not, i.e. there is already a pageflipping compositor.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Fri, 16 May 2014 20:49:30 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 16, 2014 at 12:34:08PM -0700, Jesse Barnes wrote:
> > On Fri, 16 May 2014 20:20:50 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > We haven't even fixed the major regression from enabling FBC. What's
> > > another massive slowdown?
> > 
> > I thought you had that fixed in the X driver by avoiding front buffer
> > rendering altogether.  If that's the case we just need an ioctl to opt
> > out of front buffer tracking, right?  Presumably that flag would follow
> > the current DRM_MASTER process...
> 
> No. All rendering triggers FBC updates. Ville has patches to fix the
> majority of that with only nuking FBC when front buffer rendering. (Note
> that games aren't usually affected by this because FBC is disabled by
> pageflipping.) The overhead could probably be reduced further by periodic
> nuking the FBC like (ideally) PSR. And yes X can avoid front buffer
> rendering altogether. The remaining challenge is to know when to enable it
> (the kernel doesn't give us any information about FBC or PSR after setting a
> mode) and when not, i.e. there is already a pageflipping compositor.

I thought there was a control bit for when the nuke occurred?  If not,
yeah I guess we have to go with a sw approach...

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jesse Barnes]

On Fri, 16 May 2014 12:34:08 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 16 May 2014 20:20:50 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Yes, X only sets the secure bit when it pokes the display registers, and
> > those registers should be privileged even with a cmd parser in place
> > (which they are).
> > 
> > Daniel's argument presumes that we haven't been patching out the
> > cmd parser all this time anyway.
> 
> Yeah I know we have some perf issues as it is; it would be nice if the
> overhead were so minimal that it didn't matter.  But just on principle,
> scanning secure buffers seems wrong, and I'm trying to understand why
> Daniel would want it.

Ok Daniel explained on IRC that we actually have a special whitelist
for the secure batch case.  The idea is to allow a DRM_MASTER to submit
secure batches, but still prevent a local root exploit.  I suppose that
means preventing access to most commands and registers, but allowing a
few extra things like wait events and display register updates.

I suppose it's not entirely unreasonable, but it does add complexity to
the scanner and overhead to all users; not sure it's worth it.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Volkin, Bradley D]

On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote:
> On Fri, 16 May 2014 12:34:08 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Fri, 16 May 2014 20:20:50 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Yes, X only sets the secure bit when it pokes the display registers, and
> > > those registers should be privileged even with a cmd parser in place
> > > (which they are).
> > > 
> > > Daniel's argument presumes that we haven't been patching out the
> > > cmd parser all this time anyway.
> > 
> > Yeah I know we have some perf issues as it is; it would be nice if the
> > overhead were so minimal that it didn't matter.  But just on principle,
> > scanning secure buffers seems wrong, and I'm trying to understand why
> > Daniel would want it.
> 
> Ok Daniel explained on IRC that we actually have a special whitelist
> for the secure batch case.  The idea is to allow a DRM_MASTER to submit
> secure batches, but still prevent a local root exploit.  I suppose that
> means preventing access to most commands and registers, but allowing a
> few extra things like wait events and display register updates.

Just to clarify further: the additional register whitelist and commands
are only based on DRM_MASTER. Setting I915_EXEC_SECURE is not required. So
I suppose we could stop scanning batches that have I915_EXEC_SECURE and
userspace could stop sending such batches when the parser is fully enabled.

Brad

> 
> I suppose it's not entirely unreasonable, but it does add complexity to
> the scanner and overhead to all users; not sure it's worth it.
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jesse Barnes]

On Fri, 16 May 2014 13:12:27 -0700
"Volkin, Bradley D" <bradley.d.volkin@intel.com> wrote:

> On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote:
> > On Fri, 16 May 2014 12:34:08 -0700
> > Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 
> > > On Fri, 16 May 2014 20:20:50 +0100
> > > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > Yes, X only sets the secure bit when it pokes the display registers, and
> > > > those registers should be privileged even with a cmd parser in place
> > > > (which they are).
> > > > 
> > > > Daniel's argument presumes that we haven't been patching out the
> > > > cmd parser all this time anyway.
> > > 
> > > Yeah I know we have some perf issues as it is; it would be nice if the
> > > overhead were so minimal that it didn't matter.  But just on principle,
> > > scanning secure buffers seems wrong, and I'm trying to understand why
> > > Daniel would want it.
> > 
> > Ok Daniel explained on IRC that we actually have a special whitelist
> > for the secure batch case.  The idea is to allow a DRM_MASTER to submit
> > secure batches, but still prevent a local root exploit.  I suppose that
> > means preventing access to most commands and registers, but allowing a
> > few extra things like wait events and display register updates.
> 
> Just to clarify further: the additional register whitelist and commands
> are only based on DRM_MASTER. Setting I915_EXEC_SECURE is not required. So
> I suppose we could stop scanning batches that have I915_EXEC_SECURE and
> userspace could stop sending such batches when the parser is fully enabled.

Ah ok, yeah that's another option, but now I understand where Daniel is
coming from with testing, since that's not how the current X driver
behaves.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Volkin, Bradley D]

On Thu, Mar 27, 2014 at 01:16:26PM -0700, Daniel Vetter wrote:
> On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D
> <bradley.d.volkin@intel.com> wrote:
> > On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote:
> >> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's
> >> an issue with the test or the checker:
> >>
> >> https://bugs.freedesktop.org/show_bug.cgi?id=76670
> >
> > For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit
> > set. We don't currently allow that, even from master. It sounds like there
> > might be released versions of the ddx that do this as well. If that's the
> > case, or if there are other situations where tests, etc rely on being able
> > to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I
> > think we might as well stop parsing secure batches and let them go through
> > as before.
> 
> Well for the testcase I think we can just add the missing flag. If

Which flag are you referring to here? Or are you just generally trying to say
"modify the test to not break the rules"?

> there's indeed shipping userspace out there which is getting these
> flags wrong then I think we need to silently upgrade them when copying
> the cmds over to the 2nd batch. But I guess until that need is really
> established we can hope we don't need this.

Chris, can you clarify whether shipping ddx sets GGTT bits this way?

Thanks,
Brad

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Chris Wilson]

On Thu, Mar 27, 2014 at 04:42:21PM -0700, Volkin, Bradley D wrote:
> On Thu, Mar 27, 2014 at 01:16:26PM -0700, Daniel Vetter wrote:
> > On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D
> > <bradley.d.volkin@intel.com> wrote:
> > > On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote:
> > >> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's
> > >> an issue with the test or the checker:
> > >>
> > >> https://bugs.freedesktop.org/show_bug.cgi?id=76670
> > >
> > > For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit
> > > set. We don't currently allow that, even from master. It sounds like there
> > > might be released versions of the ddx that do this as well. If that's the
> > > case, or if there are other situations where tests, etc rely on being able
> > > to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I
> > > think we might as well stop parsing secure batches and let them go through
> > > as before.
> > 
> > Well for the testcase I think we can just add the missing flag. If
> 
> Which flag are you referring to here? Or are you just generally trying to say
> "modify the test to not break the rules"?
> 
> > there's indeed shipping userspace out there which is getting these
> > flags wrong then I think we need to silently upgrade them when copying
> > the cmds over to the 2nd batch. But I guess until that need is really
> > established we can hope we don't need this.
> 
> Chris, can you clarify whether shipping ddx sets GGTT bits this way?

There have been no point releases with SRM (as far as I can remember) as
no bug reporter said that they made any difference to their hangs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jani Nikula]

On Wed, 26 Mar 2014, Kenneth Graunke <kenneth@whitecape.org> 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>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 1 +
>  drivers/gpu/drm/i915/i915_reg.h        | 2 ++
>  2 files changed, 3 insertions(+)
>
> This patch needs to go before
>
>    commit 6d42f94084b8c69813d7ecd0466c33fe561bf127
>    Author: Brad Volkin <bradley.d.volkin@intel.com>
>    Date:   Tue Feb 18 10:15:57 2014 -0800
>
>        drm/i915: Enable command parsing by default
>
> in whatever branch gets submitted to Dave Airlie.  Or, that commit needs
> to be reverted.  Otherwise, every OpenGL program will abort.  Examples
> of programs that abort include GNOME, KDE, Firefox, and glxgears.
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index bae7c2f..d4a50b9 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = {
>  	GEN7_SO_WRITE_OFFSET(1),
>  	GEN7_SO_WRITE_OFFSET(2),
>  	GEN7_SO_WRITE_OFFSET(3),
> +	OACONTROL,

Comment above gen7_render_regs array:

 * Register whitelists, sorted by increasing register offset.

You'll get a DRM_ERROR for this.

Daniel, naughty naughty for pushing this already!

BR,
Jani.


>  };
>  
>  static const u32 gen7_blt_regs[] = {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9f9e2b7..0ebc20d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -427,6 +427,8 @@
>  /* There are the 4 64-bit counter registers, one for each stream output */
>  #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
>  
> +#define OACONTROL 0x2360
> +
>  #define _GEN7_PIPEA_DE_LOAD_SL	0x70068
>  #define _GEN7_PIPEB_DE_LOAD_SL	0x71068
>  #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
> -- 
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Kenneth Graunke]

Mesa needs to be able to write OACONTROL in order to expose the
Observability Architecture's performance counters via OpenGL.

v2: Insert in proper sorted order (caught by Jani Nikula).

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 1 +
 drivers/gpu/drm/i915/i915_reg.h        | 2 ++
 2 files changed, 3 insertions(+)

Sorry, totally missed that.  Thanks for catching that, Jani.

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index bae7c2f..1cd8d3a 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -407,6 +407,7 @@ static const u32 gen7_render_regs[] = {
 	REG64(CL_PRIMITIVES_COUNT),
 	REG64(PS_INVOCATION_COUNT),
 	REG64(PS_DEPTH_COUNT),
+	OACONTROL,
 	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
 	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
 	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9f9e2b7..0ebc20d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -427,6 +427,8 @@
 /* There are the 4 64-bit counter registers, one for each stream output */
 #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
 
+#define OACONTROL 0x2360
+
 #define _GEN7_PIPEA_DE_LOAD_SL	0x70068
 #define _GEN7_PIPEB_DE_LOAD_SL	0x71068
 #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
-- 
1.9.0