Re: [PATCH v2] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 15, 2016 at 04:51:46PM +0000, Chris Wilson wrote:
> Tvrtko was looking through the execbuffer-ioctl and noticed that the
> uABI was tightly coupled to our internal engine identifiers. Close
> inspection also revealed that we leak those internal engine identifiers
> through the busy-ioctl, and those internal identifiers already do not
> match the user identifiers. Fortuitiously, there is only one user of the
> set of busy rings from the busy-ioctl, and they only wish to choose
> between the RENDER and the BLT engines.
> 
> Let's fix the userspace ABI while we still can.
> 
> v2: Update the uAPI documentation to explain the identifiers.
> 
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 18 ++++++++++++++----
>  drivers/gpu/drm/i915/intel_lrc.c        |  5 +++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  5 +++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>  include/uapi/drm/i915_drm.h             | 33 +++++++++++++++++++++++++++++----
>  5 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index bb44bad15403..85797813a3de 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4328,10 +4328,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto unref;
>  
> -	BUILD_BUG_ON(I915_NUM_RINGS > 16);
> -	args->busy = obj->active << 16;
> -	if (obj->last_write_req)
> -		args->busy |= obj->last_write_req->ring->id;
> +	args->busy = 0;
> +	if (obj->active) {
> +		int i;
> +
> +		for (i = 0; i < I915_NUM_RINGS; i++) {
> +			struct drm_i915_gem_request *req;
> +
> +			req = obj->last_read_req[i];
> +			if (req)
> +				args->busy |= 1 << (16 + req->ring->exec_id);
> +		}
> +		if (obj->last_write_req)
> +			args->busy |= obj->last_write_req->ring->exec_id;
> +	}
>  
>  unref:
>  	drm_gem_object_unreference(&obj->base);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index f5d89c845ede..4aa209483237 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2024,6 +2024,7 @@ static int logical_render_ring_init(struct drm_device *dev)
>  
>  	ring->name = "render ring";
>  	ring->id = RCS;
> +	ring->exec_id = I915_EXEC_RENDER;
>  	ring->mmio_base = RENDER_RING_BASE;
>  
>  	logical_ring_default_irqs(ring, GEN8_RCS_IRQ_SHIFT);
> @@ -2073,6 +2074,7 @@ static int logical_bsd_ring_init(struct drm_device *dev)
>  
>  	ring->name = "bsd ring";
>  	ring->id = VCS;
> +	ring->exec_id = I915_EXEC_BSD;
>  	ring->mmio_base = GEN6_BSD_RING_BASE;
>  
>  	logical_ring_default_irqs(ring, GEN8_VCS1_IRQ_SHIFT);
> @@ -2088,6 +2090,7 @@ static int logical_bsd2_ring_init(struct drm_device *dev)
>  
>  	ring->name = "bsd2 ring";
>  	ring->id = VCS2;
> +	ring->exec_id = I915_EXEC_BSD;
>  	ring->mmio_base = GEN8_BSD2_RING_BASE;
>  
>  	logical_ring_default_irqs(ring, GEN8_VCS2_IRQ_SHIFT);
> @@ -2103,6 +2106,7 @@ static int logical_blt_ring_init(struct drm_device *dev)
>  
>  	ring->name = "blitter ring";
>  	ring->id = BCS;
> +	ring->exec_id = I915_EXEC_BLT;
>  	ring->mmio_base = BLT_RING_BASE;
>  
>  	logical_ring_default_irqs(ring, GEN8_BCS_IRQ_SHIFT);
> @@ -2118,6 +2122,7 @@ static int logical_vebox_ring_init(struct drm_device *dev)
>  
>  	ring->name = "video enhancement ring";
>  	ring->id = VECS;
> +	ring->exec_id = I915_EXEC_VEBOX;
>  	ring->mmio_base = VEBOX_RING_BASE;
>  
>  	logical_ring_default_irqs(ring, GEN8_VECS_IRQ_SHIFT);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 8cd8aabcc3ff..310d151c0db2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2680,6 +2680,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  
>  	ring->name = "render ring";
>  	ring->id = RCS;
> +	ring->exec_id = I915_EXEC_RENDER;
>  	ring->mmio_base = RENDER_RING_BASE;
>  
>  	if (INTEL_INFO(dev)->gen >= 8) {
> @@ -2828,6 +2829,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  
>  	ring->name = "bsd ring";
>  	ring->id = VCS;
> +	ring->exec_id = I915_EXEC_BSD;
>  
>  	ring->write_tail = ring_write_tail;
>  	if (INTEL_INFO(dev)->gen >= 6) {
> @@ -2904,6 +2906,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  
>  	ring->name = "bsd2 ring";
>  	ring->id = VCS2;
> +	ring->exec_id = I915_EXEC_BSD;
>  
>  	ring->write_tail = ring_write_tail;
>  	ring->mmio_base = GEN8_BSD2_RING_BASE;
> @@ -2934,6 +2937,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  
>  	ring->name = "blitter ring";
>  	ring->id = BCS;
> +	ring->exec_id = I915_EXEC_BLT;
>  
>  	ring->mmio_base = BLT_RING_BASE;
>  	ring->write_tail = ring_write_tail;
> @@ -2991,6 +2995,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  
>  	ring->name = "video enhancement ring";
>  	ring->id = VECS;
> +	ring->exec_id = I915_EXEC_VEBOX;
>  
>  	ring->mmio_base = VEBOX_RING_BASE;
>  	ring->write_tail = ring_write_tail;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 7349d9258191..2067f4700caa 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -156,6 +156,7 @@ struct  intel_engine_cs {
>  	} id;
>  #define I915_NUM_RINGS 5
>  #define LAST_USER_RING (VECS + 1)
> +	unsigned int exec_id;
>  	u32		mmio_base;
>  	struct		drm_device *dev;
>  	struct intel_ringbuffer *buffer;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index acf21026c78a..6a19371391fa 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -812,10 +812,35 @@ struct drm_i915_gem_busy {
>  	/** Handle of the buffer to check for busy */
>  	__u32 handle;
>  
> -	/** Return busy status (1 if busy, 0 if idle).
> -	 * The high word is used to indicate on which rings the object
> -	 * currently resides:
> -	 *  16:31 - busy (r or r/w) rings (16 render, 17 bsd, 18 blt, etc)
> +	/** Return busy status
> +	 *
> +	 * A return of 0 implies that the object is idle (after
> +	 * having flushed any pending activity), and a non-zero return that
> +	 * the object is still in-flight on the GPU. (The GPU has not yet
> +	 * signaled completion for all pending requests that reference the
> +	 * object.)
> +	 *
> +	 * The returned dword is split into two fields to indicate both
> +	 * the engines on which the object is being read, and the
> +	 * engine on which it is currently being written (if any).
> +	 *
> +	 * The low word (bits 0:15) indicate if the object is being written
> +	 * to by any engine (there can only be one, as the GEM implicit
> +	 * synchronisation rules force writes to be serialised). Only the
> +	 * engine for the last write is reported.
> +	 *
> +	 * The high word (bits 16:31) are a bitmask of which engines are
> +	 * currently reading from the object. Multiple engines may be
> +	 * reading from the object simultaneously.
> +	 *
> +	 * The value of each engine is the same as specified in the
> +	 * EXECBUFFER2 ioctl, i.e. I915_EXEC_RENDER, I915_EXEC_BSD etc.
> +	 * Note I915_EXEC_DEFAULT is a symbolic value and is mapped to
> +	 * the I915_EXEC_RENDER engine for execution, and so it is never
> +	 * reported as active itself. Some hardware may have parallel
> +	 * execution engines, e.g. multiple media engines, which are
> +	 * mapped to the same identifier in the EXECBUFFER2 ioctl and
> +	 * so are not separately reported for busyness.
>  	 */
>  	__u32 busy;
>  };
> -- 
> 2.7.0.rc3
> 

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux