* [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* Re: [PATCH] drm/amdkfd: avoid conflicting address mappings
2021-07-15 19:59 [PATCH] drm/amdkfd: avoid conflicting address mappings 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-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 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 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
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-15 19:59 [PATCH] drm/amdkfd: avoid conflicting address mappings Alex Sierra
2021-07-15 23:09 ` Felix Kuehling
2021-07-19 21:18 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)
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.