[PATCH 1/2] drm/ttm: ensure ttm_mem_io_free is called on bo destruction

[PATCH 1/2] drm/ttm: ensure ttm_mem_io_free is called on bo destruction

[Ben Skeggs]

From: Ben Skeggs <bskeggs@redhat.com>

Nouveau will start to use ttm_mem_io_reserve to allocate BAR VM space
for VRAM mappings, and without this call GPU address space gets leaked.

Signed-off-by: Ben Skeggs <bskeggs@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index dfa163b..c373cf9 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -472,6 +472,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 	wake_up_all(&bo->event_queue);
 	spin_unlock(&glob->lru_lock);
 
+	ttm_mem_io_free(bo->bdev, &tmp_mem);
 	ttm_bo_mem_put(bo, &tmp_mem);
 }
 
-- 
1.7.3.1

[PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Ben Skeggs]

From: Ben Skeggs <bskeggs@redhat.com>

If the driver kmaps an object userspace is expecting to be mapped, the
unmap would have called down into the drivers io_unreserve() function
and potentially unmapped the pages from its BARs (for example) and they'd
no longer be accessible for the userspace mapping.

Signed-off-by: Ben Skeggs <bskeggs@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
 include/drm/ttm/ttm_bo_api.h      |    1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index ff358ad..e9dbe8b 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
 	if (num_pages > 1 && !DRM_SUSER(DRM_CURPROC))
 		return -EPERM;
 #endif
-	ret = ttm_mem_io_reserve(bo->bdev, &bo->mem);
-	if (ret)
-		return ret;
+	if (!bo->mem.bus.io_reserved) {
+		ret = ttm_mem_io_reserve(bo->bdev, &bo->mem);
+		if (ret)
+			return ret;
+		map->io_reserved = true;
+	}
 	if (!bo->mem.bus.is_iomem) {
 		return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
 	} else {
@@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
 	switch (map->bo_kmap_type) {
 	case ttm_bo_map_iomap:
 		iounmap(map->virtual);
-		ttm_mem_io_free(map->bo->bdev, &map->bo->mem);
+		if (map->io_reserved) {
+			ttm_mem_io_free(map->bo->bdev, &map->bo->mem);
+			map->io_reserved = false;
+		}
 		break;
 	case ttm_bo_map_vmap:
 		vunmap(map->virtual);
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 5afa5b5..ce998ac 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
 		ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
 	} bo_kmap_type;
 	struct ttm_buffer_object *bo;
+	bool io_reserved;
 };
 
 /**
-- 
1.7.3.1

Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Thomas Hellstrom]

On 11/04/2010 01:03 AM, Ben Skeggs wrote:
> From: Ben Skeggs<bskeggs@redhat.com>
>
> If the driver kmaps an object userspace is expecting to be mapped, the
> unmap would have called down into the drivers io_unreserve() function
> and potentially unmapped the pages from its BARs (for example) and they'd
> no longer be accessible for the userspace mapping.
>
> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
>   include/drm/ttm/ttm_bo_api.h      |    1 +
>   2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index ff358ad..e9dbe8b 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
>   	if (num_pages>  1&&  !DRM_SUSER(DRM_CURPROC))
>   		return -EPERM;
>   #endif
> -	ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
> -	if (ret)
> -		return ret;
> +	if (!bo->mem.bus.io_reserved) {
> +		ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
> +		if (ret)
> +			return ret;
> +		map->io_reserved = true;
> +	}
>   	if (!bo->mem.bus.is_iomem) {
>   		return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
>   	} else {
> @@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
>   	switch (map->bo_kmap_type) {
>   	case ttm_bo_map_iomap:
>   		iounmap(map->virtual);
> -		ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
> +		if (map->io_reserved) {
> +			ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
> +			map->io_reserved = false;
> +		}
>   		break;
>   	case ttm_bo_map_vmap:
>   		vunmap(map->virtual);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 5afa5b5..ce998ac 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
>   		ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
>   	} bo_kmap_type;
>   	struct ttm_buffer_object *bo;
> +	bool io_reserved;
>   };
>
>   /**
>

This doesn't solve the problem unfortunately. Consider the sequence

kmap->io_mem_reserve
fault()->
kunmap->io_mem_free
user_space_access()-> Invalid.

I think this needs to be fixed by us maintaining an 
mem:io_reserved_count, where all user-space triggered io_reserves count 
as 1. A mem::user_space_io_reserved flag could be protected by the 
bo::reserve lock, whereas a reserved_count can't, since strictly you're 
allowed to kmap a bo without reserving it, but only if it's pinned

/Thomas
.

Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Thomas Hellstrom]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

Ben,

I had something like the attached in mind, although it might be more 
beneficial to do the actual refcounting in drivers that needs it. Atomic 
incs and decs are expensive, but I'm not sure how expensive relative to 
function pointer calls.

Patch is only compile-tested

/Thomas


On 11/04/2010 02:08 PM, Thomas Hellstrom wrote:
> On 11/04/2010 01:03 AM, Ben Skeggs wrote:
>> From: Ben Skeggs<bskeggs@redhat.com>
>>
>> If the driver kmaps an object userspace is expecting to be mapped, the
>> unmap would have called down into the drivers io_unreserve() function
>> and potentially unmapped the pages from its BARs (for example) and 
>> they'd
>> no longer be accessible for the userspace mapping.
>>
>> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
>>   include/drm/ttm/ttm_bo_api.h      |    1 +
>>   2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
>> b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> index ff358ad..e9dbe8b 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> @@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
>>       if (num_pages>  1&&  !DRM_SUSER(DRM_CURPROC))
>>           return -EPERM;
>>   #endif
>> -    ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
>> -    if (ret)
>> -        return ret;
>> +    if (!bo->mem.bus.io_reserved) {
>> +        ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
>> +        if (ret)
>> +            return ret;
>> +        map->io_reserved = true;
>> +    }
>>       if (!bo->mem.bus.is_iomem) {
>>           return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
>>       } else {
>> @@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
>>       switch (map->bo_kmap_type) {
>>       case ttm_bo_map_iomap:
>>           iounmap(map->virtual);
>> -        ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
>> +        if (map->io_reserved) {
>> +            ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
>> +            map->io_reserved = false;
>> +        }
>>           break;
>>       case ttm_bo_map_vmap:
>>           vunmap(map->virtual);
>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>> index 5afa5b5..ce998ac 100644
>> --- a/include/drm/ttm/ttm_bo_api.h
>> +++ b/include/drm/ttm/ttm_bo_api.h
>> @@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
>>           ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
>>       } bo_kmap_type;
>>       struct ttm_buffer_object *bo;
>> +    bool io_reserved;
>>   };
>>
>>   /**
>>
>
> This doesn't solve the problem unfortunately. Consider the sequence
>
> kmap->io_mem_reserve
> fault()->
> kunmap->io_mem_free
> user_space_access()-> Invalid.
>
> I think this needs to be fixed by us maintaining an 
> mem:io_reserved_count, where all user-space triggered io_reserves 
> count as 1. A mem::user_space_io_reserved flag could be protected by 
> the bo::reserve lock, whereas a reserved_count can't, since strictly 
> you're allowed to kmap a bo without reserving it, but only if it's pinned
>
> /Thomas
> .
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-drm-ttm-Fix-up-io_mem_reserve-free.patch --]
[-- Type: text/x-patch; name="0001-drm-ttm-Fix-up-io_mem_reserve-free.patch", Size: 6918 bytes --]

>From 7fc680ed912c2f9f230d4d14eec47e34c83b3816 Mon Sep 17 00:00:00 2001
From: Thomas Hellstrom <thellstrom@vmware.com>
Date: Thu, 4 Nov 2010 19:27:51 +0100
Subject: [PATCH] drm/ttm: Fix up io_mem_reserve / free

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c      |   12 ++++++++----
 drivers/gpu/drm/ttm/ttm_bo_util.c |   34 ++++++++++++++++++++++++----------
 drivers/gpu/drm/ttm/ttm_bo_vm.c   |    4 ++--
 include/drm/ttm/ttm_bo_api.h      |    5 ++++-
 include/drm/ttm/ttm_bo_driver.h   |    8 ++++----
 mm/mmap.c                         |    2 +-
 6 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index ce46457..040d51c 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -442,6 +442,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 		bo->ttm = NULL;
 	}
 
+	ttm_mem_io_free_vm(bo->bdev, &bo->mem);
 	ttm_bo_mem_put(bo, &bo->mem);
 
 	atomic_set(&bo->reserved, 0);
@@ -704,7 +705,8 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, bool interruptible,
 
 	evict_mem = bo->mem;
 	evict_mem.mm_node = NULL;
-	evict_mem.bus.io_reserved = false;
+	evict_mem.bus.io_reserved_vm = false;
+	atomic_set(&evict_mem.bus.io_reserved_count, -1);
 
 	placement.fpfn = 0;
 	placement.lpfn = 0;
@@ -1041,7 +1043,8 @@ int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 	mem.num_pages = bo->num_pages;
 	mem.size = mem.num_pages << PAGE_SHIFT;
 	mem.page_alignment = bo->mem.page_alignment;
-	mem.bus.io_reserved = false;
+	mem.bus.io_reserved_vm = false;
+	atomic_set(&mem.bus.io_reserved_count, -1);
 	/*
 	 * Determine where to move the buffer.
 	 */
@@ -1166,7 +1169,8 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
 	bo->mem.num_pages = bo->num_pages;
 	bo->mem.mm_node = NULL;
 	bo->mem.page_alignment = page_alignment;
-	bo->mem.bus.io_reserved = false;
+	bo->mem.bus.io_reserved_vm = false;
+	atomic_set(&bo->mem.bus.io_reserved_count, -1);
 	bo->buffer_start = buffer_start & PAGE_MASK;
 	bo->priv_flags = 0;
 	bo->mem.placement = (TTM_PL_FLAG_SYSTEM | TTM_PL_FLAG_CACHED);
@@ -1555,7 +1559,7 @@ void ttm_bo_unmap_virtual(struct ttm_buffer_object *bo)
 	if (!bdev->dev_mapping)
 		return;
 	unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
-	ttm_mem_io_free(bdev, &bo->mem);
+	ttm_mem_io_free_vm(bdev, &bo->mem);
 }
 EXPORT_SYMBOL(ttm_bo_unmap_virtual);
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index ff358ad..ac2b2fa 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -75,26 +75,40 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_move_ttm);
 
-int ttm_mem_io_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+static int ttm_mem_io_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+{
+	if (bdev->driver->io_mem_reserve &&
+	    atomic_inc_and_test(&mem->bus.io_reserved_count))
+		return bdev->driver->io_mem_reserve(bdev, mem);
+	return 0;
+}
+
+static void ttm_mem_io_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+{
+	if (bdev->driver->io_mem_reserve &&
+	    atomic_add_negative(-1, &mem->bus.io_reserved_count) &&
+	    bdev->driver->io_mem_free)
+		bdev->driver->io_mem_free(bdev, mem);
+}
+
+int ttm_mem_io_reserve_vm(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 {
 	int ret;
 
-	if (!mem->bus.io_reserved) {
-		mem->bus.io_reserved = true;
-		ret = bdev->driver->io_mem_reserve(bdev, mem);
+	if (!mem->bus.io_reserved_vm) {
+		ret = ttm_mem_io_reserve(bdev, mem);
 		if (unlikely(ret != 0))
 			return ret;
+		mem->bus.io_reserved_vm = true;
 	}
 	return 0;
 }
 
-void ttm_mem_io_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+void ttm_mem_io_free_vm(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 {
-	if (bdev->driver->io_mem_reserve) {
-		if (mem->bus.io_reserved) {
-			mem->bus.io_reserved = false;
-			bdev->driver->io_mem_free(bdev, mem);
-		}
+	if (mem->bus.io_reserved_vm) {
+		mem->bus.io_reserved_vm = false;
+		ttm_mem_io_free(bdev, mem);
 	}
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index fe6cb77..6383cc3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -131,8 +131,8 @@ static int ttm_bo_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		spin_unlock(&bo->lock);
 
 
-	ret = ttm_mem_io_reserve(bdev, &bo->mem);
-	if (ret) {
+	ret = ttm_mem_io_reserve_vm(bdev, &bo->mem);
+	if (unlikely(ret != 0)) {
 		retval = VM_FAULT_SIGBUS;
 		goto out_unlock;
 	}
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 5afa5b5..9b98b53 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -74,6 +74,8 @@ struct ttm_placement {
  * @is_iomem:		is this io memory ?
  * @size:		size in byte
  * @offset:		offset from the base address
+ * @io_reserved_vm:     The VM system has a refcount in @io_reserved_count
+ * @io_reserved_count:  Refcounting the numbers of callers to ttm_mem_io_reserve
  *
  * Structure indicating the bus placement of an object.
  */
@@ -83,7 +85,8 @@ struct ttm_bus_placement {
 	unsigned long	size;
 	unsigned long	offset;
 	bool		is_iomem;
-	bool		io_reserved;
+	bool		io_reserved_vm;
+	atomic_t        io_reserved_count;
 };
 
 
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 8e0c848..e42a8d7 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -773,10 +773,10 @@ extern int ttm_bo_pci_offset(struct ttm_bo_device *bdev,
 			     unsigned long *bus_offset,
 			     unsigned long *bus_size);
 
-extern int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
-				struct ttm_mem_reg *mem);
-extern void ttm_mem_io_free(struct ttm_bo_device *bdev,
-				struct ttm_mem_reg *mem);
+extern int ttm_mem_io_reserve_vm(struct ttm_bo_device *bdev,
+				 struct ttm_mem_reg *mem);
+extern void ttm_mem_io_free_vm(struct ttm_bo_device *bdev,
+			       struct ttm_mem_reg *mem);
 
 extern void ttm_bo_global_release(struct drm_global_reference *ref);
 extern int ttm_bo_global_init(struct drm_global_reference *ref);
diff --git a/mm/mmap.c b/mm/mmap.c
index 00161a4..55935a9 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1872,7 +1872,7 @@ find_extend_vma(struct mm_struct * mm, unsigned long addr)
  *
  * Called with the mm semaphore held.
  */
-static void remove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
+static void \±XAJ-SA34Y OAD ·QA<Z9<4weremove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
 {
 	/* Update high watermark before we lower total_vm */
 	update_hiwater_vm(mm);
-- 
1.6.2.5


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Ben Skeggs]

On Thu, 2010-11-04 at 19:34 +0100, Thomas Hellstrom wrote:
> Ben,
> 
> I had something like the attached in mind, although it might be more 
> beneficial to do the actual refcounting in drivers that needs it. Atomic 
> incs and decs are expensive, but I'm not sure how expensive relative to 
> function pointer calls.
Thomas,

Thanks for that :)  It looks good to me, and appears to work as it
should.

Ben.
> 
> Patch is only compile-tested
> 
> /Thomas
> 
> 
> On 11/04/2010 02:08 PM, Thomas Hellstrom wrote:
> > On 11/04/2010 01:03 AM, Ben Skeggs wrote:
> >> From: Ben Skeggs<bskeggs@redhat.com>
> >>
> >> If the driver kmaps an object userspace is expecting to be mapped, the
> >> unmap would have called down into the drivers io_unreserve() function
> >> and potentially unmapped the pages from its BARs (for example) and 
> >> they'd
> >> no longer be accessible for the userspace mapping.
> >>
> >> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
> >> ---
> >>   drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
> >>   include/drm/ttm/ttm_bo_api.h      |    1 +
> >>   2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
> >> b/drivers/gpu/drm/ttm/ttm_bo_util.c
> >> index ff358ad..e9dbe8b 100644
> >> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> >> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> >> @@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
> >>       if (num_pages>  1&&  !DRM_SUSER(DRM_CURPROC))
> >>           return -EPERM;
> >>   #endif
> >> -    ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
> >> -    if (ret)
> >> -        return ret;
> >> +    if (!bo->mem.bus.io_reserved) {
> >> +        ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
> >> +        if (ret)
> >> +            return ret;
> >> +        map->io_reserved = true;
> >> +    }
> >>       if (!bo->mem.bus.is_iomem) {
> >>           return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
> >>       } else {
> >> @@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
> >>       switch (map->bo_kmap_type) {
> >>       case ttm_bo_map_iomap:
> >>           iounmap(map->virtual);
> >> -        ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
> >> +        if (map->io_reserved) {
> >> +            ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
> >> +            map->io_reserved = false;
> >> +        }
> >>           break;
> >>       case ttm_bo_map_vmap:
> >>           vunmap(map->virtual);
> >> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> >> index 5afa5b5..ce998ac 100644
> >> --- a/include/drm/ttm/ttm_bo_api.h
> >> +++ b/include/drm/ttm/ttm_bo_api.h
> >> @@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
> >>           ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
> >>       } bo_kmap_type;
> >>       struct ttm_buffer_object *bo;
> >> +    bool io_reserved;
> >>   };
> >>
> >>   /**
> >>
> >
> > This doesn't solve the problem unfortunately. Consider the sequence
> >
> > kmap->io_mem_reserve
> > fault()->
> > kunmap->io_mem_free
> > user_space_access()-> Invalid.
> >
> > I think this needs to be fixed by us maintaining an 
> > mem:io_reserved_count, where all user-space triggered io_reserves 
> > count as 1. A mem::user_space_io_reserved flag could be protected by 
> > the bo::reserve lock, whereas a reserved_count can't, since strictly 
> > you're allowed to kmap a bo without reserving it, but only if it's pinned
> >
> > /Thomas
> > .
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Thomas Hellstrom]

On 11/09/2010 04:03 AM, Ben Skeggs wrote:
> On Thu, 2010-11-04 at 19:34 +0100, Thomas Hellstrom wrote:
>    
>> Ben,
>>
>> I had something like the attached in mind, although it might be more
>> beneficial to do the actual refcounting in drivers that needs it. Atomic
>> incs and decs are expensive, but I'm not sure how expensive relative to
>> function pointer calls.
>>      
> Thomas,
>
> Thanks for that :)  It looks good to me, and appears to work as it
> should.
>
> Ben.
>    
Great.

I have a question, though. (CC'ing Jerome as well)

Seems to me like something is missing from the mem_reserve interface. 
Let's say you have a programmable VRAM aperture and it's full, so you 
can't honor bo map request. You'd then want to traverse a list and call 
unmap_mapping_range() to kill user-space maps and free VRAM aperture 
space, but you can't really do that since you don't have access to the 
mapping range in question...?

/Thomas







>> Patch is only compile-tested
>>
>> /Thomas
>>
>>
>> On 11/04/2010 02:08 PM, Thomas Hellstrom wrote:
>>      
>>> On 11/04/2010 01:03 AM, Ben Skeggs wrote:
>>>        
>>>> From: Ben Skeggs<bskeggs@redhat.com>
>>>>
>>>> If the driver kmaps an object userspace is expecting to be mapped, the
>>>> unmap would have called down into the drivers io_unreserve() function
>>>> and potentially unmapped the pages from its BARs (for example) and
>>>> they'd
>>>> no longer be accessible for the userspace mapping.
>>>>
>>>> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
>>>> ---
>>>>    drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
>>>>    include/drm/ttm/ttm_bo_api.h      |    1 +
>>>>    2 files changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> index ff358ad..e9dbe8b 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> @@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
>>>>        if (num_pages>   1&&   !DRM_SUSER(DRM_CURPROC))
>>>>            return -EPERM;
>>>>    #endif
>>>> -    ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
>>>> -    if (ret)
>>>> -        return ret;
>>>> +    if (!bo->mem.bus.io_reserved) {
>>>> +        ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +        map->io_reserved = true;
>>>> +    }
>>>>        if (!bo->mem.bus.is_iomem) {
>>>>            return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
>>>>        } else {
>>>> @@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
>>>>        switch (map->bo_kmap_type) {
>>>>        case ttm_bo_map_iomap:
>>>>            iounmap(map->virtual);
>>>> -        ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
>>>> +        if (map->io_reserved) {
>>>> +            ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
>>>> +            map->io_reserved = false;
>>>> +        }
>>>>            break;
>>>>        case ttm_bo_map_vmap:
>>>>            vunmap(map->virtual);
>>>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>>>> index 5afa5b5..ce998ac 100644
>>>> --- a/include/drm/ttm/ttm_bo_api.h
>>>> +++ b/include/drm/ttm/ttm_bo_api.h
>>>> @@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
>>>>            ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
>>>>        } bo_kmap_type;
>>>>        struct ttm_buffer_object *bo;
>>>> +    bool io_reserved;
>>>>    };
>>>>
>>>>    /**
>>>>
>>>>          
>>> This doesn't solve the problem unfortunately. Consider the sequence
>>>
>>> kmap->io_mem_reserve
>>> fault()->
>>> kunmap->io_mem_free
>>> user_space_access()->  Invalid.
>>>
>>> I think this needs to be fixed by us maintaining an
>>> mem:io_reserved_count, where all user-space triggered io_reserves
>>> count as 1. A mem::user_space_io_reserved flag could be protected by
>>> the bo::reserve lock, whereas a reserved_count can't, since strictly
>>> you're allowed to kmap a bo without reserving it, but only if it's pinned
>>>
>>> /Thomas
>>> .
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>        
>>      
>
>    

Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Jerome Glisse]

On Tue, Nov 9, 2010 at 2:18 AM, Thomas Hellstrom <thomas@shipmail.org> wrote:
> On 11/09/2010 04:03 AM, Ben Skeggs wrote:
>>
>> On Thu, 2010-11-04 at 19:34 +0100, Thomas Hellstrom wrote:
>>
>>>
>>> Ben,
>>>
>>> I had something like the attached in mind, although it might be more
>>> beneficial to do the actual refcounting in drivers that needs it. Atomic
>>> incs and decs are expensive, but I'm not sure how expensive relative to
>>> function pointer calls.
>>>
>>
>> Thomas,
>>
>> Thanks for that :)  It looks good to me, and appears to work as it
>> should.
>>
>> Ben.
>>
>
> Great.
>
> I have a question, though. (CC'ing Jerome as well)
>
> Seems to me like something is missing from the mem_reserve interface. Let's
> say you have a programmable VRAM aperture and it's full, so you can't honor
> bo map request. You'd then want to traverse a list and call
> unmap_mapping_range() to kill user-space maps and free VRAM aperture space,
> but you can't really do that since you don't have access to the mapping
> range in question...?
>
> /Thomas
>
>

Driver callback can callback into ttm to invalidate other bo mapping
and play with
aperture mapping (when i did the patch i think i did check locking was
ok but things
might have change since then).

The mecanism  of io_mem_reserve & io_mem_free was thought for GPU where you
can change aperture mapping to vram (ie remap any chunk of vram into
the aperture),
thus with having the capacity of invalidating others bo mapping.

I will recheck the code to see if it can still do this from locking
point of view, of course
i never wrote any ttm helper function that would be needed to handle
this (we likely
want to avoid unmapping pinned buffer or especialy kmaped buffer.

Cheers,
Jerome

Re: [PATCH 1/2] drm/ttm: ensure ttm_mem_io_free is called on bo destruction

[Thomas Hellstrom]

On 11/04/2010 01:03 AM, Ben Skeggs wrote:
> From: Ben Skeggs<bskeggs@redhat.com>
>
> Nouveau will start to use ttm_mem_io_reserve to allocate BAR VM space
> for VRAM mappings, and without this call GPU address space gets leaked.
>
> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index dfa163b..c373cf9 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -472,6 +472,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
>   	wake_up_all(&bo->event_queue);
>   	spin_unlock(&glob->lru_lock);
>
> +	ttm_mem_io_free(bo->bdev,&tmp_mem);
>   	ttm_bo_mem_put(bo,&tmp_mem);
>   }
>
>    
Ideally this should be done after the last VMA is closed, but since we 
don't count VMAs currently. This place is just as good.

However, the mem members are strictly protected by bo::reserve, so we 
should place this before we unreserve in the same function, Please use 
drm-next where ttm_bo_mem_put() is placed just before the unreserve as well.


/Thomas