* [PATCH] drm/amdkfd: avoid conflicting address mappings
@ 2021-07-19 21:18 Alex Sierra
2021-07-29 15:47 ` Felix Kuehling
2021-09-29 23:35 ` Mike Lothian
0 siblings, 2 replies; 7+ messages in thread
From: Alex Sierra @ 2021-07-19 21:18 UTC (permalink / raw)
To: amd-gfx; +Cc: Alex Sierra
[Why]
Avoid conflict with address ranges mapped by SVM
mechanism that try to be allocated again through
ioctl_alloc in the same process. And viceversa.
[How]
For ioctl_alloc_memory_of_gpu allocations
Check if the address range passed into ioctl memory
alloc does not exist already in the kfd_process
svms->objects interval tree.
For SVM allocations
Look for the address range into the interval tree VA from
the VM inside of each pdds used in a kfd_process.
Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 ++++
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 79 +++++++++++++++++++-----
2 files changed, 75 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 67541c30327a..f39baaa22a62 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1251,6 +1251,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
struct kfd_process_device *pdd;
void *mem;
struct kfd_dev *dev;
+ struct svm_range_list *svms = &p->svms;
int idr_handle;
long err;
uint64_t offset = args->mmap_offset;
@@ -1259,6 +1260,18 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
if (args->size == 0)
return -EINVAL;
+#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
+ mutex_lock(&svms->lock);
+ if (interval_tree_iter_first(&svms->objects,
+ args->va_addr >> PAGE_SHIFT,
+ (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
+ pr_err("Address: 0x%llx already allocated by SVM\n",
+ args->va_addr);
+ mutex_unlock(&svms->lock);
+ return -EADDRINUSE;
+ }
+ mutex_unlock(&svms->lock);
+#endif
dev = kfd_device_by_id(args->gpu_id);
if (!dev)
return -EINVAL;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 31f3f24cef6a..043ee0467916 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2581,9 +2581,54 @@ int svm_range_list_init(struct kfd_process *p)
return 0;
}
+/**
+ * svm_range_is_vm_bo_mapped - check if virtual address range mapped already
+ * @p: current kfd_process
+ * @start: range start address, in pages
+ * @last: range last address, in pages
+ *
+ * The purpose is to avoid virtual address ranges already allocated by
+ * kfd_ioctl_alloc_memory_of_gpu ioctl.
+ * It looks for each pdd in the kfd_process.
+ *
+ * Context: Process context
+ *
+ * Return 0 - OK, if the range is not mapped.
+ * Otherwise error code:
+ * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
+ * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
+ * a signal. Release all buffer reservations and return to user-space.
+ */
+static int
+svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start, uint64_t last)
+{
+ uint32_t i;
+ int r;
+
+ for (i = 0; i < p->n_pdds; i++) {
+ struct amdgpu_vm *vm;
+
+ if (!p->pdds[i]->drm_priv)
+ continue;
+
+ vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
+ r = amdgpu_bo_reserve(vm->root.bo, false);
+ if (r)
+ return r;
+ if (interval_tree_iter_first(&vm->va, start, last)) {
+ pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
+ amdgpu_bo_unreserve(vm->root.bo);
+ return -EADDRINUSE;
+ }
+ amdgpu_bo_unreserve(vm->root.bo);
+ }
+
+ return 0;
+}
+
/**
* svm_range_is_valid - check if virtual address range is valid
- * @mm: current process mm_struct
+ * @mm: current kfd_process
* @start: range start address, in pages
* @size: range size, in pages
*
@@ -2592,28 +2637,27 @@ int svm_range_list_init(struct kfd_process *p)
* Context: Process context
*
* Return:
- * true - valid svm range
- * false - invalid svm range
+ * 0 - OK, otherwise error code
*/
-static bool
-svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t size)
+static int
+svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
{
const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
struct vm_area_struct *vma;
unsigned long end;
+ unsigned long start_unchg = start;
start <<= PAGE_SHIFT;
end = start + (size << PAGE_SHIFT);
-
do {
- vma = find_vma(mm, start);
+ vma = find_vma(p->mm, start);
if (!vma || start < vma->vm_start ||
(vma->vm_flags & device_vma))
- return false;
+ return -EFAULT;
start = min(end, vma->vm_end);
} while (start < end);
- return true;
+ return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1) >> PAGE_SHIFT);
}
/**
@@ -2913,9 +2957,9 @@ svm_range_set_attr(struct kfd_process *p, uint64_t start, uint64_t size,
svm_range_list_lock_and_flush_work(svms, mm);
- if (!svm_range_is_valid(mm, start, size)) {
- pr_debug("invalid range\n");
- r = -EFAULT;
+ r = svm_range_is_valid(p, start, size);
+ if (r) {
+ pr_debug("invalid range r=%d\n", r);
mmap_write_unlock(mm);
goto out;
}
@@ -3016,17 +3060,18 @@ svm_range_get_attr(struct kfd_process *p, uint64_t start, uint64_t size,
uint32_t flags = 0xffffffff;
int gpuidx;
uint32_t i;
+ int r = 0;
pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n", &p->svms, start,
start + size - 1, nattr);
mmap_read_lock(mm);
- if (!svm_range_is_valid(mm, start, size)) {
- pr_debug("invalid range\n");
- mmap_read_unlock(mm);
- return -EINVAL;
- }
+ r = svm_range_is_valid(p, start, size);
mmap_read_unlock(mm);
+ if (r) {
+ pr_debug("invalid range r=%d\n", r);
+ return r;
+ }
for (i = 0; i < nattr; i++) {
switch (attrs[i].type) {
--
2.32.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdkfd: avoid conflicting address mappings
2021-07-19 21:18 [PATCH] drm/amdkfd: avoid conflicting address mappings Alex Sierra
@ 2021-07-29 15:47 ` Felix Kuehling
2021-09-29 23:35 ` Mike Lothian
1 sibling, 0 replies; 7+ messages in thread
From: Felix Kuehling @ 2021-07-29 15:47 UTC (permalink / raw)
To: Alex Sierra, amd-gfx
Am 2021-07-19 um 5:18 p.m. schrieb Alex Sierra:
> [Why]
> Avoid conflict with address ranges mapped by SVM
> mechanism that try to be allocated again through
> ioctl_alloc in the same process. And viceversa.
>
> [How]
> For ioctl_alloc_memory_of_gpu allocations
> Check if the address range passed into ioctl memory
> alloc does not exist already in the kfd_process
> svms->objects interval tree.
>
> For SVM allocations
> Look for the address range into the interval tree VA from
> the VM inside of each pdds used in a kfd_process.
>
> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
Two nitpicks inline. With that fixed, the patch is
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 ++++
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 79 +++++++++++++++++++-----
> 2 files changed, 75 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 67541c30327a..f39baaa22a62 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1251,6 +1251,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> struct kfd_process_device *pdd;
> void *mem;
> struct kfd_dev *dev;
> + struct svm_range_list *svms = &p->svms;
> int idr_handle;
> long err;
> uint64_t offset = args->mmap_offset;
> @@ -1259,6 +1260,18 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> if (args->size == 0)
> return -EINVAL;
>
> +#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
> + mutex_lock(&svms->lock);
> + if (interval_tree_iter_first(&svms->objects,
> + args->va_addr >> PAGE_SHIFT,
> + (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
> + pr_err("Address: 0x%llx already allocated by SVM\n",
> + args->va_addr);
> + mutex_unlock(&svms->lock);
> + return -EADDRINUSE;
> + }
> + mutex_unlock(&svms->lock);
> +#endif
> dev = kfd_device_by_id(args->gpu_id);
> if (!dev)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 31f3f24cef6a..043ee0467916 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2581,9 +2581,54 @@ int svm_range_list_init(struct kfd_process *p)
> return 0;
> }
>
> +/**
> + * svm_range_is_vm_bo_mapped - check if virtual address range mapped already
I find the function name a bit confusing. The name suggests something
about a BO, but the description only talks about address ranges. Also,
there is no BO parameter.
Maybe a better name would be svm_range_check_vm.
> + * @p: current kfd_process
> + * @start: range start address, in pages
> + * @last: range last address, in pages
> + *
> + * The purpose is to avoid virtual address ranges already allocated by
> + * kfd_ioctl_alloc_memory_of_gpu ioctl.
> + * It looks for each pdd in the kfd_process.
> + *
> + * Context: Process context
> + *
> + * Return 0 - OK, if the range is not mapped.
> + * Otherwise error code:
> + * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
> + * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
> + * a signal. Release all buffer reservations and return to user-space.
> + */
> +static int
> +svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start, uint64_t last)
> +{
> + uint32_t i;
> + int r;
> +
> + for (i = 0; i < p->n_pdds; i++) {
> + struct amdgpu_vm *vm;
> +
> + if (!p->pdds[i]->drm_priv)
> + continue;
> +
> + vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
> + r = amdgpu_bo_reserve(vm->root.bo, false);
> + if (r)
> + return r;
> + if (interval_tree_iter_first(&vm->va, start, last)) {
> + pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
> + amdgpu_bo_unreserve(vm->root.bo);
> + return -EADDRINUSE;
> + }
> + amdgpu_bo_unreserve(vm->root.bo);
> + }
> +
> + return 0;
> +}
> +
> /**
> * svm_range_is_valid - check if virtual address range is valid
> - * @mm: current process mm_struct
> + * @mm: current kfd_process
Please fix the variable name here: mm -> p.
Regards,
Felix
> * @start: range start address, in pages
> * @size: range size, in pages
> *
> @@ -2592,28 +2637,27 @@ int svm_range_list_init(struct kfd_process *p)
> * Context: Process context
> *
> * Return:
> - * true - valid svm range
> - * false - invalid svm range
> + * 0 - OK, otherwise error code
> */
> -static bool
> -svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t size)
> +static int
> +svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
> {
> const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
> struct vm_area_struct *vma;
> unsigned long end;
> + unsigned long start_unchg = start;
>
> start <<= PAGE_SHIFT;
> end = start + (size << PAGE_SHIFT);
> -
> do {
> - vma = find_vma(mm, start);
> + vma = find_vma(p->mm, start);
> if (!vma || start < vma->vm_start ||
> (vma->vm_flags & device_vma))
> - return false;
> + return -EFAULT;
> start = min(end, vma->vm_end);
> } while (start < end);
>
> - return true;
> + return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1) >> PAGE_SHIFT);
> }
>
> /**
> @@ -2913,9 +2957,9 @@ svm_range_set_attr(struct kfd_process *p, uint64_t start, uint64_t size,
>
> svm_range_list_lock_and_flush_work(svms, mm);
>
> - if (!svm_range_is_valid(mm, start, size)) {
> - pr_debug("invalid range\n");
> - r = -EFAULT;
> + r = svm_range_is_valid(p, start, size);
> + if (r) {
> + pr_debug("invalid range r=%d\n", r);
> mmap_write_unlock(mm);
> goto out;
> }
> @@ -3016,17 +3060,18 @@ svm_range_get_attr(struct kfd_process *p, uint64_t start, uint64_t size,
> uint32_t flags = 0xffffffff;
> int gpuidx;
> uint32_t i;
> + int r = 0;
>
> pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n", &p->svms, start,
> start + size - 1, nattr);
>
> mmap_read_lock(mm);
> - if (!svm_range_is_valid(mm, start, size)) {
> - pr_debug("invalid range\n");
> - mmap_read_unlock(mm);
> - return -EINVAL;
> - }
> + r = svm_range_is_valid(p, start, size);
> mmap_read_unlock(mm);
> + if (r) {
> + pr_debug("invalid range r=%d\n", r);
> + return r;
> + }
>
> for (i = 0; i < nattr; i++) {
> switch (attrs[i].type) {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdkfd: avoid conflicting address mappings
2021-07-19 21:18 [PATCH] drm/amdkfd: avoid conflicting address mappings Alex Sierra
2021-07-29 15:47 ` Felix Kuehling
@ 2021-09-29 23:35 ` Mike Lothian
2021-09-30 2:15 ` Felix Kuehling
1 sibling, 1 reply; 7+ messages in thread
From: Mike Lothian @ 2021-09-29 23:35 UTC (permalink / raw)
To: Alex Sierra; +Cc: amd-gfx list
Hi
This patch is causing a compile failure for me
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_chardev.c:1254:25: error:
unused variable 'svms' [-Werror,-Wunused-variable]
struct svm_range_list *svms = &p->svms;
^
1 error generated.
I'll turn off Werror
On Mon, 19 Jul 2021 at 22:19, Alex Sierra <alex.sierra@amd.com> wrote:
>
> [Why]
> Avoid conflict with address ranges mapped by SVM
> mechanism that try to be allocated again through
> ioctl_alloc in the same process. And viceversa.
>
> [How]
> For ioctl_alloc_memory_of_gpu allocations
> Check if the address range passed into ioctl memory
> alloc does not exist already in the kfd_process
> svms->objects interval tree.
>
> For SVM allocations
> Look for the address range into the interval tree VA from
> the VM inside of each pdds used in a kfd_process.
>
> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 ++++
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 79 +++++++++++++++++++-----
> 2 files changed, 75 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 67541c30327a..f39baaa22a62 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1251,6 +1251,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> struct kfd_process_device *pdd;
> void *mem;
> struct kfd_dev *dev;
> + struct svm_range_list *svms = &p->svms;
> int idr_handle;
> long err;
> uint64_t offset = args->mmap_offset;
> @@ -1259,6 +1260,18 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> if (args->size == 0)
> return -EINVAL;
>
> +#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
> + mutex_lock(&svms->lock);
> + if (interval_tree_iter_first(&svms->objects,
> + args->va_addr >> PAGE_SHIFT,
> + (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
> + pr_err("Address: 0x%llx already allocated by SVM\n",
> + args->va_addr);
> + mutex_unlock(&svms->lock);
> + return -EADDRINUSE;
> + }
> + mutex_unlock(&svms->lock);
> +#endif
> dev = kfd_device_by_id(args->gpu_id);
> if (!dev)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 31f3f24cef6a..043ee0467916 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2581,9 +2581,54 @@ int svm_range_list_init(struct kfd_process *p)
> return 0;
> }
>
> +/**
> + * svm_range_is_vm_bo_mapped - check if virtual address range mapped already
> + * @p: current kfd_process
> + * @start: range start address, in pages
> + * @last: range last address, in pages
> + *
> + * The purpose is to avoid virtual address ranges already allocated by
> + * kfd_ioctl_alloc_memory_of_gpu ioctl.
> + * It looks for each pdd in the kfd_process.
> + *
> + * Context: Process context
> + *
> + * Return 0 - OK, if the range is not mapped.
> + * Otherwise error code:
> + * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
> + * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
> + * a signal. Release all buffer reservations and return to user-space.
> + */
> +static int
> +svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start, uint64_t last)
> +{
> + uint32_t i;
> + int r;
> +
> + for (i = 0; i < p->n_pdds; i++) {
> + struct amdgpu_vm *vm;
> +
> + if (!p->pdds[i]->drm_priv)
> + continue;
> +
> + vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
> + r = amdgpu_bo_reserve(vm->root.bo, false);
> + if (r)
> + return r;
> + if (interval_tree_iter_first(&vm->va, start, last)) {
> + pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
> + amdgpu_bo_unreserve(vm->root.bo);
> + return -EADDRINUSE;
> + }
> + amdgpu_bo_unreserve(vm->root.bo);
> + }
> +
> + return 0;
> +}
> +
> /**
> * svm_range_is_valid - check if virtual address range is valid
> - * @mm: current process mm_struct
> + * @mm: current kfd_process
> * @start: range start address, in pages
> * @size: range size, in pages
> *
> @@ -2592,28 +2637,27 @@ int svm_range_list_init(struct kfd_process *p)
> * Context: Process context
> *
> * Return:
> - * true - valid svm range
> - * false - invalid svm range
> + * 0 - OK, otherwise error code
> */
> -static bool
> -svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t size)
> +static int
> +svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
> {
> const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
> struct vm_area_struct *vma;
> unsigned long end;
> + unsigned long start_unchg = start;
>
> start <<= PAGE_SHIFT;
> end = start + (size << PAGE_SHIFT);
> -
> do {
> - vma = find_vma(mm, start);
> + vma = find_vma(p->mm, start);
> if (!vma || start < vma->vm_start ||
> (vma->vm_flags & device_vma))
> - return false;
> + return -EFAULT;
> start = min(end, vma->vm_end);
> } while (start < end);
>
> - return true;
> + return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1) >> PAGE_SHIFT);
> }
>
> /**
> @@ -2913,9 +2957,9 @@ svm_range_set_attr(struct kfd_process *p, uint64_t start, uint64_t size,
>
> svm_range_list_lock_and_flush_work(svms, mm);
>
> - if (!svm_range_is_valid(mm, start, size)) {
> - pr_debug("invalid range\n");
> - r = -EFAULT;
> + r = svm_range_is_valid(p, start, size);
> + if (r) {
> + pr_debug("invalid range r=%d\n", r);
> mmap_write_unlock(mm);
> goto out;
> }
> @@ -3016,17 +3060,18 @@ svm_range_get_attr(struct kfd_process *p, uint64_t start, uint64_t size,
> uint32_t flags = 0xffffffff;
> int gpuidx;
> uint32_t i;
> + int r = 0;
>
> pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n", &p->svms, start,
> start + size - 1, nattr);
>
> mmap_read_lock(mm);
> - if (!svm_range_is_valid(mm, start, size)) {
> - pr_debug("invalid range\n");
> - mmap_read_unlock(mm);
> - return -EINVAL;
> - }
> + r = svm_range_is_valid(p, start, size);
> mmap_read_unlock(mm);
> + if (r) {
> + pr_debug("invalid range r=%d\n", r);
> + return r;
> + }
>
> for (i = 0; i < nattr; i++) {
> switch (attrs[i].type) {
> --
> 2.32.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdkfd: avoid conflicting address mappings
2021-09-29 23:35 ` Mike Lothian
@ 2021-09-30 2:15 ` Felix Kuehling
2021-09-30 16:32 ` Sierra Guiza, Alejandro (Alex)
0 siblings, 1 reply; 7+ messages in thread
From: Felix Kuehling @ 2021-09-30 2:15 UTC (permalink / raw)
To: Mike Lothian, Alex Sierra; +Cc: amd-gfx list
On 2021-09-29 7:35 p.m., Mike Lothian wrote:
> Hi
>
> This patch is causing a compile failure for me
>
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_chardev.c:1254:25: error:
> unused variable 'svms' [-Werror,-Wunused-variable]
> struct svm_range_list *svms = &p->svms;
> ^
> 1 error generated.
>
> I'll turn off Werror
I guess the struct svm_range_list *svms declaration should be under #if
IS_ENABLED(CONFIG_HSA_AMD_SVM). Alternatively, we could get rid of it
and use p->svms directly (it's used in 3 places in that function).
Would you like to propose a patch for that?
Thanks,
Felix
>
> On Mon, 19 Jul 2021 at 22:19, Alex Sierra <alex.sierra@amd.com> wrote:
>> [Why]
>> Avoid conflict with address ranges mapped by SVM
>> mechanism that try to be allocated again through
>> ioctl_alloc in the same process. And viceversa.
>>
>> [How]
>> For ioctl_alloc_memory_of_gpu allocations
>> Check if the address range passed into ioctl memory
>> alloc does not exist already in the kfd_process
>> svms->objects interval tree.
>>
>> For SVM allocations
>> Look for the address range into the interval tree VA from
>> the VM inside of each pdds used in a kfd_process.
>>
>> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 ++++
>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 79 +++++++++++++++++++-----
>> 2 files changed, 75 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 67541c30327a..f39baaa22a62 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -1251,6 +1251,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>> struct kfd_process_device *pdd;
>> void *mem;
>> struct kfd_dev *dev;
>> + struct svm_range_list *svms = &p->svms;
>> int idr_handle;
>> long err;
>> uint64_t offset = args->mmap_offset;
>> @@ -1259,6 +1260,18 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>> if (args->size == 0)
>> return -EINVAL;
>>
>> +#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
>> + mutex_lock(&svms->lock);
>> + if (interval_tree_iter_first(&svms->objects,
>> + args->va_addr >> PAGE_SHIFT,
>> + (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
>> + pr_err("Address: 0x%llx already allocated by SVM\n",
>> + args->va_addr);
>> + mutex_unlock(&svms->lock);
>> + return -EADDRINUSE;
>> + }
>> + mutex_unlock(&svms->lock);
>> +#endif
>> dev = kfd_device_by_id(args->gpu_id);
>> if (!dev)
>> return -EINVAL;
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index 31f3f24cef6a..043ee0467916 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -2581,9 +2581,54 @@ int svm_range_list_init(struct kfd_process *p)
>> return 0;
>> }
>>
>> +/**
>> + * svm_range_is_vm_bo_mapped - check if virtual address range mapped already
>> + * @p: current kfd_process
>> + * @start: range start address, in pages
>> + * @last: range last address, in pages
>> + *
>> + * The purpose is to avoid virtual address ranges already allocated by
>> + * kfd_ioctl_alloc_memory_of_gpu ioctl.
>> + * It looks for each pdd in the kfd_process.
>> + *
>> + * Context: Process context
>> + *
>> + * Return 0 - OK, if the range is not mapped.
>> + * Otherwise error code:
>> + * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
>> + * -ERESTARTSYS - A wait for the buffer to become unreserved was interrupted by
>> + * a signal. Release all buffer reservations and return to user-space.
>> + */
>> +static int
>> +svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start, uint64_t last)
>> +{
>> + uint32_t i;
>> + int r;
>> +
>> + for (i = 0; i < p->n_pdds; i++) {
>> + struct amdgpu_vm *vm;
>> +
>> + if (!p->pdds[i]->drm_priv)
>> + continue;
>> +
>> + vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
>> + r = amdgpu_bo_reserve(vm->root.bo, false);
>> + if (r)
>> + return r;
>> + if (interval_tree_iter_first(&vm->va, start, last)) {
>> + pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
>> + amdgpu_bo_unreserve(vm->root.bo);
>> + return -EADDRINUSE;
>> + }
>> + amdgpu_bo_unreserve(vm->root.bo);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * svm_range_is_valid - check if virtual address range is valid
>> - * @mm: current process mm_struct
>> + * @mm: current kfd_process
>> * @start: range start address, in pages
>> * @size: range size, in pages
>> *
>> @@ -2592,28 +2637,27 @@ int svm_range_list_init(struct kfd_process *p)
>> * Context: Process context
>> *
>> * Return:
>> - * true - valid svm range
>> - * false - invalid svm range
>> + * 0 - OK, otherwise error code
>> */
>> -static bool
>> -svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t size)
>> +static int
>> +svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
>> {
>> const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
>> struct vm_area_struct *vma;
>> unsigned long end;
>> + unsigned long start_unchg = start;
>>
>> start <<= PAGE_SHIFT;
>> end = start + (size << PAGE_SHIFT);
>> -
>> do {
>> - vma = find_vma(mm, start);
>> + vma = find_vma(p->mm, start);
>> if (!vma || start < vma->vm_start ||
>> (vma->vm_flags & device_vma))
>> - return false;
>> + return -EFAULT;
>> start = min(end, vma->vm_end);
>> } while (start < end);
>>
>> - return true;
>> + return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1) >> PAGE_SHIFT);
>> }
>>
>> /**
>> @@ -2913,9 +2957,9 @@ svm_range_set_attr(struct kfd_process *p, uint64_t start, uint64_t size,
>>
>> svm_range_list_lock_and_flush_work(svms, mm);
>>
>> - if (!svm_range_is_valid(mm, start, size)) {
>> - pr_debug("invalid range\n");
>> - r = -EFAULT;
>> + r = svm_range_is_valid(p, start, size);
>> + if (r) {
>> + pr_debug("invalid range r=%d\n", r);
>> mmap_write_unlock(mm);
>> goto out;
>> }
>> @@ -3016,17 +3060,18 @@ svm_range_get_attr(struct kfd_process *p, uint64_t start, uint64_t size,
>> uint32_t flags = 0xffffffff;
>> int gpuidx;
>> uint32_t i;
>> + int r = 0;
>>
>> pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n", &p->svms, start,
>> start + size - 1, nattr);
>>
>> mmap_read_lock(mm);
>> - if (!svm_range_is_valid(mm, start, size)) {
>> - pr_debug("invalid range\n");
>> - mmap_read_unlock(mm);
>> - return -EINVAL;
>> - }
>> + r = svm_range_is_valid(p, start, size);
>> mmap_read_unlock(mm);
>> + if (r) {
>> + pr_debug("invalid range r=%d\n", r);
>> + return r;
>> + }
>>
>> for (i = 0; i < nattr; i++) {
>> switch (attrs[i].type) {
>> --
>> 2.32.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdkfd: avoid conflicting address mappings
2021-09-30 2:15 ` Felix Kuehling
@ 2021-09-30 16:32 ` Sierra Guiza, Alejandro (Alex)
0 siblings, 0 replies; 7+ messages in thread
From: Sierra Guiza, Alejandro (Alex) @ 2021-09-30 16:32 UTC (permalink / raw)
To: Felix Kuehling, Mike Lothian; +Cc: amd-gfx list
On 9/29/2021 9:15 PM, Felix Kuehling wrote:
> On 2021-09-29 7:35 p.m., Mike Lothian wrote:
>> Hi
>>
>> This patch is causing a compile failure for me
>>
>> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_chardev.c:1254:25: error:
>> unused variable 'svms' [-Werror,-Wunused-variable]
>> struct svm_range_list *svms = &p->svms;
>> ^
>> 1 error generated.
>>
>> I'll turn off Werror
> I guess the struct svm_range_list *svms declaration should be under
> #if IS_ENABLED(CONFIG_HSA_AMD_SVM). Alternatively, we could get rid of
> it and use p->svms directly (it's used in 3 places in that function).
>
> Would you like to propose a patch for that?
I have submitted the patch that fix this for review
Regards,
Alex Sierra
>
> Thanks,
> Felix
>
>
>>
>> On Mon, 19 Jul 2021 at 22:19, Alex Sierra <alex.sierra@amd.com> wrote:
>>> [Why]
>>> Avoid conflict with address ranges mapped by SVM
>>> mechanism that try to be allocated again through
>>> ioctl_alloc in the same process. And viceversa.
>>>
>>> [How]
>>> For ioctl_alloc_memory_of_gpu allocations
>>> Check if the address range passed into ioctl memory
>>> alloc does not exist already in the kfd_process
>>> svms->objects interval tree.
>>>
>>> For SVM allocations
>>> Look for the address range into the interval tree VA from
>>> the VM inside of each pdds used in a kfd_process.
>>>
>>> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 ++++
>>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 79
>>> +++++++++++++++++++-----
>>> 2 files changed, 75 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 67541c30327a..f39baaa22a62 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -1251,6 +1251,7 @@ static int
>>> kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>>> struct kfd_process_device *pdd;
>>> void *mem;
>>> struct kfd_dev *dev;
>>> + struct svm_range_list *svms = &p->svms;
>>> int idr_handle;
>>> long err;
>>> uint64_t offset = args->mmap_offset;
>>> @@ -1259,6 +1260,18 @@ static int
>>> kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>>> if (args->size == 0)
>>> return -EINVAL;
>>>
>>> +#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
>>> + mutex_lock(&svms->lock);
>>> + if (interval_tree_iter_first(&svms->objects,
>>> + args->va_addr >> PAGE_SHIFT,
>>> + (args->va_addr + args->size -
>>> 1) >> PAGE_SHIFT)) {
>>> + pr_err("Address: 0x%llx already allocated by SVM\n",
>>> + args->va_addr);
>>> + mutex_unlock(&svms->lock);
>>> + return -EADDRINUSE;
>>> + }
>>> + mutex_unlock(&svms->lock);
>>> +#endif
>>> dev = kfd_device_by_id(args->gpu_id);
>>> if (!dev)
>>> return -EINVAL;
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index 31f3f24cef6a..043ee0467916 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -2581,9 +2581,54 @@ int svm_range_list_init(struct kfd_process *p)
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * svm_range_is_vm_bo_mapped - check if virtual address range
>>> mapped already
>>> + * @p: current kfd_process
>>> + * @start: range start address, in pages
>>> + * @last: range last address, in pages
>>> + *
>>> + * The purpose is to avoid virtual address ranges already allocated by
>>> + * kfd_ioctl_alloc_memory_of_gpu ioctl.
>>> + * It looks for each pdd in the kfd_process.
>>> + *
>>> + * Context: Process context
>>> + *
>>> + * Return 0 - OK, if the range is not mapped.
>>> + * Otherwise error code:
>>> + * -EADDRINUSE - if address is mapped already by
>>> kfd_ioctl_alloc_memory_of_gpu
>>> + * -ERESTARTSYS - A wait for the buffer to become unreserved was
>>> interrupted by
>>> + * a signal. Release all buffer reservations and return to user-space.
>>> + */
>>> +static int
>>> +svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start,
>>> uint64_t last)
>>> +{
>>> + uint32_t i;
>>> + int r;
>>> +
>>> + for (i = 0; i < p->n_pdds; i++) {
>>> + struct amdgpu_vm *vm;
>>> +
>>> + if (!p->pdds[i]->drm_priv)
>>> + continue;
>>> +
>>> + vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
>>> + r = amdgpu_bo_reserve(vm->root.bo, false);
>>> + if (r)
>>> + return r;
>>> + if (interval_tree_iter_first(&vm->va, start, last)) {
>>> + pr_debug("Range [0x%llx 0x%llx] already
>>> mapped\n", start, last);
>>> + amdgpu_bo_unreserve(vm->root.bo);
>>> + return -EADDRINUSE;
>>> + }
>>> + amdgpu_bo_unreserve(vm->root.bo);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> * svm_range_is_valid - check if virtual address range is valid
>>> - * @mm: current process mm_struct
>>> + * @mm: current kfd_process
>>> * @start: range start address, in pages
>>> * @size: range size, in pages
>>> *
>>> @@ -2592,28 +2637,27 @@ int svm_range_list_init(struct kfd_process *p)
>>> * Context: Process context
>>> *
>>> * Return:
>>> - * true - valid svm range
>>> - * false - invalid svm range
>>> + * 0 - OK, otherwise error code
>>> */
>>> -static bool
>>> -svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t
>>> size)
>>> +static int
>>> +svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t
>>> size)
>>> {
>>> const unsigned long device_vma = VM_IO | VM_PFNMAP |
>>> VM_MIXEDMAP;
>>> struct vm_area_struct *vma;
>>> unsigned long end;
>>> + unsigned long start_unchg = start;
>>>
>>> start <<= PAGE_SHIFT;
>>> end = start + (size << PAGE_SHIFT);
>>> -
>>> do {
>>> - vma = find_vma(mm, start);
>>> + vma = find_vma(p->mm, start);
>>> if (!vma || start < vma->vm_start ||
>>> (vma->vm_flags & device_vma))
>>> - return false;
>>> + return -EFAULT;
>>> start = min(end, vma->vm_end);
>>> } while (start < end);
>>>
>>> - return true;
>>> + return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1)
>>> >> PAGE_SHIFT);
>>> }
>>>
>>> /**
>>> @@ -2913,9 +2957,9 @@ svm_range_set_attr(struct kfd_process *p,
>>> uint64_t start, uint64_t size,
>>>
>>> svm_range_list_lock_and_flush_work(svms, mm);
>>>
>>> - if (!svm_range_is_valid(mm, start, size)) {
>>> - pr_debug("invalid range\n");
>>> - r = -EFAULT;
>>> + r = svm_range_is_valid(p, start, size);
>>> + if (r) {
>>> + pr_debug("invalid range r=%d\n", r);
>>> mmap_write_unlock(mm);
>>> goto out;
>>> }
>>> @@ -3016,17 +3060,18 @@ svm_range_get_attr(struct kfd_process *p,
>>> uint64_t start, uint64_t size,
>>> uint32_t flags = 0xffffffff;
>>> int gpuidx;
>>> uint32_t i;
>>> + int r = 0;
>>>
>>> pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n",
>>> &p->svms, start,
>>> start + size - 1, nattr);
>>>
>>> mmap_read_lock(mm);
>>> - if (!svm_range_is_valid(mm, start, size)) {
>>> - pr_debug("invalid range\n");
>>> - mmap_read_unlock(mm);
>>> - return -EINVAL;
>>> - }
>>> + r = svm_range_is_valid(p, start, size);
>>> mmap_read_unlock(mm);
>>> + if (r) {
>>> + pr_debug("invalid range r=%d\n", r);
>>> + return r;
>>> + }
>>>
>>> for (i = 0; i < nattr; i++) {
>>> switch (attrs[i].type) {
>>> --
>>> 2.32.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdkfd: avoid conflicting address mappings
2021-07-15 19:59 Alex Sierra
@ 2021-07-15 23:09 ` Felix Kuehling
0 siblings, 0 replies; 7+ messages in thread
From: Felix Kuehling @ 2021-07-15 23:09 UTC (permalink / raw)
To: amd-gfx, Sierra Guiza, Alejandro (Alex)
On 2021-07-15 3:59 p.m., Alex Sierra wrote:
> [Why]
> Avoid conflict with address ranges mapped by SVM
> mechanism that try to be allocated again through
> ioctl_alloc in the same process. And viceversa.
>
> [How]
> For ioctl_alloc_memory_of_gpu allocations
> Check if the address range passed into ioctl memory
> alloc does not exist already in the kfd_process
> svms->objects interval tree.
>
> For SVM allocations
> Look for the address range into the interval tree VA from
> the VM inside of each pdds used in a kfd_process.
>
> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 +++++
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 68 ++++++++++++++++++------
> 2 files changed, 66 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 67541c30327a..f39baaa22a62 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1251,6 +1251,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> struct kfd_process_device *pdd;
> void *mem;
> struct kfd_dev *dev;
> + struct svm_range_list *svms = &p->svms;
> int idr_handle;
> long err;
> uint64_t offset = args->mmap_offset;
> @@ -1259,6 +1260,18 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> if (args->size == 0)
> return -EINVAL;
>
> +#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
> + mutex_lock(&svms->lock);
> + if (interval_tree_iter_first(&svms->objects,
> + args->va_addr >> PAGE_SHIFT,
> + (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
> + pr_err("Address: 0x%llx already allocated by SVM\n",
> + args->va_addr);
> + mutex_unlock(&svms->lock);
> + return -EADDRINUSE;
> + }
> + mutex_unlock(&svms->lock);
> +#endif
> dev = kfd_device_by_id(args->gpu_id);
> if (!dev)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 31f3f24cef6a..43b31f00951a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2581,9 +2581,46 @@ int svm_range_list_init(struct kfd_process *p)
> return 0;
> }
>
> +/**
> + * svm_range_is_vm_bo_mapped - check if virtual address range mapped already
> + * @p: current kfd_process
> + * @start: range start address, in pages
> + * @last: range last address, in pages
> + *
> + * The purpose is to avoid virtual address ranges already allocated by
> + * kfd_ioctl_alloc_memory_of_gpu ioctl.
> + * It looks for each pdd in the kfd_process.
> + *
> + * Context: Process context
> + *
> + * Return 0 - OK, if the range is not mapped.
> + * Otherwise error code:
> + * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
> + * -EFAULT - if VM does not exist in specific pdd
> + */
> +static int
> +svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start, uint64_t last)
> +{
> + uint32_t i;
> +
> + for (i = 0; i < p->n_pdds; i++) {
> + struct amdgpu_vm *vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
> +
> + if (!vm)
> + return -EFAULT;
This is not necessarily an error. Not every process uses every GPU. I
think you can just skip PDDs that don't have a VM. But you also need a
NULL-check for p->pdds[i]->drm_priv. I believe it will be NULL on an
unused GPU (where user mode never called kfd_ioctl_acquire_vm).
> +
> + if (interval_tree_iter_first(&vm->va, start, last)) {
I believe access to this interval tree needs to happen under the page
table reservation lock. That means you need to
amdgpu_bo_reserve(vm->root.bo, false) (and handle the special case that
it returns -ERESTARTSYS).
> + pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
> + return -EADDRINUSE;
> + }
> + }
> +
> + return 0;
> +}
> +
> /**
> * svm_range_is_valid - check if virtual address range is valid
> - * @mm: current process mm_struct
> + * @mm: current kfd_process
> * @start: range start address, in pages
> * @size: range size, in pages
> *
> @@ -2592,28 +2629,27 @@ int svm_range_list_init(struct kfd_process *p)
> * Context: Process context
> *
> * Return:
> - * true - valid svm range
> - * false - invalid svm range
> + * 0 - OK, otherwise error code
> */
> -static bool
> -svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t size)
> +static int
> +svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
> {
> const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
> struct vm_area_struct *vma;
> unsigned long end;
> + unsigned long start_unchg = start;
>
> start <<= PAGE_SHIFT;
> end = start + (size << PAGE_SHIFT);
> -
> do {
> - vma = find_vma(mm, start);
> + vma = find_vma(p->mm, start);
> if (!vma || start < vma->vm_start ||
> (vma->vm_flags & device_vma))
> - return false;
> + return -EFAULT;
> start = min(end, vma->vm_end);
> } while (start < end);
>
> - return true;
> + return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1) >> PAGE_SHIFT);
> }
>
> /**
> @@ -2913,9 +2949,9 @@ svm_range_set_attr(struct kfd_process *p, uint64_t start, uint64_t size,
>
> svm_range_list_lock_and_flush_work(svms, mm);
>
> - if (!svm_range_is_valid(mm, start, size)) {
> - pr_debug("invalid range\n");
> - r = -EFAULT;
> + r = svm_range_is_valid(p, start, size);
> + if (r) {
> + pr_debug("invalid range r=%d\n", r);
> mmap_write_unlock(mm);
> goto out;
> }
> @@ -3016,15 +3052,17 @@ svm_range_get_attr(struct kfd_process *p, uint64_t start, uint64_t size,
> uint32_t flags = 0xffffffff;
> int gpuidx;
> uint32_t i;
> + int r = 0;
>
> pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n", &p->svms, start,
> start + size - 1, nattr);
>
> mmap_read_lock(mm);
> - if (!svm_range_is_valid(mm, start, size)) {
> - pr_debug("invalid range\n");
> + r = svm_range_is_valid(p, start, size);
> + if (r) {
> + pr_debug("invalid range r=%d\n", r);
> mmap_read_unlock(mm);
If you move the mmap_read_unlock before the if (r), you don't need to
duplicate it inside and outside the "if".
Regards,
Felix
> - return -EINVAL;
> + return r;
> }
> mmap_read_unlock(mm);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] drm/amdkfd: avoid conflicting address mappings
@ 2021-07-15 19:59 Alex Sierra
2021-07-15 23:09 ` Felix Kuehling
0 siblings, 1 reply; 7+ messages in thread
From: Alex Sierra @ 2021-07-15 19:59 UTC (permalink / raw)
To: amd-gfx; +Cc: Alex Sierra
[Why]
Avoid conflict with address ranges mapped by SVM
mechanism that try to be allocated again through
ioctl_alloc in the same process. And viceversa.
[How]
For ioctl_alloc_memory_of_gpu allocations
Check if the address range passed into ioctl memory
alloc does not exist already in the kfd_process
svms->objects interval tree.
For SVM allocations
Look for the address range into the interval tree VA from
the VM inside of each pdds used in a kfd_process.
Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 +++++
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 68 ++++++++++++++++++------
2 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 67541c30327a..f39baaa22a62 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1251,6 +1251,7 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
struct kfd_process_device *pdd;
void *mem;
struct kfd_dev *dev;
+ struct svm_range_list *svms = &p->svms;
int idr_handle;
long err;
uint64_t offset = args->mmap_offset;
@@ -1259,6 +1260,18 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
if (args->size == 0)
return -EINVAL;
+#if IS_ENABLED(CONFIG_HSA_AMD_SVM)
+ mutex_lock(&svms->lock);
+ if (interval_tree_iter_first(&svms->objects,
+ args->va_addr >> PAGE_SHIFT,
+ (args->va_addr + args->size - 1) >> PAGE_SHIFT)) {
+ pr_err("Address: 0x%llx already allocated by SVM\n",
+ args->va_addr);
+ mutex_unlock(&svms->lock);
+ return -EADDRINUSE;
+ }
+ mutex_unlock(&svms->lock);
+#endif
dev = kfd_device_by_id(args->gpu_id);
if (!dev)
return -EINVAL;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 31f3f24cef6a..43b31f00951a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2581,9 +2581,46 @@ int svm_range_list_init(struct kfd_process *p)
return 0;
}
+/**
+ * svm_range_is_vm_bo_mapped - check if virtual address range mapped already
+ * @p: current kfd_process
+ * @start: range start address, in pages
+ * @last: range last address, in pages
+ *
+ * The purpose is to avoid virtual address ranges already allocated by
+ * kfd_ioctl_alloc_memory_of_gpu ioctl.
+ * It looks for each pdd in the kfd_process.
+ *
+ * Context: Process context
+ *
+ * Return 0 - OK, if the range is not mapped.
+ * Otherwise error code:
+ * -EADDRINUSE - if address is mapped already by kfd_ioctl_alloc_memory_of_gpu
+ * -EFAULT - if VM does not exist in specific pdd
+ */
+static int
+svm_range_is_vm_bo_mapped(struct kfd_process *p, uint64_t start, uint64_t last)
+{
+ uint32_t i;
+
+ for (i = 0; i < p->n_pdds; i++) {
+ struct amdgpu_vm *vm = drm_priv_to_vm(p->pdds[i]->drm_priv);
+
+ if (!vm)
+ return -EFAULT;
+
+ if (interval_tree_iter_first(&vm->va, start, last)) {
+ pr_debug("Range [0x%llx 0x%llx] already mapped\n", start, last);
+ return -EADDRINUSE;
+ }
+ }
+
+ return 0;
+}
+
/**
* svm_range_is_valid - check if virtual address range is valid
- * @mm: current process mm_struct
+ * @mm: current kfd_process
* @start: range start address, in pages
* @size: range size, in pages
*
@@ -2592,28 +2629,27 @@ int svm_range_list_init(struct kfd_process *p)
* Context: Process context
*
* Return:
- * true - valid svm range
- * false - invalid svm range
+ * 0 - OK, otherwise error code
*/
-static bool
-svm_range_is_valid(struct mm_struct *mm, uint64_t start, uint64_t size)
+static int
+svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
{
const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
struct vm_area_struct *vma;
unsigned long end;
+ unsigned long start_unchg = start;
start <<= PAGE_SHIFT;
end = start + (size << PAGE_SHIFT);
-
do {
- vma = find_vma(mm, start);
+ vma = find_vma(p->mm, start);
if (!vma || start < vma->vm_start ||
(vma->vm_flags & device_vma))
- return false;
+ return -EFAULT;
start = min(end, vma->vm_end);
} while (start < end);
- return true;
+ return svm_range_is_vm_bo_mapped(p, start_unchg, (end - 1) >> PAGE_SHIFT);
}
/**
@@ -2913,9 +2949,9 @@ svm_range_set_attr(struct kfd_process *p, uint64_t start, uint64_t size,
svm_range_list_lock_and_flush_work(svms, mm);
- if (!svm_range_is_valid(mm, start, size)) {
- pr_debug("invalid range\n");
- r = -EFAULT;
+ r = svm_range_is_valid(p, start, size);
+ if (r) {
+ pr_debug("invalid range r=%d\n", r);
mmap_write_unlock(mm);
goto out;
}
@@ -3016,15 +3052,17 @@ svm_range_get_attr(struct kfd_process *p, uint64_t start, uint64_t size,
uint32_t flags = 0xffffffff;
int gpuidx;
uint32_t i;
+ int r = 0;
pr_debug("svms 0x%p [0x%llx 0x%llx] nattr 0x%x\n", &p->svms, start,
start + size - 1, nattr);
mmap_read_lock(mm);
- if (!svm_range_is_valid(mm, start, size)) {
- pr_debug("invalid range\n");
+ r = svm_range_is_valid(p, start, size);
+ if (r) {
+ pr_debug("invalid range r=%d\n", r);
mmap_read_unlock(mm);
- return -EINVAL;
+ return r;
}
mmap_read_unlock(mm);
--
2.32.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-30 16:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 21:18 [PATCH] drm/amdkfd: avoid conflicting address mappings Alex Sierra
2021-07-29 15:47 ` Felix Kuehling
2021-09-29 23:35 ` Mike Lothian
2021-09-30 2:15 ` Felix Kuehling
2021-09-30 16:32 ` Sierra Guiza, Alejandro (Alex)
-- strict thread matches above, loose matches on Subject: below --
2021-07-15 19:59 Alex Sierra
2021-07-15 23:09 ` Felix Kuehling
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.