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); }
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; };
/**
On 11/04/2010 01:03 AM, Ben Skeggs wrote:
From: Ben Skeggsbskeggs@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 Skeggsbskeggs@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;}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 .
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 Skeggsbskeggs@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 Skeggsbskeggs@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
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 Skeggsbskeggs@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 Skeggsbskeggs@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
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 Skeggsbskeggs@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 Skeggsbskeggs@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
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
On 11/04/2010 01:03 AM, Ben Skeggs wrote:
From: Ben Skeggsbskeggs@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 Skeggsbskeggs@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
dri-devel@lists.freedesktop.org
-
Ben Skeggs -
Jerome Glisse -
Thomas Hellstrom