[v4,4/5] drm: Add decoding of i915 ioctls

Submitted by Patrik Jakobsson on Aug. 24, 2015, 12:42 p.m.

Details

Message ID 1440420170-13337-5-git-send-email-patrik.jakobsson@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Patrik Jakobsson Aug. 24, 2015, 12:42 p.m.
First batch of i915 drm ioctls

* Makefile.am: Add compilation of drm_i915.c.
* defs.h: Add extern i915 declarations
* drm.c (drm_ioctl): Dispatch i915 ioctls.
* drm_i915.c: New file.
* xlat/drm_i915_getparams.in: New file.
* xlat/drm_i915_setparams.in: New file.
* xlat/drm_i915_ioctls.in: New file.

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 Makefile.am                |   1 +
 defs.h                     |   2 +
 drm.c                      |  15 +-
 drm_i915.c                 | 342 +++++++++++++++++++++++++++++++++++++++++++++
 xlat/drm_i915_getparams.in |  28 ++++
 xlat/drm_i915_ioctls.in    |  51 +++++++
 xlat/drm_i915_setparams.in |   4 +
 7 files changed, 442 insertions(+), 1 deletion(-)
 create mode 100644 drm_i915.c
 create mode 100644 xlat/drm_i915_getparams.in
 create mode 100644 xlat/drm_i915_ioctls.in
 create mode 100644 xlat/drm_i915_setparams.in

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index c7c6080..0af46f6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -43,6 +43,7 @@  strace_SOURCES =	\
 	desc.c		\
 	dirent.c	\
 	drm.c		\
+	drm_i915.c	\
 	epoll.c		\
 	evdev.c		\
 	eventfd.c	\
diff --git a/defs.h b/defs.h
index dd3c720..1d54295 100644
--- a/defs.h
+++ b/defs.h
@@ -614,6 +614,8 @@  extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
 extern int block_ioctl(struct tcb *, const unsigned int, long);
 #if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
 extern int drm_ioctl(struct tcb *, const unsigned int, long);
+extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int, long);
+extern int drm_i915_decode_number(struct tcb *, unsigned int);
 #endif
 extern int evdev_ioctl(struct tcb *, const unsigned int, long);
 extern int loop_ioctl(struct tcb *, const unsigned int, long);
diff --git a/drm.c b/drm.c
index 0829803..e220309 100644
--- a/drm.c
+++ b/drm.c
@@ -96,12 +96,25 @@  int drm_is_driver(struct tcb *tcp, const char *name)
 
 int drm_decode_number(struct tcb *tcp, unsigned int code)
 {
+	if (drm_is_priv(tcp->u_arg[1])) {
+		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
+			return drm_i915_decode_number(tcp, code);
+	}
+
 	return 0;
 }
 
 int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
 {
-	return 0;
+	int ret = 0;
+
+	/* Check for device specific ioctls */
+	if (drm_is_priv(tcp->u_arg[1])) {
+		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
+			ret = drm_i915_ioctl(tcp, code, arg);
+	}
+
+	return ret;
 }
 
 #endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
diff --git a/drm_i915.c b/drm_i915.c
new file mode 100644
index 0000000..9af3916
--- /dev/null
+++ b/drm_i915.c
@@ -0,0 +1,342 @@ 
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. The name of the author may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Authors:
+ *    Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
+ */
+
+#include "defs.h"
+
+#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
+
+#ifdef HAVE_DRM_H
+#include <drm.h>
+#include <i915_drm.h>
+#else
+#include <drm/drm.h>
+#include <drm/i915_drm.h>
+#endif
+
+#include "xlat/drm_i915_ioctls.h"
+#include "xlat/drm_i915_getparams.h"
+#include "xlat/drm_i915_setparams.h"
+
+static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_getparam param;
+	int value;
+
+	if (umove(tcp, arg, &param))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprints(", {param=");
+		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
+	} else if (exiting(tcp)) {
+		if (umove(tcp, (long)param.value, &value))
+			return RVAL_DECODED;
+
+		tprints(", value=");
+		switch (param.param) {
+		case I915_PARAM_CHIPSET_ID:
+			tprintf("0x%04x", value);
+			break;
+		default:
+			tprintf("%d", value);
+		}
+		tprints("}");
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_setparam param;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &param))
+			return RVAL_DECODED;
+
+		tprints(", {param=");
+		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
+		tprintf(", value=%d}", param.value);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_execbuffer2(struct tcb *tcp, const unsigned int code,
+				long arg)
+{
+	struct drm_i915_gem_execbuffer2 eb;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &eb))
+			return RVAL_DECODED;
+
+		tprintf(", {buffers_ptr=%p, buffer_count=%u, "
+			"batch_start_offset=%x, batch_len=%u, DR1=%u, DR4=%u, "
+			"num_cliprects=%u, cliprects_ptr=%p, flags=0x%Lx}",
+			(void *)eb.buffers_ptr, eb.buffer_count,
+			eb.batch_start_offset, eb.batch_len, eb.DR1, eb.DR4,
+			eb.num_cliprects, (void *)eb.cliprects_ptr, eb.flags);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_busy(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_busy busy;
+
+	if (umove(tcp, arg, &busy))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", busy.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", busy=%c, ring=%u}",
+			(busy.busy & 0x1) ? 'Y' : 'N', (busy.busy >> 16));
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_create(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_create create;
+
+	if (umove(tcp, arg, &create))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {size=%Lu", create.size);
+	} else if (exiting(tcp)) {
+		tprintf(", handle=%u}", create.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_pread(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_pread pr;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &pr))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u, offset=%Lu, size=%Lu, data_ptr=%p}",
+			pr.handle, pr.offset, pr.size, (void *)pr.data_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_pwrite(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_pwrite pw;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &pw))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u, offset=%Lu, size=%Lu, data_ptr=%p}",
+			pw.handle, pw.offset, pw.size, (void *)pw.data_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_mmap(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_mmap mmap;
+
+	if (umove(tcp, arg, &mmap))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, size=%Lu", mmap.handle, mmap.size);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu, addr_ptr=%p}",
+			mmap.offset, (void *)mmap.addr_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_mmap_gtt(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_mmap_gtt mmap;
+
+	if (umove(tcp, arg, &mmap))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", mmap.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu}", mmap.offset);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_set_domain(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_set_domain dom;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &dom))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u, read_domains=%x, write_domain=%x}",
+			dom.handle, dom.read_domains, dom.write_domain);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_madvise(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_madvise madv;
+
+	if (umove(tcp, arg, &madv))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, madv=%u", madv.handle, madv.madv);
+	} else if (exiting(tcp)) {
+		tprintf(", retained=%u}", madv.retained);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_get_tiling(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_get_tiling tiling;
+
+	if (umove(tcp, arg, &tiling))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", tiling.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", tiling_mode=%u, swizzle_mode=%u}", tiling.tiling_mode,
+			tiling.swizzle_mode);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_set_tiling(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_set_tiling tiling;
+
+	if (umove(tcp, arg, &tiling))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, tiling_mode=%u, stride=%u",
+			tiling.handle, tiling.tiling_mode, tiling.stride);
+	} else if (exiting(tcp)) {
+		tprintf(", swizzle_mode=%u}", tiling.swizzle_mode);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_userptr(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_userptr uptr;
+
+	if (umove(tcp, arg, &uptr))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {user_ptr=%p, user_size=%Lu", (void *)uptr.user_ptr,
+			uptr.user_size);
+	} else if (exiting(tcp)) {
+		tprintf(", flags=0x%x, handle=%u}", uptr.flags, uptr.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+int drm_i915_decode_number(struct tcb *tcp, unsigned int arg)
+{
+	const char *str = xlookup(drm_i915_ioctls, arg);
+
+	if (str) {
+		tprintf("%s", str);
+		return 1;
+	}
+
+	return 0;
+}
+
+int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg)
+{
+	switch (code) {
+	case DRM_IOCTL_I915_GETPARAM:
+		return i915_getparam(tcp, code, arg);
+	case DRM_IOCTL_I915_SETPARAM:
+		return i915_setparam(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_EXECBUFFER2:
+		return i915_gem_execbuffer2(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_BUSY:
+		return i915_gem_busy(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_CREATE:
+		return i915_gem_create(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_PREAD:
+		return i915_gem_pread(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_PWRITE:
+		return i915_gem_pwrite(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MMAP:
+		return i915_gem_mmap(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MMAP_GTT:
+		return i915_gem_mmap_gtt(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_SET_DOMAIN:
+		return i915_gem_set_domain(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MADVISE:
+		return i915_gem_madvise(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_GET_TILING:
+		return i915_gem_get_tiling(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_SET_TILING:
+		return i915_gem_set_tiling(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_USERPTR:
+		return i915_gem_userptr(tcp, code, arg);
+	}
+
+	return RVAL_DECODED;
+}
+
+#endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
diff --git a/xlat/drm_i915_getparams.in b/xlat/drm_i915_getparams.in
new file mode 100644
index 0000000..15275cb
--- /dev/null
+++ b/xlat/drm_i915_getparams.in
@@ -0,0 +1,28 @@ 
+I915_PARAM_IRQ_ACTIVE
+I915_PARAM_ALLOW_BATCHBUFFER
+I915_PARAM_LAST_DISPATCH
+I915_PARAM_CHIPSET_ID
+I915_PARAM_HAS_GEM
+I915_PARAM_NUM_FENCES_AVAIL
+I915_PARAM_HAS_OVERLAY
+I915_PARAM_HAS_PAGEFLIPPING
+I915_PARAM_HAS_EXECBUF2
+I915_PARAM_HAS_BSD
+I915_PARAM_HAS_BLT
+I915_PARAM_HAS_RELAXED_FENCING
+I915_PARAM_HAS_COHERENT_RINGS
+I915_PARAM_HAS_EXEC_CONSTANTS
+I915_PARAM_HAS_RELAXED_DELTA
+I915_PARAM_HAS_GEN7_SOL_RESET
+I915_PARAM_HAS_LLC
+I915_PARAM_HAS_ALIASING_PPGTT
+I915_PARAM_HAS_WAIT_TIMEOUT
+I915_PARAM_HAS_SEMAPHORES
+I915_PARAM_HAS_PRIME_VMAP_FLUSH
+I915_PARAM_HAS_VEBOX
+I915_PARAM_HAS_SECURE_BATCHES
+I915_PARAM_HAS_PINNED_BATCHES
+I915_PARAM_HAS_EXEC_NO_RELOC
+I915_PARAM_HAS_EXEC_HANDLE_LUT
+I915_PARAM_HAS_WT
+I915_PARAM_CMD_PARSER_VERSION
diff --git a/xlat/drm_i915_ioctls.in b/xlat/drm_i915_ioctls.in
new file mode 100644
index 0000000..21c3397
--- /dev/null
+++ b/xlat/drm_i915_ioctls.in
@@ -0,0 +1,51 @@ 
+/* Unfortunately i915 collides with other DRM drivers so we must specify these */
+DRM_IOCTL_I915_INIT
+DRM_IOCTL_I915_FLUSH
+DRM_IOCTL_I915_FLIP
+DRM_IOCTL_I915_BATCHBUFFER
+DRM_IOCTL_I915_IRQ_EMIT
+DRM_IOCTL_I915_IRQ_WAIT
+DRM_IOCTL_I915_GETPARAM
+DRM_IOCTL_I915_SETPARAM
+DRM_IOCTL_I915_ALLOC
+DRM_IOCTL_I915_FREE
+DRM_IOCTL_I915_INIT_HEAP
+DRM_IOCTL_I915_CMDBUFFER
+DRM_IOCTL_I915_DESTROY_HEAP
+DRM_IOCTL_I915_SET_VBLANK_PIPE
+DRM_IOCTL_I915_GET_VBLANK_PIPE
+DRM_IOCTL_I915_VBLANK_SWAP
+DRM_IOCTL_I915_HWS_ADDR
+DRM_IOCTL_I915_GEM_INIT
+DRM_IOCTL_I915_GEM_EXECBUFFER
+DRM_IOCTL_I915_GEM_EXECBUFFER2
+DRM_IOCTL_I915_GEM_PIN
+DRM_IOCTL_I915_GEM_UNPIN
+DRM_IOCTL_I915_GEM_BUSY
+DRM_IOCTL_I915_GEM_SET_CACHING
+DRM_IOCTL_I915_GEM_GET_CACHING
+DRM_IOCTL_I915_GEM_THROTTLE
+DRM_IOCTL_I915_GEM_ENTERVT
+DRM_IOCTL_I915_GEM_LEAVEVT
+DRM_IOCTL_I915_GEM_CREATE
+DRM_IOCTL_I915_GEM_PREAD
+DRM_IOCTL_I915_GEM_PWRITE
+DRM_IOCTL_I915_GEM_MMAP
+DRM_IOCTL_I915_GEM_MMAP_GTT
+DRM_IOCTL_I915_GEM_SET_DOMAIN
+DRM_IOCTL_I915_GEM_SW_FINISH
+DRM_IOCTL_I915_GEM_SET_TILING
+DRM_IOCTL_I915_GEM_GET_TILING
+DRM_IOCTL_I915_GEM_GET_APERTURE
+DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID
+DRM_IOCTL_I915_GEM_MADVISE
+DRM_IOCTL_I915_OVERLAY_PUT_IMAGE
+DRM_IOCTL_I915_OVERLAY_ATTRS
+DRM_IOCTL_I915_SET_SPRITE_COLORKEY
+DRM_IOCTL_I915_GET_SPRITE_COLORKEY
+DRM_IOCTL_I915_GEM_WAIT
+DRM_IOCTL_I915_GEM_CONTEXT_CREATE
+DRM_IOCTL_I915_GEM_CONTEXT_DESTROY
+DRM_IOCTL_I915_REG_READ
+DRM_IOCTL_I915_GET_RESET_STATS
+DRM_IOCTL_I915_GEM_USERPTR
diff --git a/xlat/drm_i915_setparams.in b/xlat/drm_i915_setparams.in
new file mode 100644
index 0000000..d93d2ea
--- /dev/null
+++ b/xlat/drm_i915_setparams.in
@@ -0,0 +1,4 @@ 
+I915_SETPARAM_USE_MI_BATCHBUFFER_START
+I915_SETPARAM_TEX_LRU_LOG_GRANULARITY
+I915_SETPARAM_ALLOW_BATCHBUFFER
+I915_SETPARAM_NUM_USED_FENCES

Comments

On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct drm_i915_getparam param;
> +	int value;
> +
> +	if (umove(tcp, arg, &param))
> +		return RVAL_DECODED;
> +
> +	if (entering(tcp)) {
> +		tprints(", {param=");
> +		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> +	} else if (exiting(tcp)) {
> +		if (umove(tcp, (long)param.value, &value))
> +			return RVAL_DECODED;

Since part of param has already been printed, RVAL_DECODED shouldn't be
returned here.  For the same reason, RVAL_DECODED shouldn't be returned
earlier in this function.

> +		tprints(", value=");
> +		switch (param.param) {
> +		case I915_PARAM_CHIPSET_ID:
> +			tprintf("0x%04x", value);

Since the value has been fetched by address stored in param.value,
it has to be printed in brackets like in printnum_int.

> +			break;
> +		default:
> +			tprintf("%d", value);

Likewise.

> +		}
> +		tprints("}");
> +	}
> +
> +	return RVAL_DECODED | 1;

This shouldn't be returned on entering(tcp).

> +}

So the whole function should look smth like this:

static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
{
	struct drm_i915_getparam param;

	if (entering(tcp)) {
		if (umove_or_printaddr(tcp, arg, &param))
			return RVAL_DECODED | 1;
		tprints(", {param=");
		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
		tprints(", value=");
		return 0;
	} else {
		int value;

		if (umove(tcp, arg, &param)) {
			tprints("???");
		} else if (!umove_or_printaddr(tcp, (long) param.value, &value)) {
			switch (param.param) {
			case I915_PARAM_CHIPSET_ID:
				tprintf("[%#04x]", value);
				break;
			default:
				tprintf("[%d]", value);
			}
		}
		tprints("}");
		return 1;
	}
}

Please apply this approach to all DRM_IOWR parsers.

> +
> +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct drm_i915_setparam param;
> +
> +	if (entering(tcp)) {
> +		if (umove(tcp, arg, &param))
> +			return RVAL_DECODED;

In this and other functions I slightly prefer
	if (umove_or_printaddr(tcp, arg, &param))
		return RVAL_DECODED | 1;
over your variant because umove_or_printaddr() handles NULL address
and !verbose(tcp) case better.
On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
[...]
> So the whole function should look smth like this:
> 
> static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> {
> 	struct drm_i915_getparam param;
> 
> 	if (entering(tcp)) {
> 		if (umove_or_printaddr(tcp, arg, &param))
> 			return RVAL_DECODED | 1;
> 		tprints(", {param=");

or rather
		tprints(", ");
		if (umove_or_printaddr(tcp, arg, &param))
			return RVAL_DECODED | 1;
		tprints("{param=");


> In this and other functions I slightly prefer
> 	if (umove_or_printaddr(tcp, arg, &param))
> 		return RVAL_DECODED | 1;
> over your variant because umove_or_printaddr() handles NULL address
> and !verbose(tcp) case better.

And the whole function will look as simple as this:

static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
{
 	struct drm_i915_setparam param;

	tprints(", ");
	if (!umove_or_printaddr(tcp, arg, &param)) {
		tprints("{param=");
		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
		tprintf(", value=%d}", param.value);
	}

	return RVAL_DECODED | 1;
}

Note the absence of entering(tcp) check because this DRM_IOW parser always
returns a value with RVAL_DECODED flag set.
On Tue, Sep 08, 2015 at 04:30:52AM +0300, Dmitry V. Levin wrote:
> On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> [...]
> > So the whole function should look smth like this:
> > 
> > static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > {
> > 	struct drm_i915_getparam param;
> > 
> > 	if (entering(tcp)) {
> > 		if (umove_or_printaddr(tcp, arg, &param))
> > 			return RVAL_DECODED | 1;
> > 		tprints(", {param=");
> 
> or rather
> 		tprints(", ");
> 		if (umove_or_printaddr(tcp, arg, &param))
> 			return RVAL_DECODED | 1;
> 		tprints("{param=");

or rather

static int
i915_getparam(struct tcb *tcp, const unsigned int code, const long arg)
{
	struct drm_i915_getparam param;
	int value;

	if (entering(tcp)) {
		tprints(", ");
		if (umove_or_printaddr(tcp, arg, &param))
			return RVAL_DECODED | 1;
		tprints("{param=");
		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
		return 0;
	}

	if (!umove(tcp, arg, &param)) {
		tprints(", value=");
		if (!umove_or_printaddr(tcp, (long) param.value, &value)) {
			switch (param.param) {
			case I915_PARAM_CHIPSET_ID:
				tprintf("[%#04x]", value);
				break;
			default:
				tprintf("[%d]", value);
			}
		}
	}
	tprints("}");
	return 1;
}

Note that those structure members that are set by the kernel on exiting
syscall shouldn't normally be printed in case of syserror(tcp).

In case of i915_getparam, no extra check is needed because param.value is
an address supplied by userspace and umove_or_printaddr already performs
all necessary checks, but in other IOWR parsers you might want to use
	if (!syserror(tcp) && !umove(tcp, arg, &param)) {
instead.
On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> > +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_i915_getparam param;
> > +	int value;
> > +
> > +	if (umove(tcp, arg, &param))
> > +		return RVAL_DECODED;
> > +
> > +	if (entering(tcp)) {
> > +		tprints(", {param=");
> > +		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> > +	} else if (exiting(tcp)) {
> > +		if (umove(tcp, (long)param.value, &value))
> > +			return RVAL_DECODED;
> 
> Since part of param has already been printed, RVAL_DECODED shouldn't be
> returned here.  For the same reason, RVAL_DECODED shouldn't be returned
> earlier in this function.
> 

The usage of RVAL_DECODED is quite confusing. RVAL_DECODED to me should be "We
decoded this don't do any fallback". How did you intent it to be used?

> > +		tprints(", value=");
> > +		switch (param.param) {
> > +		case I915_PARAM_CHIPSET_ID:
> > +			tprintf("0x%04x", value);
> 
> Since the value has been fetched by address stored in param.value,
> it has to be printed in brackets like in printnum_int.

Ok

> > +			break;
> > +		default:
> > +			tprintf("%d", value);
> 
> Likewise.
> 
> > +		}
> > +		tprints("}");
> > +	}
> > +
> > +	return RVAL_DECODED | 1;
> 
> This shouldn't be returned on entering(tcp).

If all decoding would be done when entering is finished, wouldn't it make sense
to allow RVAL_DECODED here? Still don't understand how this is intended to work.

> > +}
> 
> So the whole function should look smth like this:
> 
> static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> {
> 	struct drm_i915_getparam param;
> 
> 	if (entering(tcp)) {
> 		if (umove_or_printaddr(tcp, arg, &param))
> 			return RVAL_DECODED | 1;
> 		tprints(", {param=");
> 		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> 		tprints(", value=");
> 		return 0;
> 	} else {
> 		int value;
> 
> 		if (umove(tcp, arg, &param)) {
> 			tprints("???");
> 		} else if (!umove_or_printaddr(tcp, (long) param.value, &value)) {
> 			switch (param.param) {
> 			case I915_PARAM_CHIPSET_ID:
> 				tprintf("[%#04x]", value);
> 				break;
> 			default:
> 				tprintf("[%d]", value);
> 			}
> 		}
> 		tprints("}");
> 		return 1;
> 	}
> }
> 
> Please apply this approach to all DRM_IOWR parsers.
> 
> > +
> > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_i915_setparam param;
> > +
> > +	if (entering(tcp)) {
> > +		if (umove(tcp, arg, &param))
> > +			return RVAL_DECODED;
> 
> In this and other functions I slightly prefer
> 	if (umove_or_printaddr(tcp, arg, &param))
> 		return RVAL_DECODED | 1;
> over your variant because umove_or_printaddr() handles NULL address
> and !verbose(tcp) case better.

Agreed

> 
> 
> -- 
> ldv
On Fri, Sep 11, 2015 at 01:31:02PM +0200, Patrik Jakobsson wrote:
> On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> > > +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > > +{
> > > +	struct drm_i915_getparam param;
> > > +	int value;
> > > +
> > > +	if (umove(tcp, arg, &param))
> > > +		return RVAL_DECODED;
> > > +
> > > +	if (entering(tcp)) {
> > > +		tprints(", {param=");
> > > +		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> > > +	} else if (exiting(tcp)) {
> > > +		if (umove(tcp, (long)param.value, &value))
> > > +			return RVAL_DECODED;
> > 
> > Since part of param has already been printed, RVAL_DECODED shouldn't be
> > returned here.  For the same reason, RVAL_DECODED shouldn't be returned
> > earlier in this function.
> 
> The usage of RVAL_DECODED is quite confusing. RVAL_DECODED to me should be "We
> decoded this don't do any fallback". How did you intent it to be used?

RVAL_DECODED itself is trivial: by setting this flag parser tells its
caller that decoding is finished on entering and it shouldn't be called on
exiting of this syscall.  Setting this flag on exiting has no effect.

With regards to ioctl parsers, there might be some confusion because they
also have this unusual return code +1 semantics.  In this example, the
problem with "return RVAL_DECODED" statements is that if they happen on
exiting, it means that something has already been printed on entering
already and by returning RVAL_DECODED parser tells its caller to perform
the default action that also prints something.

That is, ioctl parser should return
- on entering:
  + any value (it's ignored) without RVAL_DECODED flag set:
    continue processing on exiting;
  + RVAL_DECODED:
    skip processing on exiting and tell the caller
    to perform default processing on entering;
  + RVAL_DECODED | (1 + RVAL_*):
    skip processing on exiting and tell the caller
    to skip default processing;
- on exiting:
  + 0 (with or without RVAL_DECODED set, it's ignored anyway):
    tell the caller to perform default processing on exiting;
  + 1 + RVAL_* (RVAL_DECODED is ignored):
    tell the caller to skip default processing.

> > > +			break;
> > > +		default:
> > > +			tprintf("%d", value);
> > 
> > Likewise.
> > 
> > > +		}
> > > +		tprints("}");
> > > +	}
> > > +
> > > +	return RVAL_DECODED | 1;
> > 
> > This shouldn't be returned on entering(tcp).
> 
> If all decoding would be done when entering is finished, wouldn't it make sense
> to allow RVAL_DECODED here? Still don't understand how this is intended to work.

Usally only IOW parsers return codes with RVAL_DECODED set on entering.
IOWR also print something on exiting so they shouldn't normally return
RVAL_DECODED on entering.