From fcb8f9f20db5adf5966e96eb07341fecc1c16318 Mon Sep 17 00:00:00 2001 From: Jordan Crouse Date: Wed, 16 Dec 2020 12:01:47 -0700 Subject: [PATCH 1/2] msm: kgsl: Stop using memdesc->usermem It shouldn't be illegal for memory descriptors to have multiple virtual memory mappings under certain circumstances. If that is the case tracking a single usermem address for each memdesc no longer makes much sense. Get rid of the memdesc->usermem member and use an atomic counter to track mappings instead. Change-Id: Ic0dedbad31bafcd1019ccc8e68657cb7e3c72727 Signed-off-by: Jordan Crouse --- drivers/gpu/msm/kgsl.c | 53 ++++++++++++++++++++++------------ drivers/gpu/msm/kgsl.h | 9 +++--- drivers/gpu/msm/kgsl_debugfs.c | 12 +++++--- drivers/gpu/msm/kgsl_trace.h | 6 ++-- 4 files changed, 50 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/msm/kgsl.c b/drivers/gpu/msm/kgsl.c index 418ad5d7e46a..d043632bfe7a 100644 --- a/drivers/gpu/msm/kgsl.c +++ b/drivers/gpu/msm/kgsl.c @@ -254,6 +254,7 @@ static struct kgsl_mem_entry *kgsl_mem_entry_create(void) kref_get(&entry->refcount); } + atomic_set(&entry->map_count, 0); return entry; } @@ -2332,7 +2333,7 @@ static int check_vma(unsigned long hostptr, u64 size) return true; } -static int memdesc_sg_virt(struct kgsl_memdesc *memdesc) +static int memdesc_sg_virt(struct kgsl_memdesc *memdesc, unsigned long useraddr) { int ret = 0; long npages = 0, i; @@ -2356,13 +2357,13 @@ static int memdesc_sg_virt(struct kgsl_memdesc *memdesc) } down_read(¤t->mm->mmap_sem); - if (!check_vma(memdesc->useraddr, memdesc->size)) { + if (!check_vma(useraddr, memdesc->size)) { up_read(¤t->mm->mmap_sem); ret = -EFAULT; goto out; } - npages = get_user_pages(memdesc->useraddr, sglen, write, pages, NULL); + npages = get_user_pages(useraddr, sglen, write, pages, NULL); up_read(¤t->mm->mmap_sem); ret = (npages < 0) ? (int)npages : 0; @@ -2400,7 +2401,6 @@ static int kgsl_setup_anon_useraddr(struct kgsl_pagetable *pagetable, entry->memdesc.pagetable = pagetable; entry->memdesc.size = (uint64_t) size; - entry->memdesc.useraddr = hostptr; entry->memdesc.flags |= (uint64_t)KGSL_MEMFLAGS_USERMEM_ADDR; if (kgsl_memdesc_use_cpu_map(&entry->memdesc)) { @@ -2408,15 +2408,15 @@ static int kgsl_setup_anon_useraddr(struct kgsl_pagetable *pagetable, /* Register the address in the database */ ret = kgsl_mmu_set_svm_region(pagetable, - (uint64_t) entry->memdesc.useraddr, (uint64_t) size); + (uint64_t) hostptr, (uint64_t) size); if (ret) return ret; - entry->memdesc.gpuaddr = (uint64_t) entry->memdesc.useraddr; + entry->memdesc.gpuaddr = (uint64_t) hostptr; } - return memdesc_sg_virt(&entry->memdesc); + return memdesc_sg_virt(&entry->memdesc, hostptr); } #ifdef CONFIG_DMA_SHARED_BUFFER @@ -2505,8 +2505,7 @@ static int kgsl_setup_dmabuf_useraddr(struct kgsl_device *device, return ret; } - /* Setup the user addr/cache mode for cache operations */ - entry->memdesc.useraddr = hostptr; + /* Setup the cache mode for cache operations */ _setup_cache_mode(entry, vma); if (kgsl_mmu_has_feature(device, KGSL_MMU_IO_COHERENT) && @@ -3564,7 +3563,12 @@ long kgsl_ioctl_gpumem_get_info(struct kgsl_device_private *dev_priv, param->flags = (unsigned int) entry->memdesc.flags; param->size = (size_t) entry->memdesc.size; param->mmapsize = (size_t) kgsl_memdesc_footprint(&entry->memdesc); - param->useraddr = entry->memdesc.useraddr; + /* + * Entries can have multiple user mappings so thre isn't any one address + * we can report. Plus, the user should already know their mappings, so + * there isn't any value in reporting it back to them. + */ + param->useraddr = 0; kgsl_mem_entry_put(entry); return result; @@ -3589,7 +3593,12 @@ long kgsl_ioctl_gpuobj_info(struct kgsl_device_private *dev_priv, param->flags = entry->memdesc.flags; param->size = entry->memdesc.size; param->va_len = kgsl_memdesc_footprint(&entry->memdesc); - param->va_addr = (uint64_t) entry->memdesc.useraddr; + /* + * Entries can have multiple user mappings so thre isn't any one address + * we can report. Plus, the user should already know their mappings, so + * there isn't any value in reporting it back to them. + */ + param->va_addr = 0; kgsl_mem_entry_put(entry); return 0; @@ -3704,6 +3713,8 @@ static void kgsl_gpumem_vm_open(struct vm_area_struct *vma) if (kgsl_mem_entry_get(entry) == 0) vma->vm_private_data = NULL; + + atomic_inc(&entry->map_count); } static vm_fault_t @@ -3727,9 +3738,13 @@ kgsl_gpumem_vm_close(struct vm_area_struct *vma) if (!entry) return; - entry->memdesc.useraddr = 0; - atomic64_sub(entry->memdesc.mapsize, &entry->priv->gpumem_mapped); - entry->memdesc.mapsize = 0; + /* + * Remove the memdesc from the mapped stat once all the mappings have + * gone away + */ + if (!atomic_dec_return(&entry->map_count)) + atomic64_sub(entry->memdesc.size, &entry->priv->gpumem_mapped); + kgsl_mem_entry_put(entry); } @@ -3761,7 +3776,8 @@ get_mmap_entry(struct kgsl_process_private *private, goto err_put; } - if (entry->memdesc.useraddr != 0) { + /* Don't allow ourselves to remap user memory */ + if (entry->memdesc.flags & KGSL_MEMFLAGS_USERMEM_ADDR) { ret = -EBUSY; goto err_put; } @@ -4018,12 +4034,11 @@ static int kgsl_mmap(struct file *file, struct vm_area_struct *vma) vma->vm_file = file; - entry->memdesc.useraddr = vma->vm_start; + atomic64_add(entry->memdesc.size, &entry->priv->gpumem_mapped); - entry->memdesc.mapsize += entry->memdesc.size; - atomic64_add(entry->memdesc.mapsize, &entry->priv->gpumem_mapped); + atomic_inc(&entry->map_count); - trace_kgsl_mem_mmap(entry); + trace_kgsl_mem_mmap(entry, vma->vm_start); return 0; } diff --git a/drivers/gpu/msm/kgsl.h b/drivers/gpu/msm/kgsl.h index 6c088450ec83..a9bd5bfdff04 100644 --- a/drivers/gpu/msm/kgsl.h +++ b/drivers/gpu/msm/kgsl.h @@ -184,11 +184,9 @@ struct kgsl_memdesc_ops { * @pagetable: Pointer to the pagetable that the object is mapped in * @hostptr: Kernel virtual address * @hostptr_count: Number of threads using hostptr - * @useraddr: User virtual address (if applicable) * @gpuaddr: GPU virtual address * @physaddr: Physical address of the memory object * @size: Size of the memory object - * @mapsize: Size of memory mapped in userspace * @priv: Internal flags and settings * @sgt: Scatter gather table for allocated pages * @ops: Function hooks for the memdesc memory type @@ -202,11 +200,9 @@ struct kgsl_memdesc { struct kgsl_pagetable *pagetable; void *hostptr; unsigned int hostptr_count; - unsigned long useraddr; uint64_t gpuaddr; phys_addr_t physaddr; uint64_t size; - uint64_t mapsize; unsigned int priv; struct sg_table *sgt; const struct kgsl_memdesc_ops *ops; @@ -271,6 +267,11 @@ struct kgsl_mem_entry { * userspace */ u64 mapped; + /** + * @map_count: Count how many vmas this object is mapped in - used for + * debugfs accounting + */ + atomic_t map_count; }; struct kgsl_device_private; diff --git a/drivers/gpu/msm/kgsl_debugfs.c b/drivers/gpu/msm/kgsl_debugfs.c index 8ee3ccdd796c..937cbb669485 100644 --- a/drivers/gpu/msm/kgsl_debugfs.c +++ b/drivers/gpu/msm/kgsl_debugfs.c @@ -184,7 +184,8 @@ static int print_mem_entry(void *data, void *ptr) flags[3] = get_alignflag(m); flags[4] = get_cacheflag(m); flags[5] = kgsl_memdesc_use_cpu_map(m) ? 'p' : '-'; - flags[6] = (m->useraddr) ? 'Y' : 'N'; + /* Show Y if at least one vma has this entry mapped (could be multiple) */ + flags[6] = atomic_read(&entry->map_count) ? 'Y' : 'N'; flags[7] = kgsl_memdesc_is_secured(m) ? 's' : '-'; flags[8] = '-'; flags[9] = '\0'; @@ -197,10 +198,13 @@ static int print_mem_entry(void *data, void *ptr) seq_printf(s, "%pK %pK %16llu %5d %9s %10s %16s %5d %16llu %6d %6d", (uint64_t *)(uintptr_t) m->gpuaddr, - (unsigned long *) m->useraddr, - m->size, entry->id, flags, + /* + * Show zero for the useraddr - we can't reliably track + * that value for multiple vmas anyway + */ + 0, m->size, entry->id, flags, memtype_str(usermem_type), - usage, (m->sgt ? m->sgt->nents : 0), m->mapsize, + usage, (m->sgt ? m->sgt->nents : 0), m->size, egl_surface_count, egl_image_count); if (entry->metadata[0] != 0) diff --git a/drivers/gpu/msm/kgsl_trace.h b/drivers/gpu/msm/kgsl_trace.h index c526eab603c7..320330e456ca 100644 --- a/drivers/gpu/msm/kgsl_trace.h +++ b/drivers/gpu/msm/kgsl_trace.h @@ -456,9 +456,9 @@ TRACE_EVENT(kgsl_mem_alloc, TRACE_EVENT(kgsl_mem_mmap, - TP_PROTO(struct kgsl_mem_entry *mem_entry), + TP_PROTO(struct kgsl_mem_entry *mem_entry, unsigned long useraddr), - TP_ARGS(mem_entry), + TP_ARGS(mem_entry, useraddr), TP_STRUCT__entry( __field(unsigned long, useraddr) @@ -470,7 +470,7 @@ TRACE_EVENT(kgsl_mem_mmap, ), TP_fast_assign( - __entry->useraddr = mem_entry->memdesc.useraddr; + __entry->useraddr = useraddr; __entry->gpuaddr = mem_entry->memdesc.gpuaddr; __entry->size = mem_entry->memdesc.size; kgsl_get_memory_usage(__entry->usage, sizeof(__entry->usage), From b566801d63d92f6b18fb44a3b9661fff46c69933 Mon Sep 17 00:00:00 2001 From: Jordan Crouse Date: Wed, 16 Dec 2020 13:01:13 -0700 Subject: [PATCH 2/2] msm: kgsl: Protect the memdesc->gpuaddr in SVM use cases When SVM is being used there can only be one GPU address assigned to the memory descriptor. Don't allow the GPU address to be changed after it has been negotiated the first time by a process. Change-Id: Ic0dedbad2a1b3ccdc2c1598a6c501b2be288d64e Signed-off-by: Jordan Crouse --- drivers/gpu/msm/kgsl.c | 31 ++++++++++++++++++++++++++++--- drivers/gpu/msm/kgsl.h | 5 +++++ drivers/gpu/msm/kgsl_iommu.c | 5 +++++ drivers/gpu/msm/kgsl_mmu.c | 9 ++++++++- drivers/gpu/msm/kgsl_sharedmem.c | 2 ++ 5 files changed, 48 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/msm/kgsl.c b/drivers/gpu/msm/kgsl.c index d043632bfe7a..3e600c2bc7bc 100644 --- a/drivers/gpu/msm/kgsl.c +++ b/drivers/gpu/msm/kgsl.c @@ -3810,19 +3810,34 @@ static unsigned long _gpu_set_svm_region(struct kgsl_process_private *private, { int ret; + /* + * Protect access to the gpuaddr here to prevent multiple vmas from + * trying to map a SVM region at the same time + */ + spin_lock(&entry->memdesc.lock); + + if (entry->memdesc.gpuaddr) { + spin_unlock(&entry->memdesc.lock); + return (unsigned long) -EBUSY; + } + ret = kgsl_mmu_set_svm_region(private->pagetable, (uint64_t) addr, (uint64_t) size); - if (ret != 0) - return ret; + if (ret != 0) { + spin_unlock(&entry->memdesc.lock); + return (unsigned long) ret; + } entry->memdesc.gpuaddr = (uint64_t) addr; + spin_unlock(&entry->memdesc.lock); + entry->memdesc.pagetable = private->pagetable; ret = kgsl_mmu_map(private->pagetable, &entry->memdesc); if (ret) { kgsl_mmu_put_gpuaddr(&entry->memdesc); - return ret; + return (unsigned long) ret; } kgsl_memfree_purge(private->pagetable, entry->memdesc.gpuaddr, @@ -3914,6 +3929,16 @@ static unsigned long get_svm_unmapped_area(struct file *file, ret = set_svm_area(file, entry, iova, len, flags); if (!IS_ERR_VALUE(ret)) return ret; + + /* + * set_svm_area will return -EBUSY if we tried to set up + * SVM on an object that already has a GPU address. If + * that happens don't bother walking the rest of the + * region + */ + if ((long) ret == -EBUSY) + return -EBUSY; + } iova = kgsl_mmu_find_svm_region(private->pagetable, diff --git a/drivers/gpu/msm/kgsl.h b/drivers/gpu/msm/kgsl.h index a9bd5bfdff04..9c701142d211 100644 --- a/drivers/gpu/msm/kgsl.h +++ b/drivers/gpu/msm/kgsl.h @@ -211,6 +211,11 @@ struct kgsl_memdesc { unsigned long attrs; struct page **pages; unsigned int page_count; + /* + * @lock: Spinlock to protect the gpuaddr from being accessed by + * multiple entities trying to map the same SVM region at once + */ + spinlock_t lock; }; /** diff --git a/drivers/gpu/msm/kgsl_iommu.c b/drivers/gpu/msm/kgsl_iommu.c index a1a4e5d151d0..ace33e5ef7c1 100644 --- a/drivers/gpu/msm/kgsl_iommu.c +++ b/drivers/gpu/msm/kgsl_iommu.c @@ -2227,6 +2227,11 @@ static int kgsl_iommu_get_gpuaddr(struct kgsl_pagetable *pagetable, goto out; } + /* + * This path is only called in a non-SVM path with locks so we can be + * sure we aren't racing with anybody so we don't need to worry about + * taking the lock + */ ret = _insert_gpuaddr(pagetable, addr, size); if (ret == 0) { memdesc->gpuaddr = addr; diff --git a/drivers/gpu/msm/kgsl_mmu.c b/drivers/gpu/msm/kgsl_mmu.c index b51ff9dc4e30..52e76bd30a35 100644 --- a/drivers/gpu/msm/kgsl_mmu.c +++ b/drivers/gpu/msm/kgsl_mmu.c @@ -409,10 +409,17 @@ void kgsl_mmu_put_gpuaddr(struct kgsl_memdesc *memdesc) if (PT_OP_VALID(pagetable, put_gpuaddr) && (unmap_fail == 0)) pagetable->pt_ops->put_gpuaddr(memdesc); + memdesc->pagetable = NULL; + + + /* + * If SVM tries to take a GPU address it will lose the race until the + * gpuaddr returns to zero so we shouldn't need to worry about taking a + * lock here + */ if (!kgsl_memdesc_is_global(memdesc)) memdesc->gpuaddr = 0; - memdesc->pagetable = NULL; } /** diff --git a/drivers/gpu/msm/kgsl_sharedmem.c b/drivers/gpu/msm/kgsl_sharedmem.c index 1d1558082afb..b37079604b64 100644 --- a/drivers/gpu/msm/kgsl_sharedmem.c +++ b/drivers/gpu/msm/kgsl_sharedmem.c @@ -609,6 +609,8 @@ void kgsl_memdesc_init(struct kgsl_device *device, (memdesc->flags & KGSL_MEMALIGN_MASK) >> KGSL_MEMALIGN_SHIFT, ilog2(PAGE_SIZE)); kgsl_memdesc_set_align(memdesc, align); + + spin_lock_init(&memdesc->lock); } void kgsl_sharedmem_free(struct kgsl_memdesc *memdesc)