[proof-of-concept/rfc] per object file mmaps

[proof-of-concept/rfc] per object file mmaps

[Dave Airlie]

The 2+3 patches are actually the code, the first was just a cleanup.

Anyways the second patch describes it best, but I've taken the approach
that we just need to keep track of what filp this object has had a handle
created on, and block mmaps on it. We could also probably block a bit
more explicitly in the driver respective mmap offset retrieval ioctls,
however we need to block in mmap itself to stop people just picking misc
address and trying them.

It doesn't really add much now, but with render nodes where we have flink_to
or fd passing it would be more useful.

I've no idea if I'll get to do much more with this, so consider it an
indication of how I'd like it done for someone else to run with :-)

Dave.

[PATCH 1/3] drm/vma/gem/ttm: consolide unmapping range

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This is just a cleanup, can probably do better, but at least it makes
the calls to the unmap_mapping_range consistent.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_vma_offset_man.c | 11 +++++++++++
 drivers/gpu/drm/i915/i915_gem.c      |  7 ++-----
 drivers/gpu/drm/ttm/ttm_bo.c         |  8 ++------
 include/drm/drm_vma_offset_man.h     |  3 +++
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_vma_offset_man.c b/drivers/gpu/drm/drm_vma_offset_man.c
index cf2e291..7456892 100644
--- a/drivers/gpu/drm/drm_vma_offset_man.c
+++ b/drivers/gpu/drm/drm_vma_offset_man.c
@@ -125,6 +125,17 @@ out_unlock:
 }
 EXPORT_SYMBOL(drm_vma_offset_setup);
 
+void drm_vma_unmap_mapping(struct address_space *dev_mapping,
+			   struct drm_vma_offset_node *node)
+{
+	if (dev_mapping && drm_vma_node_is_allocated(node)) {
+		unmap_mapping_range(dev_mapping,
+				    drm_vma_node_offset_addr(node),
+				    node->num_pages << PAGE_SHIFT, 1);
+	}
+}
+EXPORT_SYMBOL(drm_vma_unmap_mapping);
+
 int drm_vma_offset_man_init(struct drm_vma_offset_manager *man, uint64_t file_page_offset, uint64_t size)
 {
 	int ret;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 16f1b2c..8d28123 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1424,11 +1424,8 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	if (!obj->fault_mappable)
 		return;
 
-	if (obj->base.dev->dev_mapping)
-		unmap_mapping_range(obj->base.dev->dev_mapping,
-				    (loff_t)drm_vma_node_offset_addr(&obj->base.vma_offset),
-				    obj->base.size, 1);
-
+	drm_vma_unmap_mapping(obj->base.dev->dev_mapping,
+			      &obj->base.vma_offset);
 	obj->fault_mappable = false;
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 3f42621..2a7b6a6 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1597,12 +1597,8 @@ void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 
-	if (drm_vma_node_is_allocated(&bo->vma_offset) && bdev->dev_mapping) {
-		loff_t offset = (loff_t) drm_vma_node_offset_addr(&bo->vma_offset);
-		loff_t holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
-
-		unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
-	}
+	drm_vma_unmap_mapping(bdev->dev_mapping,
+			      &bo->vma_offset);
 	ttm_mem_io_free_vm(bo);
 }
 
diff --git a/include/drm/drm_vma_offset_man.h b/include/drm/drm_vma_offset_man.h
index 4211c60..b8ef845 100644
--- a/include/drm/drm_vma_offset_man.h
+++ b/include/drm/drm_vma_offset_man.h
@@ -35,6 +35,9 @@ void drm_vma_offset_destroy(struct drm_vma_offset_manager *man,
 int drm_vma_offset_man_init(struct drm_vma_offset_manager *man, uint64_t file_page_offset, uint64_t size);
 void drm_vma_offset_man_fini(struct drm_vma_offset_manager *man);
 
+void drm_vma_unmap_mapping(struct address_space *dev_mapping,
+			   struct drm_vma_offset_node *node);
+
 static inline void drm_vma_node_reset(struct drm_vma_offset_node *node)
 {
 	node->vm_node = NULL;
-- 
1.8.0.2

[PATCH 2/3] drm/ttm: add object per-file mmap validation

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

So instead of creating a whole set of per-file address spaces, and having
some sort of melt down in my understanding of the inode/address_space code,
I realised we could just keep a lot of filps that the object has been attached
to, or has had a handle created on.

At the moment this gives us nothing extra, since you can nearly always guess
the handles and gem open them, but in the future where we have fd passing
or flink to, then this should block other clients from mmaping objects they
haven't been explicitly given a handle for.

This just implements it in TTM for radeon/nouveau, vmwgfx and other drivers
would need updates as well. It might also be nice to try and consolidate
things a bit better.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_vma_offset_man.c  | 43 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_gem.c |  7 ++++++
 drivers/gpu/drm/radeon/radeon_gem.c   |  7 ++++++
 drivers/gpu/drm/ttm/ttm_bo_vm.c       | 15 +++++++++---
 include/drm/drm_vma_offset_man.h      | 18 ++++++++++++++-
 5 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_vma_offset_man.c b/drivers/gpu/drm/drm_vma_offset_man.c
index 7456892..ee2f425 100644
--- a/drivers/gpu/drm/drm_vma_offset_man.c
+++ b/drivers/gpu/drm/drm_vma_offset_man.c
@@ -91,6 +91,7 @@ int drm_vma_offset_setup(struct drm_vma_offset_manager *man,
 {
 	int ret;
 
+	INIT_LIST_HEAD(&node->flist);
 retry_pre_get:
 	ret = drm_mm_pre_get(&man->addr_space_mm);
 	if (unlikely(ret != 0))
@@ -158,3 +159,45 @@ void drm_vma_offset_man_fini(struct drm_vma_offset_manager *man)
 	write_unlock(&man->vm_lock);
 }
 EXPORT_SYMBOL(drm_vma_offset_man_fini);
+
+int drm_vma_offset_node_add_file(struct drm_vma_offset_node *node,
+				 struct file *filp)
+{
+	struct drm_vma_offset_node_per_file *fnode;
+
+	fnode = kmalloc(sizeof(*fnode), GFP_KERNEL);
+	if (!fnode)
+		return -ENOMEM;
+
+	fnode->filp = filp;
+	list_add(&fnode->lhead, &node->flist);
+	return 0;
+}
+EXPORT_SYMBOL(drm_vma_offset_node_add_file);
+
+void drm_vma_offset_node_remove_file(struct drm_vma_offset_node *node,
+			       struct file *filp)
+{
+	struct drm_vma_offset_node_per_file *fnode, *temp;
+
+	list_for_each_entry_safe(fnode, temp, &node->flist, lhead) {
+		if (fnode->filp == filp) {
+			list_del(&fnode->lhead);
+			kfree(fnode);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL(drm_vma_offset_node_remove_file);
+
+bool drm_vma_offset_node_valid_file(struct drm_vma_offset_node *node,
+				    struct file *filp)
+{
+	struct drm_vma_offset_node_per_file *fnode;
+	list_for_each_entry(fnode, &node->flist, lhead) {	
+		if (fnode->filp == filp)
+			return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(drm_vma_offset_node_valid_file);
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 6be9249..8281f5c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -74,6 +74,11 @@ nouveau_gem_object_open(struct drm_gem_object *gem, struct drm_file *file_priv)
 	struct nouveau_vma *vma;
 	int ret;
 
+	ret = drm_vma_offset_node_add_file(&nvbo->bo.vma_offset,
+					   file_priv->filp);
+	if (ret)
+		return ret;
+
 	if (!cli->base.vm)
 		return 0;
 
@@ -111,6 +116,8 @@ nouveau_gem_object_close(struct drm_gem_object *gem, struct drm_file *file_priv)
 	struct nouveau_vma *vma;
 	int ret;
 
+	drm_vma_offset_node_remove_file(&nvbo->bo.vma_offset, file_priv->filp);
+
 	if (!cli->base.vm)
 		return;
 
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
index fe5c1f6..daba965 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -146,6 +146,12 @@ int radeon_gem_object_open(struct drm_gem_object *obj, struct drm_file *file_pri
 	struct radeon_bo_va *bo_va;
 	int r;
 
+	/* allocate a file to bo */
+	r = drm_vma_offset_node_add_file(&rbo->tbo.vma_offset,
+					 file_priv->filp);
+	if (r)
+		return r;
+
 	if (rdev->family < CHIP_CAYMAN) {
 		return 0;
 	}
@@ -176,6 +182,7 @@ void radeon_gem_object_close(struct drm_gem_object *obj,
 	struct radeon_bo_va *bo_va;
 	int r;
 
+	drm_vma_offset_node_remove_file(&rbo->tbo.vma_offset, file_priv->filp);
 	if (rdev->family < CHIP_CAYMAN) {
 		return;
 	}
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 3e52e25..d111d3d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -218,7 +218,8 @@ static const struct vm_operations_struct ttm_bo_vm_ops = {
 	.close = ttm_bo_vm_close
 };
 
-static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
+static struct ttm_buffer_object *ttm_bo_vm_lookup(struct file *filp,
+						  struct ttm_bo_device *bdev,
 						  unsigned long dev_offset,
 						  unsigned long num_pages)
 {
@@ -236,10 +237,18 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
 	bo = container_of(node, struct ttm_buffer_object, vma_offset);
 	ttm_bo_reference(bo);
 
+	if (drm_vma_offset_node_valid_file(&bo->vma_offset, filp) == false) {
+		bo = NULL;
+		pr_err("Invalid buffer object for this file descriptor\n");
+		goto out;
+	}
+
 	if (!kref_get_unless_zero(&bo->kref)) {
 		bo = NULL;
 		pr_err("Could not find buffer object to map\n");
 	}
+
+out:
 	read_unlock(&bdev->vm_lock);
 	return bo;
 }
@@ -251,7 +260,7 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
 	struct ttm_buffer_object *bo;
 	int ret;
 
-	bo = ttm_bo_vm_lookup(bdev,
+	bo = ttm_bo_vm_lookup(filp, bdev,
 			      vma->vm_pgoff,
 			      (vma->vm_end - vma->vm_start) >> PAGE_SHIFT);
 	if (unlikely(bo == NULL))
@@ -313,7 +322,7 @@ ssize_t ttm_bo_io(struct ttm_bo_device *bdev, struct file *filp,
 	bool no_wait = false;
 	bool dummy;
 
-	bo = ttm_bo_vm_lookup(bdev, dev_offset, 1);
+	bo = ttm_bo_vm_lookup(filp, bdev, dev_offset, 1);
 	if (unlikely(bo == NULL))
 		return -EFAULT;
 
diff --git a/include/drm/drm_vma_offset_man.h b/include/drm/drm_vma_offset_man.h
index b8ef845..6a297f2 100644
--- a/include/drm/drm_vma_offset_man.h
+++ b/include/drm/drm_vma_offset_man.h
@@ -2,13 +2,23 @@
 #ifndef DRM_VMA_OFFSET_MAN
 #define DRM_VMA_OFFSET_MAN
 
+#include <linux/fs.h>
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <drm/drm_mm.h>
 
-struct drm_mm_node;
+/* store a linked list of file privs this object is valid on,
+   the bust a move in mmap */
 
+struct drm_vma_offset_node_per_file {
+	struct list_head lhead;
+	struct file *filp;
+};
+
+/* you'd want a linked list of file privs per node */
 struct drm_vma_offset_node {
+	struct list_head flist;
+
 	struct drm_mm_node *vm_node;
 	struct rb_node vm_rb;
 	uint64_t num_pages;
@@ -58,4 +68,10 @@ static inline uint64_t drm_vma_node_offset_addr(struct drm_vma_offset_node *node
 	return ((uint64_t) node->vm_node->start) << PAGE_SHIFT;
 }
 
+int drm_vma_offset_node_add_file(struct drm_vma_offset_node *node,
+				    struct file *filp);
+void drm_vma_offset_node_remove_file(struct drm_vma_offset_node *node,
+				     struct file *filp);
+bool drm_vma_offset_node_valid_file(struct drm_vma_offset_node *node,
+				    struct file *filp);
 #endif
-- 
1.8.0.2

[PATCH 3/3] drm/gem: start adding support for per-file object mmaps

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This adds the support for drivers that use the gem mmap call, they need
to specify DRIVER_GEM_MMAP in their feature set, so they get this behaviour.

This saves me adding 10 open/close handlers for now, if someone would like
to do that instead of midlayer, then I'd be happy to watch.

TODO: all other non-i915 drivers.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_gem.c       | 17 +++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c |  2 +-
 include/drm/drmP.h              |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index bb5ac23..dbbd736 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -240,6 +240,8 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 
 	drm_gem_remove_prime_handles(obj, filp);
 
+	if (drm_core_check_feature(dev, DRIVER_GEM_MMAP))
+		drm_vma_offset_node_remove_file(&obj->vma_offset, filp->filp);
 	if (dev->driver->gem_close_object)
 		dev->driver->gem_close_object(obj, filp);
 	drm_gem_object_handle_unreference_unlocked(obj);
@@ -280,9 +282,19 @@ again:
 
 	drm_gem_object_handle_reference(obj);
 
+	if (drm_core_check_feature(dev, DRIVER_GEM_MMAP)) {
+		ret = drm_vma_offset_node_add_file(&obj->vma_offset,
+						   file_priv->filp);
+		if (ret) {
+			drm_gem_handle_delete(file_priv, *handlep);
+			return ret;
+		}
+	}
 	if (dev->driver->gem_open_object) {
 		ret = dev->driver->gem_open_object(obj, file_priv);
 		if (ret) {
+			if (drm_core_check_feature(dev, DRIVER_GEM_MMAP))
+				drm_vma_offset_node_remove_file(&obj->vma_offset, file_priv->filp);
 			drm_gem_handle_delete(file_priv, *handlep);
 			return ret;
 		}
@@ -633,6 +645,11 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 		return drm_mmap(filp, vma);
 	}
 
+	if (drm_vma_offset_node_valid_file(offset_node, filp) == false) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	obj = container_of(offset_node, struct drm_gem_object, vma_offset);
 
 	/* Check for valid size. */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 530db83..a42cb8f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1017,7 +1017,7 @@ static struct drm_driver driver = {
 	 */
 	.driver_features =
 	    DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | /* DRIVER_USE_MTRR |*/
-	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME,
+	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME | DRIVER_GEM_MMAP,
 	.load = i915_driver_load,
 	.unload = i915_driver_unload,
 	.open = i915_driver_open,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f7186e8..b6bce07 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -155,6 +155,7 @@ int drm_err(const char *func, const char *format, ...);
 #define DRIVER_GEM         0x1000
 #define DRIVER_MODESET     0x2000
 #define DRIVER_PRIME       0x4000
+#define DRIVER_GEM_MMAP    0x8000
 
 #define DRIVER_BUS_PCI 0x1
 #define DRIVER_BUS_PLATFORM 0x2
-- 
1.8.0.2

Re: [PATCH 3/3] drm/gem: start adding support for per-file object mmaps

[Chris Wilson]

From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [proof-of-concept/rfc] per object file mmaps
To: Dave Airlie <airlied@gmail.com>, dri-devel@lists.freedesktop.org
In-Reply-To: <1355892119-13926-1-git-send-email-airlied@gmail.com>
References: <1355892119-13926-1-git-send-email-airlied@gmail.com>

On Wed, 19 Dec 2012 14:41:56 +1000, Dave Airlie <airlied@gmail.com> wrote:
> The 2+3 patches are actually the code, the first was just a cleanup.
> 
> Anyways the second patch describes it best, but I've taken the approach
> that we just need to keep track of what filp this object has had a handle
> created on, and block mmaps on it. We could also probably block a bit
> more explicitly in the driver respective mmap offset retrieval ioctls,
> however we need to block in mmap itself to stop people just picking misc
> address and trying them.
> 
> It doesn't really add much now, but with render nodes where we have flink_to
> or fd passing it would be more useful.
> 
> I've no idea if I'll get to do much more with this, so consider it an
> indication of how I'd like it done for someone else to run with :-)

Locking looks entirely non-existent for per-file mmap add/remove in the
generic gem code. Also we have a pair of hooks there that look like they
would be a convenient point to place that code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre