[PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Francisco Jerez]

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@riseup.net>
---
 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Extend the parser to check register writes against a mask/value pair.

[Francisco Jerez]

In some cases it might be unnecessary or dangerous to give userspace
the right to write arbitrary values to some register, even though it
might be desirable to give it control of some of its bits.  This patch
extends the register whitelist entries to contain a mask/value pair in
addition to the register offset.  For registers with non-zero mask,
any LRM writes and LRI writes where the bits of the immediate given by
the mask don't match the specified value will be rejected.

This will be used in my next patch to grant userspace partial write
access to some sensitive registers.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c  | 138 +++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.h |   5 +-
 2 files changed, 96 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index c4a5f73..7dd218e 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -395,16 +395,38 @@ static const struct drm_i915_cmd_table hsw_blt_ring_cmds[] = {
 
 /*
  * Register whitelists, sorted by increasing register offset.
+ */
+
+/*
+ * An individual whitelist entry granting access to register addr.  If
+ * mask is non-zero the argument of immediate register writes will be
+ * AND-ed with mask, and the command will be rejected if the result
+ * doesn't match value.
+ *
+ * Registers with non-zero mask are only allowed to be written using
+ * LRI.
+ */
+struct drm_i915_reg_descriptor {
+	u32 addr;
+	u32 mask;
+	u32 value;
+};
+
+/* Convenience macro for adding 32-bit registers. */
+#define REG32(address, ...)                             \
+	{ .addr = address, __VA_ARGS__ }
+
+/*
+ * Convenience macro for adding 64-bit registers.
  *
  * Some registers that userspace accesses are 64 bits. The register
  * access commands only allow 32-bit accesses. Hence, we have to include
  * entries for both halves of the 64-bit registers.
  */
+#define REG64(addr)                                     \
+	REG32(addr), REG32(addr + sizeof(u32))
 
-/* Convenience macro for adding 64-bit registers */
-#define REG64(addr) (addr), (addr + sizeof(u32))
-
-static const u32 gen7_render_regs[] = {
+static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
 	REG64(GPGPU_THREADS_DISPATCHED),
 	REG64(HS_INVOCATION_COUNT),
 	REG64(DS_INVOCATION_COUNT),
@@ -417,15 +439,15 @@ static const u32 gen7_render_regs[] = {
 	REG64(CL_PRIMITIVES_COUNT),
 	REG64(PS_INVOCATION_COUNT),
 	REG64(PS_DEPTH_COUNT),
-	OACONTROL, /* Only allowed for LRI and SRM. See below. */
+	REG32(OACONTROL), /* Only allowed for LRI and SRM. See below. */
 	REG64(MI_PREDICATE_SRC0),
 	REG64(MI_PREDICATE_SRC1),
-	GEN7_3DPRIM_END_OFFSET,
-	GEN7_3DPRIM_START_VERTEX,
-	GEN7_3DPRIM_VERTEX_COUNT,
-	GEN7_3DPRIM_INSTANCE_COUNT,
-	GEN7_3DPRIM_START_INSTANCE,
-	GEN7_3DPRIM_BASE_VERTEX,
+	REG32(GEN7_3DPRIM_END_OFFSET),
+	REG32(GEN7_3DPRIM_START_VERTEX),
+	REG32(GEN7_3DPRIM_VERTEX_COUNT),
+	REG32(GEN7_3DPRIM_INSTANCE_COUNT),
+	REG32(GEN7_3DPRIM_START_INSTANCE),
+	REG32(GEN7_3DPRIM_BASE_VERTEX),
 	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
 	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
 	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
@@ -434,33 +456,34 @@ static const u32 gen7_render_regs[] = {
 	REG64(GEN7_SO_PRIM_STORAGE_NEEDED(1)),
 	REG64(GEN7_SO_PRIM_STORAGE_NEEDED(2)),
 	REG64(GEN7_SO_PRIM_STORAGE_NEEDED(3)),
-	GEN7_SO_WRITE_OFFSET(0),
-	GEN7_SO_WRITE_OFFSET(1),
-	GEN7_SO_WRITE_OFFSET(2),
-	GEN7_SO_WRITE_OFFSET(3),
-	GEN7_L3SQCREG1,
-	GEN7_L3CNTLREG2,
-	GEN7_L3CNTLREG3,
+	REG32(GEN7_SO_WRITE_OFFSET(0)),
+	REG32(GEN7_SO_WRITE_OFFSET(1)),
+	REG32(GEN7_SO_WRITE_OFFSET(2)),
+	REG32(GEN7_SO_WRITE_OFFSET(3)),
+	REG32(GEN7_L3SQCREG1),
+	REG32(GEN7_L3CNTLREG2),
+	REG32(GEN7_L3CNTLREG3),
 };
 
-static const u32 gen7_blt_regs[] = {
-	BCS_SWCTRL,
+static const struct drm_i915_reg_descriptor gen7_blt_regs[] = {
+	REG32(BCS_SWCTRL),
 };
 
-static const u32 ivb_master_regs[] = {
-	FORCEWAKE_MT,
-	DERRMR,
-	GEN7_PIPE_DE_LOAD_SL(PIPE_A),
-	GEN7_PIPE_DE_LOAD_SL(PIPE_B),
-	GEN7_PIPE_DE_LOAD_SL(PIPE_C),
+static const struct drm_i915_reg_descriptor ivb_master_regs[] = {
+	REG32(FORCEWAKE_MT),
+	REG32(DERRMR),
+	REG32(GEN7_PIPE_DE_LOAD_SL(PIPE_A)),
+	REG32(GEN7_PIPE_DE_LOAD_SL(PIPE_B)),
+	REG32(GEN7_PIPE_DE_LOAD_SL(PIPE_C)),
 };
 
-static const u32 hsw_master_regs[] = {
-	FORCEWAKE_MT,
-	DERRMR,
+static const struct drm_i915_reg_descriptor hsw_master_regs[] = {
+	REG32(FORCEWAKE_MT),
+	REG32(DERRMR),
 };
 
 #undef REG64
+#undef REG32
 
 static u32 gen7_render_get_cmd_length_mask(u32 cmd_header)
 {
@@ -550,14 +573,16 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
 	return ret;
 }
 
-static bool check_sorted(int ring_id, const u32 *reg_table, int reg_count)
+static bool check_sorted(int ring_id,
+			 const struct drm_i915_reg_descriptor *reg_table,
+			 int reg_count)
 {
 	int i;
 	u32 previous = 0;
 	bool ret = true;
 
 	for (i = 0; i < reg_count; i++) {
-		u32 curr = reg_table[i];
+		u32 curr = reg_table[i].addr;
 
 		if (curr < previous) {
 			DRM_ERROR("CMD: table not sorted ring=%d entry=%d reg=0x%08X prev=0x%08X\n",
@@ -804,18 +829,20 @@ find_cmd(struct intel_engine_cs *ring,
 	return default_desc;
 }
 
-static bool valid_reg(const u32 *table, int count, u32 addr)
+static const struct drm_i915_reg_descriptor *
+find_reg(const struct drm_i915_reg_descriptor *table,
+	 int count, u32 addr)
 {
-	if (table && count != 0) {
+	if (table) {
 		int i;
 
 		for (i = 0; i < count; i++) {
-			if (table[i] == addr)
-				return true;
+			if (table[i].addr == addr)
+				return &table[i];
 		}
 	}
 
-	return false;
+	return NULL;
 }
 
 static u32 *vmap_batch(struct drm_i915_gem_object *obj,
@@ -966,6 +993,20 @@ static bool check_cmd(const struct intel_engine_cs *ring,
 		for (offset = desc->reg.offset; offset < length;
 		     offset += step) {
 			const u32 reg_addr = cmd[offset] & desc->reg.mask;
+			const struct drm_i915_reg_descriptor *reg =
+				find_reg(ring->reg_table, ring->reg_count,
+					 reg_addr);
+
+			if (!reg && is_master)
+				reg = find_reg(ring->master_reg_table,
+					       ring->master_reg_count,
+					       reg_addr);
+
+			if (!reg) {
+				DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n",
+						 reg_addr, *cmd, ring->id);
+				return false;
+			}
 
 			/*
 			 * OACONTROL requires some special handling for
@@ -987,15 +1028,22 @@ static bool check_cmd(const struct intel_engine_cs *ring,
 					*oacontrol_set = (cmd[offset + 1] != 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);
+			/*
+			 * Check the value written to the register against the
+			 * allowed mask/value pair given in the whitelist entry.
+			 */
+			if (reg->mask) {
+				if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
+					DRM_DEBUG_DRIVER("CMD: Rejected LRM to masked register 0x%08X\n",
+							 reg_addr);
+					return false;
+				}
+
+				if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) &&
+				    (offset + 2 > length ||
+				     (cmd[offset + 1] & reg->mask) != reg->value)) {
+					DRM_DEBUG_DRIVER("CMD: Rejected LRI to masked register 0x%08X\n",
+							 reg_addr);
 					return false;
 				}
 			}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c761fe0..deab6f76 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -117,6 +117,7 @@ struct intel_ringbuffer {
 };
 
 struct	intel_context;
+struct drm_i915_reg_descriptor;
 
 struct  intel_engine_cs {
 	const char	*name;
@@ -292,14 +293,14 @@ struct  intel_engine_cs {
 	/*
 	 * Table of registers allowed in commands that read/write registers.
 	 */
-	const u32 *reg_table;
+	const struct drm_i915_reg_descriptor *reg_table;
 	int reg_count;
 
 	/*
 	 * Table of registers allowed in commands that read/write registers, but
 	 * only from the DRM master.
 	 */
-	const u32 *master_reg_table;
+	const struct drm_i915_reg_descriptor *master_reg_table;
 	int master_reg_count;
 
 	/*
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Add SCRATCH1 and ROW_CHICKEN3 to the register whitelist.

[Francisco Jerez]

Only bit 27 of SCRATCH1 and bit 6 of ROW_CHICKEN3 are allowed to be
set because of security-sensitive bits we don't want userspace to mess
with.  On HSW hardware the whitelisted bits control whether atomic
read-modify-write operations are performed on L3 or on GTI, and when
set to L3 (which can be 10x-30x better performing than on GTI,
depending on the application) require great care to avoid a system
hang, so we currently program them to be handled on GTI by default.

Beignet can immediately start taking advantage of this change to
enable L3 atomics.  Mesa should eventually switch to L3 atomics too,
but a number of non-trivial changes are still required so it will
continue using GTI atomics for now.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 7dd218e..0146fe6 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -463,6 +463,13 @@ static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
 	REG32(GEN7_L3SQCREG1),
 	REG32(GEN7_L3CNTLREG2),
 	REG32(GEN7_L3CNTLREG3),
+	REG32(HSW_SCRATCH1,
+	      .mask = ~HSW_SCRATCH1_L3_DATA_ATOMICS_DISABLE,
+	      .value = 0),
+	REG32(HSW_ROW_CHICKEN3,
+	      .mask = ~(HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE << 16 |
+                        HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE),
+	      .value = 0),
 };
 
 static const struct drm_i915_reg_descriptor gen7_blt_regs[] = {
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Zhigang Gong]

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@linux.intel.com>

Thanks,
Zhigang Gong.

> -----Original Message-----
> From: Francisco Jerez [mailto:currojerez@riseup.net]
> Sent: Friday, May 29, 2015 9:44 PM
> To: intel-gfx@lists.freedesktop.org
> 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@riseup.net>
> ---
>  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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 7111 bytes --]

Zhigang Gong <zhigang.gong@linux.intel.com> writes:

> 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@linux.intel.com>
>
> Thanks,
> Zhigang Gong.
>
Thanks!

>> -----Original Message-----
>> From: Francisco Jerez [mailto:currojerez@riseup.net]
>> Sent: Friday, May 29, 2015 9:44 PM
>> To: intel-gfx@lists.freedesktop.org
>> 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@riseup.net>
>> ---
>>  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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 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 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Daniel Vetter]

On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> 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@linux.intel.com>

All three merged. Aside: Dont we need an increment for the cmd parser
version for userspace to be able to detect this?

And please follow up with a link to the beignet patches used to validate
these kernel patches for references.

Thanks, Daniel

> 
> Thanks,
> Zhigang Gong.
> 
> > -----Original Message-----
> > From: Francisco Jerez [mailto:currojerez@riseup.net]
> > Sent: Friday, May 29, 2015 9:44 PM
> > To: intel-gfx@lists.freedesktop.org
> > 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@riseup.net>
> > ---
> >  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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 817 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>> 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@linux.intel.com>
>
> All three merged.

Thanks Daniel.

> Aside: Dont we need an increment for the cmd parser version for
> userspace to be able to detect this?
>
Yeah, that would be a good idea, patch attached.

> And please follow up with a link to the beignet patches used to validate
> these kernel patches for references.
>
Zhigang, do you have a link to your Beignet patch?

> Thanks, Daniel
>
>> 
>> Thanks,
>> Zhigang Gong.
>> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-drm-i915-Bump-command-parser-version-number.patch --]
[-- Type: text/x-diff, Size: 937 bytes --]

From 9f26beaf96473800252db35c4513933ae43e3c84 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <currojerez@riseup.net>
Date: Mon, 15 Jun 2015 14:03:29 +0300
Subject: [PATCH] drm/i915: Bump command parser version number.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 0146fe6..6722098 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1219,6 +1219,7 @@ int i915_cmd_parser_get_version(void)
 	 * 2. Allow access to the MI_PREDICATE_SRC0 and
 	 *    MI_PREDICATE_SRC1 registers.
 	 * 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
+	 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
 	 */
-	return 3;
+	return 4;
 }
-- 
2.4.3


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 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 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Ville Syrjälä]

On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> >> 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@linux.intel.com>
> >
> > All three merged.
> 
> Thanks Daniel.
> 
> > Aside: Dont we need an increment for the cmd parser version for
> > userspace to be able to detect this?
> >
> Yeah, that would be a good idea, patch attached.

The old version alloweed userspace to write basically any register, the
new version allows only the whitelisted registers. I don't see how a
version number bump would help anyone.

> 
> > And please follow up with a link to the beignet patches used to validate
> > these kernel patches for references.
> >
> Zhigang, do you have a link to your Beignet patch?
> 
> > Thanks, Daniel
> >
> >> 
> >> Thanks,
> >> Zhigang Gong.
> >> 

> From 9f26beaf96473800252db35c4513933ae43e3c84 Mon Sep 17 00:00:00 2001
> From: Francisco Jerez <currojerez@riseup.net>
> Date: Mon, 15 Jun 2015 14:03:29 +0300
> Subject: [PATCH] drm/i915: Bump command parser version number.
> 
> Signed-off-by: Francisco Jerez <currojerez@riseup.net>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 0146fe6..6722098 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1219,6 +1219,7 @@ int i915_cmd_parser_get_version(void)
>  	 * 2. Allow access to the MI_PREDICATE_SRC0 and
>  	 *    MI_PREDICATE_SRC1 registers.
>  	 * 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
> +	 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
>  	 */
> -	return 3;
> +	return 4;
>  }
> -- 
> 2.4.3
> 




> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Daniel Vetter]

On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> > 
> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> > >> 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@linux.intel.com>
> > >
> > > All three merged.
> > 
> > Thanks Daniel.
> > 
> > > Aside: Dont we need an increment for the cmd parser version for
> > > userspace to be able to detect this?
> > >
> > Yeah, that would be a good idea, patch attached.
> 
> The old version alloweed userspace to write basically any register, the
> new version allows only the whitelisted registers. I don't see how a
> version number bump would help anyone.

Oops, totally missed the context of patch 1. Jani I think that one's for
you too ...

Thanks for pointing this out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 1735 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
>> > Daniel Vetter <daniel@ffwll.ch> writes:
>> > 
>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>> > >> 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@linux.intel.com>
>> > >
>> > > All three merged.
>> > 
>> > Thanks Daniel.
>> > 
>> > > Aside: Dont we need an increment for the cmd parser version for
>> > > userspace to be able to detect this?
>> > >
>> > Yeah, that would be a good idea, patch attached.
>> 
>> The old version alloweed userspace to write basically any register, the
>> new version allows only the whitelisted registers. I don't see how a
>> version number bump would help anyone.
>
> Oops, totally missed the context of patch 1. Jani I think that one's for
> you too ...
>
IMHO the version bump is still useful for userspace to find out whether
it can use plain LRIs to write the L3 atomic chicken bits.  It's true
that as Ville said it would have been possible for userspace to write
the same bits before this series by building a batch specifically
crafted to cheat the command parser, but I don't think we want userspace
to rely on a command parser bug (e.g. because we may want to back-port
the fix to earlier kernel versions).

> Thanks for pointing this out.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 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 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Jani Nikula]

On Mon, 15 Jun 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
>> > Daniel Vetter <daniel@ffwll.ch> writes:
>> > 
>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>> > >> 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@linux.intel.com>
>> > >
>> > > All three merged.
>> > 
>> > Thanks Daniel.
>> > 
>> > > Aside: Dont we need an increment for the cmd parser version for
>> > > userspace to be able to detect this?
>> > >
>> > Yeah, that would be a good idea, patch attached.
>> 
>> The old version alloweed userspace to write basically any register, the
>> new version allows only the whitelisted registers. I don't see how a
>> version number bump would help anyone.
>
> Oops, totally missed the context of patch 1. Jani I think that one's for
> you too ...

Cherry-picked to drm-intel-next-fixes.

BR,
Jani.


>
> Thanks for pointing this out.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 2113 bytes --]

Francisco Jerez <currojerez@riseup.net> writes:

> Daniel Vetter <daniel@ffwll.ch> writes:
>
>> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
>>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
>>> > Daniel Vetter <daniel@ffwll.ch> writes:
>>> > 
>>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>>> > >> 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@linux.intel.com>
>>> > >
>>> > > All three merged.
>>> > 
>>> > Thanks Daniel.
>>> > 
>>> > > Aside: Dont we need an increment for the cmd parser version for
>>> > > userspace to be able to detect this?
>>> > >
>>> > Yeah, that would be a good idea, patch attached.
>>> 
>>> The old version alloweed userspace to write basically any register, the
>>> new version allows only the whitelisted registers. I don't see how a
>>> version number bump would help anyone.
>>
>> Oops, totally missed the context of patch 1. Jani I think that one's for
>> you too ...
>>
> IMHO the version bump is still useful for userspace to find out whether
> it can use plain LRIs to write the L3 atomic chicken bits.  It's true
> that as Ville said it would have been possible for userspace to write
> the same bits before this series by building a batch specifically
> crafted to cheat the command parser, but I don't think we want userspace
> to rely on a command parser bug (e.g. because we may want to back-port
> the fix to earlier kernel versions).
>
Ping.  I cannot use these registers from userspace until the command
parser version number is bumped.

>> Thanks for pointing this out.
>> -Daniel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 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 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

[Daniel Vetter]

On Mon, Aug 31, 2015 at 01:19:47AM +0300, Francisco Jerez wrote:
> Francisco Jerez <currojerez@riseup.net> writes:
> 
> > Daniel Vetter <daniel@ffwll.ch> writes:
> >
> >> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
> >>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
> >>> > Daniel Vetter <daniel@ffwll.ch> writes:
> >>> > 
> >>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> >>> > >> 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@linux.intel.com>
> >>> > >
> >>> > > All three merged.
> >>> > 
> >>> > Thanks Daniel.
> >>> > 
> >>> > > Aside: Dont we need an increment for the cmd parser version for
> >>> > > userspace to be able to detect this?
> >>> > >
> >>> > Yeah, that would be a good idea, patch attached.
> >>> 
> >>> The old version alloweed userspace to write basically any register, the
> >>> new version allows only the whitelisted registers. I don't see how a
> >>> version number bump would help anyone.
> >>
> >> Oops, totally missed the context of patch 1. Jani I think that one's for
> >> you too ...
> >>
> > IMHO the version bump is still useful for userspace to find out whether
> > it can use plain LRIs to write the L3 atomic chicken bits.  It's true
> > that as Ville said it would have been possible for userspace to write
> > the same bits before this series by building a batch specifically
> > crafted to cheat the command parser, but I don't think we want userspace
> > to rely on a command parser bug (e.g. because we may want to back-port
> > the fix to earlier kernel versions).
> >
> Ping.  I cannot use these registers from userspace until the command
> parser version number is bumped.

Queued for -next, thanks for the patch.
-Daniel

> 
> >> Thanks for pointing this out.
> >> -Daniel
> >> -- 
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx