The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed after this patchset applied and enable the atomic in L3 at beignet side. So, Reviewed-by: Zhigang Gong <zhigang.gong@xxxxxxxxxxxxxxx> Thanks, Zhigang Gong. > -----Original Message----- > From: Francisco Jerez [mailto:currojerez@xxxxxxxxxx] > Sent: Friday, May 29, 2015 9:44 PM > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Ville Syrjälä; Zhigang Gong; Brad Volkin > Subject: [PATCH 1/3] drm/i915: Fix command parser to validate multiple > register access with the same command. > > Until now the software command checker assumed that commands could read > or write at most a single register per packet. This is not necessarily the case, > MI_LOAD_REGISTER_IMM expects a variable-length list of offset/value pairs > and writes them in sequence. The previous code would only check whether > the first entry was valid, effectively allowing userspace to write unrestricted > registers of the MMIO space by sending a multi-register write with a legal first > register, with potential security implications on Gen6 and 7 hardware. > > Fix it by extending the drm_i915_cmd_descriptor table to represent > multi-register access and making validate_cmd() iterate for all register offsets > present in the command packet. > > Signed-off-by: Francisco Jerez <currojerez@xxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 74 > ++++++++++++++++++++-------------- > drivers/gpu/drm/i915/i915_drv.h | 5 +++ > 2 files changed, 48 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c > b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 61ae8ff..c4a5f73 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor > common_cmds[] = { > CMD( MI_SEMAPHORE_MBOX, SMI, !F, 0xFF, > R ), > CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, > R ), > CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, > W, > - .reg = { .offset = 1, .mask = 0x007FFFFC } ), > + .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 } ), > CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, > W | B, > .reg = { .offset = 1, .mask = 0x007FFFFC }, > .bits = {{ > @@ -939,7 +939,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs > *ring) > > static bool check_cmd(const struct intel_engine_cs *ring, > const struct drm_i915_cmd_descriptor *desc, > - const u32 *cmd, > + const u32 *cmd, u32 length, > const bool is_master, > bool *oacontrol_set) > { > @@ -955,38 +955,49 @@ static bool check_cmd(const struct intel_engine_cs > *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. > + * Get the distance between individual register offset > + * fields if the command can perform more than one > + * access at a time. > */ > - if (reg_addr == OACONTROL) { > - if (desc->cmd.value == MI_LOAD_REGISTER_MEM) { > - DRM_DEBUG_DRIVER("CMD: Rejected LRM to > OACONTROL\n"); > - return false; > + const u32 step = desc->reg.step ? desc->reg.step : length; > + u32 offset; > + > + for (offset = desc->reg.offset; offset < length; > + offset += step) { > + const u32 reg_addr = cmd[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) { > + DRM_DEBUG_DRIVER("CMD: Rejected LRM to > OACONTROL\n"); > + return false; > + } > + > + if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) > + *oacontrol_set = (cmd[offset + 1] != 0); > } > > - if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) > - *oacontrol_set = (cmd[2] != 0); > - } > - > - if (!valid_reg(ring->reg_table, > - ring->reg_count, reg_addr)) { > - if (!is_master || > - !valid_reg(ring->master_reg_table, > - ring->master_reg_count, > - reg_addr)) { > - DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in > command: 0x%08X (ring=%d)\n", > - reg_addr, > - *cmd, > - ring->id); > - return false; > + if (!valid_reg(ring->reg_table, > + ring->reg_count, reg_addr)) { > + if (!is_master || > + !valid_reg(ring->master_reg_table, > + ring->master_reg_count, > + reg_addr)) { > + DRM_DEBUG_DRIVER("CMD: Rejected register > 0x%08X in command: 0x%08X (ring=%d)\n", > + reg_addr, *cmd, > + ring->id); > + return false; > + } > } > } > } > @@ -1110,7 +1121,8 @@ int i915_parse_cmds(struct intel_engine_cs *ring, > break; > } > > - if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) { > + if (!check_cmd(ring, desc, cmd, length, is_master, > + &oacontrol_set)) { > ret = -EINVAL; > break; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8ae6f7f..3850288 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2228,10 +2228,15 @@ struct drm_i915_cmd_descriptor { > * Describes where to find a register address in the command to check > * against the ring's register whitelist. Only valid if flags has the > * CMD_DESC_REGISTER bit set. > + * > + * A non-zero step value implies that the command may access multiple > + * registers in sequence (e.g. LRI), in that case step gives the > + * distance in dwords between individual offset fields. > */ > struct { > u32 offset; > u32 mask; > + u32 step; > } reg; > > #define MAX_CMD_DESC_BITMASKS 3 > -- > 2.3.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
