Re: [RFC 00/22] Gen7 batch buffer command parser

["Xiang, Haihao" <haihao.xiang@intel.com>]

On Tue, 2013-11-26 at 20:35 +0100, Daniel Vetter wrote: 
> Hi Brad,
> 
> On Tue, Nov 26, 2013 at 08:51:17AM -0800, bradley.d.volkin@intel.com wrote:
> > From: Brad Volkin <bradley.d.volkin@intel.com>
> > 
> > Certain OpenGL features (e.g. transform feedback, performance monitoring)
> > require userspace code to submit batches containing commands such as
> > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some
> > generations of the hardware will noop these commands in "unsecure" batches
> > (which includes all userspace batches submitted via i915) even though the
> > commands may be safe and represent the intended programming model of the device.
> > 
> > This series introduces a software command parser similar in operation to the
> > command parsing done in hardware for unsecure batches. However, the software
> > parser allows some operations that would be noop'd by hardware, if the parser
> > determines the operation is safe, and submits the batch as "secure" to prevent
> > hardware parsing. Currently the series implements this on IVB and HSW.
> > 
> > The series is divided into several phases:
> > 
> > patches 01-09: These implement infrastructure and the command parsing algorithm,
> >                all behind a module parameter. I expect some discussion and
> > 	       rework, but hopefully there's nothing too controversial.
> > patches 10-17: These define the checks performed by the parser.
> >                I expect much discussion :)
> > patches 18-20: In a final pass over the command checks, I found some issues with
> >                the definitions. They looked painful to rebase in, so I've added
> > 	       them here.
> > patches 21-22: These enable the parser by default. It runs on all batches except
> >                those that set the I915_EXEC_SECURE flag in the execbuffer2 call.
> 
> I think long-term we should even scan secure batches. We'd need to allow
> some registers which only the drm master (i.e. owner of the display
> hardware) is allowed to do, e.g. for scanline waits. But once we have that
> we should be able to port all current users of secure batches over to
> scanned batches and so enforce this everywhere by default.
> 
> The other issue is that igt tests assume to be able to run some evil
> tests, so maybe we don't actually want this.
> 
> > There are follow-up patches to libdrm and to i-g-t. The i-g-t tests are very
> > basic and do not test all of the commands used by the parser on the assumption
> > that I'm likely to make the same mistakes in both the parser and the test.
> 
> Yeah, I agree that just checking whether commands all go through (or not)
> as expected adds very little value on top of the few tests you have done.
> I think we should take a look at some corner cases which might trip up
> your checker a bit though:
> - I think we should check batchbuffer chaining and make sure it works on
>   the vcs ring and not anywhere else (we can't ever break shipping libva
>   which uses this).

Besides the vcs ring, we also use batchbuffer chaining on the render
ring for video post processing, video motion estimation and motion
compensation(on ILK),  A fixed length batch buffer isn't suitable for
those operations as those operations are based on a macroblock instead
of a frame. It would be better to make sure batchbuffer chaining works
on the render ring too.


> - Some tests to trip up your parser should be done, like 3D commands that
>   fall off the end of the batch bo. Or commands that span page boundaries.
>   The later isn't an issue atm since you use vmap, but we should switch to
>   per-page kmap since the vmap overhead is fairly horrible.
> 
> > I've run the i-g-t gem_* tests, the piglit quick tests (w/Mesa git from a few
> > days ago), and generally used an Ubuntu 13.10 IVB system with the parser
> > running. Aside from a failure described below, I don't think there are any
> > regressions. That is, piglit claims some regressions, but from manually running
> > the tests I think these are false positives. However, I could use help in
> > getting broader testing, particularly around performance. In general, I see less
> > than 3% performance impact on HSW, with more like 10% impact for pathological
> > batch sizes. But we'll certainly want to run relevant benchmarks beyond what
> > I've done.
> 
> Yeah, a microbenchmark that just shovels MI_NOP batches of various sizes
> through the checker and bypassing it (with EXEC_SECURE) would be really
> good. Maybe even some variable-sized commands (all the state setup stuff
> should be useful for that) to keep things interesting. Some variation is
> also important to have some good cache thrasing going on (since your check
> tables are fairly large I think).
> 
> > At this point there are a couple of known issues and potential improvements.
> > 
> > 1) VLV. The parser is currently disabled for VLV. One type of check performed by
> >    the parser is that commands which access memory do so via PPGTT. VLV does not
> >    have PPGTT enabled at this time. I chose to implement the PPGTT checks via
> >    generic bit checking infrastructure in the parser, so they are not easily
> >    disabled for VLV. For now, I'm disabling parsing altogether in the hope that
> >    PPGTT can be enabled for VLV in the near future.
> 
> We need ppgtt for the parser anyway, since otherwise userspace can submit
> a self-modifying batch. Checking for that is impossible, so allowing sw
> checked batches without the ppgtt/ggtt split would be a decent security
> hole.
> 
> > 2) Coherency. I've found two types of coherency issues when reading the batch
> >    buffer from the CPU during execbuffer2. Looking for help with both issues.
> >     i. First, the i-g-t test gem_cpu_reloc blits to a batch buffer and the
> >        parser isn't properly waiting for the operation to complete before
> >        parsing. I tried adding i915_gem_object_sync(batch_obj, [ring|NULL])
> >        but that actually caused more failures.
> 
> This synchronization should happen when processing the relocations. The
> batch itself isn't modified by the gpu, we simply upload it using the
> blitter. So this going wrong indicates there's some issue somewhere ...
> 
> 
> >    ii. Second, on VLV, I've seen cache coherency issues when userspace writes
> >        the batch via pwrite fast path before calling execbuffer2. The parser
> >        reads stale data. This works fine on IVB and HSW, so I believe it's an
> >        LLC vs. non-LLC issue. I'm just unclear on what the correct flushing or
> >        synchronization is for this scenario.
> 
> Imo we take a good look at the optimized buffer read/write code from
> i915_gem_shmem_pread (for reading the userspace batch) and
> i915_gem_shmem_pwrite (for writing to the checked buffer). If we do the
> checking in-line with the reading this should also bring down the overhead
> massively compared to the current solution (those shmem read/write
> functions are fairly optimized).
> 
> > 3) 2nd-level batches. The parser currently allows MI_BATCH_BUFFER_START commands
> >    in userspace batches without parsing them. The media driver uses 2nd-level
> >    batches, so a solution is required. I have some ideas but don't want to delay
> >    the review process for what I have so far. It may be that the 2nd-level
> >    parsing is only needed for VCS and the current code (plus rejecting BBS)
> >    would be sufficient for RCS.
> 
> Afaik only libva uses second-level batches, and only on the vcs. So I hope
> we can just submit those as unpriviledged batches if possible. If that's
> not possible it'll get fairly ugly I fear :(
> 
> > 4) Command buffer copy. To avoid CPU modifications to buffers after parsing, and
> >    to avoid GPU modifications to buffers via EUs or commands in the batch, we
> >    should copy the userspace batch buffer to memory that userspace does not
> >    have access to, map it into GGTT, and execute that batch buffer. I have a
> >    sense of how to do this for 1st-level batches, but it would need changes to
> >    tie in with the 2nd-level batch parsing I think, so I've again held off.
> 
> Yeah, we need the copying for otherwise the parsing is fairly pointless.
> I've stumbled over some of your internally patches and had a quick look at
> them. Two high-level comments:
> 
> - Using the existing active buffer lru instead of manual pinning would
>   better integrate with the eviction code. For an example of in-kernel
>   objects (and not userspace objects) using this look at the hw context
>   code.
> - Imo we should tag all buffers as purgeable while they're in the cache.
>   That way the shrinker will automatically drop the backing storage if
>   memory runs low (and thanks to the active list lru only when the gpu has
>   stopped processing the batch). That way we can just keep on allocating
>   buffers if they're all busy without any concern for running out of
>   memory.
> 
> I'll try to read through your patch pile in the next few days, this is
> just the very-very high-level stuff that came to mind immediately.
> 
> Cheers, Daniel
> > 
> > Brad Volkin (22):
> >   drm/i915: Add data structures for command parser
> >   drm/i915: Initial command parser table definitions
> >   drm/i915: Hook command parser tables up to rings
> >   drm/i915: Add per-ring command length decode functions
> >   drm/i915: Implement command parsing
> >   drm/i915: Add a HAS_CMD_PARSER getparam
> >   drm/i915: Add support for rejecting commands during parsing
> >   drm/i915: Add support for checking register accesses
> >   drm/i915: Add support for rejecting commands via bitmasks
> >   drm/i915: Reject unsafe commands
> >   drm/i915: Add register whitelists for mesa
> >   drm/i915: Enable register whitelist checks
> >   drm/i915: Enable bit checking for some commands
> >   drm/i915: Enable PPGTT command parser checks
> >   drm/i915: Reject commands that would store to global HWS page
> >   drm/i915: Reject additional commands
> >   drm/i915: Add parser data for perf monitoring GL extensions
> >   drm/i915: Reject MI_ARB_ON_OFF on VECS
> >   drm/i915: Fix length handling for MFX_WAIT
> >   drm/i915: Fix MI_STORE_DWORD_IMM parser defintion
> >   drm/i915: Clean up command parser enable decision
> >   drm/i915: Enable command parsing by default
> > 
> >  drivers/gpu/drm/i915/Makefile              |   3 +-
> >  drivers/gpu/drm/i915/i915_cmd_parser.c     | 712 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_dma.c            |   3 +
> >  drivers/gpu/drm/i915/i915_drv.c            |   5 +
> >  drivers/gpu/drm/i915/i915_drv.h            |  96 ++++
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  15 +
> >  drivers/gpu/drm/i915/i915_reg.h            |  66 +++
> >  drivers/gpu/drm/i915/intel_ringbuffer.c    |   2 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.h    |  25 +
> >  include/uapi/drm/i915_drm.h                |   1 +
> >  10 files changed, 927 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c
> > 
> > -- 
> > 1.8.4.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>