On Thu, Mar 06, 2014 at 03:10:50PM +0200, Jani Nikula wrote:
> On Tue, 18 Feb 2014, bradley.d.volkin@xxxxxxxxx wrote:
> > From: Brad Volkin <bradley.d.volkin@xxxxxxxxx>
> >
> > The command parser scans batch buffers submitted via execbuffer ioctls before
> > the driver submits them to hardware. At a high level, it looks for several
> > things:
> >
> > 1) Commands which are explicitly defined as privileged or which should only be
> > used by the kernel driver. The parser generally rejects such commands, with
> > the provision that it may allow some from the drm master process.
> > 2) Commands which access registers. To support correct/enhanced userspace
> > functionality, particularly certain OpenGL extensions, the parser provides a
> > whitelist of registers which userspace may safely access (for both normal and
> > drm master processes).
> > 3) Commands which access privileged memory (i.e. GGTT, HWS page, etc). The
> > parser always rejects such commands.
> >
> > See the overview comment in the source for more details.
> >
> > This patch only implements the logic. Subsequent patches will build the tables
> > that drive the parser.
> >
> > v2: Don't set the secure bit if the parser succeeds
> > Fail harder during init
> > Makefile cleanup
> > Kerneldoc cleanup
> > Clarify module param description
> > Convert ints to bools in a few places
> > Move client/subclient defs to i915_reg.h
> > Remove the bits_count field
> >
> > OTC-Tracker: AXIA-4631
> > Change-Id: I50b98c71c6655893291c78a2d1b8954577b37a30
> > Signed-off-by: Brad Volkin <bradley.d.volkin@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/i915_cmd_parser.c | 485 +++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_drv.h | 93 ++++++
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 18 ++
> > drivers/gpu/drm/i915/i915_params.c | 5 +
> > drivers/gpu/drm/i915/i915_reg.h | 12 +
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 32 ++
> > 8 files changed, 648 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 4850494..3569122 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -14,6 +14,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
> > i915_gem_gtt.o \
> > i915_gem_stolen.o \
> > i915_gem_tiling.o \
> > + i915_cmd_parser.o \
> > i915_params.o \
> > i915_sysfs.o \
> > i915_trace_points.o \
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > new file mode 100644
> > index 0000000..7a5756e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -0,0 +1,485 @@
> > +/*
> > + * Copyright © 2013 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Brad Volkin <bradley.d.volkin@xxxxxxxxx>
> > + *
> > + */
> > +
> > +#include "i915_drv.h"
> > +
> > +/**
> > + * DOC: i915 batch buffer command parser
> > + *
> > + * Motivation:
> > + * 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.
> > + *
> > + * The software command parser is 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.
> > + *
> > + * Threats:
> > + * At a high level, the hardware (and software) checks attempt to prevent
> > + * granting userspace undue privileges. There are three categories of privilege.
> > + *
> > + * First, commands which are explicitly defined as privileged or which should
> > + * only be used by the kernel driver. The parser generally rejects such
> > + * commands, though it may allow some from the drm master process.
> > + *
> > + * Second, commands which access registers. To support correct/enhanced
> > + * userspace functionality, particularly certain OpenGL extensions, the parser
> > + * provides a whitelist of registers which userspace may safely access (for both
> > + * normal and drm master processes).
> > + *
> > + * Third, commands which access privileged memory (i.e. GGTT, HWS page, etc).
> > + * The parser always rejects such commands.
> > + *
> > + * The majority of the problematic commands fall in the MI_* range, with only a
> > + * few specific commands on each ring (e.g. PIPE_CONTROL and MI_FLUSH_DW).
> > + *
> > + * Implementation:
> > + * Each ring maintains tables of commands and registers which the parser uses in
> > + * scanning batch buffers submitted to that ring.
> > + *
> > + * Since the set of commands that the parser must check for is significantly
> > + * smaller than the number of commands supported, the parser tables contain only
> > + * those commands required by the parser. This generally works because command
> > + * opcode ranges have standard command length encodings. So for commands that
> > + * the parser does not need to check, it can easily skip them. This is
> > + * implementated via a per-ring length decoding vfunc.
> > + *
> > + * Unfortunately, there are a number of commands that do not follow the standard
> > + * length encoding for their opcode range, primarily amongst the MI_* commands.
> > + * To handle this, the parser provides a way to define explicit "skip" entries
> > + * in the per-ring command tables.
> > + *
> > + * Other command table entries map fairly directly to high level categories
> > + * mentioned above: rejected, master-only, register whitelist. The parser
> > + * implements a number of checks, including the privileged memory checks, via a
> > + * general bitmasking mechanism.
> > + */
> > +
> > +static u32 gen7_render_get_cmd_length_mask(u32 cmd_header)
> > +{
> > + u32 client = (cmd_header & INSTR_CLIENT_MASK) >> INSTR_CLIENT_SHIFT;
> > + u32 subclient =
> > + (cmd_header & INSTR_SUBCLIENT_MASK) >> INSTR_SUBCLIENT_SHIFT;
> > +
> > + if (client == INSTR_MI_CLIENT)
> > + return 0x3F;
> > + else if (client == INSTR_RC_CLIENT) {
> > + if (subclient == INSTR_MEDIA_SUBCLIENT)
> > + return 0xFFFF;
> > + else
> > + return 0xFF;
> > + }
> > +
> > + DRM_DEBUG_DRIVER("CMD: Abnormal rcs cmd length! 0x%08X\n", cmd_header);
> > + return 0;
> > +}
> > +
> > +static u32 gen7_bsd_get_cmd_length_mask(u32 cmd_header)
> > +{
> > + u32 client = (cmd_header & INSTR_CLIENT_MASK) >> INSTR_CLIENT_SHIFT;
> > + u32 subclient =
> > + (cmd_header & INSTR_SUBCLIENT_MASK) >> INSTR_SUBCLIENT_SHIFT;
> > +
> > + if (client == INSTR_MI_CLIENT)
> > + return 0x3F;
> > + else if (client == INSTR_RC_CLIENT) {
> > + if (subclient == INSTR_MEDIA_SUBCLIENT)
> > + return 0xFFF;
> > + else
> > + return 0xFF;
> > + }
> > +
> > + DRM_DEBUG_DRIVER("CMD: Abnormal bsd cmd length! 0x%08X\n", cmd_header);
> > + return 0;
> > +}
> > +
> > +static u32 gen7_blt_get_cmd_length_mask(u32 cmd_header)
> > +{
> > + u32 client = (cmd_header & INSTR_CLIENT_MASK) >> INSTR_CLIENT_SHIFT;
> > +
> > + if (client == INSTR_MI_CLIENT)
> > + return 0x3F;
> > + else if (client == INSTR_BC_CLIENT)
> > + return 0xFF;
> > +
> > + DRM_DEBUG_DRIVER("CMD: Abnormal blt cmd length! 0x%08X\n", cmd_header);
> > + return 0;
> > +}
> > +
> > +static void validate_cmds_sorted(struct intel_ring_buffer *ring)
> > +{
> > + int i;
> > +
> > + if (!ring->cmd_tables || ring->cmd_table_count == 0)
> > + return;
> > +
> > + for (i = 0; i < ring->cmd_table_count; i++) {
> > + const struct drm_i915_cmd_table *table = &ring->cmd_tables[i];
> > + u32 previous = 0;
> > + int j;
> > +
> > + for (j = 0; j < table->count; j++) {
> > + const struct drm_i915_cmd_descriptor *desc =
> > + &table->table[i];
> > + u32 curr = desc->cmd.value & desc->cmd.mask;
> > +
> > + if (curr < previous)
> > + DRM_ERROR("CMD: table not sorted ring=%d table=%d entry=%d cmd=0x%08X prev=0x%08X\n",
> > + ring->id, i, j, curr, previous);
> > +
> > + previous = curr;
> > + }
> > + }
> > +}
> > +
> > +static void check_sorted(int ring_id, const u32 *reg_table, int reg_count)
> > +{
> > + int i;
> > + u32 previous = 0;
> > +
> > + for (i = 0; i < reg_count; i++) {
> > + u32 curr = reg_table[i];
> > +
> > + if (curr < previous)
> > + DRM_ERROR("CMD: table not sorted ring=%d entry=%d reg=0x%08X prev=0x%08X\n",
> > + ring_id, i, curr, previous);
> > +
> > + previous = curr;
> > + }
> > +}
> > +
> > +static void validate_regs_sorted(struct intel_ring_buffer *ring)
> > +{
> > + check_sorted(ring->id, ring->reg_table, ring->reg_count);
> > + check_sorted(ring->id, ring->master_reg_table, ring->master_reg_count);
> > +}
> > +
> > +/**
> > + * i915_cmd_parser_init_ring() - set cmd parser related fields for a ringbuffer
> > + * @ring: the ringbuffer to initialize
> > + *
> > + * Optionally initializes fields related to batch buffer command parsing in the
> > + * struct intel_ring_buffer based on whether the platform requires software
> > + * command parsing.
> > + */
> > +void i915_cmd_parser_init_ring(struct intel_ring_buffer *ring)
> > +{
> > + if (!IS_GEN7(ring->dev))
> > + return;
> > +
> > + switch (ring->id) {
> > + case RCS:
> > + ring->get_cmd_length_mask = gen7_render_get_cmd_length_mask;
> > + break;
> > + case VCS:
> > + ring->get_cmd_length_mask = gen7_bsd_get_cmd_length_mask;
> > + break;
> > + case BCS:
> > + ring->get_cmd_length_mask = gen7_blt_get_cmd_length_mask;
> > + break;
> > + case VECS:
> > + /* VECS can use the same length_mask function as VCS */
> > + ring->get_cmd_length_mask = gen7_bsd_get_cmd_length_mask;
> > + break;
> > + default:
> > + DRM_ERROR("CMD: cmd_parser_init with unknown ring: %d\n",
> > + ring->id);
> > + BUG();
> > + }
> > +
> > + validate_cmds_sorted(ring);
> > + validate_regs_sorted(ring);
>
> So if you come to rely on the tables being sorted later on, I'd like the
> above functions to return whether everything was okay or not, and BUG()
> here if not. This can be a follow-up, and *must* be added before doing
> anything that really requires the tables to be sorted.
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
Merged the first 2 patches from this series, thanks.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
